usePreparedStatement attribute in loadData element has no effect

[jdhmoreno]

Documented on issue
#1091

Copied issue description here

Environment

Liquibase Version: 3.8.9

Liquibase Integration & Version: maven liquibase plugin, also version 3.8.9; maven version 3.6.3

Liquibase Extension(s) & Version:

Database Vendor & Version: SQLServer 2012

Operating System Type & Version: Ubuntu running Docker 19.0.3

Description

Regardless of whether the attribute 'usePreparedStatements' is set to 'true' or 'false' in 'loadData' elements, Liquibase always uses prepared statements (so it behaves as if it were harcoded to 'true')

This is the cause why when we try to load a CSV file where a 'DATE' column is set to 'NULL' we get the following error:

{NOFORMAT}
Error setting up or running Liquibase:
[ERROR] Migration failed for change set liquibase/xxx:
[ERROR] Reason: liquibase.exception.DatabaseException: java.sql.BatchUpdateException: Implicit conversion from data type varbinary to datetime2 is not allowed. Use the CONVERT function to run this query.
{NOFORMAT}
This was already reported in https://liquibase.jira.com/browse/CORE-3399
As a workaround to overcome this problem, I've tried to set the 'usePreparedStatements' (in order to force the creation of liquibase's InsertStatements) to 'false' but it has no effect.

It all seems to be in Liquibase's 'LoadDataChange' class, in the condition that starts in line 464 from the liquibase codebase, GIT tag 3.8.9:

                // end of: iterate through all the columns of a CSV line

                // Try to use prepared statements if any of the two following conditions apply:
                // 1. There is no other option than using a prepared statement (e.g. in cases of LOBs)
                // 2. The database supports batched statements (for improved performance) AND we are not in an
                //    "SQL" mode (i.e. we generate an SQL file instead of actually modifying the database).
                if
                (
                    (needsPreparedStatement ||
                        (databaseSupportsBatchUpdates &&
                                !(ExecutorService.getInstance().getExecutor(database) instanceof LoggingExecutor)
                        )
                    )
                    && hasPreparedStatementsImplemented()
                ) {
                    anyPreparedStatements = true;
                    ExecutablePreparedStatementBase stmt =
                        this.createPreparedStatement(
                            database, getCatalogName(), getSchemaName(), getTableName(), columnsFromCsv,
                            getChangeSet(), getResourceAccessor()
                        );
                    batchedStatements.add(stmt);
                } else {
                    InsertStatement insertStatement =
                        this.createStatement(getCatalogName(), getSchemaName(), getTableName());

                    for (ColumnConfig column : columnsFromCsv) {
                        String columnName = column.getName();
                        Object value = column.getValueObject();

                        if (value == null) {
                            value = "NULL";
                        }

                        insertStatement.addColumnValue(columnName, value);
                    }

                    statements.add(insertStatement);
                }
                // end of: will we use a PreparedStatement?
            }
            // end of: loop for every input line from the CSV file

The value of the flag 'needsPreparedStatement' is not taken into account in that condition, because:

1. It is part of an OR condition where the second term is TRUE as long as the database supports batch updates (SQLServer does as stated in the JDBC driver) and
2. the liquibase command run does not generate LOGs (liquibase:update does not generate logs)

So 'needsPreparedStatement' will have no effect.

Steps To Reproduce

• Given a running instance of SQL Server 2012
• And a JDK version 11 or above (e.g. openjdk 11)
• And the following POM.xml

<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0"
  xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
  xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
  <modelVersion>4.0.0</modelVersion>

  <groupId>org.liquibase</groupId>
  <artifactId>test.database.problem</artifactId>
  <version>1.0.0</version>
  <name>Problem Dates in SQLServer</name>
  <packaging>jar</packaging>

  <properties>
    <database.port>YOUR SQL SERVER PORT HERE</database.port>
    <database.host>YOUR SQL SERVER HOST HERE</database.host>
    <database.username>YOUR DATABASE USERNAME HERE</database.username>
    <database.password>YOUR DATBASE PASSWORD HERE</database.password>
  </properties>

  <dependencies>
    <dependency>
      <groupId>com.microsoft.sqlserver</groupId>
      <artifactId>mssql-jdbc</artifactId>
      <version>8.3.0.jre11-preview</version>
    </dependency>
    <dependency>
      <groupId>org.liquibase</groupId>
      <artifactId>liquibase-core</artifactId>
      <version>3.8.9</version>
    </dependency>
  </dependencies>

  <build>
    <plugins>
      <plugin>
        <groupId>org.liquibase</groupId>
        <artifactId>liquibase-maven-plugin</artifactId>
        <version>3.8.9</version>
        <configuration>
          <dropFirst>true</dropFirst>
          <verbose>true</verbose>
          <driver>com.microsoft.sqlserver.jdbc.SQLServerDriver</driver>
          <url>jdbc:sqlserver://${database.host}:${database.port}</url>
          <username>${database.username}</username>
          <password>${database.password}</password>
          <changeLogFile>changelog-master.xml</changeLogFile>
        </configuration>
      </plugin>
    </plugins>
  </build>
