Multiple persistence units follow up #11519
Conversation
| if (capabilities.isMissing(Capability.TRANSACTIONS)) { | ||
| // JTA is necessary for blocking Hibernate ORM but not necessarily for Hibernate Reactive | ||
| if (capabilities.isMissing(Capability.TRANSACTIONS) | ||
| && capabilities.isMissing(Capability.HIBERNATE_REACTIVE)) { |
| @@ -575,7 +577,9 @@ private void handleHibernateORMWithNoPersistenceXml( | |||
| jdbcDataSources, applicationArchivesBuildItem, launchMode, | |||
| systemProperties, nativeImageResources, hotDeploymentWatchedFiles, persistenceUnitDescriptors); | |||
|
|
|||
| storageEngines.add(persistenceUnitEntry.getValue().dialect.storageEngine); | |||
| if (persistenceUnitEntry.getValue().dialect.storageEngine.isPresent()) { | |||
There was a problem hiding this comment.
there's a corner case which might have a surprise effect to users:
- DB-1 using MySQL (or MariaDB) and sets a weird engine like
BLACKHOLE - DB-2 using MySQL (or MariaDB) and doesn't set anything
- all data written on DB-2 disappears as well without warning ?
IMO we need to require full consistency among the MySQL/MariaDB configured PUs
There was a problem hiding this comment.
Yeah, the issue is that we have no way to detect this properly. We could assert the db-kind but people can do whatever they want with that.
We could catch 99% of the mistakes though, probably better than what I did.
There was a problem hiding this comment.
Ultimately the engine variable is read by the MySQL Dialect. So we know it's going to affect people either if they choose any of the extensions ofthe MySQLDialect (explicitly), or if we'll be chosing such a dialect based on the db-kind .. isn't that 100% ?
There was a problem hiding this comment.
I don't know. Frankly, I think that's a lot of work for something that is half baked in ORM :/. This should never has been included in ORM that way. I'll see what I can do.
There was a problem hiding this comment.
Sure, it's half baked and I suppose there's better use of your time. Feel free to postpone it into a different issue?
| // if there is any issue when bootstrapping Hibernate Validator. | ||
| if (capabilities.isPresent(Capability.HIBERNATE_VALIDATOR)) { | ||
| descriptor.getProperties().setProperty(AvailableSettings.JPA_VALIDATION_MODE, ValidationMode.CALLBACK.name()); | ||
| } |
| @@ -17,7 +17,8 @@ | |||
| .setExpectedException(ConfigurationException.class) | |||
| .setArchiveProducer(() -> ShrinkWrap.create(JavaArchive.class) | |||
| .addClass(User.class) | |||
| .addAsResource("application-multiple-persistence-units-invalid.properties", "application.properties")); | |||
| .addAsResource("application-multiple-persistence-units-undefined-packages.properties", | |||
| "application.properties")); | |||
gsmet
force-pushed
the
multi-persistence-units-follow-up
branch
from
3e9a8a8
to
50ee306
Compare
|
@Sanne I added a commit along the lines of what we discussed for the storage engines. The MySQL/MariaDB test is a bit naive but I think it will do. We can refine later if someone complains. |
The storage engine is only used by MySQL/MariaDB so a setup with a MySQL PU using a storage engine and a PostgreSQL PU not using any is perfectly valid.
With auto, if we have an error when bootstrapping the Bean Validation integration, the error will be swallowed and all validation will be ignored. It's far better to report an error in this case.
gsmet
force-pushed
the
multi-persistence-units-follow-up
branch
from
50ee306
to
140dfcf
Compare
| /** | ||
| * This is just to have the dialect matching MySQL and trigger the MySQL + storage engines check. | ||
| */ | ||
| public static class H2DialectWithMySQLInTheName extends H2Dialect { |
There was a problem hiding this comment.
Yeah, I didn't want to end up with MySQL there :).
|
thanks @gsmet ! It's not bullet-proof but I always merge progress ;) |
Sanne
added
the
triage/waiting-for-ci
|
Yeah I agree, it's not perfect but better than nothing :). |
@Sanne these are some small changes we talked about.
The Hibernate Validator is related to yesterday's PR: this way we won't miss bootstrap error anymore. Didn't include it in yesterday's PR because it would have conflicted with the Multi persistence units one.