Adding paging capabilities for the admin entity-providers endpoints

[rmartinc]

Draft changes for the identity-providers rest API. No real implementation changes at db level.

[jonkoops]

Perhaps this can also be less generic then 'Response'? Is there no way we can do a IdentityProviderRepresentation as a response?

[rmartinc]

I just changed the description that was copied from the other method. I can do it. No problem with this.

[jonkoops]

Do we not have some sort of generic solution for pagination? We would need to know how many pages are left to do proper pagination on the client side. Ideally a response for this endpoint would include all the information needed to handle pagination state:

{
  "hasNextPage": true,
  "hasPreviousPage": true,
  "currentPage": 3,
  "totalPages": 5,
  "entriesPerPage": 20,
  "totalEntries": 100,
  "results": [
    {
      "id": 1,
      "name": "Foo"
    },
    {
      "id": 2,
      "name": "Bar"
    }
  ]
}

Something like a PaginatedResponse<IdentityProviderRepresentation> would be great, so it can be re-used between endpoints.

[rmartinc]

@jonkoops AFAIK there is no standard response in keycloak for pagination. The other endpoints just have this, a count method and a find method (more or less, you can see the users for example, here and here). That's why I followed the same idea.

If we want to create a generic solution I would propose a much simpler response with no pages. The reason is that some storages (ldap for example) are extremely far from the idea of common paging (in ldap you don't know the total number of entries that matches a filter for example). For that I would only return something like the following:

{
  "firstResult": 100,
  "numResults": 50,
  "results": [... 50 results ...],
  "hasMore": true
}

And the console would do something different, something like showing the initial list and a button saying more. If you click it the next bunch of results are added to the list. The user can always filter or refine the search to get less results.

For sure I would not give so much data for pagination as you propose, the server doesn't control the page. I think that part is for the frontend. So if we want to main something like we have now but with a better response. I vote for something like:

{
  "hasMore": true,
  "firstResult": 10,
  "numResults": 20,
  "totalEntries": 100,
  "results": [... 20 results ]
}

If we go with something like this the method will be new (I have reused the same method in this test). We need more people of team to decide about this point. And @mposolda should be involved for sure.

[jonkoops]

The reason is that some storages (ldap for example) are extremely far from the idea of common paging (in ldap you don't know the total number of entries that matches a filter for example).

I see, yeah that is very unfortunate indeed. Reading this I feel like we can split this into two types of pagination:

1. Finite paging, the total number of entries is known and this information can be constructed.
2. Infinite paging, here we can only determine the cursor, but we have no idea of where the end is.

It feel to me that a large amount of our data can fit in nr. 1, so I would not go and compromise the data structure of all API endpoints for these edge-cases. But perhaps I am mistaken, and this is a large amount of our data. I think you would know better if this is the case.

Naturally, we do need to handle these edge-cases, and I think your solution makes sense.

And the console would do something different, something like showing the initial list and a button saying more. If you click it the next bunch of results are added to the list. The user can always filter or refine the search to get less results.

I like this approach, at least for the places in the API where we cannot determine an accurate count.

For sure I would not give so much data for pagination as you propose, the server doesn't control the page.

I have to disagree slightly with that. The server is is certainly not responsible for maintaining state (such as the current page we're on), but I do believe it is the responsibility of the API to serve as a single source of truth where possible (including computed state).

However, this is more of a philosophical debate, and I would already be more than happy if we can already compute the page state 👍 We can always add this later if we feel like it is necessary.

What would also be great is if we could have some generalized error handling for common pagination errors, such as exceeding the max page size, or exceeding the bounds of the entries. I imagine this is something we could do in a generalized abstraction.

Also agree, we should get more of the team involved if we do want to generalize this into a common pattern. I'd really like to get this specced out, as (inconsistent) pagination is something that frequently is a point of frustration (and bugs) for the UI team.

@mposolda @ssilvert would definitely be interested in your take on this as well.

[ssilvert]

Also agree, we should get more of the team involved if we do want to generalize this into a common pattern. I'd really like to get this specced out, as (inconsistent) pagination is something that frequently is a point of frustration (and bugs) for the UI team.

@mposolda @ssilvert would definitely be interested in your take on this as well.

I agree with @jonkoops on this. The UI needs to know what it is dealing with. Perhaps providers should have a choice of implementing a paginated or unpaginated version of each interface. And the UI should be able to ask if pagination is supported so we know how to get and display the data.

[mposolda]

@rmartinc @ssilvert @jonkoops The proposed ideas are great, however here the main question is if we:

1. Follow the same pattern for identity-providers pagination like we're already doing for bunch of other Keycloak objects, which already support pagination (users, clients, roles etc)
2. Introduce some new pagination approach (v2 pagination), which might be better and more effective as described in the ideas proposed above (among others, it would allow to avoid sending separate request to "count" endpoint etc)

My vote is to go with (1) for identity-providers pagination and follow the same as we do for all other Keycloak pagination entities.

I don't think we should introduce some new approach just for identity-providers TBH and make it inconsistent with other objects. It would makes more sense to postpone this possibly to admin REST API v2 (which is planned to be done at some point) and make sure it is consistent for all objects.

[jonkoops]

I am okay with deferring this to a potential v2. But be aware that currently none of the UIs actually use any of the /count endpoints.

[jonkoops]

Why do we have a separate count endpoint? Under what circumstances should we be using this? If this a separate pre-flight that the client has to do to determine the pagination that is less than ideal as:

1. This complicates client logic for something we already fully know the state of on the server.
2. This state can become out of sync with reality as new records are inserted or deleted.
3. Because of nr. 2 we now need to do this request every time we also request the identity providers.
4. This results in poorer performance due to network overhead (although not significantly I imagine).
5. We cannot handle this generically in the client, as endpoints do not implement this consistently.

[rmartinc]

Yeah! It's the same, I did this way because it how it's usually done. See my previous comment.

[ghost]

Unreported flaky test detected, please review

[ghost]

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.admin.IdentityProviderTest#testFind

Keycloak CI - Legacy Store IT (oracle)

Expected: [github, google], was: [facebook, github]: arrays first differed at element [0]; expected:<[github]> but was:<[facebook]>
	at org.junit.internal.ComparisonCriteria.arrayEquals(ComparisonCriteria.java:78)
	at org.junit.internal.ComparisonCriteria.arrayEquals(ComparisonCriteria.java:28)
	at org.junit.Assert.internalArrayEquals(Assert.java:534)
	at org.junit.Assert.assertArrayEquals(Assert.java:285)
...

Expected: [github, google], was: [facebook, google]: arrays first differed at element [0]; expected:<[github]> but was:<[facebook]>
	at org.junit.internal.ComparisonCriteria.arrayEquals(ComparisonCriteria.java:78)
	at org.junit.internal.ComparisonCriteria.arrayEquals(ComparisonCriteria.java:28)
	at org.junit.Assert.internalArrayEquals(Assert.java:534)
	at org.junit.Assert.assertArrayEquals(Assert.java:285)
...

Report flaky test

[cgeorgilakis]

I believe that for Identity Provider pager we can create a toBriefRepresentation method in ModelToRepresentation class.
In pager, only little values of Identity Providers are used. No need to transform configs for 100 Identity Providers and send all this information in ui.

[rmartinc]

The tests are failing because didn't ordered the results. I can do it if necessary for the tests.

@cgeorgilakis Yes, we can add also the toBriefRepresentation if necessary. Maybe with just some attributes like foe example alias, displayName and the providerId. That's a good idea.

[mposolda]

@rmartinc @cgeorgilakis +1 for toBriefRepresentation if we don't have that already for identity providers.

[mposolda]

@jonkoops We discussed with @rmartinc to do some small adjustments to this PR (like support for toBriefRepresentation and probably support for sorting of idps). However we hope to stick with the same approach like it's currently used for other objects (users, clients etc).

IMO possible improvements for pagination can be done in unified way for all objects when we have some "Admin REST API v2". Is it ok with you?

[jonkoops]

@mposolda Yeah, I have no issue with that, this issue needs to get resolved sooner than later. But we'll have to start thinking about a V2 API sometime in the near future, this is kinda all duct tape and toothpicks at the moment.

[mposolda]

@jonkoops Yes, I understand and I agree with V2 API need. Thanks!

[mposolda]

Closing as this was replaced by #22003