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

Java/jOOQ: Add code generation add-on for Gradle, with reflection from SQL DDL files #11

Merged
merged 2 commits into from
Jan 25, 2023

Conversation

amotl
Copy link
Member

@amotl amotl commented Jan 24, 2023

About

As a followup to GH-9, this patch adds the Gradle-based jOOQ code generation add-on. It can be invoked like:

./gradlew generateJooq

The style of code generation exercised here works by reflecting the database schema from existing SQL DDL files.

✅ Contrary to GH-10, this works well. ✅

Details

This time, the code within the src/generated folder has truly been generated using jOOQ 3.17.7.

@amotl amotl force-pushed the amo/jooq/codegen-sql branch 2 times, most recently from f715102 to 86a1483 Compare January 24, 2023 22:39
@mkleen
Copy link

mkleen commented Jan 25, 2023

This looks promising. I guess this only works without having any CrateDB-specific schema definitions since it has to pass the jOQL parser and will then be created in a H2 memory database.

@mkleen
Copy link

mkleen commented Jan 25, 2023

Can we then create the domain model based on the code generation from the SQL files ?

into a set of Java classes. This feature currently does not work with CrateDB
yet. The code provided within the ``src/generated`` directory has not been
derived by reflecting the database schema from CrateDB.
- `jOOQ's code generator`_ currently does not work with directly connecting to
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably mention that since we are using the psql jdbc driver and we are bound to JOOQ parser, we can only use sql-standard/psql based definitions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this only works without having any CrateDB-specific schema definitions since it has to pass the jOQL parser and will then be created in a H2 memory database.

I would probably mention that since we are using the psql jdbc driver and we are bound to JOOQ parser, we can only use sql-standard/psql based definitions.

You are right. Thanks!

11:06:01 SEVERE DDLDatabase Error        : Your SQL string could not be parsed or interpreted. This may have a variety of reasons, including:
- The jOOQ parser doesn't understand your SQL
- The jOOQ DDL simulation logic (translating to H2) cannot simulate your SQL

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've improved the "Caveats" section with 15ae4cd. Thank you again.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi again. I've diverted the documentation updates to GH-12 after the review.

@@ -17,13 +15,6 @@
/**
* This class is generated by jOOQ.
*/
@Generated(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having the code generated is ok as long as it's possible to do so out of the box.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the description stated, all these files are generated now. For whatever reason these comments don't get generated anymore.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, i did not know this.

@@ -40,7 +40,7 @@ public void testExampleWithDynamicSchema() throws SQLException, IOException {

// Check number of records.
DSLContext db = app.getDSLContext();
int count = db.fetchCount(DSL.selectFrom("testdrive.book"));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a few more advanced queries would be nice.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I've planned to exercise more advanced queries within a subsequent iteration.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The backlog now enumerates a number of items which may be addressed on behalf of later iterations.

Copy link
Member

@seut seut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool stuff, just some more minor suggestions.

Comment on lines +78 to +79
- We have not been able to make multiple SQL DDL statements work within a
single SQL bootstrap file at ``src/main/resources/bootstrap.sql``.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really a caveat? Isn't defining dedicated SQL files for each table even a cleaner way?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure. I think when using a framework managing your data migrations, there is usually one file per schema-change iteration, containing multiple ALTER TABLE statements. This caveat is here as a reminder that I discovered issues in this area, and that I would like to revisit the topic on a subsequent iteration.

Copy link
Member Author

@amotl amotl Jan 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See also #11 (review) below.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This statement has been removed. Apparently, it was based on a quirk on my side, and I have not been able to observe a problem in this area again.

by-language/java-jooq/jooq.gradle Outdated Show resolved Hide resolved
Comment on lines +93 to +91
property {
key = 'unqualifiedSchema'
value = 'none'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this confuses a bit as in the examples, no schema is used. I think this works as the testdrive is passed into the connection string as the database parameter which CrateDB then uses as the default schema name. Maybe rather use testdrive here and not inside the connection string and/or use the schema also in the application?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already tried to use a real value there, but the setting apparently only accepts public or none.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See also #12 (review).

@amotl amotl force-pushed the amo/jooq/codegen-sql branch from 86a1483 to 15ae4cd Compare January 25, 2023 10:19
@amotl amotl requested review from seut and mkleen January 25, 2023 10:20
Copy link
Member

@seut seut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link

@mkleen mkleen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice 👍

Base automatically changed from amo/jooq/baseline to main January 25, 2023 10:42
@amotl amotl force-pushed the amo/jooq/codegen-sql branch from 15ae4cd to ab9187e Compare January 25, 2023 10:42
This time, the code within the `src/generated` folder has truly been
generated using jOOQ 3.17.7.

- The code generator used for that is `DDLDatabase` [1].
- The relevant upstream documentation is [2].

[1] org.jooq.meta.extensions.ddl.DDLDatabase
[2] https://www.jooq.org/doc/latest/manual/code-generation/codegen-ddl/
@amotl amotl force-pushed the amo/jooq/codegen-sql branch from ab9187e to 6870718 Compare January 25, 2023 13:59
@amotl amotl merged commit 538e160 into main Jan 25, 2023
@amotl amotl deleted the amo/jooq/codegen-sql branch January 25, 2023 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants