Type detection seemingly order-dependent in PreparedBatch

[jchambers]

We've run into a curious problem where we'll get a BatchUpdateException ("column [X] is of type uuid but expression is of type character varying") if we attempt to execute a prepared batch against Postgres with a non-null UUID in one row and a null UUID in the next row. Here's a test case:

package com.example;

import com.opentable.db.postgres.junit.EmbeddedPostgresRules;
import com.opentable.db.postgres.junit.SingleInstancePostgresRule;
import org.jdbi.v3.core.Jdbi;
import org.jdbi.v3.core.statement.PreparedBatch;
import org.junit.After;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;

import java.util.UUID;

public class NullUuidBatchTest {

    @Rule
    public SingleInstancePostgresRule database = EmbeddedPostgresRules.singleInstance();

    private Jdbi jdbi;

    @Before
    public void setUp() {
        jdbi = Jdbi.create(database.getEmbeddedPostgres().getPostgresDatabase());

        jdbi.useHandle(handle -> {
            handle.execute("CREATE TABLE test (id SERIAL PRIMARY KEY, uuid UUID)");
        });
    }

    @After
    public void tearDown() {
        jdbi.useHandle(handle -> {
            handle.execute("DROP TABLE test");
        });
    }

    @Test
    public void testInsertNullUuid() {
        jdbi.useHandle(handle -> {
            final PreparedBatch batch = handle.prepareBatch("INSERT INTO test (uuid) VALUES (:uuid)");

            batch.bind("uuid", (UUID)null);
            batch.add();

            batch.execute();
        });
    }

    @Test
    public void testInsertNonNullUuid() {
        jdbi.useHandle(handle -> {
            final PreparedBatch batch = handle.prepareBatch("INSERT INTO test (uuid) VALUES (:uuid)");

            batch.bind("uuid", UUID.randomUUID());
            batch.add();

            batch.execute();
        });
    }

    @Test
    public void testInsertNullFirst() {
        jdbi.useHandle(handle -> {
            final PreparedBatch batch = handle.prepareBatch("INSERT INTO test (uuid) VALUES (:uuid)");

            batch.bind("uuid", (UUID)null);
            batch.add();

            batch.bind("uuid", UUID.randomUUID());
            batch.add();

            batch.execute();
        });
    }

    @Test
    public void testInsertNonNullFirst() {
        jdbi.useHandle(handle -> {
            final PreparedBatch batch = handle.prepareBatch("INSERT INTO test (uuid) VALUES (:uuid)");

            batch.bind("uuid", UUID.randomUUID());
            batch.add();

            batch.bind("uuid", (UUID)null);
            batch.add();

            batch.execute();
        });
    }
}

Three of the tests pass, but testInsertNonNullFirst fails with an exception. Here's the pom, for your convenience:

<?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>com.example</groupId>
    <artifactId>jdbi-null-uuid-demo</artifactId>
    <version>1.0-SNAPSHOT</version>

    <build>
        <plugins>
            <plugin>
                <groupId>org.apache.maven.plugins</groupId>
                <artifactId>maven-compiler-plugin</artifactId>
                <configuration>
                    <source>8</source>
                    <target>8</target>
                </configuration>
            </plugin>
        </plugins>
    </build>

    <dependencyManagement>
        <dependencies>
            <dependency>
                <groupId>org.jdbi</groupId>
                <artifactId>jdbi3-bom</artifactId>
                <type>pom</type>
                <version>3.14.4</version>
                <scope>import</scope>
            </dependency>
        </dependencies>
    </dependencyManagement>

    <dependencies>
        <dependency>
            <groupId>org.jdbi</groupId>
            <artifactId>jdbi3-core</artifactId>
        </dependency>

        <dependency>
            <groupId>junit</groupId>
            <artifactId>junit</artifactId>
            <version>4.13</version>
            <scope>test</scope>
        </dependency>

        <dependency>
            <groupId>com.opentable.components</groupId>
            <artifactId>otj-pg-embedded</artifactId>
            <version>0.13.3</version>
            <scope>test</scope>
        </dependency>
    </dependencies>
</project>

I'll plan on digging in and seeing if I can provide a more helpful analysis shortly.

[stevenschlansker]

Does the problem persist if you install the PostgresPlugin? It's not installed automatically by the rule, so you can test without it when need be.

[jchambers]

Installing PostgresPlugin does appear to resolve the issue.

