Automated Test: feature-group-concurrency-implementation

[admin-coderabbit]

This pull request was automatically created by @coderabbitai/e2e-reviewer.

Batch created pull request.

Summary by CodeRabbit

• Bug Fixes

  • Improved null-safety in group operations when retrieving subgroup counts.

• Tests

  • Added concurrent access testing for group creation, deletion, and read operations.

[coderabbit-eval]

📝 Walkthrough

Walkthrough

This pull request refactors group caching and retrieval logic to improve null-safety, adds clarity to method overrides, removes an unused helper method, and introduces concurrency testing for group operations. The changes span cache layer optimization, utility cleanup, and concurrent access validation.

Changes

Cohort / File(s)	Summary
Cache Layer Improvements model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/GroupAdapter.java, model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedGroup.java	Modified group retrieval to delegate to modelSupplier.get() with null-safety check in getSubGroupsCount(), and added @Override annotation to getRealm() method for clarity.
Utility Cleanup services/src/main/java/org/keycloak/utils/GroupUtils.java	Removed unused private helper method groupMatchesSearchOrIsPathElement() that performed search term and subgroup matching logic.
Concurrency Testing tests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java	Added new test method createMultiDeleteMultiReadMulti() that verifies concurrent read operations on groups while deletions occur simultaneously, using atomic primitives and exception capture.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 With caches cleaned and null-checks tight,
Groups leap through threads both left and right,
Helper methods fade to dust away,
While tests ensure no race delays!
A safer, swifter warren bright!

🚥 Pre-merge checks | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description is minimal and provides no substantive information about what changes were made, why they were made, or their impact.	Provide a detailed description explaining the purpose of the changes, what was modified, why it was necessary, and any testing considerations.
Title check	❓ Inconclusive	The title 'Automated Test: feature-group-concurrency-implementation' is vague and does not clearly describe the main changes in the pull request.	Replace with a descriptive title that summarizes the primary change, such as 'Add concurrent group read/delete test and fix GroupAdapter null handling'

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feature-group-concurrency-implementation

---

_{Comment @coderabbit eval help to get the list of available commands and usage tips.}

[coderabbit-eval]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/GroupAdapter.java (1)

253-269: ⚠️ Potential issue | 🟠 Major

Inconsistent null-safety: similar methods lack null checks.

The getSubGroupsCount() method now includes a null-safe check on modelSupplier.get(), but the adjacent overloaded getSubGroupsStream() methods (lines 256, 262, 268) call modelSupplier.get().getSubGroupsStream(...) directly without null checks. If concurrent deletion can cause a null model in one case, it can in all.

Consider applying the same defensive pattern to these methods for consistency:

🛡️ Proposed fix for consistent null-safety

     `@Override`
     public Stream<GroupModel> getSubGroupsStream(String search, Integer firstResult, Integer maxResults) {
         if (isUpdated()) return updated.getSubGroupsStream(search, firstResult, maxResults);
-        return modelSupplier.get().getSubGroupsStream(search, firstResult, maxResults);
+        GroupModel model = modelSupplier.get();
+        return model == null ? Stream.empty() : model.getSubGroupsStream(search, firstResult, maxResults);
     }

     `@Override`
     public Stream<GroupModel> getSubGroupsStream(Integer firstResult, Integer maxResults) {
         if (isUpdated()) return updated.getSubGroupsStream(firstResult, maxResults);
-        return modelSupplier.get().getSubGroupsStream(firstResult, maxResults);
+        GroupModel model = modelSupplier.get();
+        return model == null ? Stream.empty() : model.getSubGroupsStream(firstResult, maxResults);
     }

     `@Override`
     public Stream<GroupModel> getSubGroupsStream(String search, Boolean exact, Integer firstResult, Integer maxResults) {
         if (isUpdated()) return updated.getSubGroupsStream(search, exact, firstResult, maxResults);
-        return modelSupplier.get().getSubGroupsStream(search, exact, firstResult, maxResults);
+        GroupModel model = modelSupplier.get();
+        return model == null ? Stream.empty() : model.getSubGroupsStream(search, exact, firstResult, maxResults);
     }

🤖 Fix all issues with AI agents

