Automated Test: feature-group-concurrency-implementation #324
Conversation
Closes #40368 Signed-off-by: vramik <vramik@redhat.com>
📝 WalkthroughWalkthroughUpdates to group caching and model handling include adding null-safety to group count retrieval, annotating overridden methods, removing an unused helper method, and introducing a multi-threaded concurrent test for group operations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/GroupAdapter.java`:
- Around line 272-276: getSubGroupsCount currently returns null when
modelSupplier.get() is null which violates GroupModel's contract; change the
method (referencing getSubGroupsCount(), modelSupplier, updated and isUpdated())
to return 0L instead of null when no underlying model exists (or ensure
modelSupplier never returns null), so callers like GroupUtils receive a non-null
long consistent with sibling methods that assume a non-null model.
In `@tests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java`:
- Around line 119-134: In createMultiDeleteMultiReadMultiMulti, each created
group (obtained via ApiUtil.getCreatedId(response) and currently added to
groupUuuids) must also be registered with managedRealm.cleanup().add(...)
immediately after creation so the teardown will remove the group even if the
test aborts; locate the creation block in the createMultiDeleteMultiReadMulti
method where groupUuuids.add(groupUuid) is called and add a cleanup registration
using managedRealm.cleanup().add that invokes the
groups().group(groupUuid).remove() action for that groupUuid.
- Around line 136-158: The background reader thread is started without a
reference and not joined, so its in-flight exceptions may be added to
caughtExceptions after the test assertion; capture the Thread returned by new
Thread(...) (e.g., readerThread), start it, and after setting
deletedAll.set(true) call readerThread.join() (or catch/propagate
InterruptedException) before asserting that caughtExceptions is empty to ensure
the reader has finished and no late exceptions are missed.
🧹 Nitpick comments (1)
tests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java (1)
121-121: Typo:groupUuuids→groupUuids.The variable has three u's instead of two. Also on Line 152.
| public Long getSubGroupsCount() { | ||
| if (isUpdated()) return updated.getSubGroupsCount(); | ||
| return getGroupModel().getSubGroupsCount(); | ||
| GroupModel model = modelSupplier.get(); | ||
| return model == null ? null : model.getSubGroupsCount(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all usages of getSubGroupsCount() to see if callers handle null
rg -n --type=java 'getSubGroupsCount' -C 3Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 3321
🏁 Script executed:
#!/bin/bash
# Check modelSupplier.get() calls in GroupAdapter
cat model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/GroupAdapter.java | head -300 | tail -50Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 1797
🏁 Script executed:
#!/bin/bash
# Get the full context of getSubGroupsStream methods to see their pattern
rg -n --type=java -A 4 'getSubGroupsStream' model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/GroupAdapter.javaRepository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 1658
🏁 Script executed:
#!/bin/bash
# Check JPA GroupAdapter getSubGroupsCount implementation
rg -n --type=java -A 10 'public Long getSubGroupsCount' model/jpa/src/main/java/org/keycloak/models/jpa/GroupAdapter.javaRepository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 646
🏁 Script executed:
#!/bin/bash
# Verify the GroupModel interface documentation for getSubGroupsCount
rg -n --type=java -B 3 -A 5 'Never returns.*null' server-spi/src/main/java/org/keycloak/models/GroupModel.javaRepository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 1897
🏁 Script executed:
#!/bin/bash
# Check if there are any null-safe usages of getSubGroupsCount elsewhere
rg -n --type=java 'getSubGroupsCount' -C 2 | head -100Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 2628
Method violates interface contract by returning null when it must never be null.
The interface contract in GroupModel explicitly states: @return The number of groups beneath this group. Never returns {@code null}. However, this implementation returns null when modelSupplier.get() returns null (line 275).
This breaks the contract and will cause failures in callers:
- GroupUtils (line 90) calls
setSubGroupCount(group.getSubGroupsCount())with no null handling — passingnullwill cause silent failures or NPE on unboxing. - The default implementation (GroupModel:298-299) and JPA implementation never return null.
Additionally, the null guard is inconsistent with sibling methods: getSubGroupsStream overloads (lines 256, 262, 268) call modelSupplier.get() without null checks. Either modelSupplier.get() can be null (then all methods need guarding), or it cannot be null (then this guard is unneeded and incorrect).
Fix: Return 0L instead of null to satisfy the interface contract, or ensure modelSupplier.get() is never null.
🤖 Prompt for AI Agents
In
`@model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/GroupAdapter.java`
around lines 272 - 276, getSubGroupsCount currently returns null when
modelSupplier.get() is null which violates GroupModel's contract; change the
method (referencing getSubGroupsCount(), modelSupplier, updated and isUpdated())
to return 0L instead of null when no underlying model exists (or ensure
modelSupplier never returns null), so callers like GroupUtils receive a non-null
long consistent with sibling methods that assume a non-null model.
| public void createMultiDeleteMultiReadMulti() { | ||
| // create multiple groups | ||
| List<String> groupUuuids = new ArrayList<>(); | ||
| IntStream.range(0, 100).forEach(groupIndex -> { | ||
| GroupRepresentation group = new GroupRepresentation(); | ||
| group.setName("Test Group " + groupIndex); | ||
| try (Response response = managedRealm.admin().groups().add(group)) { | ||
| boolean created = response.getStatusInfo().getFamily() == Response.Status.Family.SUCCESSFUL; | ||
| if (created) { | ||
| final String groupUuid = ApiUtil.getCreatedId(response); | ||
| groupUuuids.add(groupUuid); | ||
| } else { | ||
| fail("Failed to create group: " + response.getStatusInfo().getReasonPhrase()); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Created groups are not registered for cleanup.
If the test fails during the deletion loop (Lines 152-154), the previously created groups will remain in the realm and leak into other tests. Other tests in this file use managedRealm.cleanup().add(...) to register teardown actions. Consider registering each group for cleanup at creation time, so they are removed even if the test aborts early.
🛡️ Proposed fix — register cleanup alongside creation
if (created) {
final String groupUuid = ApiUtil.getCreatedId(response);
groupUuuids.add(groupUuid);
+ managedRealm.cleanup().add(r -> r.groups().group(groupUuid).remove());
} else {🤖 Prompt for AI Agents
In `@tests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java`
around lines 119 - 134, In createMultiDeleteMultiReadMultiMulti, each created
group (obtained via ApiUtil.getCreatedId(response) and currently added to
groupUuuids) must also be registered with managedRealm.cleanup().add(...)
immediately after creation so the teardown will remove the group even if the
test aborts; locate the creation block in the createMultiDeleteMultiReadMulti
method where groupUuuids.add(groupUuid) is called and add a cleanup registration
using managedRealm.cleanup().add that invokes the
groups().group(groupUuid).remove() action for that groupUuid.
| AtomicBoolean deletedAll = new AtomicBoolean(false); | ||
| List<Exception> caughtExceptions = new CopyOnWriteArrayList<>(); | ||
| // read groups in a separate thread | ||
| 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(); | ||
|
|
||
| // delete groups | ||
| groupUuuids.forEach(groupUuid -> { | ||
| managedRealm.admin().groups().group(groupUuid).remove(); | ||
| }); | ||
| deletedAll.set(true); | ||
|
|
||
| assertThat(caughtExceptions, Matchers.empty()); | ||
| } |
There was a problem hiding this comment.
Race condition: reader thread may still be running when assertion executes.
The background thread is fired-and-forgotten — no reference is kept, and Thread.join() is never called. After deletedAll.set(true) on Line 155, the assertion on Line 157 runs immediately, but the reader thread may still be mid-request. If that in-flight call throws, the exception is appended to caughtExceptions after the assertion has already passed, producing a false-positive test result.
Store a reference to the thread and join() it before asserting:
🐛 Proposed fix
- 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 -> {
managedRealm.admin().groups().group(groupUuid).remove();
});
deletedAll.set(true);
+ readerThread.join();
+
assertThat(caughtExceptions, Matchers.empty());Note: join() throws InterruptedException, so the method signature will need to declare throws InterruptedException (or catch it).
🤖 Prompt for AI Agents
In `@tests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java`
around lines 136 - 158, The background reader thread is started without a
reference and not joined, so its in-flight exceptions may be added to
caughtExceptions after the test assertion; capture the Thread returned by new
Thread(...) (e.g., readerThread), start it, and after setting
deletedAll.set(true) call readerThread.join() (or catch/propagate
InterruptedException) before asserting that caughtExceptions is empty to ensure
the reader has finished and no late exceptions are missed.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
Bug Fixes
Tests
Refactor