8000 Add support for filtering by enabled attribute on users count endpoint by msvechla · Pull Request #9842 · keycloak/keycloak · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add support for filtering by enabled attribute on users count endpoint #9842

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 13, 2022
Merged

Add support for filtering by enabled attribute on users count endpoint #9842

merged 1 commit into from
Apr 13, 2022

Conversation

msvechla
Copy link
Contributor
@msvechla msvechla commented Jan 27, 2022 •
edited by abstractj
Loading

We now allow filtering users by their enabled status on the users/count endpoint.

Let me know if you have any feedback or suggestions to get this merged.
Thanks for this awesome project!

Resolves #10896

@msvechla msvechla mentioned this pull request Jan 27, 2022
Closed
@msvechla
Copy link
Contributor Author

@hmlnarik maybe you or someone else can let the CI run for this PR? Should be a quite simple change that would help us a lot

@msvechla
Copy link
Contributor Author
msvechla commented Feb 3, 2022

@stianst @abstractj @mposolda @pdrozd could someone trigger the CI for this simple change to check whether it is valid?

@abstractj
Copy link
Contributor

@msvechla triggered, reading the changes you submitted. I believe it's necessary to add tests.

@msvechla msvechla force-pushed the users_count_enabled_filter branch from 70dec4b to 85a4cbf Compare February 7, 2022 17:19
@msvechla
Copy link
Contributor Author
msvechla commented Feb 7, 2022

@abstractj thanks for the hint, I added some tests. Can you take another look and run the CI?

Thanks a lot!

@msvechla
Copy link
Contributor Author
msvechla commented Feb 8, 2022

Thanks for triggering the CI @abstractj. My newly added tests were failing with:

UserTest.countUsersByEnabledFilter:651 expected:<2> but was:<4>

I only created 3 users in my test, looks like the search/count was also counting the service account in the realm.

I tried to debug this further and I committed a new test version, however, I don't understand why they are failing now. I even tried to filter out service accounts by specifying an email.

This is how I tried to debug the issue:

A simple search after I created my test users reveals the following:

 List<UserRepresentation> users = realm.users().search(null, null, null, "@enabledfilter.com", null, null,enabled, null);
 users.stream().forEach(u -> System.out.println(u.getUsername() + u.isEnabled()));

enabled1true
enabled2true

Afterward, I count the enabled users in the realm, which should yield 2 as to the search above:

// count users that are enabled
assertThat(realm.users().count(null, null, null, "@enabledfilter.com", null, null, enabled), is(2));

The test however returns:

java.lang.AssertionError:
Expected: is <2>
but: was <3>

Do you have an idea what I might be overlooking @abstractj? Could it be related to: #8849?

I committed the current state of the tests with the debug output to this PR, would be awesome if you could take a quick look at it.

Thanks a lot for your input & help!

@msvechla msvechla force-pushed the users_count_enabled_filter branch 2 times, most recently from 6dac488 to fc34db5 Compare February 8, 2022 16:21
@msvechla
Copy link
Contributor Author
msvechla commented Feb 8, 2022

@abstractj sorry for the confusion, I did some more debugging and noticed I also had to update the JPA.

The newly added tests are now successful on my local machine.
Would be awesome if you can trigger another CI run.

Thanks a lot for your support!

@msvechla
Copy link
Contributor Author
msvechla commented Feb 8, 2022 •
edited
Loading

Looks like the builds failed due to some transient error, unrelated to my changes:

Caused by: org.apache.maven.wagon.TransferFailedException: transfer failed for https://maven.repository.redhat.com/ga/org/picketlink/picketlink-api/2.5.5.SP12-redhat-00009/picketlink-api-2.5.5.SP12-redhat-00009.jar, status: 500 Internal Server Error

I think we have to re-run the pipeline again please @hmlnarik .

@msvechla msvechla force-pushed the users_count_enabled_filter branch from fc34db5 to dad1d78 Compare February 10, 2022 10:42
@msvechla
Copy link
Contributor Author

Okay the service accounts bite me again. When only running my newly added user tests directly from my IDE, the service account is not present. When I run the entire suite, the service accounts are there. This is why I did not notice the issue when testing locally:

disabled1 false disabled1@enabledfilter.com
enabled1 true enabled1@enabledfilter.com
enabled2 true enabled2@enabledfilter.com
service-account-test-client true null

I now filter by email address for all of my tests as well, so they are not affected by the hidden service accounts in the realm.

I am sorry this is taking so many iterations, but its my first contribution to this project and I have to get used to some of the testing behaviour. Thanks a lot for sticking with me!

I adapted the tests, can you please re-run the CI @abstractj ?

@msvechla
Copy link
Contributor Author

@abstractj @hmlnarik could you please re-trigger the CI?

@msvechla
Copy link
Contributor Author

@abstractj @hmlnarik sorry for pinging you again, could you please trigger the CI?

@msvechla
Copy link
Contributor Author

@abstractj did you cancel the CI run on purpose?

@abstractj
Copy link
Contributor

@msvechla I did the opposite, approved to run :)

@msvechla
Copy link
Contributor Author

@abstractj okay weird, for me it says 2 jobs (Keycloak CI / Bu 8000 ild (pull_request) — Build,
Keycloak Operator CI / Build (pull_request) — Build) have been canceled and now all other checks are skipped as the tests rely on the builds.

