Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

H2 dialect should cast binary data of unknown length as VARBINARY, not as BINARY #15195

Closed
htran1 opened this issue Jun 8, 2023 · 7 comments
Closed

Comments

@htran1
Copy link

htran1 commented Jun 8, 2023

Expected behavior

A HashMap serialized as bytes should be inserted as bytes.

Actual behavior

The data cannot be deserialized. Looks like the toString() of the byte array was inserted.

Steps to reproduce the problem

DDL:

create table if not exists test (
            id varchar(40) not null primary key,
            -- Map<String, Long>
            ts_map other);

ts_map column uses a converter.

new ForcedType().withUserType('java.util.Map<String, Long>').withConverter('test.h2.jooq.HashMapConverter').withIncludeExpression('(ts_map)')))

The converter to method serializes to a byte[] and the from method deserializes to a Map<String, Long>.

Code:

    public void testMerge() throws SQLException {
        Connection conn = DriverManager.getConnection("jdbc:h2:/tmp/h2test", "sa", "");
        DSLContext dslContext = DSL.using(conn, SQLDialect.H2);

        dslContext.dropTableIfExists(JTest.TEST).execute();
        dslContext.createTable(JTest.TEST).columns(JTest.TEST.fields()).execute();

        dslContext.execute("show tables;");

        JTestRecord record = new JTestRecord();
        record.attach(dslContext.configuration());
        record.setId("1");
        record.setTsMap(new HashMap<>());

// Insert works
        record.insert();
        JTestRecord record2 = dslContext.selectFrom(JTest.TEST).where(JTest.TEST.ID.eq("1")).fetchOne();

        JTestRecord record3 = new JTestRecord();
        record3.attach(dslContext.configuration());
        record3.setId("2");
        record3.setTsMap(new HashMap<>());

        // Is the failure because the merge is inserting the toString() of a byte array for the TS_MAP column?
//        select "ID"
//        from final table (
//                /* [#12925] a536d9c4-684c-46ca-8f00-95e738fe7807 */
//                merge into "TEST"
//                using (
//                        select
//        '2' "ID",
//                '[B@37e99c0f' "TS_MAP"
//  ) "t"
//        on "TEST"."ID" = "t"."ID"
//        when matched then update set
//        "TEST"."ID" = "t"."ID",
//                "TEST"."TS_MAP" = "t"."TS_MAP"
//        when not matched then insert ("ID", "TS_MAP")
//        values (
//                "t"."ID",
//                "t"."TS_MAP"
//        )
//) "TEST"
        record3.merge();

        // This fails due to a bad value for the ts_map column.
        JTestRecord record4 = dslContext.selectFrom(JTest.TEST).where(JTest.TEST.ID.eq("2")).fetchOne();
    }

jOOQ Version

3.18.4

Database product and version

2.1.212

Java Version

No response

OS Version

No response

JDBC driver name and version (include name if unofficial driver)

No response

@lukaseder
Copy link
Member

Thank you for your report. Can you post the code of your HashMapConverter, please?

3.19 Other improvements automation moved this from To do to Done Jun 9, 2023
@htran1
Copy link
Author

htran1 commented Jun 9, 2023

import org.apache.commons.lang3.SerializationUtils;
import org.jooq.impl.AbstractConverter;

import java.util.HashMap;
import java.util.Map;

public class HashMapConverter extends AbstractConverter<Object, Map<String, Long>> {
    public static final HashMapConverter INSTANCE = new HashMapConverter();

    @SuppressWarnings("unchecked")
    public HashMapConverter() {
        super(Object.class, (Class<Map<String, Long>>) (Class<?>) Map.class);
    }

    @Override
    public Object to(Map<String, Long> m) {
        return SerializationUtils.serialize((HashMap) m);
    }

    @Override
    public Map<String, Long> from(Object o) {
        if (o == null) {
            return null;
        }

        // If the object is already a map then return it, else deserialize from bytes.
        if (o instanceof Map) {
            return (Map<String, Long>) o;
        }

        return SerializationUtils.deserialize((byte[]) o);
    }
}

@htran1
Copy link
Author

htran1 commented Jun 12, 2023

@lukaseder, I posted the HashMapConverter above. How come this issue is closed? Is this user error? Can you advise how I can get the merge to bind the serialized object instead of the Object.toString() value?

@lukaseder
Copy link
Member

Yeah, wrong button, sorry. I'll look into it soon

@lukaseder lukaseder reopened this Jun 12, 2023
3.19 Other improvements automation moved this from Done to In progress Jun 12, 2023
@lukaseder
Copy link
Member

I can reproduce this issue. Regarding:

// Is the failure because the merge is inserting the toString() of a byte array for the TS_MAP column?

That toString() serialisation is misleading. It might be a problem if you ran a STATIC_STATEMENT, indeed. But the generated SQL in my reproducer is correct:

select "ID" from final table (/* [#12925] 6903578c-469f-4291-aa9b-f9334a791d45 */ 
  merge into "PUBLIC"."T_15195" 
  using (
    select 
      cast(? as int) "ID", 
      cast(? as binary) "MAP" -- This looks correct
    ) "t" on "PUBLIC"."T_15195"."ID" = "t"."ID" 
  when matched then update set 
    "PUBLIC"."T_15195"."ID" = "t"."ID",
    "PUBLIC"."T_15195"."MAP" = "t"."MAP" 
  when not matched then insert ("ID", "MAP") 
  values ("t"."ID", "t"."MAP")
) "T_15195"

The reason must be something else.

@lukaseder
Copy link
Member

The problem is likely the cast as binary in fact, rather than casting the bind value as varbinary. Changing that fixes the issue for me.

@lukaseder lukaseder changed the title Merge inserts malformed record for H2 with OTHER type H2 dialect should cast binary data of unknown length as VARBINARY, not as BINARY Jun 13, 2023
@lukaseder
Copy link
Member

Fixed in versions:

Note that in my opinion, a better approach would be to declare the column as VARBINARY instead of OTHER, and implement a Converter<byte[], Map<String, Long>>

3.19 Other improvements automation moved this from In progress to Done Jun 13, 2023
lukaseder added a commit that referenced this issue Jun 13, 2023
lukaseder added a commit that referenced this issue Jun 13, 2023
lukaseder added a commit that referenced this issue Jun 13, 2023
lukaseder added a commit that referenced this issue Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

2 participants