Remove unnecessary clone operations when building user representations - #51215
Open
pedroigor wants to merge 1 commit into
Open
Remove unnecessary clone operations when building user representations#51215pedroigor wants to merge 1 commit into
pedroigor wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Updates Keycloak’s user-profile representation and attribute handling to avoid unnecessary cloning/metadata computation, particularly when building UserRepresentations.
Changes:
- Adds a
toRepresentation(full, includeMetadata)overload to skip building user-profile metadata when not needed. - Propagates
UnmanagedAttributePolicyintoDefaultAttributes/ServiceAccountAttributesinstead of reading configuration from the session provider each time. - Refactors profile metadata decoration/caching to reduce cloning.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| services/src/main/java/org/keycloak/userprofile/ServiceAccountAttributes.java | Passes unmanaged-attribute policy into service-account attribute handling. |
| services/src/main/java/org/keycloak/userprofile/DeclarativeUserProfileProvider.java | Computes unmanaged-attribute policy once per attributes creation; adjusts metadata decoration clone flow. |
| server-spi-private/src/main/java/org/keycloak/userprofile/UserProfile.java | Adds representation overload to control metadata inclusion. |
| server-spi-private/src/main/java/org/keycloak/userprofile/DefaultUserProfile.java | Implements metadata inclusion flag to avoid unnecessary metadata creation. |
| server-spi-private/src/main/java/org/keycloak/userprofile/DefaultAttributes.java | Uses injected unmanaged-attribute policy; reduces metadata cloning via read-only accessor. |
| server-spi-private/src/main/java/org/keycloak/models/utils/ModelToRepresentation.java | Uses new representation overload to skip metadata creation; simplifies brief/full mapping. |
Comment on lines
+140
to
+142
| default <R extends AbstractUserRepresentation> R toRepresentation(boolean full, boolean includeMetadata) { | ||
| return toRepresentation(full); | ||
| } |
Comment on lines
+359
to
+366
| UserRepresentation rep = profile.toRepresentation(!brief, false); | ||
| RealmModel realm = session.getContext().getRealm(); | ||
|
|
||
| rep = brief ? | ||
| ModelToRepresentation.toBriefRepresentation(user, rep, false) : | ||
| ModelToRepresentation.toRepresentation(session, realm, user, rep, false); | ||
|
|
||
| rep.setUserProfileMetadata(null); | ||
| if (brief) { | ||
| ModelToRepresentation.toBriefRepresentation(user, rep, false); | ||
| } else { | ||
| ModelToRepresentation.toRepresentation(session, realm, user, rep, false); | ||
| } |
Comment on lines
582
to
586
| public Map<String, Object> getAnnotations(String name) { | ||
| AttributeMetadata metadata = getMetadata(name); | ||
| AttributeMetadata metadata = getMetadataReadOnly(name); | ||
|
|
||
| if (metadata == null) { | ||
| return Collections.emptyMap(); |
Closes keycloak#51214 Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
server-spi-private/src/main/java/org/keycloak/userprofile/UserProfile.java:142
- The new
includeMetadataparameter is currently ignored in the default implementation, which makes the API contract misleading and can break callers expecting metadata suppression (e.g.,toRepresentation(full, false)). Consider making this method non-default (forcing implementations to handleincludeMetadata), or have the default implementation actively remove metadata whenincludeMetadataisfalse(e.g., create the representation viatoRepresentation(full)and then clearuserProfileMetadataon the returned representation).
default <R extends AbstractUserRepresentation> R toRepresentation(boolean full, boolean includeMetadata) {
return toRepresentation(full);
}
server-spi-private/src/main/java/org/keycloak/models/utils/ModelToRepresentation.java:366
- Previously the return value of
toBriefRepresentation(...)/toRepresentation(...)was assigned back torep. This change discards the returned value, which can alter behavior if those helpers ever return a different instance (even if they currently mutate in place). To preserve prior semantics and avoid future regressions, keep assigning the returned value back torep(or refactor the helpers to be clearlyvoid/mutating-only if that’s the intended contract).
if (brief) {
ModelToRepresentation.toBriefRepresentation(user, rep, false);
} else {
ModelToRepresentation.toRepresentation(session, realm, user, rep, false);
}
Comment on lines
272
to
283
| public AttributeMetadata getMetadata(String name) { | ||
| AttributeMetadata metadata = getMetadataReadOnly(name); | ||
| return metadata != null ? metadata.clone() : null; | ||
| } | ||
|
|
||
| private AttributeMetadata getMetadataReadOnly(String name) { | ||
| if (unmanagedAttributes.containsKey(name)) { | ||
| return createUnmanagedAttributeMetadata(name); | ||
| } | ||
|
|
||
| return Optional.ofNullable(metadataByAttribute.get(name)) | ||
| .map(AttributeMetadata::clone) | ||
| .orElse(null); | ||
| return metadataByAttribute.get(name); | ||
| } |
vramik
approved these changes
Jul 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #51214