Repository navigation
Improve retrieval of large read-only resources via admin API - #52034
Conversation
There was a problem hiding this comment.
Pull request overview
Optimizes large Admin API user searches by suppressing automatic JPA flushes during read-only retrieval.
Changes:
- Adds a session-scoped JPA read-only flag.
- Applies commit-only flush mode to queries when enabled.
- Enables this mode for Admin API user searches.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
UsersResource.java |
Enables read-only JPA behavior for user searches. |
EntityManagerProxy.java |
Applies commit-only query flush mode. |
EntityManagerProxyTest.java |
Updates the proxy factory invocation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
server-spi-private/src/main/java/org/keycloak/connections/jpa/support/EntityManagerProxy.java:105
- The existing
EntityManagerProxyTestonly checks close tracking and passes a null session, so the new read-only flush behavior is untested. Add cases with read-only and writable sessions that verifyFlushModeType.COMMITis applied only to queries created while read-only.
if ((batched || readOnly) && result instanceof Query query) {
| boolean previous = readOnly; | ||
| readOnly = true; | ||
| try { | ||
| return supplier.get().onClose(() -> readOnly = previous); |
There was a problem hiding this comment.
There are no nested streams here. If there is a need in the future (I don't see a valid usecase to nest these read-only wrappers) it can be handled then.
| * Executes the given supplier in read-only mode. The read-only flag is set before invoking the | ||
| * supplier and cleared when the returned stream is closed, making it safe for lazily consumed streams. | ||
| */ | ||
| <T> Stream<T> runAsReadOnly(Supplier<Stream<T>> supplier); |
There was a problem hiding this comment.
Works for me. But if you think we can take the opportunity and have a more generic method that returns instead of a Stream<T>, I think it will be best to support methods that return objects other than a stream.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
server-spi-private/src/main/java/org/keycloak/connections/jpa/support/EntityManagerProxy.java:105
- The existing
EntityManagerProxyTestonly adapts the factory call, so the new read-only branch is untested. Add a unit test with a read-only session and anEntityManagerreturning a mockQuerythat verifiessetFlushMode(COMMIT), plus a non-read-only case to guard the default behavior.
if ((batched || readOnly) && result instanceof Query query) {
Closes keycloak#52033 Signed-off-by: Martin Kanis <mkanis@ibm.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
server-spi-private/src/main/java/org/keycloak/connections/jpa/support/EntityManagerProxy.java:105
- The updated proxy test passes a null session and only checks closure, leaving this new read-only branch untested. Add cases with a session and returned
Querythat verifyFlushModeType.COMMITis applied when read-only and is not applied when read-only is false.
boolean readOnly = session != null && session.isReadOnly();
try {
flushInBatchIfEnabled(method);
Object result = method.invoke(em, args);
if ((batched || readOnly) && result instanceof Query query) {
services/src/main/java/org/keycloak/services/DefaultKeycloakSession.java:170
- The new session-state contract has no tests, so regressions in the immediate-return, lazy-stream, close, and exceptional-supplier paths would go unnoticed. Add focused
DefaultKeycloakSessiontests that assert the flag is active only for the intended scope and is restored afterward.
public <T> T runAsReadOnly(Supplier<T> supplier) {
boolean previous = readOnly;
readOnly = true;
try {
T result = supplier.get();
if (result instanceof Stream<?> stream) {
return (T) stream.onClose(() -> readOnly = previous);
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testWithOTPAndRecoveryCodesAtLevel2Keycloak CI - Forms IT (chrome) |
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testWithOTPAndRecoveryCodesAtLevel2Keycloak CI - Forms IT (chrome) |
Closes #52033