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

Do not statically reference a Connection from GenerationTool #2350

Closed
ben-manes opened this issue Mar 21, 2013 · 4 comments
Closed

Do not statically reference a Connection from GenerationTool #2350

ben-manes opened this issue Mar 21, 2013 · 4 comments

Comments

@ben-manes
Copy link
Contributor

In #2283, the connection was made static when we were investigating how to integrate with Gradle. The issues encountered are not longer problems and now our workarounds are. Please remove the ability to specify the connection as a static, as its unused by existing plugins.

This is causing a problem in a multi-project Gradle build, where the code generation is performed as a build step (see gist). The schema is created by applying Flyway migrations to a persisted H2, e.g. under ${projectDir}/build/db/flyway, and then the GenerationTool is run against it. For different sub-projects, a different database file should be connected to so that only that module's tables are generated.

The static connection results in the wrong behavior, as it may either try to use a closed connection or, due to a race, generate the wrong schema. A plugin or tool fix of null'ing out the connection wouldn't work if Gradle's parallel building is enabled. The short-term workaround for early migrations to jOOQ is that a subsequent build succeeds, as an incremental build skips up-to-date tasks.

Unfortunately this is a blocking issue of introducing jOOQ as it would not pass a CI build.

@ben-manes
Copy link
Contributor Author

Forced the dependency back to RC1 to workaround this issue. That unblocks us for now :)

@lukaseder
Copy link
Member

I like the fact that the Connection and ClassLoader instances can be configured now. But it's true, making things static is a bit short-sighted.

Let's review some requirements:

  • GenerationTool.main(String[]) is needed for standalone code generation calls, e.g. from the shell
  • GenerationTool.main(Configuration) is needed by the Maven plugin (although this doesn't have to be static)

So what about internally creating a GenerationTool instance with connection and loader fields? Then, parallel builds wouldn't get in each other's way

@ben-manes
Copy link
Contributor Author

hmm, I swore I replied to this...

The plugin's task does not use either the Connection or ClassLoader setters. It uses the GenerationTool.main(Configuration) after parsing with GenerationTool.load(InputStream). The problems I encountered appeared to only occur for an ad hoc task, where I had thought it would be easier to quickly experiment and flush out a working proof-of-concept.

I like the instance solution. I tend to have main methods that perform the minimal bootstrap and delegating to instance world (e.g. create a DI, fetch the configured Jetty instance, start server). A GenerationTool with either setters or a bulder and a some run method sounds like a clean approach.

@lukaseder
Copy link
Member

Thanks for the feedback. I've changed things to be able to use a GenerationTool instance, which can be configured with a ClassLoader and / or a Connection, without modifying static references. This will probably solve #2330 as well. Feel free to reopen this issue, if it doesn't work for you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants