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. WalkthroughThe PR adds a Batch utility for chunked processing and applies batch-based data access (batch size 200) across multiple repositories (Authority, Device, Vault, Group, User, EffectiveVaultAccess). It introduces a concrete effective_group_membership table (replacing the view), adds indexes and named/native queries, and adds an EffectiveGroupMembership.Repository with batch update/delete methods. Vault entity no longer exposes effectiveMembers. VaultRole gains a bypassForRealmRole element and VaultRoleFilter short-circuits checks when the realm role applies and returns NotFound for invalid vault IDs. KeycloakAuthorityPuller switches to batched persist/delete and calls EffectiveGroupMembership updates. Tests and test data setup/teardown are updated; new integration tests for effective group membership and license limits are added. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes
Possibly related PRs
Suggested reviewers
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
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.