Skip to content

Optimize composite role evaluation#13146

Closed
ahus1 wants to merge 1 commit intokeycloak:mainfrom
ahus1:is-12888-optimized-role-read
Closed

Optimize composite role evaluation#13146
ahus1 wants to merge 1 commit intokeycloak:mainfrom
ahus1:is-12888-optimized-role-read

Conversation

@ahus1
Copy link
Copy Markdown
Member

@ahus1 ahus1 commented Jul 15, 2022

Closes #12888

@ahus1 ahus1 self-assigned this Jul 15, 2022
@hmlnarik hmlnarik added the status/hold PR should not be merged. On hold for later. label Jul 19, 2022
@hmlnarik
Copy link
Copy Markdown
Contributor

This PR shows a decent way to optimize role evaluation.
The biggest concern is how to implement it in a way compatible with tree store.
As agreed on the internal call, I'm putting this on hold due to team capacity, need for considering the database-specific syntax for recursive query evaluation, and current tree store unavailability. The work will be picked again once the tree store is in and the team capacity allows it.

Copy link
Copy Markdown
Contributor

@vramik vramik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @ahus1 for creating the PR. I've commented with some suggestions.

Comment on lines +113 to +121
Set<MapRoleEntity> result = new HashSet<>();
@SuppressWarnings("unchecked") List<String> sqlResult = (List<String>) nativeQuery.getResultList();
for (String targetRole : targetRoles) {
result.add(read(targetRole));
}
for (String targetRole : sqlResult) {
result.add(read(targetRole));
}
return result;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to use criteria() instead of for loops?

Suggested change
Set<MapRoleEntity> result = new HashSet<>();
@SuppressWarnings("unchecked") List<String> sqlResult = (List<String>) nativeQuery.getResultList();
for (String targetRole : targetRoles) {
result.add(read(targetRole));
}
for (String targetRole : sqlResult) {
result.add(read(targetRole));
}
return result;
@SuppressWarnings("unchecked") List<String> sqlResult = (List<String>) nativeQuery.getResultList();
targetRoles.addAll(sqlResult);
DefaultModelCriteria<RoleModel> mcb = criteria();
mcb = mcb.compare(SearchableFields.REALM_ID, Operator.EQ, realmId)
.compare(SearchableFields.ID, Operator.IN, targetRoles);
return read(withCriteria(mcb)).collect(Collectors.toSet());

note: if it'll be decided to return Stream, then return statement could loo like: return read(withCriteria(mcb));

return getAttributes().getOrDefault(name, Collections.EMPTY_LIST).stream();
}

public RealmModel getRealm() {
Copy link
Copy Markdown
Contributor

@vramik vramik Jul 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method could be probably introduced in RoleModel as the "other" models also has access to the RealmModel as well. This way it could be avoided to cast RoleModel to MapRoleAdapter in MapRoleProvider.

@hmlnarik I'm not sure whether this can be postponed too much as the RoleModel is located in server-spi package and I'm not sure if we would be able to change it in future.


boolean hasRole(String realmId, Set<String> roleIds, Set<String> targetRoleIds);

Set<V> expandCompositeRoles(String realmId, Set<String> targetRoles);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the Stream would be returned we might get rid of some collecting-and-then-making-stream-again.

Suggested change
Set<V> expandCompositeRoles(String realmId, Set<String> targetRoles);
Stream<V> expandCompositeRoles(String realmId, Set<String> targetRoles);

@ahus1
Copy link
Copy Markdown
Member Author

ahus1 commented Jan 23, 2026

Closing this one as obsolete as there is no more map store.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status/hold PR should not be merged. On hold for later.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve querying of composite roles

3 participants