Repository navigation
Add @DefaultValue to document the max param used in several Admin REST APIs - #47561
Conversation
| @@ -121,7 +121,7 @@ public Stream<ClientRepresentation> getClients(@Parameter(description = "filter | |||
| @Parameter(description = "whether this is a search query or a getClientById query") @QueryParam("search") @DefaultValue("false") boolean search, | |||
| @QueryParam("q") String searchQuery, | |||
| @Parameter(description = "the first result") @QueryParam("first") Integer firstResult, | |||
| @Parameter(description = "the max results to return") @QueryParam("max") Integer maxResults) { | |||
| @Parameter(description = "the max results to return") @QueryParam("max") @DefaultValue("100") Integer maxResults) { | |||
There was a problem hiding this comment.
I didn't check all the other endpoints, but at least for this query there is currently no such default.
Are you wanting this to be an enhancement, or was it unintended?
There was a problem hiding this comment.
Thank you @shawkins for responding.
This was an intended change, and no, this is not an enhancement request. This endpoint is a bit jank. If we look at how ClientsResource.getClients works, we can see that it makes four different SQL queries depending on the parameters given to getClientModels:
- If
searchQueryis defined, thenrealm.searchClientByAttributes(...)is called.RealmAdapter.searchClientByAttributesthen callsClientProvider.searchClientsByAttributes.JpaRealmProvider.searchClientsByAttributessets a defaultmaxResultin the method callpaginateQuerywhich sets max toDEFAULT_MAX_RESULTSif its value isnullor less than0before returning the clientModels. - If no ClientID is set, then
realm.getClientsStream(firstResult, maxResults)orrealm.getClientsStream(). Both of these end up atJpaRealmProvider.getClientsStream, which then callsJpaRealmProvider.getClientIdsStream. This method then callspaginateQuerybefore returning the clientModels. - Then, if
searchit callsrealm.searchClientByClientIdStream(...). Like the last 2 SQL calls, it eventually ends up atpaginateQuerybefore returning client models. - Finally, if none of the above get triggered, it returns a single client that matches the ClientID that's been given.
It seems to set the max by default, but it is somewhat obfuscated as it's done in the JPA-related classes.
The only thing I'm not sure about is how Provider classes work (like ClientProvider/ClientStorageManager/JpaRealmProvider/RealmCacheSession) I assume the Jpa is always used in some way and in the case of other providers (like ClientStorageManager, RealmCacheSession) caches the Jpa response in some way?
There was a problem hiding this comment.
It seems to set the max by default, but it is somewhat obfuscated as it's done in the JPA-related classes.
PaginationUtils.DEFAULT_MAX_RESULTS is effectively no max. You can see from the issue link there that is being done as a fix some hibernate bug.
The only thing I'm not sure about is how Provider classes work (like ClientProvider/ClientStorageManager/JpaRealmProvider/RealmCacheSession) I assume the Jpa is always used in some way and in the case of other providers (like ClientStorageManager, RealmCacheSession) caches the Jpa response in some way?
Jpa is typically where things are fetched yes. As for caching - there's the JPA persistence context, then an infinispan layer of caching over that, which has its own managed entities (basically a higher level persistence context) and also puts / gets things from the infinispan caches.
edit: clarified that PaginationUtils.DEFAULT_MAX_RESULTS is different from Constants.DEFAULT_MAX_RESULTS
There was a problem hiding this comment.
I see, my bad. Thank you for correcting me. I've amend my commit to remove this @DefaultValue, as well as the ones for RoleContainerResource#getRoles and OrganizationInvitationResource#getInvitations as both I thought had a default value due to PaginationUtils.paginateQuery being called.
There was a problem hiding this comment.
Hey @shawkins, I've made some changes if you have time to review them :)
But if you're aware and are busy with other things, no worries
There was a problem hiding this comment.
@IestynGage thank you for updating #47561 (comment)
A couple of thoughts:
- as we DefaultValues in other places in our services wrt pagination (first and max), the changes proposed here seem good
- it would be better to have a String constant defined by Constants.DEFAULT_MAX_RESULTS, rather than individual string literals.
- for other maintainers to help clarify why we have Constants.DEFAULT_MAX_RESULTS being applied in only some of the endpoints - and if there's a follow-up that needs to be done, or a convention that should be followed by admin api v2. cc @vmuzikar @mposolda
There was a problem hiding this comment.
I've introduced a Constants.DEFAULT_MAX_RESULTS_STR with a comment on when to use it.
I'm not sure what you mean by the third bullet point, is it meant to be a internal hidden note? Anyways, your are correct that I'm not sure why Constants.DEFAULT_MAX_RESULTS is applied to only some of the endpoints. TBH I just assumed it was becasue no one has focused on a consistent API design or a potentially backwards-compatible reason.
There was a problem hiding this comment.
I've introduced a Constants.DEFAULT_MAX_RESULTS_STR with a comment on when to use it.
Thank you
I'm not sure what you mean by the third bullet point, is it meant to be a internal hidden note?
Sorry, it wasn't quite a complete thought - tagged in a couple of others for further review.
|
Summary of where each endpoint's default is set.
Edit: Strikethrough any defaults I mistakenly thought was set by PaginationUtils, which is incorrect. See comment. |
cc9b97c to
928a7e1
Compare
| @@ -572,7 +572,7 @@ public ManagementPermissionReference setManagementPermissionsEnabled(final @Path | |||
| public Stream<UserRepresentation> getUsersInRole(final @Parameter(description = "the role name.") @PathParam("role-name") String roleName, | |||
| @Parameter(description = "Boolean which defines whether brief representations are returned (default: false)") @QueryParam("briefRepresentation") Boolean briefRepresentation, | |||
| @Parameter(description = "first result to return. Ignored if negative or {@code null}.") @QueryParam("first") Integer firstResult, | |||
| @Parameter(description = "maximum number of results to return. Ignored if negative or {@code null}.") @QueryParam("max") Integer maxResults) { | |||
| @Parameter(description = "maximum number of results to return. Ignored if negative or {@code null}.") @QueryParam("max") @DefaultValue("100") Integer maxResults) { | |||
There was a problem hiding this comment.
Does this description make sense? Does ignore mean the default is used?
There was a problem hiding this comment.
That's a good point, the description is inaccurate.
If changed it and the method comment to sayNegative = Unbounded maxSize. Anything else I could think of was wordy or I was worried it would continue to suggest that negative=Default value.
There was a problem hiding this comment.
What you have now is much clearer.
b81ab1b to
793fe87
Compare
|
@IestynGage can you check the failing tests? It could be that even a few more of these endpoints don't have a consistent max default. |
@shawkins I had a look. The Java/DB related one fail with the same test @Test
@DatabaseTest
public void testPaginationRoles() {
for (int i = 0; i < 15; i++) {
String roleName = "role" + i;
RoleRepresentation role = makeRole(roleName);
rolesRsc.create(role);
managedClient.cleanup().add(c -> c.roles().deleteRole(roleName));
AdminEventAssertion.assertEvent(adminEvents.poll(), OperationType.CREATE, AdminEventPaths.clientRoleResourcePath(clientDbId, roleName), role, ResourceType.CLIENT_ROLE);
}
List<RoleRepresentation> resultSearchWithoutPagination = rolesRsc.list();
assertEquals(15, resultSearchWithoutPagination.size());
...
List<RoleRepresentation> resultSearchPaginationIncoherentParams = rolesRsc.list(1, null);
assertTrue(resultSearchPaginationIncoherentParams.size() >= 15);
// ↑↑↑
// failed line I think?
}Anyways one of the changes I made was adding the max param to this endpoint. I don't think this caused the failure but I removed it just incase... |
This is used to generate Keycloak Admin REST API. Without this a 'null' value is used, which suggests that returning a paginated list is optional. Closes keycloak#47560 Signed-off-by: Iestyn <33298011+IestynGage@users.noreply.github.com>
There was a problem hiding this comment.
@IestynGage Thank you for the PR.
| /** | ||
| * Used by {@code DefaultValue} annotation for when a REST endpoints max size default is set by {@link #DEFAULT_MAX_RESULTS}. | ||
| */ | ||
| public static final String DEFAULT_MAX_RESULTS_STR = "" + DEFAULT_MAX_RESULTS; |
There was a problem hiding this comment.
This would be cleaner but it's not blocking, just a nitpick.
| public static final String DEFAULT_MAX_RESULTS_STR = "" + DEFAULT_MAX_RESULTS; | |
| public static final String DEFAULT_MAX_RESULTS_STR = String.valueOf(DEFAULT_MAX_RESULTS); |
There was a problem hiding this comment.
I agree, but unfortunately, Java doesn't 🤷
When compiling, a element value must be a constant expression is thrown.
@vmuzikar I think the confusion you had (which was shared by @IestynGage) is justified - it's not clear at first glance what the intended behavior is. The changes @IestynGage made here document things correct as they are, but we'll still potentialy want to consider follow-ups:
|
) This is used to generate Keycloak Admin REST API. Without this a 'null' value is used, which suggests that returning a paginated list is optional. Closes keycloak#47560 Signed-off-by: Iestyn <33298011+IestynGage@users.noreply.github.com>
I got tripped up with this and assumed one of the endpoints didn't return paginated response by default.
Hopefully fixing the doc will mean this won't happen again :)
Closes #47560