Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CIAM-6389 Pagination for groups/subgroups in Admin Console groups user groups #22493

Closed

Conversation

eatikrh
Copy link
Contributor

@eatikrh eatikrh commented Aug 16, 2023

This PR is a proof-of-concept to help with the discussion of the approach to solve the performance issues with many groups/subgroups in searches. The main idea is not to populate the full subgroups but to only populate a single line of ancestry from the groups in the search result set to the top-level group. Fine-grained permissions are checked on the result set. Below are the changes involved with this POC.

GroupDialogPicker.tsx

  • switched places of null and filter on line 83: isSearching ? null : { search: filter },
  • made it a non-top-level group search on line 95: count = (await adminClient.groups.count({ search: filter, top: true }))

GroupTree.tsx

  • made it a non-toplevel group search on line 95: const count = (await adminClient.groups.count({ search, top: true }))

RealmCacheSession.java

  • added searchForGroupByNameNoAncestryStream method

JpaRealmProvider.java

  • Changed the definition of getGroupsCountByNameContaining method this method was using searchForGroupByNameStream to count the results. Which creates a hierarchy to top level groups resulting in counting the top level groups and in my dataset this was always 1. By only counting the group ids using the name query "getGroupIdsByNameContaining" we are avoiding database hits that was used create the group representation objects.
  • added searchForGroupByNameNoAncestryStream method

GroupStorageManager.java

  • added searchForGroupByNameNoAncestryStream method

MapGroupProvider.java

  • Made the same performance improvement in getGroupsCountByNameContaining() method as in JpaRealmProvider
  • added searchForGroupByNameNoAncestryStream method

GroupsResource.java

  • in listGroups method we are now calling the searchForGroupByNameNoAncestryStream method. we are applying the fine grained permissions on the groups in the result set, then converting to single ancestor hierarchy using the newly introduced GroupUtils.toAncestorsLine() method.

GroupLookupProvider.java

  • added searchForGroupByNameNoAncestryStream method to the interface

ModelToRepresentation.java

  • added searchForGroupByNameNoAncestryStream method

GroupsResource.java

  • modified getGroups endpoint method to use searchForGroupByNameNoAncestryStream if a search term is used. (this needs more tests in different usages. since this is part of the group API we may not be aware of all the usages.)

GroupUtils.java

  • This is where the main logic of single-line-ancestry is implemented: toAncestorsLine method uses a static class GroupRepresentationExtended based on GroupRepresentation class to keep track of the parent ids. it also uses a HashMap to keep track of processed groups which eliminates repeated database hits.
    List tree keeps the top-level groups with only the subgroups
    linking the search results are populated.

HardcodedGroupStorageProvider.java

  • added searchForGroupByNameNoAncestryStream method

@eatikrh eatikrh changed the title Pagination for groups/subgroups in Admin Console groups user grops CIAM-6389 Pagination for groups/subgroups in Admin Console groups user grops Aug 16, 2023
@eatikrh eatikrh changed the title CIAM-6389 Pagination for groups/subgroups in Admin Console groups user grops CIAM-6389 Pagination for groups/subgroups in Admin Console groups user groups Aug 16, 2023
@cypress
Copy link

cypress bot commented Aug 16, 2023

1 failed and 1 flaky tests on run #8538 ↗︎

1 526 48 0 Flakiness 1

Details:

Merge 00ccd4f into 080b53f...
Project: Keycloak Admin UI Commit: e0160b3054 ℹ️
Status: Failed Duration: 20:38 💡
Started: Aug 21, 2023 1:06 PM Ended: Aug 21, 2023 1:27 PM
Failed  cypress/e2e/clients_test.spec.ts • 1 failed test • chrome

View Output Video

Test Artifacts
Clients test > Service account tab test > Assign Screenshots Video
Flakiness  cypress/e2e/realm_settings_tabs_test.spec.ts • 1 flaky test • chrome

View Output Video