</project>

• And the following changelog-master.xml under src/main/resources

<?xml version="1.0" encoding="UTF-8"?>

<databaseChangeLog
        xmlns="http://www.liquibase.org/xml/ns/dbchangelog"
        xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
        xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog
        http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-3.8.xsd">

    <changeSet author="jdhmoreno" id="testProblemSqlServer">

        <createTable remarks="Contains the column with a DATE" tableName="TABLE_WITH_DATE_COLUMN">
            <column name="ID" type="bigint">
                <constraints primaryKey="true" primaryKeyName="PK_TABLE"/>
            </column>
            <column name="DATE_COLUMN" remarks="The date column" type="DATETIME2"/>
        </createTable>

        <loadData relativeToChangelogFile="true" file="test_date.csv" tableName="TABLE_WITH_DATE_COLUMN" usePreparedStatements="false">
            <column header="ID" name="ID" type="NUMERIC"/>
            <column header="DATE_COLUMN" name="DATE_COLUMN" type="DATETIME2"/>
        </loadData>

    </changeSet>
    
</databaseChangeLog>

• And the following file test_date.csv under src/main/resources

{NOFORMAT}
"ID","DATE_COLUMN"
"1","NULL"
"2","NULL"
"3","NULL"
{NOFORMAT}

• When I run the following maven command

mvn clean resources:resources liquibase:update 

• Then I got the following error:

{NOFORMAT}
[ERROR] Failed to execute goal org.liquibase:liquibase-maven-plugin:3.8.9:update (default-cli) on project test.database.problem:
[ERROR] Error setting up or running Liquibase:
[ERROR] Migration failed for change set liquibase/changelog-master.xml::testProblemSqlServer::jdhmoreno:
[ERROR] Reason: liquibase.exception.DatabaseException: java.sql.BatchUpdateException: Implicit conversion from data type varbinary to datetime2 is not allowed. Use the CONVERT function to run this query.
[ERROR] -> [Help 1]
{NOFORMAT}

Expected/Desired Behavior

The idea would be that, when usePreparedStatements="false" is set in the loadData element, no PreparedStatements are used.

Proposed solution / idea

A solution might be to have the following condition in the class 'LoadDataChange', line 455

                if
                (
                    (needsPreparedStatement &&
                        (databaseSupportsBatchUpdates &&
                                !(ExecutorService.getInstance().getExecutor(database) instanceof LoggingExecutor)
                        )
                    )
                    && hasPreparedStatementsImplemented()
                )

This way, needsPreparedStatement (which in turns depends on the 'usePreparedStatement' attribute) will drive the creation of PreparedStatements or InsertStatements.
This should help solving the 'date' issue shown above, leaving to the developer the option of enabling them or not.

Notes

A more detailed look on why SQLServer 2012 behaves that way when a SQL NULL is sent for a DATE column can be found in the following StackOverflow thread:
https://stackoverflow.com/questions/43935316/error-inserting-null-or-empty-value-in-date-column-mapped-to-localdate-type-in-j

Liquibase Internal QA Requirements

0. Reproduce the bug using Liquibase 3.8.9
1. Manually validate the fix using the reproduction steps in the ticket description.

• MVN
• CLI

2. When passing manually, add a Liquibase functional regression test.

• Configure test to execute only on SQL Server.
  • MVN
  • CLI

Note to Community: The functional Liquibase tests run on our internal build system and are not visible to the broader community.

┆Issue is synchronized with this Jira Bug by Unito
┆fixVersions: Community 4.x,Liquibase 4.3.2

[ro-rah]

@jdhmoreno! That's a fast turn-around, you're awesome!

Now the canned stuff:
Thanks for your pull request!

Here’s what happens next:

A member of the Liquibase team will take a look at your contribution and may suggest
changes, additional tests, or provide other feedback. The PR will be prioritized
according to our internal development and testing capacity.

