@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

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

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

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

Thanks a lot!

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!

@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!

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 .

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 ?

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

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

@abstractj did you cancel the CI run on purpose?

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

@abstractj okay weird, for me it says 2 jobs (Keycloak CI / Build (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 :)

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

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

@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!

@msvechla yes, please rebase.

@abstractj I rebased and updated the PR

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

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

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

@vramik @thomasdarimont any udpate on the review?

@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!

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).

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

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:

@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?

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