Test Artifacts
Realm settings tabs tests > Accessibility tests for realm settings > Check a11y violations on client policies tab/ creating policy Output Screenshots

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@ghost ghost added the flaky-test label Aug 16, 2023
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Unreported flaky test detected, please review

@ghost
Copy link

ghost commented Aug 16, 2023

Unreported flaky test detected

If the below 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.actions.AppInitiatedActionTotpSetupTest#setupOtpPolicyChangedHotp

Keycloak CI - Base IT (1)

org.openqa.selenium.TimeoutException: 
java.net.SocketTimeoutException: Read timed out
Build info: version: '3.14.0', revision: 'aacccce0', time: '2018-08-02T20:19:58.91Z'
System info: host: 'fv-az573-208', ip: '10.1.0.134', os.name: 'Linux', os.arch: 'amd64', os.version: '5.15.0-1042-azure', java.version: '17.0.8'
Driver info: driver.version: HtmlUnitDriver
...

Report flaky test

org.keycloak.testsuite.actions.AppInitiatedActionTotpSetupTest#setupTotpRegisterManual

Keycloak CI - Base IT (1)

java.lang.AssertionError
	at org.junit.Assert.fail(Assert.java:87)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.junit.Assert.assertTrue(Assert.java:53)
	at org.keycloak.testsuite.actions.AppInitiatedActionTotpSetupTest.setupTotpRegisterManual(AppInitiatedActionTotpSetupTest.java:197)
...

Report flaky test

org.keycloak.testsuite.actions.AppInitiatedActionTotpSetupTest#setupTotpExisting

Keycloak CI - Base IT (1)

java.lang.AssertionError: Event expected
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.junit.Assert.assertNotNull(Assert.java:713)
	at org.keycloak.testsuite.AssertEvents.poll(AssertEvents.java:82)
...

Report flaky test

* added searchForGroupByNameNoAncestryStream method

GroupDialogPicker.tsx
* switched places of null and filter on line 83:  isSearching ? null : { search: filter },
* made it a non-toplevel group search on line 95:  count = (await adminClient.groups.count({ search: filter, top: true }))

GroupTree.tsx
* made it a non-toplevel group search on line 95: const count = (await adminClient.groups.count({ search, top: true }))

RealmCacheSession.java
* added searchForGroupByNameNoAncestryStream method

JpaRealmProvider.java
* Changed the definition of getGroupsCountByNameContaining method
this method was using searchForGroupByNameStream to count the results. Which is creates a
hierarchy to top level groups resulting in counting the top level groups and in my dataset this was always 1
By only counting the group ids using the name query "getGroupIdsByNameContaining" we are avoiding
Database hits that was used create the group representation objects.
* added searchForGroupByNameNoAncestryStream method

GroupStorageManager.java
* added searchForGroupByNameNoAncestryStream method

MapGroupProvider.java
* Made the same performance improvement in getGroupsCountByNameContaining() method as in JpaRealmProvider
* added searchForGroupByNameNoAncestryStream method

GroupsResource.java
* in listGroups method we are not calling the searchForGroupByNameNoAncestryStream method.
we are applying the fine grained permissions on the groups in the result set, then
converting to single ancestor hierarchy using the newly introduced GroupUtils.toAncestorsLine() method.

GroupLookupProvider.java
* added searchForGroupByNameNoAncestryStream method to the interface

ModelToRepresentation.java
* added searchForGroupByNameNoAncestryStream method

GroupsResource.java
* modified getGroups endpoint method to use searchForGroupByNameNoAncestryStream if
a search term is used. (this needs more tests in different usages.
since this is part of the group API we may not be aware of all the usages.)

GroupUtils.java
* This is where the main logic of single-line-ancestry is implemented:
toAncestorsLine method uses a static class GroupRepresentationExtended based on
GroupRepresentation class to keep track of the parent ids. it also uses a HashMap
to keep track of processed groups which eliminates repeated database hits.
 List<GroupRepresentationExtended> tree keeps the top-level groups with only the subgroups
 linking the search results are populated.