Maybe its just an issue on my end on GitHub, if the tests are running fine for you, then all is good :)

@msvechla
Copy link
Contributor Author

@abstractj can you take a look again? I think your trigger did not run the CI

@msvechla
Copy link
Contributor Author

@abstractj for me the CI always shows as canceled, any idea whats going wrong here?

@msvechla
Copy link
Contributor Author

@abstractj can you please take a look? Something is wrong with the CI. It never ran when you re-triggered it in the past days.

Could it be that I have to rebase?

Thanks a lot for your help!

@abstractj
Copy link
Contributor

@msvechla yes, please rebase.

@msvechla
Add support for filtering by enabled attribute on users count endpoint
3d82a88 
We now allow to filter users by their enabled status on the users/count endpoint
@msvechla msvechla force-pushed the users_count_enabled_filter branch from dad1d78 to 3d82a88 Compare February 23, 2022 16:36
@msvechla
Copy link
Contributor Author

@abstractj I rebased and updated the PR

@msvechla
Copy link
Contributor Author

Thanks for taking care @abstractj , looks like we are finally ready to go. Sorry for the back and forth.

@msvechla
Copy link
Contributor Author
msvechla commented Mar 1, 2022

@abstractj can we get this merged then? Or do you have further input? Thanks for checking!

@abstractj
Copy link
Contributor
abstractj commented Mar 3, 2022 •
edited
Loading

It seems somewhat related with #6140. @vramik if you have the chance, could you please take a look?

@msvechla
Copy link
Contributor Author

@vramik @thomasdarimont any udpate on the review?

@msvechla
Copy link
Contributor Author

@abstractj @vramik @thomasdarimont another week has passed, can we find someone to take a look at this? Our team is waiting for this and it would be nice to have it in the next release.

Thanks a lot for your consideration and effort!

vramik
vramik approved these changes Mar 22, 2022
View reviewed changes
Copy link
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.

@msvechla, thank you for the PR!

@abstractj
Copy link
Contributor

Thanks for reviewing @vramik.

@pedroigor @stianst whenever you have some time this week, could you please review? I'm slightly concerned about introducing those changes for a few reasons:

  • It introduces a new endpoint /count responsible to retrieve the number of results. Even though it is a valid implementation, for a scenario with 500000 users we may face some performance degradation. Pagination was implemented exactly to prevent scenarios like this.
  • One of the recommended best practices is to return the number of records in the header, something like X-Total-Count

Maybe I'm overthinking considering that we already do something similar to for the groups' endpoint (GET /{realm}/groups/count).

@abstractj abstractj requested review from pedroigor and stianst March 22, 2022 19:43
@abstractj
Copy link
Contributor

@msvechla I could not find the GH issue to link here. If you have some time, could you please include it?

@msvechla
Copy link
Contributor Author
msvechla commented Mar 22, 2022 •
edited
Loading

Thanks for reviewing @vramik.

@pedroigor @stianst whenever you have some time this week, could you please review? I'm slightly concerned about introducing those changes for a few reasons:

  • It introduces a new endpoint /count responsible to retrieve the number of results. Even though it is a valid implementation, for a scenario with 500000 users we may face some performance degradation. Pagination was implemented exactly to prevent scenarios like this.
  • One of the recommended best practices is to return the number of records in the header, something like X-Total-Count

Maybe I'm overthinking considering that we already do something similar to for the groups' endpoint (GET /{realm}/groups/count).

@abstractj I think there is a misunderstanding. I am not adding a new rest endpoint here.
The GET /{realm}/users/count endpoint has been there for a long time: https://www.keycloak.org/docs-api/17.0/rest-api/index.html#_users_resource

I am merely adding support to filter by an additional enabled attribute on the existing endpoint, as I already mentioned in the original PR description.

You might have been confused by the first diff, but here I am just overloading an existing function, so it can be called with different parameters. Same as it already has been done by other contributors before, see:

keycloak/integration/admin-client/src/main/java/org/keycloak/admin/client/resource/UsersResource.java

Line 190 in 3d82a88

@Path("count")
and following lines

@abstractj
Copy link
Contributor

@msvechla you are correct on it. I overlooked the issue reading GitHub diff, but reviewing it again your changes make sense.

Could you please link the GH issue here?

abstractj
abstractj approved these changes Mar 23, 2022
View reviewed changes
Copy link
Contributor
@abstractj abstractj left a comment

Choose a reason for hiding this comment

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

Approving it. But we need the GH issue ticket before the merge.

@msvechla msvechla mentioned this pull request Mar 23, 2022
Closed
@msvechla
Copy link
Contributor Author

Thanks @abstractj , there was no Issue yet, but I created one here: #10896

@abstractj abstractj added kind/enhancement Categorizes a PR related to an enhancement and removed missing/issue labels Mar 23, 2022
@abstractj abstractj merged commit 820ab52 into keycloak:main Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abstractj abstractj abstractj approved these changes

@vramik vramik vramik approved these changes

@thomasdarimont thomasdarimont Awaiting requested review from thomasdarimont

@pedroigor pedroigor Awaiting requested review from pedroigor

@stianst stianst Awaiting requested review from stianst

Assignees
No one assigned
Labels
area/admin/api kind/enhancement Categorizes a PR related to an enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support Filtering by Enabled Attribute on /Users/Count Endpoint
3 participants
@msvechla @abstractj @vramik
0