Address #511 Support for import.sql and related property in JPA with no persistence.xml #538
Conversation
| * specify the location of a load script. | ||
| * The location specified in this property is relative to the root of the persistence unit. | ||
| */ | ||
| @ConfigProperty(name="sql-load-script-source") |
There was a problem hiding this comment.
I don't think we can have some properties with - and others with _ (see show_sql just above). We must be consistent in what casing we use. Also, why not shorter such as importFile (or import-file or import_file depending on what style we standardise on)?
There was a problem hiding this comment.
Same remark here about the convention. I checked the existing code and for now it's a big mix of dash or camel case. We need to decide about this and stick to it.
There was a problem hiding this comment.
@Sanne those would work for me.
| String file = hibernateOrmConfig | ||
| .flatMap(c -> c.sqlLoadScriptSource) | ||
| .orElse("/import.sql"); //default Hibernate ORM file imported | ||
| desc.getProperties().setProperty(AvailableSettings.HBM2DDL_LOAD_SCRIPT_SOURCE, file); |
There was a problem hiding this comment.
Can we check for the file's existence and not set the property if it does not exist? This would avoid the warning, no?
There was a problem hiding this comment.
That would have been my intention too but I'm wondering if you have enough context to load the file properly?
There was a problem hiding this comment.
+1 we need to check for its existance, as we'll also need to register the resource for being included in the native binary - or don't register it if there's no file.
Currently that's done in a different block, IMO we should check once and make good use of it.
Also the warning about the file not existing should be triggered here, not at "final boot"
There was a problem hiding this comment.
Shouldn't it be:
if (loadScriptPath.isPresent()) {
desc.getProperties().setProperty(AvailableSettings.HBM2DDL_LOAD_SCRIPT_SOURCE, file);
}
?
I think that's why you still have the Hibernate warning. I can fix it myself and merge, just wanted to check you didn't have a good reason to not do that?
There was a problem hiding this comment.
Ah, fixed. Thanks Guillaume. I pushed a new version.
|
I've pushed a new version that
We still have the following pending problems and questions to address
|
|
Just opened #550 to fix the config name issue before it becomes evident we're just making shit up and throwing it together ;) |
|
Toning down the WARN: +1 to defer additional tests I'd go for custom (shorter?) property names rather than the JPA ones, so to give us some flexibility on their exact semantics without confusing the users about them. About "decide for /import.sql vs META-INF/import.sql" .. my instinct is that in this case it doesn't belong in META-INF but I should find some proper guidance about such stuff. I'll send an email to the mailing list about finding some consistency on that. |
Alternative file name can be set with shamrock.hibernate.sql-load-script-source
|
So the WARN is gone after the latest changes. I still have this one though, when there's no script to run (and none configured):
I'll get rid of it, since we're on the topic.. |
|
Actually in native I get both following lines !?
|
|
Pushed changes to rest-crud demo: https://github.com/jbossas/protean-rest-http-crud-demo/commit/3bf29dd7ccf68a63e84ffdcc71fbb76aa0ea3806 |
|
Merged this one. Thanks! |
Code is OK I think.
Doc is done.
There is an annoying warning raised by Hibernate when no
import.sqlis present.We would need #406 to address that.
Test is partial: A proper testing strategy can be done once #534 is fixed.
Should we merge and add a follow up?