In `@tests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java`:
- Line 121: Rename the misspelled variable groupUuuids to groupUuids in the
GroupTest class: update the declaration (List<String> groupUuuids = new
ArrayList<>();) and every usage of groupUuuids (including the two later
references mentioned) to the corrected identifier groupUuids so the code
compiles and reads correctly.
- Around line 118-158: The reader thread in createMultiDeleteMultiReadMulti is
never joined, so the test can finish before it completes; change the anonymous
Thread to a named Thread (e.g., reader), start it, perform deletions as-is, set
deletedAll.set(true), then call reader.join() and handle InterruptedException
(fail the test or re-interrupt) so any exceptions added to caughtExceptions
after deletions are observed before assertThat runs; keep the existing
caughtExceptions and deletedAll variables and the
managedRealm.admin().groups().groups(...) read loop unchanged.

🧹 Nitpick comments (1)

tests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java (1)

144-147: Empty catch block swallows exception details during debugging.

The blank line in the catch block (line 145) appears unintentional. While exceptions are captured in caughtExceptions, consider logging them for easier debugging if the test fails.

📝 Proposed improvement

                 } catch (Exception e) {
-
                     caughtExceptions.add(e);
+                    caughtExceptions.add(e);
+                    // Optionally log for debugging: System.err.println("Read exception: " + e.getMessage());
                 }

[coderabbit-eval]

⚠️ Potential issue | 🟠 Major

Thread is not joined: test may complete before reader thread finishes.

The reader thread (lines 139-149) is started but never joined. The test assertion at line 157 may execute before the thread has finished its final iteration, potentially missing exceptions. Additionally, if an exception occurs in the final read after deletedAll is set to true, it won't be captured.

🧵 Proposed fix to properly synchronize thread completion

         AtomicBoolean deletedAll = new AtomicBoolean(false);
         List<Exception> caughtExceptions = new CopyOnWriteArrayList<>();
         // read groups in a separate thread
-        new Thread(() -> {
+        Thread readerThread = new Thread(() -> {
             while (!deletedAll.get()) {
                 try {
                     // just loading briefs
                     managedRealm.admin().groups().groups(null, 0, Integer.MAX_VALUE, true);
                 } catch (Exception e) {
-
                     caughtExceptions.add(e);
                 }
             }
-        }).start();
+        });
+        readerThread.start();

         // delete groups
-        groupUuuids.forEach(groupUuid -> {
+        groupUuuids.forEach(groupUuid -> {
             managedRealm.admin().groups().group(groupUuid).remove();
         });
         deletedAll.set(true);

+        // Wait for reader thread to complete
+        try {
+            readerThread.join(5000); // 5 second timeout
+        } catch (InterruptedException e) {
+            Thread.currentThread().interrupt();
+            fail("Reader thread interrupted");
+        }
+
         assertThat(caughtExceptions, Matchers.empty());

🤖 Prompt for AI Agents

In `@tests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java`
around lines 118 - 158, The reader thread in createMultiDeleteMultiReadMulti is
never joined, so the test can finish before it completes; change the anonymous
Thread to a named Thread (e.g., reader), start it, perform deletions as-is, set
deletedAll.set(true), then call reader.join() and handle InterruptedException
(fail the test or re-interrupt) so any exceptions added to caughtExceptions
after deletions are observed before assertThat runs; keep the existing
caughtExceptions and deletedAll variables and the
managedRealm.admin().groups().groups(...) read loop unchanged.

[coderabbit-eval]

⚠️ Potential issue | 🟡 Minor

Typo in variable name: groupUuuids should be groupUuids.

The variable has an extra 'u'.

✏️ Proposed fix

-        List<String> groupUuuids = new ArrayList<>();
+        List<String> groupUuids = new ArrayList<>();

Also update references at lines 129 and 152.

🤖 Prompt for AI Agents

In `@tests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java` at
line 121, Rename the misspelled variable groupUuuids to groupUuids in the
GroupTest class: update the declaration (List<String> groupUuuids = new
ArrayList<>();) and every usage of groupUuuids (including the two later
references mentioned) to the corrected identifier groupUuids so the code
compiles and reads correctly.