8000 Adding paging capabilities for the admin entity-providers endpoints by rmartinc · Pull Request #21487 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

Adding paging capabilities for the admin entity-providers endpoints #21487

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

Closed
rmartinc wants to merge 1 commit into from
Closed

Adding paging capabilities for the admin entity-providers endpoints #21487

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions ...lient-jee/src/main/java/org/keycloak/admin/client/resource/IdentityProvidersResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import javax.ws.rs.Path;
import javax.ws.rs.PathParam;
import javax.ws.rs.Produces;
import javax.ws.rs.QueryParam;
import javax.ws.rs.core.MediaType;
import javax.ws.rs.core.Response;
import java.util.List;
Expand All @@ -43,6 +44,16 @@ public interface IdentityProvidersResource {
@Produces(MediaType.APPLICATION_JSON)
List<IdentityProviderRepresentation> findAll();

@GET
@Path("instances")
@Produces(MediaType.APPLICATION_JSON)
List<IdentityProviderRepresentation> find(@QueryParam("search") String search, @QueryParam("first") Integer firstResult, @QueryParam("max") Integer maxResults);

@GET
@Path("instances/count")
@Produces(MediaType.APPLICATION_JSON)
Integer count(@QueryParam("search") String search);

@POST
@Path("instances")
@Consumes(MediaType.APPLICATION_JSON)
Expand Down
75 changes: 65 additions & 10 deletions services/src/main/java/org/keycloak/services/resources/admin/IdentityProvidersResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@
import org.keycloak.services.ErrorResponse;
import org.keycloak.services.resources.KeycloakOpenAPI;
import org.keycloak.services.resources.admin.permissions.AdminPermissionEvaluator;
import org.keycloak.utils.ReservedCharValidator;
import org.keycloak.utils.StringUtil;

import jakarta.ws.rs.BadRequestException;
import jakarta.ws.rs.Consumes;
Expand All @@ -50,17 +52,18 @@
import jakarta.ws.rs.Path;
import jakarta.ws.rs.PathParam;
import jakarta.ws.rs.Produces;
import jakarta.ws.rs.QueryParam;
import jakarta.ws.rs.core.MediaType;
import jakarta.ws.rs.core.MultivaluedMap;
import jakarta.ws.rs.core.Response;
import java.io.IOException;
import java.io.InputStream;
import java.util.Map;
import java.util.Objects;
import java.util.function.Predicate;
import java.util.stream.Stream;

import static jakarta.ws.rs.core.Response.Status.BAD_REQUEST;
import org.keycloak.utils.ReservedCharValidator;

/**
* @resource Identity Providers
Expand All @@ -71,8 +74,8 @@ public class IdentityProvidersResource {

private final RealmModel realm;
private final KeycloakSession session;
private AdminPermissionEvaluator auth;
private AdminEventBuilder adminEvent;
private final AdminPermissionEvaluator auth;
private final AdminEventBuilder adminEvent;

public IdentityProvidersResource(RealmModel realm, KeycloakSession session, AdminPermissionEvaluator auth, AdminEventBuilder adminEvent) {
this.realm = realm;
Expand All @@ -82,7 +85,7 @@ public IdentityProvidersResource(RealmModel realm, KeycloakSession session, Admi
}

/**
* Get identity providers
* Get the identity provider factory for a provider id.
*
* @param providerId Provider id
* @return
Expand All @@ -92,8 +95,8 @@ public IdentityProvidersResource(RealmModel realm, KeycloakSession session, Admi
@NoCache
@Produces(MediaType.APPLICATION_JSON)
@Tag(name = KeycloakOpenAPI.Admin.Tags.IDENTITY_PROVIDERS)
@Operation( summary = "Get identity providers")
public Response getIdentityProviders(@Parameter(description = "Provider id") @PathParam("provider_id") String providerId) {
@Operation( summary = "Get the identity provider factory for that provider id")
public Response getIdentityProviders(@Parameter(description = "The provider id to get the factory") @PathParam("provider_id") String providerId) {
@jonkoops jonkoops Jul 6, 2023
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.

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

@rmartinc rmartinc Jul 6, 2023
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

this.auth.realm().requireViewIdentityProviders();
IdentityProviderFactory providerFactory = getProviderFactoryById(providerId);
if (providerFactory != null) {
Expand Down Expand Up @@ -166,23 +169,75 @@ public Map<String, String> importFrom(@Parameter(description = "JSON body") Map<
}

/**
* Get identity providers
* List identity providers.
*
* @return
* @param search Filter to search specific providers by name. Search can be prefixed (name*), contains (*name*) or exact (\"name\"). Default prefixed.
* @param firstResult Pagination offset
* @param maxResults Maximum results size (defaults to 100)
* @return The list of providers.
*/
@GET
@Path("instances")
@NoCache
@Produces(MediaType.APPLICATION_JSON)
@Tag(name = KeycloakOpenAPI.Admin.Tags.IDENTITY_PROVIDERS)
@Operation( summary = "Get identity providers")
public Stream<IdentityProviderRepresentation> getIdentityProviders() {
@Operation(summary = "List identity providers")
public Stream<IdentityProviderRepresentation> getIdentityProviders(
@jonkoops jonkoops Jul 6, 2023
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.

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 rmartinc Jul 6, 2023
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@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 jonkoops Jul 6, 2023
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.

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 ssilvert Jul 6, 2023
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.

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 mposolda Jul 19, 2023
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.

@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 jonkoops Jul 19, 2023
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.

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.

@Parameter(description = "Filter specific providers by name. Search can be prefix (name*), contains (*name*) or exact (\"name\"). Default prefixed.") @QueryParam("search") String search,
@Parameter(description = "Pagination offset") @QueryParam("first") Integer firstResult,
@Parameter(description = "Maximum results size (defaults to 100)") @QueryParam("max") Integer maxResults) {
this.auth.realm().requireViewIdentityProviders();

if (firstResult == null) {
firstResult = 0;
}
if (maxResults == null) {
maxResults = 100;
}

return realm.getIdentityProvidersStream()
.filter(predicateByName(search))
.skip(firstResult)
.limit(maxResults)
.map(provider -> StripSecretsUtils.strip(ModelToRepresentation.toRepresentation(realm, provider)));
}

/**
* Count identity providers.
*
* @param search Filter to search specific providers by name. Search can be prefixed (name*), contains (*name*) or exact (\"name\"). Default prefixed.
* @return Integer count of providers.
*/
@GET
@Path("instances/count")
@NoCache
@Produces(MediaType.APPLICATION_JSON)
@Tag(name = KeycloakOpenAPI.Admin.Tags.IDENTITY_PROVIDERS)
@Operation( summary = "Count identity providers")
public Integer countIdentityProviders(
@jonkoops jonkoops Jul 6, 2023
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.

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 rmartinc Jul 6, 2023
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

@Parameter(description = "Filter to search specific providers by name. Search can be prefixed (name*), contains (*name*) or exact (\"name\"). Default prefixed.") @QueryParam("search") String search) {
this.auth.realm().requireViewIdentityProviders();

return Math.toIntExact(this.realm.getIdentityProvidersStream().filter(predicateByName(search)).count());
}

private Predicate<IdentityProviderModel> predicateByName(final String search) {
if (StringUtil.isBlank(search)) {
return (m) -> true;
} else if (search.startsWith("\"") && search.endsWith("\"")) {
final String name = search.substring(1, search.length() - 1);
return (m) -> m.getAlias().equals(name);
} else if (search.startsWith("*") && search.endsWith("*")) {
final String name = search.substring(1, search.length() - 1);
return (m) -> m.getAlias().contains(name);
} else if (search.endsWith("*")) {
final String name = search.substring(0, search.length() - 1);
return (m) -> m.getAlias().startsWith(name);
} else {
return (m) -> m.getAlias().startsWith(search);
}
}

/**
* Create a new identity provider
*
Expand Down
27 changes: 24 additions & 3 deletions ...rquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/IdentityProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,12 +131,33 @@ public class IdentityProviderTest extends AbstractAdminTest {
+ "LXrAUVcsR73oTngrhRfwUSmPrjjK0kjcRb6HL9V/+wh3R/6mEd59U08ExT8N38rhmn0CI3ehMdebReprP7U8=";

@Test
public void testFindAll() {
public void testFind() {
create(createRep("github", "github"));
create(createRep("google", "google"));

create(createRep("facebook", "facebook"));
create(createRep("linkedin", "linkedin"));
create(createRep("twitter", "twitter"));

Assert.assertEquals((Integer) 5, realm.identityProviders().count(null));
Assert.assertNames(realm.identityProviders().findAll(), "github", "google", "facebook", "linkedin", "twitter");

Assert.assertNames(realm.identityProviders().find(null, 0, 2), "github", "google");
Assert.assertNames(realm.identityProviders().find(null, 2, 2), "facebook", "linkedin");
Assert.assertNames(realm.identityProviders().find(null, 4, 2), "twitter");

Assert.assertEquals((Integer) 2, realm.identityProviders().count("g"));
Assert.assertNames(realm.identityProviders().find("g", 0, 5), "github", "google");

Assert.assertEquals((Integer) 2, realm.identityProviders().count("g*"));
Assert.assertNames(realm.identityProviders().find("g*", 0, 5), "github", "google");
Assert.assertNames(realm.identityProviders().find("g*", 0, 1), "github");
Assert.assertNames(realm.identityProviders().find("g*", 1, 1), "google");

Assert.assertEquals((Integer) 2, realm.identityProviders().count("*oo*"));
Assert.assertNames(realm.identityProviders().find("*oo*", 0, 5), "google", "facebook");

Assert.assertNames(realm.identityProviders().findAll(), "google", "facebook");
Assert.assertEquals((Integer) 1, realm.identityProviders().count("\"twitter\""));
Assert.assertNames(realm.identityProviders().find("\"twitter\"", 0, 5), "twitter");
}

@Test
Expand Down
0