At the same time, it still feels a little buggy that a thing that works in isolation (setting a null UUID) can stop working depending on what else is happening in a batch. I'd think "install the PostgresPlugin" would be a full resolution to this issue if all of the tests that insert a null UUID were failing, but that's not the case and I'm concerned that this points to an underlying issue elsewhere.

Thanks for the fast reply!

[stevenschlansker]

I agree, it's an odd behavior. JDBC drivers have different requirements around null handling - in particular, Postgres is much pickier about declared types on null values. We can keep this issue open to investigate if it's feasible to fix the base behavior, but if it's not straightforward - we do generally recommend installing the vendor plugin like PostgresPlugin since sometimes different vendors do have truly incompatible needs.

[stevenschlansker]

Ah, I am not so sure we can fix this as stated originally.
In particular:

batch.bind("uuid", (UUID)null);

This will lose all information about the UUID type -- nulls do not carry dynamic casting information along.

You could instead write:

batch.bindByType("uuid", null, UUID.class)

and that has some hope of working. If not, we can consider it a bug.

[jon-signal]

bindByType does, indeed, resolve the problem.

I'll certainly defer to your expertise, but it still seems very strange to me that this works:

batch.bind("uuid", (UUID)null);
...
batch.bind("uuid", UUID.randomUUID());

...but this doesn't:

batch.bind("uuid", UUID.randomUUID());
...
batch.bind("uuid", (UUID)null);

If anything (and knowing nothing about the internals), I would have expected exactly the opposite (e.g. that in the first case, the batch doesn't have enough information to infer the type of the "uuid" column when binding null). Still, if you're convinced that this is as it should be, that's cool with me.

Thanks!

[stevenschlansker]

@jon-signal I agree with your assessment that the behavior in this case isn't exactly intuitive. It's accidental and unfortunately it's long-standing, and probably not something we can fix in all cases.

The default UUID binding strategy in Jdbi core (bind as varchar) works for most databases (say, H2, Mysql, sqlite). Postgres has stronger typing, and will not implicitly cast from an explicitly declared varchar -- which means you need to add a cast to uuid in your SQL, which sucks.

To avoid this, in the PostgresPlugin we override the behavior of binding UUIDs to use jdbcStatement.setObject(uuid, Types.OTHER). This doesn't (or at least didn't) work on any of the above databases, but solves the problem in Postgres -- the driver understands the UUID type directly, null or not.

In prepared batches, one way we achieve performance gain is by relying on the type of the bound arguments not to change in an incompatible way from row to row.

So in your working case:

batch.bind("uuid", (UUID)null);
...
batch.bind("uuid", UUID.randomUUID());

the type of the first argument as far as Jdbi can see is (Object) null (The cast is lost because Java.) Jdbi goes ??? and ends up falling back to default null behavior, setObject(null, Types.OTHER). Note that this is coincidentally the same call that the Postgres UUID null binding would make, were PostgresPlugin installed - and so this works! And later rows also end up working, as the types are fixed by the first row - and varchar and uuid sent as Types.OTHER look exactly the same to the wire protocol.

In the non-working case, the type of the first argument is UUID as you expect. Core UUID behavior is to bind as a varchar since that works on "most" databases. But postgres fails with a type error pretty early on.

One thing we can do is to add a bind(String, UUID) overload to the SqlStatement interface. This will fix the issue going forward for this case specifically once you recompile. However, we can't add bind() overloads for everything - in particular user types - so the caveat still applies generally, in Postgres, null is typed and the type of the first row in prepared batches matters. I opened #1768 for this.

The general fixes:

• Use SqlObject and we can inspect the method declaration to know the type even if null is passed in at runtime
• bindByType tries to do something sensible (but in a vendor-specific way, so it's important to have the plugin installed) even when the value is null
• You can always add casts to uuid in your SQL and that works regardless of the vendor plugin

If we went back in time, we might not have added the core UUID strategy since it's incompatible with Postgres. But it'd be rude to remove it now, it's been there for quite a while.

Let me know if you think of a good way to make the situation better.
If nothing else, we could add a little explanation to the docs.

[jchambers]

Understood. This is a way more thoughtful explanation and fix than I think I earned by gesturing vaguely in the direction of a the problem, but thank you!

Totally understand the constraints on the problem space, and I think the changes in #1768 make a ton of sense. Thank you for addressing this! I'm more than happy with the explanation and updates, and certainly consider this issue resolved for my part (but don't want to be presumptuous and close it if you think there's still more to do).

[stevenschlansker]

Let's have this issue be to add some kind of explanation to the docs :)

[hgschmie]

Closing, still tracked in #2390.