HardcodedGroupStorageProvider.java
* added searchForGroupByNameNoAncestryStream method
alice-wondered and others added 9 commits August 17, 2023 16:44
… in use as well as scalability

Modify Provider layer to allow for pagination on subgroups
Modify Provider layer to allow for searching for subgroups by ID and Name
Modify GroupModel to pass handling of subgroup lookups to the storage provider rather than performing the search on the model object
Modify GroupsResource to take advantage of new storage functions, preventing loading of excess entities into memory from DB
Modify GroupResource to allow for retrieval of all of the subgroups on an object, paginated, and sorted
Modify construction of group hierarchy to load a vertical slice of the relevant information, i.e. only loading groups from leaf -> root without inclusion of siblings
Changes to specific implementations of all of the above interfaces for the main storage providers
Update all tests to show that there are no regressions in the changed behavior
Add a count field to GroupRepresentation that lets a client know how many subgroups are on the group without having to retrieve them
Modify relevant external resources to use new functionality on the storage layer (UserResource, RealmAdminResource, AccountRestService...)
resolves keycloak#22374
… in use as well as scalability

Modify Provider layer to allow for pagination on subgroups
Modify Provider layer to allow for searching for subgroups by ID and Name
Modify GroupModel to pass handling of subgroup lookups to the storage provider rather than performing the search on the model object
Modify GroupsResource to take advantage of new storage functions, preventing loading of excess entities into memory from DB
Modify GroupResource to allow for retrieval of all of the subgroups on an object, paginated, and sorted
Modify construction of group hierarchy to load a vertical slice of the relevant information, i.e. only loading groups from leaf -> root without inclusion of siblings
Changes to specific implementations of all of the above interfaces for the main storage providers
Update all tests to show that there are no regressions in the changed behavior
Add a count field to GroupRepresentation that lets a client know how many subgroups are on the group without having to retrieve them
Modify relevant external resources to use new functionality on the storage layer (UserResource, RealmAdminResource, AccountRestService...)
resolves keycloak#22374
…hes in an efficient manner before attempting to load a full resource and calculating a cache-miss
main work in with this commit is in DefaultLazyLoader. added a bind method to be able to
pass search, first and max parameters to the supplier method.
Althugh this lazy loader is supposed to cache the data with the results of a search query is not
suitable for caching.
…load the the complete

subgroup tree while browsing the subgroups.

GroupPickerDialog:
* While browsing the subgroups, the subgroup count is now delivered in the payload.
The group of which subgroups are currently browsed is the last item in the breadcrumb.
In the code is is the list called navigation. Clicking on a group that has subgroups used
to call the admin rest api for groups which does not populate the subgroup count. It now
calls ui-ext/groups/{id} which populates the subgroup count.

* clicking next, previous or changing the page size now makes a rest call to get subgroups
to be displayed.

js/libs/keycloak-admin-client/src/defs/groupRepresentation.ts
* added subGroupCount as and optional property

rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/GroupsResource.java
 * toAncestorsLine(session now also accepts session and realm

services/src/main/java/org/keycloak/services/resources/admin/GroupsResource.java
* toAncestorsLine(session now also accepts session and realm

services/src/main/java/org/keycloak/utils/GroupUtils.java
*  toAncestorsLine method now sets the subgroupCount

Formatting and removal of unused identifiers in:
model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/DefaultLazyLoader.java
model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedGroup.java
model/jpa/src/main/java/org/keycloak/models/jpa/entities/GroupEntity.java
…load the the complete

subgroup tree while browsing the subgroups.

GroupPickerDialog:
* While browsing the subgroups, the subgroup count is now delivered in the payload.
The group of which subgroups are currently browsed is the last item in the breadcrumb.
In the code is is the list called navigation. Clicking on a group that has subgroups used
to call the admin rest api for groups which does not populate the subgroup count. It now
calls ui-ext/groups/{id} which populates the subgroup count.

* clicking next, previous or changing the page size now makes a rest call to get subgroups
to be displayed.

js/libs/keycloak-admin-client/src/defs/groupRepresentation.ts
* added subGroupCount as and optional property

rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/GroupsResource.java
 * toAncestorsLine(session now also accepts session and realm

services/src/main/java/org/keycloak/services/resources/admin/GroupsResource.java
* toAncestorsLine(session now also accepts session and realm

services/src/main/java/org/keycloak/utils/GroupUtils.java
*  toAncestorsLine method now sets the subgroupCount

Formatting and removal of unused identifiers in:
model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/DefaultLazyLoader.java
model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedGroup.java
model/jpa/src/main/java/org/keycloak/models/jpa/entities/GroupEntity.java

Merge branch 'CIAM-6389-paginate-subgroups-wip-1' into CIAM-6389-paginate-subgroups-in-search
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Unreported flaky test detected, please review

@ghost
Copy link

ghost commented Aug 24, 2023

Unreported flaky test detected

If the below 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.VerifyProfileTest#testAttributeRequiredButNotSelectedByScopeIsNotRenderedOnVerificationScreenForcedByAnotherAttribute

Keycloak CI - Forms IT (chrome)

java.lang.IllegalArgumentException: No enum constant org.keycloak.testsuite.pages.AppPage.RequestType.
	at java.base/java.lang.Enum.valueOf(Enum.java:273)
	at org.keycloak.testsuite.pages.AppPage$RequestType.valueOf(AppPage.java:56)
	at org.keycloak.testsuite.pages.AppPage.getRequestType(AppPage.java:49)
	at jdk.internal.reflect.GeneratedMethodAccessor554.invoke(Unknown Source)
...

Report flaky test

org.keycloak.testsuite.forms.RecoveryAuthnCodesAuthenticatorTest#test03AuthenticateRecoveryAuthnCodes

Keycloak CI - Forms IT (firefox)

org.openqa.selenium.TimeoutException: 
Navigation timed out after 10000 ms
Build info: version: '3.14.0', revision: 'aacccce0', time: '2018-08-02T20:19:58.91Z'
System info: host: 'fv-az162-859', ip: '10.1.0.82', os.name: 'Linux', os.arch: 'amd64', os.version: '5.15.0-1041-azure', java.version: '17.0.8'
Driver info: org.openqa.selenium.firefox.FirefoxDriver
...

Report flaky test

@mhajas
Copy link
Contributor

mhajas commented Aug 24, 2023

@eatikrh org.keycloak.testsuite.forms.RecoveryAuthnCodesAuthenticatorTest#test03AuthenticateRecoveryAuthnCodes
is unrelated to your changes, it also failed here: #22463 I reported it as a flaky test here: #22691

@eatikrh
Copy link
Contributor Author

eatikrh commented Aug 24, 2023

@mhajas thank you for lookinng into it.

alice-wondered and others added 3 commits August 24, 2023 09:25
…of searches in an efficient manner before attempting to load a full resource and calculating a cache-miss"

This reverts commit e8116ad.
@ghost
Copy link

ghost commented Aug 24, 2023

Unreported flaky test detected

If the below 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.RecoveryAuthnCodesAuthenticatorTest#test02SetupRecoveryAuthnCodesLogoutOtherSessionsNotChecked

Keycloak CI - Forms IT (chrome)

java.lang.IllegalArgumentException: No enum constant org.keycloak.testsuite.pages.AppPage.RequestType.
	at java.base/java.lang.Enum.valueOf(Enum.java:273)
	at org.keycloak.testsuite.pages.AppPage$RequestType.valueOf(AppPage.java:56)
	at org.keycloak.testsuite.pages.AppPage.getRequestType(AppPage.java:49)
	at jdk.internal.reflect.GeneratedMethodAccessor554.invoke(Unknown Source)
...

Report flaky test

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Unreported flaky test detected, please review

@eatikrh eatikrh closed this Sep 1, 2023
@eatikrh
Copy link
Contributor Author

eatikrh commented Sep 1, 2023

This has been superseded by PR PR#22700

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

Successfully merging this pull request may close these issues.

None yet

4 participants