Adds support to Postgres #44

pgeadas · 2024-06-19T16:42:24Z

Description

Adds support for postgres driver:

org.postgresql.Driver

Caveats

Backticks are a non-standard used by MySQL. Postgres does not support them by default.

src/main/java/io/seqera/migtool/MigTool.java

src/main/java/io/seqera/migtool/Driver.java

src/main/java/io/seqera/migtool/MigTool.java

src/main/java/io/seqera/migtool/Helper.java

src/main/java/io/seqera/migtool/MigTool.java

src/main/java/io/seqera/migtool/Dialect.java

README.md

pgeadas · 2024-07-26T09:50:15Z

Are we merging this at some point? @swampie @pditommaso @tcrespog

tcrespog · 2024-07-26T11:03:06Z

Are we merging this at some point? @swampie @pditommaso @tcrespog

It's okay for me (I approved it). Should we wait for @swampie and @pditommaso to check it? Please, Matteo & Paolo, let us know.

pditommaso · 2024-07-26T16:08:59Z

src/main/java/io/seqera/migtool/MigTool.java

-                while( itr.hasNext() ) {
-                    Path it = itr.next();
+            try (DirectoryStream<Path> stream = Files.newDirectoryStream(Paths.get(path))) {
+                for (Path it : stream) {


WHat's the value of this change? don't see related to the support for Postgress

Apart from clarity, it uses try with resources which automatically closes the stream after usage.

pditommaso · 2024-07-26T16:11:04Z

src/main/java/io/seqera/migtool/MigTool.java

+    public static class Builder {
+        private String driverName;
+        private Driver driver;
+        private String url;
+        private String user;
+        private String password;
+        private String dialectName;
+        private Dialect dialect;
+        private String locations;
+        private ClassLoader classLoader;
+        private Pattern pattern;
+
+        public Builder withDriver(String driverName) {
+            this.driverName = driverName;
+            return this;
+        }
+
+        public Builder withUrl(String url) {
+            this.url = url;
+            return this;
+        }
+
+        public Builder withUser(String user) {
+            this.user = user;
+            return this;
+        }
+
+        public Builder withPassword(String password) {
+            this.password = password;
+            return this;
+        }
+
+        public Builder withDialect(String dialectName) {
+            this.dialectName = dialectName;
+            return this;
+        }
+
+        public Builder withLocations(String locations) {
+            this.locations = locations;
+            return this;
+        }
+
+        public Builder withClassLoader(ClassLoader loader) {
+            this.classLoader = loader;
+            return this;
+        }
+
+        public Builder withPattern(String pattern) {
+            if(pattern != null && !pattern.isEmpty())
+                this.pattern = Pattern.compile(pattern);
+            return this;
+        }
+
+        public MigTool build() {
+            this.driver = driverName != null ? Driver.fromDriverName(driverName) : Driver.fromUrl(url);
+            this.dialect = dialectName != null ? Dialect.fromDialectName(dialectName) : Dialect.fromUrl(url);
+            return new MigTool(this);
+        }
+    }
+


Bit useless. It was avoid on purpose to not implement the builder pattern because it's just redundant

There are two main points for this change:

As it is, although it looks like a builder pattern, it is not which is confusing.

The app will fail to run if we do not set up the url first, since there are other fields that depend on it.

migtool/src/main/java/io/seqera/migtool/App.java

Lines 50 to 52 in c131b38

.withUrl(url)

.withDialect(dialect!=null ? dialect : dialectFromUrl(url))

.withDriver(driver!=null ? driver : driverFromUrl(url))

It might "work" in this case since we receive the url as a parameter and configure it directly with those parameters, but to instantiate MigTool from another application without a cli (like the seqera compute manager service), this is just a source of potential issues and bugs (as it happened, hence the change).

pditommaso · 2024-07-26T16:13:38Z

src/main/java/io/seqera/migtool/strategy/DialectQueryBuilder.java

+ * Since the other dialects supported allow the use of backticks, only two builders are added
+ * {@link PostgresQueryBuilder} and {@link NotPostgresQueryBuilder}.
+ **/
+public abstract class DialectQueryBuilder {


I found this solution overkill. It would be enough to have a driver pattern having a default implementation (a couple of methods) covering the current statement and the version for Postgres.

What is a driver pattern? Could you provide an example of what you mean?

That kind of things that's simpler to implement, that to describe. If we want to adhere to GoF definition, it would be a template pattern, but also Jdbc driver using a similar approach, this is why I had used that term.

Compare this implementation with the one in this PR (I hadn't time to add tests)

I considered this approach, but I don't like it because the common SQL needs to be duplicated in both implementations. Meaning that if we need to change one of the queries in one place, we need to change it in both files (Default and Postgres).

In the end, we also needed to add 3 classes here, so I don't see what we gained using this approach. :)

And this is not a template pattern. As I explain here.

I am proposing a solution now that solves both issues: the one regarding being overkill (since we only have 2 implementations) and the one that implies having the sql string repeated in the subclasses.

Would this be acceptable in your opinion? I think it merges the best of both worlds @pditommaso .

pditommaso · 2024-07-26T16:17:50Z

src/main/java/io/seqera/migtool/Dialect.java

+import jakarta.annotation.Nullable;
+import java.util.List;
+
+public enum Dialect {


Not sure what value adds Dialect and Driver as enum. it would make more sense to implement a driver pattern that can both the type and the method specialisation for the statement specific for postgres. Likely it would require a Driver interface, plus an abstract implementation for the common stuff, and concrete class for each support DB.

An alternative approach could be using a delegate pattern and passing the concrete class as "argument"

I think that any approach is better than having the names of the drivers/dialects hardcoded as strings, so I just typed them. An enum seemed simple enough for this purpose.

An alternative approach could be using a delegate pattern and passing the concrete class as "argument"

Why are we trying to find alternative approaches here? What is the issue with this one? There are multiple ways of solving the same thing, and as I said, this is already an improvement over what we had.

pgeadas · 2024-08-06T12:55:39Z

@pditommaso Could you review my comments please?

Added jakarta annotations; added new constructor to be compatible with Java; changed the visibility of a method, to avoid the need of extending the base class just to use it

pgeadas · 2024-08-08T18:07:43Z

src/main/java/io/seqera/migtool/MigRecord.java

@@ -40,6 +40,16 @@ static Language from(String fileName) {
    boolean isPatch;
    boolean isOverride;

+    /**


For example this is missing @pditommaso , otherwise it won't work from Java...

Is this related to Postgres?

Is this related to Postgres?

This won't work from Java, but I guess it is not needed for Postgres.

pditommaso · 2024-08-08T19:24:21Z

Sorry Pedro, I'm closing in favour of #49. While I'm sure this work correctly, I believe is not a good PR because it conflates together many different things: support for Postres, support for Java, general refactoring, unnecessary complexity, changes the MigTool public API, etc.

Why I think this is bad:

make difficult if not impossible to make an effective code review, because there are too many unrelated changes;
make it difficult to recreate the history and the rationale of the changes;
it can introduce unexpected bugs or side-effects;
it's contrary to simplicity and frugality principles;
last and even more importantly, software source code is a collaborative effort and shared knowledge. it represents a mental model for all the people that have worked with it and know how to deal with it. Making a radical refactoring - without a substantial reason - it means that all these people (me, Thomas, Veronica, whoever) will need to spend time to learn new data structures, new behaviours, etc for no clear reason.

I don't want to be further pedantic, but I'b be happy to discuss more if needed.

pgeadas · 2024-08-08T20:55:11Z

make difficult if not impossible to make an effective code review, because there are too many unrelated changes;

This point I can agree with. Some changes are unrelated to Postgres and were done just to make it easier to implement that change. I can split this in different PR's if this is the issue.

Three PR's refactors with:

adding the enums instead of hardcoded Strings in the code
adding the real builder pattern (as it is, we need to do things from the outside to build the MigTool every time)
adding the real template pattern (as it is, the queries are not following any template, if the query changes we need to manually change two classes)

But this is by no means a radical refactor, this is just a cleanup.

it's contrary to simplicity and frugality principles;

I don't think hardcoded strings and methods that do 3 or 4 things at the same time, can be considered simplicity.

it means that all these people (me, Thomas, Veronica, whoever) will need to spend time to learn new data structures, new behaviours, etc for no clear reason.
unnecessary complexity

I think an enum to replace hardcoded Strings and encapsulate behaviours that right now exist on a static helper class, is not "for no reason" and removes complexity, does not increase it. 🤷🏻‍♂️
In fact, I assure you it would be way easier to understand for new people and even the old ones when they need to come back to it.

last and even more importantly, software source code is a collaborative effort and shared knowledge.

I don't think that I am the one missing this point here 👼🏻 The PR already has several approvals from other team members...

Actually me and @tcrespog had discussed these improvements together, in a collaborative way...

I don't want to be further pedantic, but I'b be happy to discuss more if needed.

I could give you all the reasons in the world that it would not have made a difference, so I don't think it is worth discussing more, you are right @pditommaso 🥲

pgeadas requested review from pditommaso, swampie and tcrespog June 19, 2024 16:42

pgeadas force-pushed the notask-add-support-to-postgres branch 5 times, most recently from 9558e5b to cb76924 Compare June 19, 2024 20:43

pditommaso reviewed Jun 19, 2024

View reviewed changes

src/main/java/io/seqera/migtool/MigTool.java Outdated Show resolved Hide resolved

pgeadas force-pushed the notask-add-support-to-postgres branch from cb76924 to 911a817 Compare June 19, 2024 21:03

tcrespog reviewed Jun 20, 2024

View reviewed changes

pgeadas force-pushed the notask-add-support-to-postgres branch from c7a3638 to 8bd78dc Compare June 20, 2024 15:29

tcrespog approved these changes Jun 21, 2024

View reviewed changes

src/main/java/io/seqera/migtool/Dialect.java Outdated Show resolved Hide resolved

README.md Show resolved Hide resolved

pgeadas force-pushed the notask-add-support-to-postgres branch 2 times, most recently from ffd9512 to ba83f79 Compare June 21, 2024 10:48

pgeadas requested a review from pditommaso June 28, 2024 15:29

swampie approved these changes Jul 26, 2024

View reviewed changes

pditommaso requested changes Jul 26, 2024

View reviewed changes

pgeadas added 7 commits August 7, 2024 13:09

Adds support to Postgres

78af5a4

Removed 'test containers driver'; small improvements

43dfbb2

Refactored MigTool to use a standard Builder pattern

cfec41c

Refactored MigTool to use a query builder

39e7942

Improve documentation

2be572a

Small fixes

fe6454e

Added jakarta annotations; added new constructor to be compatible with Java; changed the visibility of a method, to avoid the need of extending the base class just to use it

refactor: simplify how to choose the sql template

9b14cd3

pgeadas force-pushed the notask-add-support-to-postgres branch from faf697f to 9b14cd3 Compare August 7, 2024 11:10

pgeadas requested a review from pditommaso August 8, 2024 15:27

pgeadas commented Aug 8, 2024

View reviewed changes

pditommaso closed this Aug 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support to Postgres #44

Adds support to Postgres #44

pgeadas commented Jun 19, 2024 •

edited

Loading

pgeadas commented Jul 26, 2024

tcrespog commented Jul 26, 2024

pditommaso Jul 26, 2024

pgeadas Aug 5, 2024

pditommaso Jul 26, 2024

pgeadas Aug 5, 2024 •

edited

Loading

pditommaso Jul 26, 2024

pgeadas Aug 5, 2024

pditommaso Aug 7, 2024

pgeadas Aug 7, 2024 •

edited

Loading

pditommaso Jul 26, 2024

pgeadas Aug 5, 2024

pgeadas commented Aug 6, 2024

pgeadas Aug 8, 2024

pditommaso Aug 8, 2024 •

edited

Loading

pgeadas Aug 8, 2024

pditommaso commented Aug 8, 2024

pgeadas commented Aug 8, 2024 •

edited

Loading

	.withUrl(url)
	.withDialect(dialect!=null ? dialect : dialectFromUrl(url))
	.withDriver(driver!=null ? driver : driverFromUrl(url))

Adds support to Postgres #44

Adds support to Postgres #44

Conversation

pgeadas commented Jun 19, 2024 • edited Loading

Description

Caveats

pgeadas commented Jul 26, 2024

tcrespog commented Jul 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgeadas Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgeadas Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgeadas commented Aug 6, 2024

Choose a reason for hiding this comment

pditommaso Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pditommaso commented Aug 8, 2024

pgeadas commented Aug 8, 2024 • edited Loading

pgeadas commented Jun 19, 2024 •

edited

Loading

pgeadas Aug 5, 2024 •

edited

Loading

pgeadas Aug 7, 2024 •

edited

Loading

pditommaso Aug 8, 2024 •

edited

Loading

pgeadas commented Aug 8, 2024 •

edited

Loading