SQL optimizations #372
SQL optimizations #372
Conversation
* add `@VaultRole(bypassForRealmRole = true, realmRole="admin")` * remove `Vault.effectiveMembers`
this data is rarely updated but frequently read. Its recursive nature makes it very expensive, but the newly introduced methods in `EffectiveGroupMembership.Repository` allow for partial updates.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a Batch utility for chunked collection processing and updates multiple repository methods (Authority, Device, Vault, Group, User, EffectiveVaultAccess) to use batching. Introduces a new EffectiveGroupMembership entity with repository and named queries plus a DB migration creating the effective_group_membership table, indexes, and updated views. Adjusts VaultRole/VaultRoleFilter to support a realm-role bypass and to return 404 for invalid vault IDs. KeycloakAuthorityPuller is changed to persist users/groups in bulk and to invoke EffectiveGroupMembership updates. Several tests and integration tests are added or adapted for batching, effective memberships, and license-limit scenarios. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java (1)
128-135: Missing null check insyncUpdatedGroupsmay cause NPE.The null guard was added to
syncAddedGroups(lines 100-102) but the same pattern insyncUpdatedGroupsat line 134 lacks this protection. IfuserRepo.findById(addId)returnsnull, adding it tomemberscould cause issues downstream.for (var addId : diff(wantIds, haveIds)) { var databaseUser = databaseUsers.get(addId); if (databaseUser == null) { // User might have been just added, fetch from database databaseUser = userRepo.findById(addId); } - databaseGroup.getMembers().add(databaseUser); + if (databaseUser != null) { + databaseGroup.getMembers().add(databaseUser); + } }
🧹 Nitpick comments (5)
backend/src/test/java/org/cryptomator/hub/api/GroupsResourceIT.java (1)
93-97: Consider removing unusedSQLExceptionfrom method signature.The test no longer uses direct SQL operations, so
throws SQLExceptionis unnecessary.@Test @DisplayName("GET /groups/group1/effective-members contains direct and subgroup members") -public void testGetEffectiveUsers() throws SQLException { +public void testGetEffectiveUsers() { when().get("/groups/{groupId}/effective-members", "group1") .then().statusCode(200) .body("id", hasItems("user1", "user999")); }backend/src/test/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPullerTest.java (1)
97-108: Assertion may be too permissive.
Mockito.argThat(addedUserIds::containsAll)verifies thataddedUserIdscontains all elements from the argument, but doesn't verify the argument doesn't have extra elements. This could miss bugs where extra IDs are incorrectly passed.Consider using
Mockito.eq(addedUserIds)for exact match, or verify bidirectional containment:Mockito.argThat(arg -> addedUserIds.containsAll(arg) && arg.containsAll(addedUserIds))backend/src/test/java/org/cryptomator/hub/api/ExceedingLicenseLimitsIT.java (3)
88-94: Remove unuseduser999variable.
user999is defined but never persisted (line 96 only persistsuser91, user92, user93, user94, user95A). This appears to be dead code.- var user999 = new User(); - user999.setId("user999"); - user999.setName("User 999"); - user999.setEcdhPublicKey("ecdh_public999"); - user999.setEcdsaPublicKey("ecdsa_public999"); - user999.setPrivateKeys("private999"); - user999.setSetupCode("setup999"); - userRepo.persist(user91, user92, user93, user94, user95A);
223-226: Fix typos in method name and display name.The method name
testUnockBlockedExceedingLicenseHardLimitis missing the 'l' in "Unlock", and the@DisplayNamehas "toke" instead of "token".@Test @Order(8) -@DisplayName("Unlock/legacyUnlock is blocked if (effective vault users with toke) > license seats") -public void testUnockBlockedExceedingLicenseHardLimit() throws SQLException { +@DisplayName("Unlock/legacyUnlock is blocked if (effective vault users with token) > license seats") +public void testUnlockBlockedExceedingLicenseHardLimit() throws SQLException {
210-221: Test order gap: Order jumps from 5 to 7.
testCreateVaultExceedingSeatshas@Order(5)andtestUnlockAllowedExceedingLicenseSoftLimithas@Order(7), skipping 6. If this is intentional (placeholder for future test), it's fine. Otherwise, consider using consecutive order values for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
backend/src/main/java/org/cryptomator/hub/api/VaultResource.java(1 hunks)backend/src/main/java/org/cryptomator/hub/entities/Authority.java(2 hunks)backend/src/main/java/org/cryptomator/hub/entities/Batch.java(1 hunks)backend/src/main/java/org/cryptomator/hub/entities/Device.java(1 hunks)backend/src/main/java/org/cryptomator/hub/entities/EffectiveGroupMembership.java(2 hunks)backend/src/main/java/org/cryptomator/hub/entities/EffectiveVaultAccess.java(1 hunks)backend/src/main/java/org/cryptomator/hub/entities/Group.java(3 hunks)backend/src/main/java/org/cryptomator/hub/entities/User.java(2 hunks)backend/src/main/java/org/cryptomator/hub/entities/Vault.java(1 hunks)backend/src/main/java/org/cryptomator/hub/filters/VaultRole.java(1 hunks)backend/src/main/java/org/cryptomator/hub/filters/VaultRoleFilter.java(1 hunks)backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java(5 hunks)backend/src/main/resources/org/cryptomator/hub/flyway/V21__Optimize_Group_Membership.sql(1 hunks)backend/src/test/java/org/cryptomator/hub/api/ExceedingLicenseLimitsIT.java(1 hunks)backend/src/test/java/org/cryptomator/hub/api/GroupsResourceIT.java(1 hunks)backend/src/test/java/org/cryptomator/hub/api/VaultResourceIT.java(7 hunks)backend/src/test/java/org/cryptomator/hub/filters/VaultRoleFilterTest.java(1 hunks)backend/src/test/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPullerTest.java(9 hunks)backend/src/test/resources/org/cryptomator/hub/flyway/V9999__Test_Data.sql(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-10-17T15:24:07.752Z
Learnt from: overheadhunter
Repo: cryptomator/hub PR: 296
File: backend/src/test/java/org/cryptomator/hub/filters/VaultRoleFilterTest.java:183-186
Timestamp: 2024-10-17T15:24:07.752Z
Learning: In `VaultRoleFilterTest`, remember that the `securityContext` mock is set in the outer class's setup method at line 46, which applies to nested classes as well. Avoid suggesting redundant setup in nested classes.
Applied to files:
backend/src/main/java/org/cryptomator/hub/filters/VaultRole.javabackend/src/main/java/org/cryptomator/hub/filters/VaultRoleFilter.javabackend/src/test/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPullerTest.javabackend/src/test/java/org/cryptomator/hub/filters/VaultRoleFilterTest.javabackend/src/test/java/org/cryptomator/hub/api/VaultResourceIT.java
📚 Learning: 2025-06-06T09:18:42.468Z
Learnt from: SailReal
Repo: cryptomator/hub PR: 343
File: backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java:27-32
Timestamp: 2025-06-06T09:18:42.468Z
Learning: In the KeycloakAuthorityPuller and similar syncer components, don't add broad exception handling to scheduled methods. The Syncer execution context is designed to handle crashes, and it's better to let exceptions propagate rather than catching them broadly. Catching Exception is considered bad practice in this codebase.
Applied to files:
backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java
🧬 Code graph analysis (6)
backend/src/main/java/org/cryptomator/hub/entities/Group.java (1)
backend/src/main/java/org/cryptomator/hub/entities/Batch.java (1)
Batch(8-47)
backend/src/main/java/org/cryptomator/hub/entities/Device.java (1)
backend/src/main/java/org/cryptomator/hub/entities/Batch.java (1)
Batch(8-47)
backend/src/main/java/org/cryptomator/hub/entities/Authority.java (1)
backend/src/main/java/org/cryptomator/hub/entities/Batch.java (1)
Batch(8-47)
backend/src/main/java/org/cryptomator/hub/entities/User.java (1)
backend/src/main/java/org/cryptomator/hub/entities/Batch.java (1)
Batch(8-47)
backend/src/main/java/org/cryptomator/hub/api/VaultResource.java (1)
frontend/src/common/backend.ts (1)
VaultRole(64-64)
backend/src/main/java/org/cryptomator/hub/entities/Vault.java (1)
backend/src/main/java/org/cryptomator/hub/entities/Batch.java (1)
Batch(8-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL analysis (java)
- GitHub Check: Agent
🔇 Additional comments (27)
backend/src/main/java/org/cryptomator/hub/entities/User.java (1)
179-181: Batched user deletion implementation looks correct
deleteByIdscorrectly splits the ID collection into batches of 200 and sums the affected-row counts from each delete. Empty collections and non-existent IDs safely yield 0 without extra queries.backend/src/main/resources/org/cryptomator/hub/flyway/V21__Optimize_Group_Membership.sql (1)
3-43: Migration for effective_group_membership and effective_vault_access is consistent and well-scopedThe new index, table definition (including FKs and the
group_id <> member_idcheck), recursive population query with explicit depth cap, and rebuilteffective_vault_accessview all look coherent and aligned with the new entity model. The UNION-based combination of direct and group-based vault access is also appropriate.backend/src/test/resources/org/cryptomator/hub/flyway/V9999__Test_Data.sql (1)
32-35: Test data for effective_group_membership matches migration semanticsThe inserted rows mirror the
group_membershipentries and use the same/group/memberpath format as the migration CTE, which keeps tests aligned with production behavior.backend/src/main/java/org/cryptomator/hub/filters/VaultRoleFilter.java (1)
44-56: Realm-role bypass and invalid-vault handling are consistent with the annotation contractThe early exit when
bypassForRealmRole()is true and the caller has the configured realm role cleanly skips vault-role checks, and the switch toNotFoundExceptionfor an unparsable/missing vaultId parameter gives a clearer signal of misconfiguration. The remaining permission and missing-vault logic is unchanged.backend/src/main/java/org/cryptomator/hub/entities/Device.java (1)
160-164: Batched device lookup preserves semantics and improves scalabilityThe new
findAllInListcorrectly concatenates per-batch query streams, preserving result semantics (including duplicates) while limiting IN-clause size and handling empty ID lists gracefully by returning an empty stream.backend/src/main/java/org/cryptomator/hub/filters/VaultRole.java (2)
36-41: bypassForRealmRole fits cleanly with filter behavior and docsThe new
bypassForRealmRoleelement and its Javadoc align with the early-exit logic inVaultRoleFilterand clearly describe the semantics of using a realm role as an override. Defaults are safe (falsewith empty realmRole).
42-48: realmRole Javadoc correctly scopes when the role is consultedThe clarification that
realmRoleis only relevant whenbypassForRealmRole()is enabled oronMissingVault()isREQUIRE_REALM_ROLEmatches how the filter uses it and should prevent misconfiguration.backend/src/main/java/org/cryptomator/hub/entities/Group.java (1)
47-49: Group deleteByIds batching mirrors User and looks correctThe repository’s
deleteByIdsmethod follows the same 200-size batching pattern as inUser.Repository, safely summing delete counts and avoiding oversized IN clauses while handling empty or non-existent IDs without surprises.backend/src/main/java/org/cryptomator/hub/api/VaultResource.java (1)
387-396: LGTM: Clean delegation of access control to annotation.The
@VaultRoleannotation now handles both vault membership and admin bypass semantics, removing the need for manual access checks. The redundantfindByIdOptionalcall in the method body (line 394) acts as a safety net, though the filter should already handle missing vaults viaonMissingVault = NOT_FOUND.backend/src/main/java/org/cryptomator/hub/entities/Authority.java (1)
90-95: LGTM: Batch processing for IN-clause optimization.The batch approach correctly avoids oversized IN clauses. One consideration: the returned
Streamis composed of lazily-evaluated partial streams. Ensure callers consume the result within the same transactional context to avoid Hibernate session issues.backend/src/test/java/org/cryptomator/hub/filters/VaultRoleFilterTest.java (1)
49-56: LGTM: Test correctly reflects updated error semantics.Changing from
ForbiddenExceptiontoNotFoundExceptionfor missing vault ID is semantically correct—an unparseable or missing path parameter indicates the resource cannot be located, not an authorization failure.backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java (2)
27-28: LGTM: Batch operations with effective membership updates.The refactor to batch persistence/deletion with corresponding
effectiveGroupMembershipRepoupdates is well-structured. The membership materialization is correctly triggered after both additions and deletions.Also applies to: 61-62, 68-69, 107-108, 114-115
100-102: Good: Defensive null check for member resolution.The null guard correctly handles edge cases where a user referenced in Keycloak may not exist in the database (e.g., filtered out, sync timing issues).
backend/src/main/java/org/cryptomator/hub/entities/Batch.java (3)
20-26: LGTM!The consumer-based batch processing logic is correct. The
Math.minensures proper handling of the final partial batch.
28-36: LGTM!The fold-style batch processing correctly accumulates results across batches.
38-46: LGTM!The list aggregation overload provides flexibility for batch result collection. The caller is responsible for combining batch results appropriately.
backend/src/main/java/org/cryptomator/hub/entities/EffectiveGroupMembership.java (5)
22-34: LGTM - Well-designed recursive CTE for group membership hierarchy.The recursive query correctly traverses the group membership graph with a depth limit of 10 to prevent infinite recursion in case of cyclic references. The
ON CONFLICT DO NOTHINGhandles duplicate paths gracefully.
40-53: LGTM!The group-scoped update query correctly limits the recursive traversal to the specified groups while maintaining the same depth protection.
59-72: LGTM!The user-scoped update correctly traverses upward through the group hierarchy to find all groups a user effectively belongs to via nested memberships.
126-144: LGTM!The batch-oriented update methods correctly chunk operations to avoid SQL parameter limits while maintaining delete-then-insert consistency within each batch.
119-124: This concern is not applicable. ThefullUpdate()method is not called anywhere in the codebase, and Panache/Quarkus provides implicit transaction handling for all repository method invocations. ThedeleteAll()andexecuteUpdate()operations execute as a single atomic transaction automatically.Likely an incorrect or invalid review comment.
backend/src/test/java/org/cryptomator/hub/api/GroupsResourceIT.java (1)
46-66: LGTM - Clean repository-based test data setup.The test setup correctly creates hierarchical group membership (user999 → group999 → group1) and updates the effective membership table. This aligns well with the pattern used in
ExceedingLicenseLimitsIT.backend/src/test/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPullerTest.java (3)
39-62: LGTM - Well-designed stream capture for verification.The
doAnswerpattern correctly captures streamed entities for later assertion, enabling verification of batch persistence without modifying production code.
83-85: Potential issue withSet.of()and CSV parsing edge cases.
Set.of()throwsIllegalArgumentExceptionif the array contains duplicates or nulls. The CSV value",;foo"would produce["", "foo"]after split, and",;,"would produce["", ""]causing a duplicate exception. The current test data appears safe, but this is fragile.Consider using
Arrays.stream(...).collect(Collectors.toSet())for robustness, or document that CSV inputs must not contain empty/duplicate values.
232-243: LGTM!The group persistence verification correctly checks the group properties and member associations established in the test setup.
backend/src/test/java/org/cryptomator/hub/api/VaultResourceIT.java (2)
97-121: LGTM - Setup correctly creates test users and group associations.The setup properly initializes user998 and user999, associates them with group2, and updates the effective membership table.
631-636: LGTM - Test expectations updated to reflect new test data.The
memberSize=3expectation correctly accounts for the two additional users (user998, user999) added to group2 in setup.
backend/src/main/java/org/cryptomator/hub/entities/EffectiveVaultAccess.java
Outdated
Show resolved
Hide resolved
backend/src/test/java/org/cryptomator/hub/api/ExceedingLicenseLimitsIT.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This pull request introduces significant performance optimizations for database operations through batch processing, refactors user/group synchronization logic, and enhances the access control mechanism with role-based bypasses for vault operations.
- Introduces a new
Batchutility class to process database operations in batches of 200 records, improving efficiency for large datasets - Converts the
effective_group_membershipview to a table with optimized indexes and adds batch update methods for group/user synchronization - Adds
bypassForRealmRoleto@VaultRoleannotation, allowing admin users to bypass vault-specific role checks
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
backend/src/main/java/org/cryptomator/hub/entities/Batch.java |
New utility class for batch processing collections with configurable batch sizes |
backend/src/main/java/org/cryptomator/hub/entities/Vault.java |
Removes dead code (effectiveMembers field) and updates findAllInList to use batch processing |
backend/src/main/java/org/cryptomator/hub/entities/User.java |
Adds deleteByIds method using batch processing |
backend/src/main/java/org/cryptomator/hub/entities/Group.java |
Adds deleteByIds method using batch processing |
backend/src/main/java/org/cryptomator/hub/entities/EffectiveVaultAccess.java |
Updates countSeatsOccupiedByUsers to use batch processing |
backend/src/main/java/org/cryptomator/hub/entities/EffectiveGroupMembership.java |
Adds Repository with batch update/delete methods for groups and users, includes recursive SQL queries for membership calculation |
backend/src/main/java/org/cryptomator/hub/entities/Device.java |
Updates findAllInList to use batch processing |
backend/src/main/java/org/cryptomator/hub/entities/Authority.java |
Updates findAllInList to use batch processing |
backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java |
Refactors synchronization to persist users/groups in bulk and update effective memberships in batches |
backend/src/main/java/org/cryptomator/hub/filters/VaultRole.java |
Adds bypassForRealmRole parameter to allow skipping vault role checks for specific realm roles |
backend/src/main/java/org/cryptomator/hub/filters/VaultRoleFilter.java |
Implements realm role bypass logic and changes exception type from ForbiddenException to NotFoundException for invalid vault IDs |
backend/src/main/java/org/cryptomator/hub/api/VaultResource.java |
Updates get() method to use @VaultRole annotation instead of manual access checks |
backend/src/main/resources/org/cryptomator/hub/flyway/V21__Optimize_Group_Membership.sql |
Migration that converts effective_group_membership from view to table with indexes and populates it with recursive query |
backend/src/test/resources/org/cryptomator/hub/flyway/V9999__Test_Data.sql |
Adds test data for effective_group_membership table |
backend/src/test/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPullerTest.java |
Updates tests to verify bulk persist operations and effective membership updates |
backend/src/test/java/org/cryptomator/hub/filters/VaultRoleFilterTest.java |
Updates exception type expectation from ForbiddenException to NotFoundException |
backend/src/test/java/org/cryptomator/hub/api/VaultResourceIT.java |
Refactors test setup/cleanup to use entity repositories instead of raw SQL, adds shared test data setup |
backend/src/test/java/org/cryptomator/hub/api/GroupsResourceIT.java |
Refactors test setup/cleanup to use entity repositories instead of raw SQL |
backend/src/test/java/org/cryptomator/hub/api/ExceedingLicenseLimitsIT.java |
New test class extracted from VaultResourceIT to test license limit scenarios with proper setup/cleanup using entity repositories |
Comments suppressed due to low confidence (1)
backend/src/main/java/org/cryptomator/hub/entities/Group.java:31
- getMembers exposes the internal representation stored in field members. The value may be modified after this call to getMembers.
getMembers exposes the internal representation stored in field members. The value may be modified after this call to getMembers.
getMembers exposes the internal representation stored in field members. The value may be modified after this call to getMembers.
getMembers exposes the internal representation stored in field members. The value may be modified after this call to getMembers.
getMembers exposes the internal representation stored in field members. The value may be modified after this call to getMembers.
getMembers exposes the internal representation stored in field members. The value may be modified after this call to getMembers.
getMembers exposes the internal representation stored in field members. The value may be modified after this call to getMembers.
getMembers exposes the internal representation stored in field members. The value may be modified after this call to getMembers.
getMembers exposes the internal representation stored in field members. The value may be modified after this call to getMembers.
public Set<Authority> getMembers() {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
backend/src/test/java/org/cryptomator/hub/api/ExceedingLicenseLimitsIT.java
Show resolved
Hide resolved
backend/src/test/java/org/cryptomator/hub/api/ExceedingLicenseLimitsIT.java
Outdated
Show resolved
Hide resolved
backend/src/test/java/org/cryptomator/hub/api/ExceedingLicenseLimitsIT.java
Outdated
Show resolved
Hide resolved
backend/src/test/java/org/cryptomator/hub/api/ExceedingLicenseLimitsIT.java
Outdated
Show resolved
Hide resolved
backend/src/test/java/org/cryptomator/hub/api/VaultResourceIT.java
Outdated
Show resolved
Hide resolved
backend/src/test/java/org/cryptomator/hub/api/ExceedingLicenseLimitsIT.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/src/main/java/org/cryptomator/hub/entities/Vault.java (1)
262-267: Stream concatenation in a loop may cause performance issues.Using
Stream.concatrepeatedly in a loop creates a deeply nested chain of lazy streams. For large result sets or many batches, this can lead to:
- Stack overflow during terminal operations due to recursive evaluation
- Increased memory overhead from wrapper streams
Consider collecting to a list instead:
public Stream<Vault> findAllInList(List<UUID> ids) { - return Batch.of(200).run(ids, Stream.of(), (batch, result) -> { - Stream<Vault> partialResult = find("#Vault.allInList", Parameters.with("ids", batch)).stream(); - return Stream.concat(result, partialResult); - }); + List<Vault> results = new java.util.ArrayList<>(); + Batch.of(200).run(ids, (batch) -> { + find("#Vault.allInList", Parameters.with("ids", batch)).stream().forEach(results::add); + }); + return results.stream(); }Alternatively, if you want to maintain lazy streaming semantics, consider using
Stream.flatMapwith a supplier that yields batch results on demand.backend/src/test/java/org/cryptomator/hub/api/VaultResourceIT.java (1)
97-127: Consider cleaning up group2 modifications in teardown.The
setupTestDatamethod addsuser998anduser999to the existinggroup2(lines 114-117), butcleanupTestDataonly deletes the users without removing them from group2 first. While cascade deletes handle related database tables (per learnings), the in-memorygroup2entity's members collection may still reference deleted users if group2 is accessed later in the test lifecycle.To ensure full cleanup symmetry, consider removing the users from group2 before deletion:
@AfterEach @Transactional public void cleanupTestData() { + var group2 = groupRepo.findById("group2"); + if (group2 != null) { + group2.getMembers().removeIf(u -> Set.of("user998", "user999").contains(u.getId())); + groupRepo.persist(group2); + } userRepo.deleteByIds(List.of("user998", "user999")); }Note: This is only necessary if group2's state matters for other tests that might access it after cleanup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/src/main/java/org/cryptomator/hub/entities/Batch.java(1 hunks)backend/src/main/java/org/cryptomator/hub/entities/EffectiveVaultAccess.java(2 hunks)backend/src/main/java/org/cryptomator/hub/entities/Vault.java(1 hunks)backend/src/test/java/org/cryptomator/hub/api/ExceedingLicenseLimitsIT.java(1 hunks)backend/src/test/java/org/cryptomator/hub/api/VaultResourceIT.java(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/main/java/org/cryptomator/hub/entities/Batch.java
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-08T11:37:46.953Z
Learnt from: overheadhunter
Repo: cryptomator/hub PR: 372
File: backend/src/test/java/org/cryptomator/hub/api/ExceedingLicenseLimitsIT.java:116-121
Timestamp: 2025-12-08T11:37:46.953Z
Learning: In the Cryptomator Hub backend schema, foreign key constraints use ON DELETE CASCADE for authority/user references. Specifically, deleting users or groups from the authority table automatically cascades to delete related rows in vault_access and access_token tables. Test cleanup methods that delete users/groups via repositories do not need explicit cleanup of these dependent tables.
Applied to files:
backend/src/test/java/org/cryptomator/hub/api/ExceedingLicenseLimitsIT.javabackend/src/main/java/org/cryptomator/hub/entities/Vault.javabackend/src/test/java/org/cryptomator/hub/api/VaultResourceIT.java
📚 Learning: 2024-10-17T15:24:07.752Z
Learnt from: overheadhunter
Repo: cryptomator/hub PR: 296
File: backend/src/test/java/org/cryptomator/hub/filters/VaultRoleFilterTest.java:183-186
Timestamp: 2024-10-17T15:24:07.752Z
Learning: In `VaultRoleFilterTest`, remember that the `securityContext` mock is set in the outer class's setup method at line 46, which applies to nested classes as well. Avoid suggesting redundant setup in nested classes.
Applied to files:
backend/src/test/java/org/cryptomator/hub/api/ExceedingLicenseLimitsIT.javabackend/src/test/java/org/cryptomator/hub/api/VaultResourceIT.java
🧬 Code graph analysis (1)
backend/src/main/java/org/cryptomator/hub/entities/Vault.java (1)
backend/src/main/java/org/cryptomator/hub/entities/Batch.java (1)
Batch(8-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Tests
🔇 Additional comments (5)
backend/src/main/java/org/cryptomator/hub/entities/EffectiveVaultAccess.java (1)
156-161: Deduplication withSet.copyOfaddresses the double-counting concern.Using
Set.copyOf(userIds)before batching ensures each user ID is counted only once across all batches. This correctly resolves the previously flagged issue where duplicates could lead to over-counting occupied seats.Note that
Set.copyOf()will throwNullPointerExceptionifuserIdsis null or contains null elements. If upstream callers could pass nulls, consider adding validation or documenting the precondition.backend/src/test/java/org/cryptomator/hub/api/VaultResourceIT.java (1)
630-634: LGTM - assertion correctly reflects new test data.The updated assertion from
memberSize=2tomemberSize=3correctly accounts for the two users (user998anduser999) now added togroup2in thesetupTestDatamethod.backend/src/test/java/org/cryptomator/hub/api/ExceedingLicenseLimitsIT.java (3)
184-200: Raw SQL insertion bypasses JPA but acceptable for integration testing.The test uses raw SQL to insert into
vault_access(lines 186-190) to set up a specific scenario. While this bypasses JPA/Hibernate entity management and could lead to cache inconsistencies, it's acceptable for integration tests that need to create precise database states.Based on learnings, the cleanup method correctly relies on cascade deletes to remove these entries when the group is deleted.
218-237: Raw SQL insertion pattern is consistent and cleanup will handle it.Similar to the previous test, this test uses raw SQL to insert multiple
access_tokenentries (lines 220-229). The cleanup approach is correct—deleting the users will cascade to remove these access tokens per the schema's foreign key constraints.Based on learnings, cascade delete behavior ensures test cleanup is complete.
65-106: Test data setup is well-structured.The setup correctly:
- Creates 5 test users with appropriate attributes
- Builds a group with 4 members
- Updates effective group memberships
- Establishes vault access for testing archived vault scenarios
The approach of using repositories for setup rather than raw SQL makes the test more maintainable and aligned with the application's data access patterns.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/src/test/java/org/cryptomator/hub/entities/EffectiveGroupMembershipIT.java (1)
70-89: Missing assertion for u4's effective membership in g1.Based on the hierarchy (g1 ← g2 ← u4), user
u4should be an effective member ofg1. Consider adding this assertion for completeness:Assertions.assertTrue(effectiveGroupMembershipRepo.isMember("g1", "u1")); Assertions.assertTrue(effectiveGroupMembershipRepo.isMember("g1", "u2")); Assertions.assertTrue(effectiveGroupMembershipRepo.isMember("g1", "u3")); +Assertions.assertTrue(effectiveGroupMembershipRepo.isMember("g1", "u4")); Assertions.assertTrue(effectiveGroupMembershipRepo.isMember("g2", "u2"));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/src/main/java/org/cryptomator/hub/api/VaultResource.java(1 hunks)backend/src/main/java/org/cryptomator/hub/entities/EffectiveGroupMembership.java(2 hunks)backend/src/main/resources/org/cryptomator/hub/flyway/V21__Optimize_Group_Membership.sql(1 hunks)backend/src/test/java/org/cryptomator/hub/api/GroupsResourceIT.java(1 hunks)backend/src/test/java/org/cryptomator/hub/entities/EffectiveGroupMembershipIT.java(1 hunks)backend/src/test/resources/org/cryptomator/hub/flyway/V9999__Test_Data.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/test/resources/org/cryptomator/hub/flyway/V9999__Test_Data.sql
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-08T11:37:46.953Z
Learnt from: overheadhunter
Repo: cryptomator/hub PR: 372
File: backend/src/test/java/org/cryptomator/hub/api/ExceedingLicenseLimitsIT.java:116-121
Timestamp: 2025-12-08T11:37:46.953Z
Learning: In the Cryptomator Hub backend schema, foreign key constraints use ON DELETE CASCADE for authority/user references. Specifically, deleting users or groups from the authority table automatically cascades to delete related rows in vault_access and access_token tables. Test cleanup methods that delete users/groups via repositories do not need explicit cleanup of these dependent tables.
Applied to files:
backend/src/main/resources/org/cryptomator/hub/flyway/V21__Optimize_Group_Membership.sqlbackend/src/test/java/org/cryptomator/hub/api/GroupsResourceIT.java
🧬 Code graph analysis (2)
backend/src/main/java/org/cryptomator/hub/api/VaultResource.java (1)
frontend/src/common/backend.ts (1)
VaultRole(64-64)
backend/src/main/java/org/cryptomator/hub/entities/EffectiveGroupMembership.java (2)
backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java (1)
ApplicationScoped(18-147)backend/src/main/java/org/cryptomator/hub/entities/Batch.java (1)
Batch(8-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Tests
🔇 Additional comments (10)
backend/src/main/resources/org/cryptomator/hub/flyway/V21__Optimize_Group_Membership.sql (5)
5-5: Indexes well-designed for expected query patterns.The addition of
GROUP_MEMBERSHIP_IDX_MEMBER(line 5) and the two indexes oneffective_group_membership(lines 24, 27) are appropriately chosen: member_id for bottom-up lookups and GIN index on the array column for filtering by intermediate groups. Combined with the primary key on (group_id, member_id), this covers both top-down and bottom-up access patterns efficiently.Also applies to: 24-24, 27-27
12-20: Table structure and constraints correctly implement cascade deletion semantics.The foreign key constraints on both
group_idandmember_id(lines 17-18) with ON DELETE CASCADE are consistent with the established pattern documented in the codebase. The CHECK constraint (line 19) prevents self-membership, which is logically sound. The composite primary key on (group_id, member_id) is appropriate for the effective membership model.
31-39: Recursive CTE logic correctly computes transitive group closures with depth bounding.The CTE correctly builds the transitive closure of group memberships:
- Base case (line 32): Seeds with direct group memberships, initializing intermediate_group_ids with the root group.
- Recursive case (lines 35-37): Properly accumulates the path through intermediate groups as it traverses up the hierarchy.
- Depth limit of 10 (line 38) prevents infinite recursion and bounds the intermediate_group_ids array size.
Verify that this depth limit accommodates your expected group nesting scenarios.
40-40: ON CONFLICT DO NOTHING appropriately handles duplicates in transitive closure.Using
ON CONFLICT DO NOTHING(line 40) is safe and idempotent, preventing duplicate inserts without throwing errors if the migration is run multiple times during development/testing.
42-46: View correctly unifies direct and group-based vault access.The recreated
effective_vault_accessview properly handles two access resolution paths:
- Direct vault access (line 43): Users/groups with direct permissions.
- Transitive group access (lines 45-46): Members of groups that have vault permissions.
The UNION prevents duplicates if a member has both direct and group-based access to the same vault.
backend/src/main/java/org/cryptomator/hub/api/VaultResource.java (1)
384-396: Clean declarative authorization refactor.The annotation-based approach properly configures vault access: members/owners can access, admins can bypass via realm role, and missing vaults return 404 instead of 403. The inline
NotFoundExceptionon line 394 provides a reasonable defense-in-depth fallback.backend/src/test/java/org/cryptomator/hub/entities/EffectiveGroupMembershipIT.java (1)
29-68: Well-structured test setup and teardown.The hierarchical group structure is clearly documented, and the setup/teardown methods properly manage test isolation. The reliance on
ON DELETE CASCADEfor cleanup is correct per the schema.backend/src/test/java/org/cryptomator/hub/api/GroupsResourceIT.java (1)
46-72: Clean repository-based test setup.The refactor from JDBC to repository-driven setup improves readability and consistency with other tests. The effective membership update via
updateGroupsensures test assertions work correctly.backend/src/main/java/org/cryptomator/hub/entities/EffectiveGroupMembership.java (2)
19-69: Well-designed recursive CTEs with appropriate safeguards.The queries properly handle:
- Depth limit of 10 to prevent infinite recursion in cyclic group structures
ON CONFLICT DO NOTHINGfor idempotency- PostgreSQL-specific array operations for efficient deletion
Note: The
deleteGroupsquery uses PostgreSQL's array overlap operator (&&), coupling this to PostgreSQL. This is fine if PostgreSQL is the target database.
111-152: Solid batch processing implementation.The repository correctly:
- Uses
Batch.of(200)to chunk operations and avoid parameter limits- Converts to
String[]for the PostgreSQL array overlap operator indeleteGroups- Passes collection directly for
INclauses in insert queries- Provides a package-visible
isMemberhelper for testing
# Conflicts: # backend/src/test/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPullerTest.java
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java (1)
130-137: Missing null check for consistency withsyncAddedGroups.Lines 101-103 in
syncAddedGroupsguard against null users, but here insyncUpdatedGroupsthedatabaseUserfromfindById(line 134) could be null and would be added directly to the members set.for (var addId : diff(wantIds, haveIds)) { var databaseUser = databaseUsers.get(addId); if (databaseUser == null) { // User might have been just added, fetch from database databaseUser = userRepo.findById(addId); } - databaseGroup.getMembers().add(databaseUser); + if (databaseUser != null) { + databaseGroup.getMembers().add(databaseUser); + } }
🧹 Nitpick comments (2)
backend/src/test/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPullerTest.java (1)
97-108: Consider stricter argument matching for verification.
Mockito.argThat(addedUserIds::containsAll)at line 98 (and similar patterns elsewhere) only verifies that the actual argument contains all expected IDs—it won't fail if extra unexpected IDs are passed. For stricter verification, consider usingMatchers.equalTo(addedUserIds)or a bidirectional containment check.- Mockito.verify(effectiveGroupMembershipRepo).updateUsers(Mockito.argThat(addedUserIds::containsAll)); + Mockito.verify(effectiveGroupMembershipRepo).updateUsers(addedUserIds);backend/src/main/resources/org/cryptomator/hub/flyway/V22__Optimize_Group_Membership.sql (1)
30-40: The depth limit of 10 is an intentional design choice and is reasonable for group hierarchies.PostgreSQL best practices recommend explicit depth limits in recursive CTEs to prevent infinite loops and row explosion. Industry standards for group nesting are typically 3–4 levels deep, so a limit of 10 provides a safe margin. The current implementation correctly implements this safeguard across all three membership query variants (fullUpdate, updateGroups, updateUsers).
However, adding a comment explaining this design choice would improve code clarity for future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/src/main/java/org/cryptomator/hub/api/VaultResource.java(1 hunks)backend/src/main/java/org/cryptomator/hub/entities/Authority.java(2 hunks)backend/src/main/java/org/cryptomator/hub/entities/User.java(2 hunks)backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java(5 hunks)backend/src/main/resources/org/cryptomator/hub/flyway/V22__Optimize_Group_Membership.sql(1 hunks)backend/src/test/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPullerTest.java(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/main/java/org/cryptomator/hub/entities/User.java
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: overheadhunter
Repo: cryptomator/hub PR: 372
File: backend/src/test/java/org/cryptomator/hub/api/ExceedingLicenseLimitsIT.java:116-121
Timestamp: 2025-12-08T11:37:46.953Z
Learning: In the Cryptomator Hub backend schema, foreign key constraints use ON DELETE CASCADE for authority/user references. Specifically, deleting users or groups from the authority table automatically cascades to delete related rows in vault_access and access_token tables. Test cleanup methods that delete users/groups via repositories do not need explicit cleanup of these dependent tables.
📚 Learning: 2025-06-06T09:18:42.468Z
Learnt from: SailReal
Repo: cryptomator/hub PR: 343
File: backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java:27-32
Timestamp: 2025-06-06T09:18:42.468Z
Learning: In the KeycloakAuthorityPuller and similar syncer components, don't add broad exception handling to scheduled methods. The Syncer execution context is designed to handle crashes, and it's better to let exceptions propagate rather than catching them broadly. Catching Exception is considered bad practice in this codebase.
Applied to files:
backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java
📚 Learning: 2025-12-08T11:37:46.953Z
Learnt from: overheadhunter
Repo: cryptomator/hub PR: 372
File: backend/src/test/java/org/cryptomator/hub/api/ExceedingLicenseLimitsIT.java:116-121
Timestamp: 2025-12-08T11:37:46.953Z
Learning: In the Cryptomator Hub backend schema, foreign key constraints use ON DELETE CASCADE for authority/user references. Specifically, deleting users or groups from the authority table automatically cascades to delete related rows in vault_access and access_token tables. Test cleanup methods that delete users/groups via repositories do not need explicit cleanup of these dependent tables.
Applied to files:
backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.javabackend/src/test/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPullerTest.javabackend/src/main/resources/org/cryptomator/hub/flyway/V22__Optimize_Group_Membership.sql
🧬 Code graph analysis (2)
backend/src/main/java/org/cryptomator/hub/entities/Authority.java (1)
backend/src/main/java/org/cryptomator/hub/entities/Batch.java (1)
Batch(8-50)
backend/src/main/java/org/cryptomator/hub/api/VaultResource.java (1)
frontend/src/common/backend.ts (1)
VaultRole(64-64)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Tests
🔇 Additional comments (9)
backend/src/main/java/org/cryptomator/hub/api/VaultResource.java (1)
386-388: Verify@RolesAllowed("user")doesn’t block realm-admin bypass.
Even withbypassForRealmRole = true, realmRole = "admin", callers still must satisfy@RolesAllowed("user"). If “admin” tokens/users might not also carry “user”, the bypass won’t be reachable.If that’s a possibility in your deployment, consider expanding the roles allowed here (or ensure role mapping guarantees admin ⊇ user).
backend/src/main/java/org/cryptomator/hub/entities/Authority.java (1)
102-107: LGTM! Batched querying approach looks correct.The batching implementation properly handles empty collections and aggregates results. One minor note:
Stream.concatcreates a nested structure that could theoretically cause stack depth issues with very large collections (thousands of batches), but with a batch size of 200, this would require 200k+ IDs to become problematic—unlikely in practice.backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java (2)
27-28: Good integration of EffectiveGroupMembership repository.The pattern of persisting/deleting entities first, then updating effective memberships maintains correct data consistency. The batch-oriented approach aligns well with the PR's optimization goals.
Also applies to: 61-62, 68-69, 108-109, 115-116
101-103: Good null safety addition here.The null check before adding to
memberscorrectly handles the edge case where a user might not exist in the database.backend/src/test/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPullerTest.java (2)
44-63: Well-structured test setup for capturing streamed persists.The
doAnswerpattern correctly captures entities from streams for later verification, enabling assertions on the actual persisted objects rather than just mock interactions.
232-244: Assertions correctly validate persisted group properties.The nested Hamcrest matchers verify all relevant properties including
memberscontent, providing good coverage of the batch persistence behavior.backend/src/main/resources/org/cryptomator/hub/flyway/V22__Optimize_Group_Membership.sql (3)
1-5: Good index addition for bottom-up lookups.The index on
member_idwill significantly improve queries that need to find all groups a member belongs to.
12-20: Well-designed table structure with appropriate constraints.The composite primary key, CASCADE foreign keys, and self-membership check constraint provide good data integrity. The CASCADE behavior aligns with the existing schema patterns (per learnings about authority deletion).
42-46: Clean view recreation using materialized membership data.The
UNIONcorrectly combines direct vault access with group-derived access. Using the materializedeffective_group_membershiptable instead of recursive lookups at query time will improve read performance significantly.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java (1)
121-147: Bug: possiblenullmember added in updated-groups path (Line 137).
UnlikesyncAddedGroups,syncUpdatedGroupsaddsdatabaseUserwithout a null-check; if the user can’t be found, you’ll add null (or NPE later / violate DB constraints).@@ for (var addId : diff(wantIds, haveIds)) { var databaseUser = databaseUsers.get(addId); if (databaseUser == null) { // User might have been just added, fetch from database databaseUser = userRepo.findById(addId); } - databaseGroup.getMembers().add(databaseUser); + if (databaseUser != null) { + databaseGroup.getMembers().add(databaseUser); + } } @@ - if (!wantIds.containsAll(haveIds) || !haveIds.containsAll(wantIds)) { + if (!wantIds.equals(haveIds)) { idsOfGroupsWithChangedMembers.add(id); } } - effectiveGroupMembershipRepo.updateGroups(idsOfGroupsWithChangedMembers); + if (!idsOfGroupsWithChangedMembers.isEmpty()) { + effectiveGroupMembershipRepo.updateGroups(idsOfGroupsWithChangedMembers); + } }
🧹 Nitpick comments (1)
backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java (1)
86-110: Potential N+1 lookups when resolving members; consider keepingdatabaseUserswarm.
The null-guard is good, but the per-memberuserRepo.findById(...)fallback can get expensive becausedatabaseUsersdoes not include just-added users. Consider collecting added users into a list, persisting them, and inserting them intodatabaseUsersso member resolution stays in-memory.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-06T09:18:42.468Z
Learnt from: SailReal
Repo: cryptomator/hub PR: 343
File: backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java:27-32
Timestamp: 2025-06-06T09:18:42.468Z
Learning: In the KeycloakAuthorityPuller and similar syncer components, don't add broad exception handling to scheduled methods. The Syncer execution context is designed to handle crashes, and it's better to let exceptions propagate rather than catching them broadly. Catching Exception is considered bad practice in this codebase.
Applied to files:
backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Tests
🔇 Additional comments (3)
backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java (3)
8-29: New EffectiveGroupMembership wiring looks fine; good job not adding broad exception handling.
The injection/use fits the syncer pattern, and (per prior feedback) you kept exception propagation intact.
113-118: No action required. TheupdateGroups()method queries only thegroup_membershiptable via a recursive CTE and does not depend on group rows existing. Deleting from the groups table before callingupdateGroups()is safe.
50-63: Verifypersist(Stream)+ flush semantics beforeeffectiveGroupMembershipRepo.updateUsers(...).
IfupdateUsers()queries the DB, it may not see newly persisted users without explicit flush. Also, thedatabaseUsersparameter passed to this method remains stale, causing later code to need per-id DB lookups instead of using the in-memory map.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/src/main/java/org/cryptomator/hub/entities/Batch.java (1)
12-24: Validation correctly prevents infinite loop, but is duplicated.The validation for positive batch size correctly addresses the infinite loop concern raised in the past review. However, the check is duplicated in both the static factory method and the private constructor. Since
of(int)always calls the constructor, only the constructor validation is necessary.Apply this diff to remove the redundant validation in the factory method:
public static Batch of(int size) { - if (size <= 0) { - throw new IllegalArgumentException("Batch size must be positive"); - } return new Batch(size); }backend/src/test/java/org/cryptomator/hub/entities/BatchTest.java (1)
24-31: Good batch size coverage.The parameterized test effectively validates that all elements are processed correctly across various batch sizes. All tests use
List.of(), which exercises the optimized path but not theList.copyOf()fallback for non-List collections.Consider adding a test case with a non-List collection (e.g.,
Set.of(...)) to ensureList.copyOf()works correctly:@Test public void testWithNonListCollection() { List<Integer> result = new ArrayList<>(); Batch.of(3).run(Set.of(1, 2, 3, 4, 5), result::addAll); Assertions.assertEquals(5, result.size()); Assertions.assertTrue(result.containsAll(List.of(1, 2, 3, 4, 5))); }backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java (1)
123-123: Good optimization: Track only groups with changed members.The selective update of groups with membership changes reduces unnecessary work. The equality check is correct, though it could be simplified.
Consider simplifying the set equality check for readability:
-if (!wantIds.containsAll(haveIds) || !haveIds.containsAll(wantIds)) { +if (!wantIds.equals(haveIds)) { idsOfGroupsWithChangedMembers.add(id); }Also applies to: 144-148
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/src/main/java/org/cryptomator/hub/entities/Batch.java(1 hunks)backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java(6 hunks)backend/src/test/java/org/cryptomator/hub/entities/BatchTest.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-06T09:18:42.468Z
Learnt from: SailReal
Repo: cryptomator/hub PR: 343
File: backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java:27-32
Timestamp: 2025-06-06T09:18:42.468Z
Learning: In the KeycloakAuthorityPuller and similar syncer components, don't add broad exception handling to scheduled methods. The Syncer execution context is designed to handle crashes, and it's better to let exceptions propagate rather than catching them broadly. Catching Exception is considered bad practice in this codebase.
Applied to files:
backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java
🧬 Code graph analysis (1)
backend/src/test/java/org/cryptomator/hub/entities/BatchTest.java (1)
backend/src/main/java/org/cryptomator/hub/entities/Batch.java (1)
Batch(8-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Tests
🔇 Additional comments (13)
backend/src/main/java/org/cryptomator/hub/entities/Batch.java (2)
26-32: LGTM!The batching logic is correct and efficient. The instanceof pattern matching avoids unnecessary copying when the collection is already a List, and
Math.minproperly handles the last batch boundary.
34-42: LGTM!The accumulator-based run method correctly implements a fold operation over batches. The result is properly threaded through each iteration.
backend/src/test/java/org/cryptomator/hub/entities/BatchTest.java (4)
16-22: LGTM!Properly tests the validation for invalid batch sizes, ensuring both negative and zero values are rejected.
33-40: LGTM!Properly validates that the Consumer is never invoked when the input collection is empty, preventing unnecessary work.
42-53: LGTM!Effectively tests the accumulator variant across multiple batch sizes, ensuring that the fold operation correctly aggregates results across batch boundaries.
55-62: LGTM!Properly validates that the BiFunction is never invoked when the input collection is empty, consistent with the Consumer variant test.
backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java (7)
8-8: LGTM: Dependency injection for effective membership tracking.The new repository dependency is correctly injected and used consistently throughout the sync methods to propagate membership changes.
Also applies to: 27-28
68-69: LGTM: Batch deletion with membership update.The batch deletion followed by membership state propagation is correctly implemented.
101-103: Good defensive programming: Null check prevents corrupt membership.The null guard ensures that only resolved users are added to the group's member set, preventing potential NPEs or data integrity issues downstream.
115-116: LGTM: Batch deletion with membership update.The batch deletion followed by membership propagation is correctly implemented.
137-139: Good defensive programming: Consistent null check.The null guard is consistent with the one added in
syncAddedGroupsand prevents corrupt membership state.
52-62: Verify that the persist method accepts Stream.The refactored code passes a
Stream<User>directly touserRepo.persist(). Ensure that the repository's persist method accepts Stream or Iterable parameters. If it only accepts individual entities, this will fail at runtime or only persist the first element.Run the following script to check the persist method signature in the User.Repository:
#!/bin/bash # Description: Check the persist method signature in User.Repository # Search for User.Repository class definition and persist method ast-grep --pattern $'class Repository { $$$ persist($$$) { $$$ } $$$ }' # Also search for persist method declarations in User entity rg -nP --type=java -A5 'class\s+User\s*\{' | head -50 # Search for persist method in repository base classes rg -nP --type=java 'persist\s*\([^)]*Stream'
88-109: No action required. Thepersist()method in Quarkus's PanacheRepositoryBase accepts Stream parameters. Both the production code and unit tests confirm that passingStream<Group>togroupRepo.persist()is valid and working correctly.Likely an incorrect or invalid review comment.
1 participant
This pull request introduces several improvements to batch processing for database operations, refactors group and user synchronization logic, and enhances the access control mechanism for vaults. The most important changes are grouped below by theme.
Batch Processing Improvements:
Batchto handle batch operations on collections, improving efficiency for database queries and updates. Several repository methods now use this class to process large lists in batches.Authority,Device,Vault,EffectiveVaultAccess,Group, andUserto utilize the newBatchclass for efficient batch querying and deletion [1] [2] [3] [4] [5] [6].Group and User Synchronization Refactor:
KeycloakAuthorityPullersynchronization logic to persist added users and groups in bulk and update effective group memberships in batches, rather than processing each item individually. Deletions are also batched [1] [2] [3].EffectiveGroupMembership.Repositoryclass with methods for full and partial updates and deletions of group memberships, utilizing batch operations and new named queries.Access Control Enhancements:
VaultRoleannotation andVaultRoleFilterto allow bypassing vault-specific role checks if the user has a specified realm role (e.g., "admin"), simplifying access logic and making it more flexible [1] [2] [3].Entity Model Cleanup:
effectiveMembersfield and related methods from theVaultentity, as it became dead code after adding@VaultRoletoVaultResource.get(...)[1] [2].These changes collectively improve performance, maintainability, and flexibility in handling large sets of users, groups, and access control.