We’ll let you know when it’s ready to move to the next step or if any changes are needed.

[ro-rah]

Hey @jdhmoreno,

I'm trying to size this merge request for the internal Liquibase team, and I think I can size it smaller (and therefore get the change in faster) if we have unit tests. I would write it myself but I'm still a newbie myself.

Here is our documentation and example for writing unit test.

For this PR that means we would create a matching unit test file here:
liquibase/liquibase-core/src/test/java/liquibase/change/LoadDataChangeTest.java

Up for this? :) Let me know. That would make it a RiskLow in my mind, otherwise I would peg it with RiskMedium, which would have to wait till at least 1-2 more sprints.

Thanks,

Ronak

[jdhmoreno]

hi @ro-rah

yeah, no prob, need to scrape some time to do them.

sadly I've to the following error (attached) when running the tests (haven't added any tests yet, just got the error running mvn test).

looks strange, though. I could successfully run the tests early this week on a fork of the 3.8.9 tag.
now I'm writing on top of master/HEAD

Some context jdk/mvn --version:

openjdk 11.0.6 2020-01-14
OpenJDK Runtime Environment (build 11.0.6+10-post-Ubuntu-1ubuntu118.04.1)
OpenJDK 64-Bit Server VM (build 11.0.6+10-post-Ubuntu-1ubuntu118.04.1, mixed mode, sharing) 

Apache Maven 3.6.3 (cecedd343002696d0abb50b32b541b8a6ba2883f)
Maven home: <BLABLA ;-)>
Java version: 11.0.6, vendor: Ubuntu, runtime: /usr/lib/jvm/java-11-openjdk-amd64
Default locale: en, platform encoding: UTF-8
OS name: "linux", version: "4.4.0-18362-microsoft", arch: "amd64", family: "unix"

Got the errors attached, any ideas?
Otherwise I'll have a look later on.

2020-04-17T18-13-22_867.dumpstream.txt
2020-04-17T18-13-22_867-jvmRun1.dumpstream.txt

[ro-rah]

Hey @jdhmoreno ,

Okay, bear with me here, looks like jococo is choking, maybe it does not like OpenJDK 11?
jacoco/jacoco#663

Although that is pretty old issue...and it looks like they added support for Java 11 & 12, just not sure if it is for OpenJDK. I'm bringing this issue up internally too.

Thanks,

Ronak

[jdhmoreno]

Hey @ro-rah,

Something's fishy with openjdk. it cannot compile on any of the ones I've tried (from version 9 to 14)

In any case, with an Oracle JDK 8 did work, so I kept working on the unit tests.

I've added a unit test that checks what's happening when the flag 'usePreparedStatements' is set to FALSE (defaults to TRUE) but I'm unsure about what's the difference between a new MSSQLDatabase object and MockDatabase.

The behaviour to test has to do with the database's ability to 'supportsBatchUpdates', which in both cases is always 'false' --> due to the creation of an 'offline' connection, whereas in a real MSSqlserver instance, that will be 'true'.

I guess we should somehow mock it?

In the other hand, all the regression tests pass successfully.

[codecov]

Codecov Report

Merging #1098 into master will increase coverage by 0.00%.
The diff coverage is 40.00%.

@@            Coverage Diff            @@
##             master    #1098   +/-   ##
=========================================
  Coverage     47.71%   47.72%           
- Complexity     7477     7479    +2     
=========================================
  Files           757      757           
  Lines         36278    36278           
  Branches       6624     6624           
=========================================
+ Hits          17311    17312    +1     
+ Misses        16660    16659    -1     
  Partials       2307     2307           

Impacted Files	Coverage Δ	Complexity Δ
...ain/java/liquibase/change/core/LoadDataChange.java	53.03% <40.00%> (+0.30%)	69.00 <0.00> (+2.00)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update baee65d...49026d9. Read the comment docs.

[ro-rah]

This is great! Let me see if we can take it as is. I wanted to point out that our Travis build results are viewable down below, and you can see our output of running the tests. Just info for any future PRs you may submit.

Thanks for sticking with it and getting the test to work with Open JDK 8, that saves a ton of time for us isolating the issue, so I also thank you on that account.

I'll keep ya posted!

[ro-rah]

closes #1091

[ro-rah]

labeled this RiskLow as this is currently broken, this PR aims to fix.

[liquibot]

➤ Erzsebet Carmean commented:

@jdhmoreno, wow! Thank you for the excellent repro steps! I'm in QA and love well-documented bugs!