Repository navigation
Stop mutating shared ParsedPersistenceXmlDescriptor in build steps - #51628
Conversation
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Stops mutating the shared ParsedPersistenceXmlDescriptor during Quarkus build steps by switching entity registration to Quarkus’ JPA model contribution mechanism (closes #47482).
Changes:
- Introduces
JpaModelPersistenceUnitContributionBuildItemto contribute entities instead of mutating the parsed persistence unit descriptor. - Refactors default PU entity discovery to collect entities and emit a contribution build item.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| List<String> additionalEntities = new ArrayList<>(); | ||
| for (AnnotationInstance annotation : annotations) { | ||
| AnnotationTarget target = annotation.target(); | ||
| String targetName = target.asClass().name().toString(); | ||
|
|
||
| if (!userManagedEntities.contains(targetName) | ||
| && (!targetName.startsWith("org.keycloak") || targetName.startsWith("org.keycloak.testsuite"))) { | ||
| descriptor.addClasses(targetName); | ||
| additionalEntities.add(targetName); | ||
| } | ||
| } | ||
|
|
||
| if (!additionalEntities.isEmpty()) { | ||
| jpaModelPuContributions.produce(new JpaModelPersistenceUnitContributionBuildItem( | ||
| descriptor.getName(), descriptor.getPersistenceUnitRootUrl(), | ||
| additionalEntities, Collections.emptyList())); | ||
| } |
There was a problem hiding this comment.
Not valid - it doesn't account for the constructor's defensive copy into a TreeSet.
| jpaModelPuContributions.produce(new JpaModelPersistenceUnitContributionBuildItem( | ||
| descriptor.getName(), descriptor.getPersistenceUnitRootUrl(), | ||
| additionalEntities, Collections.emptyList())); |
|
@Pepo48 I look at that Base IT (4) and it looks suspicious as there is some DB failure, please investigate. |
…lDescriptor Closes keycloak#47482 Signed-off-by: Peter Zaoral <pzaoral@redhat.com>
| @Override | ||
| public List<String> getManagedClassNames() { | ||
| return List.copyOf(super.getManagedClassNames()); | ||
| } |
…lDescriptor Closes keycloak#47482 Signed-off-by: Peter Zaoral <pzaoral@redhat.com>
|
@michalvavrik I noticed that yesterday and as you can see based on the timeline here, I tried multiple reruns already, because I could not reproduce it locally. The CI failure ( The fix needs both: That said, this is a workaround. During debugging I realized that it's not Keycloak issue, not even Quarkus one, but the root cause is in Hibernate ORM. Hibernate uses Jira for issue tracking and I don't have access to create issues there, so I'm not sure what the correct process is for contributing without the Jira project access. The question is then whether we want to proceed with this PR until the Hibernate fix (which would probably need to be backported in order to be able to consume it early). |
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.federation.ldap.LDAPUserLoginTest#loginLDAPUserAuthenticationSimpleEncryptionStartTLSKeycloak CI - Java Distribution IT (windows-latest - temurin - 21) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
quarkus/deployment/src/main/java/org/keycloak/quarkus/deployment/ConcurrentSafeParsedPersistenceXmlDescriptor.java:42
- Please add a focused concurrency regression test that repeatedly calls
addClasseswhile snapshottinggetManagedClassNames. This class is the core fix for the reported CME, but currently no test exercises it, despite the repository requiring functional/integration coverage for changes (CONTRIBUTING.md:28).
public synchronized List<String> getManagedClassNames() {
return List.copyOf(super.getManagedClassNames());
|
@Pepo48 just to double check - is there a simple approach to serializing the steps? I know you had a concern about the performance impact - do you have a general idea in terms of milliseconds or microseconds? |
|
I see @Pepo48 .
Neither do I, I just followed official links and authenticated under RH account and unauthorized 😀 but we can easily ask Hibernate team or create Quarkus issue. I never bothered by creating Hibernate issues, they just fix Quarkus issue by marking it as blocked external and linking it to Hibernate tracker they fix :-)
If it is Hibernate bug, then it must also be reproducible with Quarkus? I am asking because there is still a part that I don't understand - isn't this issue simply happening because build steps are running concurrently and we are using it in a way that they didn't anticipate? What I am after is that instead of the synchronization fix you provided, I'd love to know how they enforce order in Quarkus that avoids it. |
|
@marko-bekhta gave me this link https://hibernate.zulipchat.com/#narrow/channel/132096-hibernate-user/topic/Jira.20issue.20creation/near/602213094 so it is a known issue and IIUC there is a workaround mentioned in the Zulip. |
|
I'd report it through Quarkus in any case, we consume Hibernate through them. But since without the fix we have flaky tests, we need to go ahead with the fix on Keycloak side. |
Unless I'm overlooking something obvious, Quarkus doesn't let one extension control the execution order of another extension's build steps. The only way would be a change on the Quarkus side.
Seems like no Quarkus step ever calls So I tried one more thing, instead of workarounds, I tried to follow Quarkus convention - I moved the modification into the producer step so the descriptor is fully built before anyone can read it. One caveat is that the original code excluded entities that already belong to a user-defined persistence unit. That check depended on reading the full list of persistence units, which isn't available in the step that creates one. To keep the check, the producer step reads the user-defined persistence XML files directly. Again, from what I see this goes against how Quarkus normally handles it, but I haven't found any other option that would avoid bringing back the race. Please, take a look and let me know, which one of the approaches seems to be better. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
quarkus/deployment/src/main/java/org/keycloak/quarkus/deployment/KeycloakProcessor.java:557
- This no longer excludes entities explicitly assigned to a user-defined persistence unit, so every non-Keycloak
@Entityis also added to the default unit. For example,com.acme.provider.legacy.jpa.entity.Realmis listed innew-user-store, andCustomJpaEntityProviderDistTest.testUserManagedEntityNotAddedToDefaultPUrequires it to appear only once; retain the user-managed exclusion while avoiding descriptor mutation, e.g. by producing a separate default-unit JPA model contribution after collecting those class names.
if (!targetName.startsWith("org.keycloak") || targetName.startsWith("org.keycloak.testsuite")) {
additionalEntities.add(targetName);
…lDescriptor Closes keycloak#47482 Signed-off-by: Peter Zaoral <pzaoral@redhat.com>
There was a problem hiding this comment.
I think this is good approach.
|
@Pepo48 - thanks for looping me into this one, but as this is diving deeply into Quarkus and how you plan to consume fixes and releases, I'm afraid I can't help much here. I'm still interested in fix, as I'm affected by the unstable test, and - to my understanding - this might also happen when people use the Keycloak distribution. With that, I've marked this one as a blocker for the upcoming release. |
Closes #47482