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

Meta.ddl() generates broken DDL for columns of unknown types #15303

Closed
mgramin opened this issue Jun 29, 2023 · 9 comments
Closed

Meta.ddl() generates broken DDL for columns of unknown types #15303

mgramin opened this issue Jun 29, 2023 · 9 comments

Comments

@mgramin
Copy link

mgramin commented Jun 29, 2023

Expected behavior

We expect that Meta.ddl() exports a valid DDL for a table with tsvector columns:

CREATE TABLE public.film (
    fulltext tsvector
);

Actual behavior

Meta.ddl() exports broken DDL for that tables:

create table "public"."film" (
  "fulltext" any
)

Steps to reproduce the problem

Create a table in DB:

CREATE TABLE public.film (
    fulltext tsvector
);

Run this code:

        Connection conn = DriverManager.getConnection("jdbc:postgresql://localhost:6000/postgres", "postgres", "postgres");
        Configuration configuration = new DefaultConfiguration().set(conn).set(SQLDialect.POSTGRES);
        Meta meta = using(configuration).meta();

        Arrays.stream(meta.filterSchemas(v -> v.getName().equalsIgnoreCase("public"))
                        .ddl()
                        .queries())
                .map(Object::toString)
                .forEach(System.out::println);

Executing the resulting query against a different database or schema:

create table "public"."film" (
  "fulltext" any
)

leads to the error:

SQL Error [42601]: ERROR: syntax error at or near "any"

jOOQ Version

jOOQ Professional Edition 3.18.4

Database product and version

PostgreSQL 15.2 (Debian 15.2-1.pgdg110+1) on x86_64-pc-linux-gnu, compiled by gcc (Debian 10.2.1-6) 10.2.1 20210110, 64-bit

Java Version

openjdk 17.0.2 2022-01-18

OS Version

Microsoft Windows [Version 10.0.19044.2846]

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

org.postgresql:postgresql:42.6.0

@lukaseder
Copy link
Member

Thanks for your report. We currently don't support that vendor specific type. While it would be possible to support it:

A more likely solution to be implemented soon is this data type registry for custom data types and translations:

There's no short term solution to this right now. As a workaround / hack, you can create "support" for this type by instantiating your own DefaultDataType (which will then register itself in the internal data type registry, making it available for lookups). Knowing that this is a hack, relying on internals.

Closing this as a duplicate of #2938, #5713

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

mgramin commented Jun 30, 2023

Thank you for your answer.

Got it. But could we simply save the name of the type to keep the exported DDL valid?

If I am not mistaken, something similar has been done with the DDL of complex views - #15236

@lukaseder
Copy link
Member

lukaseder commented Jun 30, 2023

I see what you mean. It's not the same thing as #15236, where we don't parse an entire view if there's a single problem with its contents, but we still try to get column and data type information separately.

Here, we would simply maintain the data type obtained from JDBC, whatever that is. Please note that it might not be reliable at all, i.e. a tsvector[] might be reported as _tsvector by PostgreSQL for example. You'll be relying on something that jOOQ won't integration test for you, so "regressions" aren't unlikely.

@lukaseder
Copy link
Member

Hmm, never mind. The _type underscore seems to be accepted by PostgreSQL as an alias for type[]!

This works:

create table x (i _point);
insert into x values (array['(1, 1)'::point])

@lukaseder lukaseder changed the title Meta.ddl() generates broken DDL for tsvector columns Meta.ddl() generates broken DDL for columns of unknown types Jun 30, 2023
@lukaseder
Copy link
Member

I just tried this test:

@Test
public void testPostgresMetaVendorSpecificTypes() throws Exception {
    try {

        // [#15303] The ParserCoverageListener parses all rendered SQL, thus registering the
        //          point data type as a new DefaultDataType, making the test pass for the
        //          wrong reasons.
        System.setProperty("org.jooq.test.parser-coverage-listener", "false");

        create().execute("create table t (c point)");
        create().execute("insert into t values ('(1, 1)')");
        assertEquals("(1.0,1.0)", create().fetchValue("select c from t").toString());

        Queries queries =
        create().meta()
                .filterSchemas(s -> s.getName().equals(schema().getName()))
                .filterTables(t -> t.getName().equals("t"))
                .ddl(new DDLExportConfiguration().flags(DDLFlag.TABLE));
        create().execute("drop table t");
        println(queries).executeBatch();
        create().execute("insert into t values ('(1, 1)')");
        assertThrows(DataAccessException.class, () -> create().execute("insert into t values ('not a point')"));
    }
    finally {
        System.setProperty("org.jooq.test.parser-coverage-listener", "true");
        ignoreThrows(() -> create().execute("drop table t"));
    }
}

It kept passing, because we have some instrumentation that tries to increase the test coverage of the parser like this:

    static class ParserCoverageListener implements ExecuteListener {

        @Override
        public void renderEnd(ExecuteContext ctx) {
            if (!Boolean.parseBoolean(System.getProperty("org.jooq.test.pretty-printer", "true")))
                return;

            if (!Boolean.parseBoolean(System.getProperty("org.jooq.test.parser-coverage-listener", "true")))
                return;

            if (ctx.sql() != null) {
                try {
                    ctx.configuration()
                       .deriveSettings(s -> s.withParseDialect(ctx.dialect()))
                       .dsl()
                       .parser()
                       .parse(ctx.sql());
                }
                catch (Exception e) {
                    PARSE_EXCEPTIONS.putIfAbsent(ctx.sql(), e);
                }
            }
        };
    }

That made sure the test never failed, because the data type was parsed and instanciated with new DefaultDataType(...), as I suggested as a workaround.

So, again, as a workaround, you could just initialise all data types from pg_type in your application in a similar way. Of course, I'll still fix this, but until the fix ships, the workaround will do.

@lukaseder
Copy link
Member

With the fix, the above now generates:

create table "public"."t" (
  "c" point
);

I'm reluctant to backport this fix to patch releases. Users may have implemented assumptions based on the type being SQLDataType.OTHER and then implemented manual handling code in whatever form. Those assumptions would break if I backport a fix that will now return a vendor specific data type.

Given the workarounds, I think it's acceptable to release this only in jOOQ 3.19, as an "incompatible change"

@mgramin
Copy link
Author

mgramin commented Jun 30, 2023

It's great, thank you.

@lukaseder
Copy link
Member

Implemented in jOOQ 3.19.0

@lukaseder
Copy link
Member

Note that this won't maintain any lengths, precision, scales or other additional type attributes that jOOQ knows nothing about. For most of these vendor specific data types, that's probably OK

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