-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Unzip zip files in hibernate HBM2DDL_IMPORT_FILES setting #47338
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
base: main
Are you sure you want to change the base?
Unzip zip files in hibernate HBM2DDL_IMPORT_FILES setting #47338
Conversation
Thanks for your pull request! Your pull request does not follow our editorial rules. Could you have a look?
This message is automatically generated by a bot. |
/cc @gsmet (hibernate-orm) |
🎊 PR Preview acaf40a has been successfully built and deployed to https://quarkus-pr-main-47338-preview.surge.sh/version/main/guides/
|
bdb6042
to
a1815aa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I have a doubt about how files get loaded from the filesystem and gave a pointer for updating the documentation. Otherwise the implementation/tests LGTM.
I think the CI failure in your fork (https://github.com/alihmzyv/quarkus/actions/runs/14433305195) is transient and can be ignored.
if (name.startsWith("file://")) { | ||
try { | ||
var file = new File(name.substring(7)); | ||
log.tracef("Successfully loaded resource '%s' from file", name); | ||
return file.toURI().toURL(); | ||
} catch (MalformedURLException e) { | ||
log.errorf("Error while trying to load resource %s from file", name); | ||
throw new IllegalArgumentException(e); | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this was added in order to support loading SQL files from the filesystem, because it didn't work before?
Is the class loader service really the right place for loading resources from the filesystem? I mean it's fine if it's how Hibernate ORM does this outside of Quarkus, but otherwise, I'd avoid it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have any other suggestion? Cause as I found there are ugly ways of adding files to classpath at runtime, so I can add unzipped files to the classpath when unzipping instead of creating a temp dir.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point is hibernate orm is, I think, already able to load these scripts from the filesystem, so I wouldn't have expected you to need this kind of change.
If you just copied the original code from hibernate orm, it's fine. Otherwise there's something fishy going on.
I am currently away from my computer but should be back around the end of next week. Sorry for the delay, but if you don't figure it out by then, I will have a closer look. Or maybe @lucamolteni will know 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it is not from hibernate. Hibernate calls this locateResource method and therefore only able to load the files from the classpath, but since the unzipped files are not in classpath, this code snippet is added to load them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for letting me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @yrodiere , i think i got your point.
Hibernate using some simple mechanism to get file either from classpath or file: https://github.com/hibernate/hibernate-orm/blob/018b8eeda3627e114ec25bd48407ccb9c47564ce/hibernate-core/src/main/java/org/hibernate/tool/schema/internal/Helper.java#L97
I will explore, how we can reuse or implement something similar, instead of modifying classloader
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @yrodiere
I removed change from classloader, and now it used underlying hibernate reading implementation - yes, he able to read from file. But for this I was need to do one (not beatiful) change, let me explain why.
This is the change i have to made:
runtimeSettingsBuilder.put(AvailableSettings.HBM2DDL_IMPORT_FILES, null); runtimeSettingsBuilder.put(AvailableSettings.JAKARTA_HBM2DDL_LOAD_SCRIPT_SOURCE, newValue);
The thing is - hibernate reading scripts from both files and classpath only, when processing JAKARTA_HBM2DDL_LOAD_SCRIPT_SOURCE setting. When it processing deprecated HBM2DDL_IMPORT_FILES setting, is reading from classpath only. I not sure why it working like this, but this is proof in the hibernate sources:
- this is where logic of processing of input sources happening:
hibernate#applyImportScript method
inside 2 methods called:applyImportScript
andapplyImportFiles
- inside
applyImportScript
calling methodgetImportScriptSetting
, which is working with JAKARTA_HBM2DDL_LOAD_SCRIPT_SOURCE - property, which is not used in quarkus. Also, usage of this property is actually contradicts with caller method documentation in hibernate - by documentation, it should read HBM2DDL_IMPORT_FILES property );
So this methodapplyImportScript
is reading from both files and classpath (regardless name) - second method which called is
applyImportFiles
This method inside calling methodinterpretLegacyImportScriptSetting
, which is reading only from classpath; this method is reading from HBM2DDL_IMPORT_FILES property (which is used by quarkus).
So by current implementation of input script passing from quarkus to hibernate, it will not read from file.
My conclusion:
inside hibernate there is possible error in methods naming and documentation:
- method
applyImportFiles
is supposed to process legacy setting (HBM2DDL_IMPORT_FILES), supposed to have nameapplyImportSources
- even inside this method there is a call of legacy method: interpretLegacyImportScriptSetting - method
applyImportScript
supposed to be namedapplyImportFiles
; documentation to this method is also misleading - instead of HBM2DDL_IMPORT_FILES is processing JAKARTA_HBM2DDL_LOAD_SCRIPT_SOURCE setting.
@yrodiere what do you think? Currently what i've done - i using new JAKARTA_HBM2DDL_LOAD_SCRIPT_SOURCE
setting instead of deprecated one;
Actually more proper way is to change it in upstream (inside HibernateOrmProcessor
itself) - but i a bit affraid to take this call, I still new to quarkus and hibernate. Will appreciate any inputs and feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into this.
Actually more proper way is to change it in upstream (inside HibernateOrmProcessor itself) - but i a bit affraid to take this call, I still new to quarkus and hibernate. Will appreciate any inputs and feedback.
I agree there is an inconsistency in AbstractSchemaPopulator#applyImportSources
.
I'm not too worried about the inconsistency and documentation error themselves, but I suspect the inconsistency would lead to duplicate script execution if you were to, say, set HBM2DDL_IMPORT_FILES
to import1.sql
? Then both applyImportScript
and applyImportFiles
would apply the script.
I wouldn't go as far as submitting a PR upstream (yet), but it would already be a great help if you could investigate if there actually is misbehavior that affects application developers (like the one I imagined just above), and if so report the issue at https://hibernate.atlassian.net/browse/HHH with a reproducer (using the test case templates) . Then we can discuss on that report what to do about it.
what do you think? Currently what i've done - i using new JAKARTA_HBM2DDL_LOAD_SCRIPT_SOURCE setting instead of deprecated one;
I think switching to the Jakarta property is a good idea, but you should do that everywhere, not just in the specific code you changed. In particular I'd expect this code to be updated, and consequently I'd expect your input here to come from JAKARTA_HBM2DDL_LOAD_SCRIPT_SOURCE
instead of HBM2DDL_IMPORT_FILES
.
There are other places to be updated as well, like this one or that one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @yrodiere , this is very helpful comment for me.
Sure, ill migrate all usage of this deprecated property in quarkus to new property; also, I will do investigation in hibernate
@@ -739,6 +739,7 @@ your entity changes or any change to your `import.sql` is immediately picked up | |||
==== | |||
By default, in `dev` and `test` modes, Hibernate ORM, upon boot, will read and execute the SQL statements in the `/import.sql` file (if present). | |||
You can change the file name by changing the property `quarkus.hibernate-orm.sql-load-script` in `application.properties`. | |||
You can also provide a `.zip` file in the same way which should contain only the files containing the sql statements to be executed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should also update the documentation of the property itself; see:
Lines 48 to 85 in bc3fe92
/** | |
* Paths to files containing the SQL statements to execute when Hibernate ORM starts. | |
* | |
* The files are retrieved from the classpath resources, | |
* so they must be located in the resources directory (e.g. `src/main/resources`). | |
* | |
* The default value for this setting differs depending on the Quarkus launch mode: | |
* | |
* * In dev and test modes, it defaults to `import.sql`. | |
* Simply add an `import.sql` file in the root of your resources directory | |
* and it will be picked up without having to set this property. | |
* Pass `no-file` to force Hibernate ORM to ignore the SQL import file. | |
* * In production mode, it defaults to `no-file`. | |
* It means Hibernate ORM won't try to execute any SQL import file by default. | |
* Pass an explicit value to force Hibernate ORM to execute the SQL import file. | |
* | |
* If you need different SQL statements between dev mode, test (`@QuarkusTest`) and in production, use Quarkus | |
* https://quarkus.io/guides/config#configuration-profiles[configuration profiles facility]. | |
* | |
* [source,property] | |
* .application.properties | |
* ---- | |
* %dev.quarkus.hibernate-orm.sql-load-script = import-dev.sql | |
* %test.quarkus.hibernate-orm.sql-load-script = import-test.sql | |
* %prod.quarkus.hibernate-orm.sql-load-script = no-file | |
* ---- | |
* | |
* [NOTE] | |
* ==== | |
* Quarkus supports `.sql` file with SQL statements or comments spread over multiple lines. | |
* Each SQL statement must be terminated by a semicolon. | |
* ==== | |
* | |
* @asciidoclet | |
*/ | |
// @formatter:on | |
@ConfigDocDefault("import.sql in dev and test modes ; no-file otherwise") | |
Optional<List<@WithConverter(TrimmedStringConverter.class) String>> sqlLoadScript(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
…g zip files as import files. unzip zip files in hibernate HBM2DDL_IMPORT_FILES setting.
a1815aa
to
57d9afa
Compare
Status for workflow
|
Status for workflow
|
Hello @alihmzyv , could you please give me access to push to this repo? I made a small changes |
Hey @kto-viktor. You'd better push to your own fork, and send a new PR from there. People don't usually give push access to their fork to others :) |
Hi @kto-viktor. I added you as a collaborator. If you want you can accept it. I actually would be happy if my small contribution would be helpful as well). Thanks |
…bm2ddl.import_files; remove zip file reading from FlatClassLoaderService;
Fixes #46789