test: use liquibase test harness #71

olavloite · 2021-02-26T12:56:55Z

Adds the standard Liquibase test harness suite tests to the Spanner Liquibase integration. The tests can be executed from the IDE (IntelliJ), and works both when the project has been imported as a Maven and as a Gradle project.

All tests that cover features that are not supported by Cloud Spanner (e.g. views, dropping primary keys, ...) are skipped.
Tests that use tables without primary keys in the default test suite are overridden by a Spanner specific version that does use a primary key.

All tests that cover features that are supported by Cloud Spanner succeed (on the emulator). For this, a couple of functional changes have been made to the Spanner Liquibase integration:

A Spanner-specific AddColumnGenerator has been added to ensure that the statement includes the COLUMN keyword (i.e. ALTER TABLE Foo ADD COLUMN Bar ...`). This is currently only required by the emulator, but as it is to be expected that many users will use the emulator to test their applications, it is important that the integration works as much as possible with the emulator.
A Spanner-specific AddForeignKeyConstraintGenerator has been added to ensure that any OnUpdate and OnDelete specifications are ignored, as this is not supported by Cloud Spanner. This is also in line with the Liquibase behavior for other databases that do not support these keywords.
An extra check was added to the CreateTableGenerator for Spanner. An array-out-of-bounds exception could be thrown if a table without primary keys was being generated. Now it throws an understandable exception, as Cloud Spanner does not support tables without a primary key.

This change also touches a large number of files as it adds the ICloudSpanner marker interface. Using a marker interface instead of a class for checking whether the database in use is Cloud Spanner is better, as it allows us to generate proxy objects when only one small feature of the default Liquibase implementation needs to be adjusted for a specific change. This strategy is applied to the new AddColumnGenerator to add the COLUMN keyword to the generated SQL.

mescanne

Are we not going to maintain the maven and gradle build separately?

olavloite · 2021-02-26T18:24:03Z

Are we not going to maintain the maven and gradle build separately?

This is only a draft to discuss with the people from Liquibase on how to get the test harness to work. This PR is green, but I expected it to fail as it should include additional tests that should fail.

olavloite · 2021-03-04T16:14:35Z

cc @voulg

olavloite · 2021-03-04T16:19:06Z

src/main/java/liquibase/ext/spanner/CloudSpanner.java


-public class CloudSpanner extends AbstractJdbcDatabase {
+public class CloudSpanner extends AbstractJdbcDatabase implements ICloudSpanner {


Adding this interface makes it a lot easier to create proxies of the Cloud Spanner database. This can be very useful when a change type only needs very small adjustments from the standard generator. Instead of copy-pasting the code from the standard generator into the Spanner specific generator, a proxy object can be used to only override specific behavior at that moment. See the new AddColumnGeneratorSpanner for an example, where the only change is that we need it to generate ALTER TABLE ADD COLUMN columnName instead of ALTER TABLE ADD columnName.

olavloite · 2021-03-04T16:19:56Z

src/main/java/liquibase/ext/spanner/change/AddColumnChangeSpanner.java

+      if (statement instanceof UpdateStatement) {
+        UpdateStatement updateStatement = (UpdateStatement) statement;
+        if (updateStatement.getWhereClause() == null) {
+          updateStatement.setWhereClause("TRUE");


Automatically add a WHERE TRUE clause to update statements that fill the initial value of a column.

olavloite · 2021-03-04T16:21:02Z

src/main/java/liquibase/ext/spanner/change/AddLookupTableChangeSpanner.java

@@ -36,7 +37,7 @@

  @Override
  public boolean supports(Database database) {
-    return (database instanceof CloudSpanner);
+    return (database instanceof ICloudSpanner);


Checking against ICloudSpanner (the interface) instead of CloudSpanner ensures that changes that generate a proxy for the database are still considered Cloud Spanner databases.

olavloite · 2021-03-04T16:23:11Z

src/main/java/liquibase/ext/spanner/sqlgenerator/AddColumnGeneratorSpanner.java

+
+    @Override
+    protected String generateSingleColumnSQL(AddColumnStatement statement, Database database) {
+        InvocationHandler handler = (proxy, method, args) -> {


Generate a proxy that will add the COLUMN keyword in front of the column name when it is needed specifically for this change.

olavloite · 2021-03-05T12:42:20Z

@mescanne Would you mind taking a second look? This should now work for both Gradle and Maven. Note that the Liquibase harness test suite can (currently) only be executed in IntelliJ, and that you must use Java 8 as the execution environment.

@voulg and @kristyldatical Please feel free to add any comments / questions you might have as well.

mescanne

Thorough and great to be integrated with Liquibase's test harness. Some comments in there I'd be interested in your take on.

mescanne · 2021-03-05T13:23:08Z

src/main/java/liquibase/ext/spanner/sqlgenerator/AddColumnGeneratorSpanner.java

+        ICloudSpanner databaseWithColumnKeyword = (ICloudSpanner) Proxy.newProxyInstance(ICloudSpanner.class.getClassLoader(),
+                new Class[] { ICloudSpanner.class },
+                handler);
+        return super.generateSingleColumnSQL(statement, databaseWithColumnKeyword);


It looks like the proxied Database is only scoped to generateSingleColumnSQL. I believe this addition of COLUMN is only needed for the emulator but not real Spanner -- I did start looking into changing the emulator itself, which I think was acceptable.

We can use the proxy in the meantime, but I'd have a strong preference to use it in the long term.

It looks like the proxied Database is only scoped to generateSingleColumnSQL.

Correct, but the generateMultipleColumns method will just call generateSingleColumnSQL multiple times, so it covers that scenario as well.

I agree that the preference would be to not need the additional COLUMN keyword, as it is indeed only needed for the emulator. Once that is fixed, we can remove this.

mescanne · 2021-03-05T13:25:17Z

src/main/java/liquibase/ext/spanner/sqlgenerator/AddDefaultValueGeneratorSpanner.java

@@ -16,6 +16,7 @@
 import liquibase.database.Database;
 import liquibase.exception.ValidationErrors;
 import liquibase.ext.spanner.CloudSpanner;


We can remove the import of liquibase.ext.spanner.CloudSpanner as well.

Good point, done.

mescanne · 2021-03-05T13:34:50Z

src/test/resources/docker/init-cloudspanner.sql

+
+-- To run Liquibase Harness test suite:
+-- 1. Start Cloud Spanner emulator with `docker run -p 9010:9010 -p 9020:9020 gcr.io/cloud-spanner-emulator/emulator`
+-- 2. Execute the script below to initialize a database on projects/my-project/instances/my-instance/databases/my-database


Does the emulator automatically created my-instance and my-database? That was always a challenge using the emulator for me.

Ah, yeah, that's a good point. I currently have a locally built version of the JDBC driver that does that :-) So I forgot to put in the documentation that you need to do that manually for the time being. Once this PR has been merged and a new version has been released, it will no longer be necessary.

mescanne · 2021-03-05T13:54:05Z

pom.xml

@@ -52,7 +52,7 @@
    <java.version>1.8</java.version>
    <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>

-    <liquibase.version>3.8.9</liquibase.version>
+    <liquibase.version>4.3.1</liquibase.version>


Gradle build uses 4.2.0. Is it worth maintaining Gradle separately from Maven? We can look to drop Maven -- it would be better from a CI perspective to use a single build system.

Yes, I would also prefer switching to just one. The only thing I (or someone else) has to configure is the release process to Maven central in the Gradle setup. That is currently only setup in the pom.
I'll look into that in a separate PR.

mescanne · 2021-03-05T14:03:17Z

src/main/java/liquibase/ext/spanner/sqlgenerator/CreateTableGeneratorSpanner.java

-        database.escapeColumnNameList(
-            StringUtils.join(statement.getPrimaryKeyConstraint().getColumns(), ", ")));
-    buffer.append(")");
+    if (statement.getPrimaryKeyConstraint() != null && statement.getPrimaryKeyConstraint().getColumns() != null) {


Pure style (and I'm not a Google style expert) question. Wouldn't this be preferred -->
// If no PK constraint, leave it as it is and let Spanner itself throw the error
if (statement.getPrimaryKeyConstraint() == null || statement.getPrimaryKeyConstraint().getColumns() == null) {
return res;
}

The (open source) style guide does not explicitly state so, but I agree that in this case it's certainly more readable.

mescanne

Looks good - still want to get the emulator fixed, and the new JDBC auto-create-instance&database looks good, too.

olavloite added 2 commits February 26, 2021 13:50

test: use liquibase test harness

bb90fd1

cleanup: remove unnecessary changes

f1c9182

google-cla bot added the cla: yes CLA signed label Feb 26, 2021

mescanne approved these changes Feb 26, 2021

View reviewed changes

olavloite added 2 commits February 26, 2021 20:01

test: try with example from cassandra

80a0e52

test: liquibase harness test suite works

96b8c5d

olavloite commented Mar 4, 2021

View reviewed changes

olavloite added 2 commits March 4, 2021 17:28

docs: add comments

dc69075

build: enable Liquibase test harness in Gradle build

6619a9d

olavloite marked this pull request as ready for review March 5, 2021 12:39

mescanne reviewed Mar 5, 2021

View reviewed changes

olavloite added 3 commits March 5, 2021 17:37

chore: remove unused imports

c021d70

docs: add step for creating instance and database

b09e71c

fix: simplify if statement

f392277

mescanne approved these changes Mar 5, 2021

View reviewed changes

olavloite merged commit c4e9eba into master Mar 8, 2021

olavloite deleted the liquibase-test-harness branch March 8, 2021 06:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: use liquibase test harness #71

test: use liquibase test harness #71

olavloite commented Feb 26, 2021 •

edited

mescanne left a comment

olavloite commented Feb 26, 2021

olavloite commented Mar 4, 2021

olavloite Mar 4, 2021

olavloite Mar 4, 2021

olavloite Mar 4, 2021

olavloite Mar 4, 2021

olavloite commented Mar 5, 2021

mescanne left a comment

mescanne Mar 5, 2021

olavloite Mar 5, 2021

mescanne Mar 5, 2021

olavloite Mar 5, 2021

mescanne Mar 5, 2021

olavloite Mar 5, 2021

mescanne Mar 5, 2021

olavloite Mar 5, 2021

mescanne Mar 5, 2021

olavloite Mar 5, 2021

mescanne left a comment


		public class CloudSpanner extends AbstractJdbcDatabase {
		public class CloudSpanner extends AbstractJdbcDatabase implements ICloudSpanner {

test: use liquibase test harness #71

test: use liquibase test harness #71

Conversation

olavloite commented Feb 26, 2021 • edited

mescanne left a comment

Choose a reason for hiding this comment

olavloite commented Feb 26, 2021

olavloite commented Mar 4, 2021

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

olavloite commented Mar 5, 2021

mescanne left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mescanne left a comment

Choose a reason for hiding this comment

olavloite commented Feb 26, 2021 •

edited