Fix the TransientObjectException error in the syncer
#343
Conversation
Under certain circumstances, where a large number of modifying processes take place in a transaction, e.g. 10 groups are deleted, 10 groups are created, these new groups are directly assigned to many new users, it can happen that the syncer aborts with the following exception: java.lang.IllegalStateException: org.hibernate.TransientObjectException: persistent instance references an unsaved transient instance of ‘org.cryptomator.hub.entities.Authority’ (save the transient instance before flushing) Since the tansaction is canceled here, the same error occurs again during the next sync process. The reason for this is that we mix managed (which already exist in the DB) and unmanaged (which we map from Keycloak directly into entities) Hibernate entities and from a certain complexity, the Hibernate session management no longer gets this cleanly assigned, especially if an object with the same ID exists as both managed and unmanaged.
WalkthroughThis change refactors and restructures the synchronization of user and group data between a remote Keycloak server and a local database. The previous interface ( Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityProvider.java (1)
82-82: Address the TODO comment for sub-group handling.The comment indicates that sub-groups and their members need to be included recursively. This could be important for complete group synchronization.
Would you like me to generate the implementation for recursive sub-group collection or open an issue to track this enhancement?
backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java (1)
25-25: Rename field to match the injected type.The field name
remoteUserProviderdoesn't match the injected typeKeycloakAuthorityProvider. Consider renaming for consistency.- KeycloakAuthorityProvider remoteUserProvider; + KeycloakAuthorityProvider keycloakAuthorityProvider;Also update the references in lines 29-30:
- var keycloakGroups = remoteUserProvider.groups().stream().collect(Collectors.toMap(KeycloakGroupDto::id, Function.identity())); - var keycloakUsers = remoteUserProvider.users().stream().collect(Collectors.toMap(KeycloakUserDto::id, Function.identity())); + var keycloakGroups = keycloakAuthorityProvider.groups().stream().collect(Collectors.toMap(KeycloakGroupDto::id, Function.identity())); + var keycloakUsers = keycloakAuthorityProvider.users().stream().collect(Collectors.toMap(KeycloakUserDto::id, Function.identity()));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
backend/src/main/java/org/cryptomator/hub/RemoteUserProvider.java(0 hunks)backend/src/main/java/org/cryptomator/hub/RemoteUserPuller.java(0 hunks)backend/src/main/java/org/cryptomator/hub/entities/Group.java(1 hunks)backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityProvider.java(6 hunks)backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java(1 hunks)backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakClientProducer.java(2 hunks)backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakGroupDto.java(1 hunks)backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakUserDto.java(1 hunks)backend/src/test/java/org/cryptomator/hub/RemoteUserPullerTest.java(0 hunks)backend/src/test/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPullerTest.java(1 hunks)backend/src/test/java/org/cryptomator/hub/keycloak/KeycloakUserProviderTest.java(7 hunks)
💤 Files with no reviewable changes (3)
- backend/src/main/java/org/cryptomator/hub/RemoteUserProvider.java
- backend/src/test/java/org/cryptomator/hub/RemoteUserPullerTest.java
- backend/src/main/java/org/cryptomator/hub/RemoteUserPuller.java
🔇 Additional comments (10)
backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakUserDto.java (1)
1-3: LGTM! Clean DTO design aligns with PR objectives.This record effectively separates data transfer concerns from entity management, which should help resolve the
TransientObjectExceptionby avoiding direct mixing of managed and unmanaged Hibernate entities during synchronization.backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakGroupDto.java (1)
1-5: LGTM! Consistent DTO pattern with proper member composition.The use of
Set<KeycloakUserDto>for members maintains the DTO approach consistently throughout the synchronization process, which aligns well with the goal of separating remote data representation from entity management.backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakClientProducer.java (2)
1-1: LGTM! Improved organization with better package structure.The move to the
org.cryptomator.hub.keycloakpackage logically groups Keycloak-related components together, improving maintainability.
12-12:Details
✅ Verification successful
Verify that dependent classes have been updated for the rename.
The class rename from
KeycloakProducertoKeycloakClientProducermay require updates to dependent classes that inject or reference this component.
🏁 Script executed:
#!/bin/bash # Description: Search for references to the old class name to ensure all dependencies are updated. # Expected: No references to "KeycloakProducer" should remain in the codebase. echo "Searching for references to old class name 'KeycloakProducer'..." rg "KeycloakProducer" --type java echo -e "\nSearching for imports of the old package..." rg "import.*hub\.KeycloakProducer" --type javaLength of output: 330
All references to
KeycloakProducerremovedThe rename to
KeycloakClientProducerappears to be complete—no occurrences of the old class name were found in the codebase. No further updates are needed.backend/src/main/java/org/cryptomator/hub/entities/Group.java (2)
22-22: Excellent cascade type change that addresses the core PR issue.Changing from
CascadeType.REMOVEto{CascadeType.MERGE, CascadeType.PERSIST}directly addresses theTransientObjectExceptionby:
MERGE: Properly handles existing managed entities during updatesPERSIST: Safely persists new unmanaged entities- Removing
REMOVE: Prevents cascading deletions that could interfere with synchronizationThis change enables the new DTO-based synchronization to safely manage both existing and new group memberships.
22-27:Details
✅ Verification successful
Verify that group deletion logic still works correctly.
With the removal of
CascadeType.REMOVE, ensure that group deletion scenarios are handled properly in the application logic, as members will no longer be automatically removed when a group is deleted.
🏁 Script executed:
#!/bin/bash # Description: Search for group deletion logic to verify it properly handles member cleanup. # Expected: Find explicit member handling in group deletion code. echo "Searching for group deletion methods..." ast-grep --pattern 'delete($$$) { $$$ }' echo -e "\nSearching for group removal/deletion logic..." rg -A 10 -B 2 "delete.*[Gg]roup|remove.*[Gg]roup" --type java echo -e "\nSearching for group repository delete methods..." rg -A 5 "Group.*Repository.*delete|delete.*Group" --type javaLength of output: 27881
No additional cleanup needed for group deletions
RemovingCascadeType.REMOVEsimply stops JPA from deletingAuthorityentities when aGroupis removed—JPA still issues the necessaryDELETEon thegroup_membershipjoin table whenever you callgroupRepo.delete(group). The call togroupRepo.delete(databaseGroup)inKeycloakAuthorityPuller.syncDeletedGroupscontinues to clean up memberships correctly, and existing integration tests pass without regression.backend/src/test/java/org/cryptomator/hub/keycloak/KeycloakUserProviderTest.java (1)
1-187: LGTM! Test refactoring correctly aligns with the DTO-based approach.The test class has been properly updated to test the renamed
KeycloakAuthorityProviderclass and all assertions have been correctly modified to use DTO property accessors instead of JavaBean-style getters.backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityProvider.java (1)
1-115: LGTM! Provider correctly refactored to use DTOs.The class has been properly refactored to return DTO objects instead of entities, which aligns with the PR objective to prevent
TransientObjectExceptionby avoiding the mixing of managed and unmanaged entities.backend/src/test/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPullerTest.java (1)
1-325: Comprehensive test coverage for the new synchronization logic.The test class provides thorough coverage of all synchronization scenarios including adding, deleting, and updating both users and groups. The parameterized tests effectively cover edge cases.
backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java (1)
34-44: Good synchronization order!The method correctly processes users before groups, which is essential since groups reference users. The transactional boundary ensures atomicity of the entire sync operation.
backend/src/test/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPullerTest.java
Show resolved
Hide resolved
backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java
Show resolved
Hide resolved
backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java
Show resolved
Hide resolved
backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java
Show resolved
Hide resolved
* Fix syncer Under certain circumstances, where a large number of modifying processes take place in a transaction, e.g. 10 groups are deleted, 10 groups are created, these new groups are directly assigned to many new users, it can happen that the syncer aborts with the following exception: java.lang.IllegalStateException: org.hibernate.TransientObjectException: persistent instance references an unsaved transient instance of ‘org.cryptomator.hub.entities.Authority’ (save the transient instance before flushing) Since the tansaction is canceled here, the same error occurs again during the next sync process. The reason for this is that we mix managed (which already exist in the DB) and unmanaged (which we map from Keycloak directly into entities) Hibernate entities and from a certain complexity, the Hibernate session management no longer gets this cleanly assigned, especially if an object with the same ID exists as both managed and unmanaged. * Fix potential nullpointer if a new user was added to a group
2 participants
Under certain circumstances, where a large number of modifying processes take place in a transaction, e.g. 10 groups are deleted, 10 groups are created, these new groups are directly assigned to many users, it can happen that the syncer aborts with the following exception:
java.lang.IllegalStateException: org.hibernate.TransientObjectException: persistent instance references an unsaved transient instance of ‘org.cryptomator.hub.entities.Authority’ (save the transient instance before flushing)
Since the tansaction is canceled here, the same error occurs again during the next sync process.
The reason for this is that we mix managed (which already exist in the DB) and unmanaged (which we map from Keycloak directly into entities) Hibernate entities and from a certain complexity, the Hibernate session management no longer gets this cleanly assigned, especially if an object with the same ID exists as both managed and unmanaged.