Replies: 4 comments 12 replies
-
|
Thank you for the idea. The same applies to the use case of a user obtained from the cache but in the meantime removed externally (e.g. from LDAP) - once the fields in the LDAP object would be accessed that are not stored in this cache, it would be the same situation of a partially loaded object and the same measures should apply. Hence I agree that the case of partially loaded model should be clearly signalled to the caller. Before going forward, it is important to understand where the raising and handling the exception takes place. From that we will get the right exception parent (whether it should be a Could you please add the following information for a client use case (mentioned in the description) and user use case (mentioned here):
|
Beta Was this translation helpful? Give feedback.
-
The exception in the example above will he thrown in Similar exceptions could be raised in other places throughout a storage provider implementation; it might also make sense to wrap some of the Hibernate/JPA runtime exceptions later in case we want to handle them in a higher level of the stack.
I assume catching and handling would be done in the services as handling would require context, but I am not sure if I fully understand the layers correctly yet. I would target at code handling lists; if this happens when handling a single entity I would rather throw the exception and show an error to the caller. Handling of the exception example above will be done in ClientResource. The |
Beta Was this translation helpful? Give feedback.
-
|
(I thought about the service/logical/physical layer a bit more: Usually I would go by package name or maven module to identify what is in a layer. With services it is straight-forward to me: everything in a package with "service" in it would be a service. Everything with "jpa" in its package name sounds quite physical, but would be "storage" according to https://github.com/keycloak/keycloak-community/blob/main/design/keycloak.x/storage-architecture.md? I wonder what classes in the "org.keycloak.models.map.*" packages would be. It would be simpler for me if the layers would repeat in the package names as they do for "service") |
Beta Was this translation helpful? Give feedback.
-
|
Thank you.
The layers are assigned correctly. Here comes the question though: What would be the proposal to not involve including Stream<ClientRepresentation> s = clientModels
/* ... */
.map(illegalModelToNull(c -> {
ClientRepresentation representation = null;
if (canView || auth.clients().canView(c)) {
representation = ModelToRepresentation.toRepresentation(c, session);
representation.setAccess(auth.clients().getAccess(c));
} else if (!viewableOnly && auth.clients().canView(c)) {
representation = new ClientRepresentation();
representation.setId(c.getId());
representation.setClientId(c.getClientId());
representation.setDescription(c.getDescription());
}
return representation;
}))
.filter(Objects::nonNull);Is there any better solution? |
Beta Was this translation helpful? Give feedback.
-
|
I think the key to a cleaner processing is using The following assumes that also This would result into code like this: which a helper function The helper function could be placed in ModelToRepresentation, as it will be used together with the toRepresentation() methods that are present there as well. Another thought on the transient error indicator: The concept of exceptions being transient or permanent might be orthogonal to the exception hierarchy that is evolving, that is: there could be different exceptions all over the ModelException hierarchy that are transient, not necessarily in the storage area. It could evolve as a flag in ModelException or as a marker interface. Therefore: let's focus on picking one exception class that clearly identifies the situation "entity has been removed" and allows to return the empty stream in the handleModelExceptions(), while any other exception related to storage (for example entity has been modified) would not be caught here and would bubble up and lead to a 500 error for the caller. |
Beta Was this translation helpful? Give feedback.
-
|
The What is the value of introducing the I like the idea of extending <M, R> Stream<R> ModelToRepresentation.toRepresentation(Stream<M> models, Function<M, R> transformer) {
return models.map(m -> try { return transformer(m);} catch (ModelIllegalStateException e) { return null; })
.filter(Objects::nonNull);
}
This need not be the removal, but for certain the entity supporting the model became illegal. The entity may become illegal by expiring or other ways, so we should not concentrate only on a single painpoint of now. That's why As for the transient error indicator, I agree we can postpone this discussion for now. |
Beta Was this translation helpful? Give feedback.
-
|
Introducing the Optional makes the code more readable IMHO; I like to avoid returning nulls. I wanted to make it explicit that the transformation returns one instance or no instance. Whether the short-lived streams are a performance penalty or not would be hard for me to tell without a JMH test. At the same time the function you propose is elegant and readable, and with some JavaDoc to indicate that the transformation can return null it is good to go for me. To better understand the |
Beta Was this translation helpful? Give feedback.
-
|
For a concurrent modification, a store might try to reload the entity in a consistent way. With the map example at hand and JPA this could be impossible as part of the model has already been mapped, and the exception occurs when retrieving additional parts of the model. Throwing an exception and showing a 500 to the caller would be the only option I see here. Even if a storage might be able to reload the data consistently on a per-entity level, the entity might have been modified in a way that it would no longer fit the search criteria used in the search. Therefore I would rule out recovery from a concurrent modification situation as it would require reading the state as it what when the search was performed. I would opt for an exception and a 500 response code in cases of concurrent modification. |
Beta Was this translation helpful? Give feedback.
-
With a new client being added concurrently, the returned list would be a consistent state that was valid at the time of querying. There would also be some causal consistency: after the client has been added, the client can be retrieved via the list of clients until it is removed. If a concurrent modification would remove a client from the list of returned clients, this would no longer hold true. |
Beta Was this translation helpful? Give feedback.
-
|
Now I see better how you would like it to resolve, while it is a good idea, it is not viable. The transaction is only started and finished with the start / end of request processing (see Keycloak request filter). Reloading the entity is thus not an option, as you have stated as well, since reloading would require a retry, and this is not possible at this moment. It is also not nice to throw 500 since part of the response including headers might have already been sent to the client, and also this could increase a DoS attack surface (if you modify clients fast enough, there would be decent chance to block valid requests to list the clients). For the same reason, I'd vouch against retrying the request in such a case. Hence the remaining option is to do the best effort which is finish sending the stream successfully, with all the records most of the time. The UI / client can show a warning if the number of returned clients does not match the expected one. |
Beta Was this translation helpful? Give feedback.
-
Thanks for the the reasoning for that decision. I'll see that it ends up at least as a piece of JavaDoc next to the exception and the function we agreed to put into
If this is the path to go, this should be documented somewhere for the caller as it would come as a surprise. The current documentation of the Admin API is generated from the JavaDoc of the ClientsResource. Would this be the place to add it?
AFAIK the admin API's response doesn't provide that information, just a stream of ClientRepresentations, therefore the caller will have no way to know that a client got missing. |
Beta Was this translation helpful? Give feedback.
-
|
After talking to @hmlnarik today, we agreed to document the decisions taken in JavaDoc. While some of the JavaDoc documentation from ClientResource ends up in the REST API documentation automatically, additional documentation is out of scope for this issue. The situation that the caller will not know about filtered out rows needs to be addressed in a future redesign of the REST API of Keycloak. A JavaDoc note in ClientResource should guide that future implementation. Next step: I'll create a GitHub issue summarizing the tasks for the implementation. |
Beta Was this translation helpful? Give feedback.
-
|
GitHub issue created and assigned to me: #9645 |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
-
Problem statement
When accessing data from Keycloak (either read or write), there can be different kinds of problems, usually in the kind of transactions failing, database constraints violated, lazy loading issues.
In the example of the JPA map storage when listing all the clients for a REST Admin API caller, it first reads a subset of the fields for performance reasons. Later, when retrieving all details, it will fetch the additional fields. In this case the record in the table could have been deleted or modified.
It could be seen when running ConcurrencyTest#createRemoveClient() with a JPA map storage configuration: The server returns a 500 error when retrieving a list of clients while one of the clients in the list is removed by another client concurrently.
Approach
When the JPA map storage provider recognizes that the entity has been removed from the database, it should throw an exception that extends the ModelException (for example: ObjectRemovedException). The ClientResource#getClients() could then catch the exception and filter the instance from the list.
There seems to be an existing handling of exceptions in place, although it is
try { c.getClientId(); return true; } catch (Exception ex) { return false; })In the mid-term there might be additional exceptions like ObjectModifiedException for optimistic locking exceptions. In that case the ClientResource will be more difficult to recover and would most likely pass a 500 error to the caller.
Benefits
The caller would retrieve a list of valid clients even if one client of the list is removed concurrently.
Alternatives
The caller could receive a 500 exception. The caller would then still additional information if the problem is a transient problem and when to retry the call.
Beta Was this translation helpful? Give feedback.
All reactions