-
Notifications
You must be signed in to change notification settings - Fork 3k
Added class name check to remove warning #50722
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?
Added class name check to remove warning #50722
Conversation
…xed due to package-info.java - Closes: quarkusio#50572
|
/cc @gsmet (hibernate-orm) |
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 @Aryant-Tripathi, this is a good start. I've left a few comments to start with, but I wanted to remind you we need a non-regression test that ideally would verify no warning is thrown for any package-info.java found in the index while still generating proxy classes for all entities correctly.
| for (AdditionalJpaModelBuildItem additionalJpaModelBuildItem : additionalJpaModelBuildItems) { | ||
| managedClassAndPackageNames.add(additionalJpaModelBuildItem.getClassName()); | ||
| } | ||
|
|
||
| PreGeneratedProxies proxyDefinitions = generateProxies(managedClassAndPackageNames, | ||
| PreGeneratedProxies proxyDefinitions = generateProxies(managedClassAndPackageNames, managedClassesName, |
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.
We'll need to add the additionalJpaModelBuildItems class-names as well, otherwise we'll fail to generate proxies for any entity class contained there.
...-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmProcessor.java
Outdated
Show resolved
Hide resolved
|
|
||
| if(!jpaModel.getEntityClassNames().contains(modelClassName)) { | ||
| // We only care about class name here | ||
| continue; | ||
| } | ||
|
|
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 don't think this is correct, as you can see within this for loop we add the modelClassName to the model.allModelClassAndPackageNames() on line 1257 even if it's not an entity.
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.
Yeah, unfortunately this patch is too simple and would cause trouble down the way.
Really, you can't use existing information in jpaModel, you need to create a separate Set @Aryant-Tripathi.
See the explanation I gave in this comment: #50572 (comment)
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.
Sorry but this is still not enough. I added a comment further up in this file, hopefully this will clear things up...
If you think my comment does not make sense, please say so, so we can discuss that. don't just ignore it :)
|
|
||
| if(!jpaModel.getEntityClassNames().contains(modelClassName)) { | ||
| // We only care about class name here | ||
| continue; | ||
| } | ||
|
|
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.
Yeah, unfortunately this patch is too simple and would cause trouble down the way.
Really, you can't use existing information in jpaModel, you need to create a separate Set @Aryant-Tripathi.
See the explanation I gave in this comment: #50572 (comment)
- Added Package info non regression test for verfying the index warning
...-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmProcessor.java
Outdated
Show resolved
Hide resolved
| // is used for packages too, and it relies (indirectly) on getManagedClassNames(). | ||
| managedClassAndPackageNames.addAll(pud.getManagedClassNames()); | ||
| } | ||
|
|
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 also need to update the for loop above, to have an equivalent of managedClassAndPackageNames.addAll(pud.getManagedClassNames()); but for the managedClassesName set.
Now, obviously you cannot use pud.getManagedClassNames() for that, because that thing also returns package names. Which is where my comment should start to make sense: #50572 (comment)
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 then believe sir, we have to add one more data member inside QuarkusPersistenceUnitDescriptor class, which only contains the classes name. We can take use of that data member in creating set of only managed class name.
If it seems ok, may I proceed with those changes ?
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.
Yes this seems perfectly reasonable :)
|
|
||
| if(!jpaModel.getEntityClassNames().contains(modelClassName)) { | ||
| // We only care about class name here | ||
| continue; | ||
| } | ||
|
|
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.
Sorry but this is still not enough. I added a comment further up in this file, hopefully this will clear things up...
If you think my comment does not make sense, please say so, so we can discuss that. don't just ignore it :)
…ed due to package-info. Closes: quarkusio#50572
| public List<String> getManagedClassAndPackagesNames() { | ||
| return managedClassAndPackagesNames; | ||
| } | ||
|
|
||
| @Override | ||
| public List<String> getManagedClassNames() { |
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.
Unfortunately you cannot do that. getManagedClassNames() is inherited, it's something from Hibernate ORM and it's really is intended to hold both class names and package names. See org.hibernate.jpa.boot.spi.PersistenceUnitDescriptor.
So, you need to make sure getManagedClassNames() will continue to return both class names and package names.
The method you'll add will need to be the one that only returns class names, for example you could name it getManagedClassNamesOnly().
| @Override | ||
| public ClassTransformer getClassTransformer() { | ||
| // We transform classes during the build, not on bootstrap. | ||
| return null; | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return "QuarkusPersistenceUnitDescriptor{" + | ||
| "name='" + name + '\'' + | ||
| ", providerHelper=" + providerHelper + | ||
| ", providerClassName='" + providerClassName + '\'' + | ||
| ", useQuotedIdentifiers=" + useQuotedIdentifiers + | ||
| ", transactionType=" + persistenceUnitTransactionType + | ||
| ", persistenceUnitTransactionType=" + persistenceUnitTransactionType + | ||
| ", validationMode=" + validationMode + | ||
| ", sharedCacheMode=" + sharedCacheMode + | ||
| ", managedClassAndPackagesNames=" + managedClassAndPackagesNames + | ||
| ", managedClassNames=" + managedClassNames + | ||
| ", properties=" + properties + | ||
| ", isReactive=" + reactive + | ||
| ", reactive=" + reactive + | ||
| '}'; | ||
| } | ||
|
|
||
| @Override | ||
| public ClassTransformer getClassTransformer() { | ||
| // We transform classes during the build, not on bootstrap. | ||
| return null; | ||
| } |
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.
These changes seem unnecessary. Please drop them?
| if (!managedClassesName.contains(managedClassOrPackageName)) { | ||
| // we don't generate proxies for packages: | ||
| continue; | ||
| } |
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 could simply iterate on managedClassesName instead of iterating on managedClassAndPackageNames?
|
|
||
| if (jpaModel.getEntityClassNames().contains(modelClassName)) { | ||
| if (managedClassNames.contains(modelClassName)) { | ||
| model.entityClassNames().add(modelClassName); |
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.
This is incorrect and should be reverted.
| public QuarkusPersistenceUnitDescriptor(String name, | ||
| QuarkusPersistenceUnitProviderHelper providerHelper, | ||
| PersistenceUnitTransactionType persistenceUnitTransactionType, | ||
| List<String> managedClassNames, | ||
| List<String> managedClassAndPackagesNames, List<String> managedClassNames, | ||
| Properties properties, boolean reactive) { |
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 believe you did not update calls to this constructor in HibernateOrmProcessor, so it likely doesn't compile anymore. Please try to compile locally before pushing :)
| new HibernateReactivePersistenceUnitProviderHelper(), | ||
| PersistenceUnitTransactionType.RESOURCE_LOCAL, | ||
| new ArrayList<>(model == null ? Collections.emptySet() : model.allModelClassAndPackageNames()), | ||
| new ArrayList<>(model == null ? Collections.emptySet() : model.entityClassNames()), |
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.
This is incorrect. We want all model classes here.
- Entity classes =~ classes annontated with @entity
- Managed classes =~ entity classes + classes annontated with @MappedSuperclass or @embeddable
- All model classes =~ managed classes + some additional model types used in entity attributes, possibly enums, etc.
You need something like:
| new ArrayList<>(model == null ? Collections.emptySet() : model.entityClassNames()), | |
| new ArrayList<>(model == null ? Collections.emptySet() : model.allModelClassNames()), |
And that requires adding data to JpaPersistenceUnitModel, and to anything that contributes to it.
Again, you will have an easier time if you read my comment and have a look at the parts of the code I pointed out: #50572 (comment)
- fixes quarkusio#50618 Co-authored-by: George Gastaldi <[email protected]>
Bumps [io.quarkus.gizmo:gizmo2](https://github.com/quarkusio/gizmo) from 2.0.0.Beta6 to 2.0.0.Beta8. - [Release notes](https://github.com/quarkusio/gizmo/releases) - [Commits](quarkusio/gizmo@2.0.0.Beta6...2.0.0.Beta8) --- updated-dependencies: - dependency-name: io.quarkus.gizmo:gizmo2 dependency-version: 2.0.0.Beta8 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]>
Bumps [com.google.errorprone:error_prone_annotations](https://github.com/google/error-prone) from 2.42.0 to 2.43.0. - [Release notes](https://github.com/google/error-prone/releases) - [Commits](google/error-prone@v2.42.0...v2.43.0) --- updated-dependencies: - dependency-name: com.google.errorprone:error_prone_annotations dependency-version: 2.43.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>
In all these wiring classes, we can do without parameters or debug info. Gizmo 1 weren't adding parameters and debug info so we are on par with what we used to do with Gizmo 1.
Allow JUnit5 to create the instance Signed-off-by: Michael Edgar <[email protected]>
Bumps `flyway.version` from 11.14.1 to 11.15.0. Updates `org.flywaydb:flyway-core` from 11.14.1 to 11.15.0 - [Release notes](https://github.com/flyway/flyway/releases) - [Commits](flyway/flyway@flyway-11.14.1...flyway-11.15.0) Updates `org.flywaydb:flyway-sqlserver` from 11.14.1 to 11.15.0 Updates `org.flywaydb:flyway-mysql` from 11.14.1 to 11.15.0 Updates `org.flywaydb:flyway-database-oracle` from 11.14.1 to 11.15.0 Updates `org.flywaydb:flyway-database-postgresql` from 11.14.1 to 11.15.0 Updates `org.flywaydb:flyway-database-db2` from 11.14.1 to 11.15.0 Updates `org.flywaydb:flyway-database-derby` from 11.14.1 to 11.15.0 Updates `org.flywaydb:flyway-database-mongodb` from 11.14.1 to 11.15.0 --- updated-dependencies: - dependency-name: org.flywaydb:flyway-core dependency-version: 11.15.0 dependency-type: direct:production update-type: version-update:semver-minor - dependency-name: org.flywaydb:flyway-sqlserver dependency-version: 11.15.0 dependency-type: direct:production update-type: version-update:semver-minor - dependency-name: org.flywaydb:flyway-mysql dependency-version: 11.15.0 dependency-type: direct:production update-type: version-update:semver-minor - dependency-name: org.flywaydb:flyway-database-oracle dependency-version: 11.15.0 dependency-type: direct:production update-type: version-update:semver-minor - dependency-name: org.flywaydb:flyway-database-postgresql dependency-version: 11.15.0 dependency-type: direct:production update-type: version-update:semver-minor - dependency-name: org.flywaydb:flyway-database-db2 dependency-version: 11.15.0 dependency-type: direct:production update-type: version-update:semver-minor - dependency-name: org.flywaydb:flyway-database-derby dependency-version: 11.15.0 dependency-type: direct:production update-type: version-update:semver-minor - dependency-name: org.flywaydb:flyway-database-mongodb dependency-version: 11.15.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>
Signed-off-by: Phillip Kruger <[email protected]>
This reverts commit 52ba3d5.
Add fallback to use root implementor when no matching handleRequest method is found in the class hierarchy. Closes quarkusio#49413
…rnateormprocessor
Added class name check to remove warning that package cannot be indexed due to package-info.java
Closes: HibernateOrmProcessor causes Warning that package cannot be indexed due to package-info.java #50572
[Please describe here what your change is about]