Repository navigation
refinements related to service methods, serdes - #40110
Conversation
|
+100 for NON_ABSENT (or at least NON_NULL). I'd really hate to have ambiguity between null and not set. |
9d1a0bd to
2bb4747
Compare
4bb5cdf to
81c956b
Compare
and adding a default to representation Signed-off-by: Steve Hawkins <shawkins@redhat.com>
|
@vmuzikar @mabartos and others, I believe this is ready. Other than using a JsonProperty annotation to describe the protocol default, there is nothing else that is in the initial scope wrt defaults. We have talked about a pipeline of defaulting, validation, and canonicalization that should be performed prior to storage. In reviewing the old logic the defaulting and validation seem to be handled in RepresentationToModel, adjunct logic like the LoginProtocolFactory, and in the service layer. Since we are mapping directly to the model layer, we will end up needing to replicate all that logic as it's dependent upon the legacy representation logic. It might be worth capturing why the approach of mapping directly to the model layer is preferable initially to an approach where we layer on top of the v1 representation. |
| return updateClient(realm, client); | ||
| } | ||
|
|
||
| ClientRepresentation updateClient(RealmModel realm, ClientRepresentation client) throws ServiceException; |
There was a problem hiding this comment.
Will we need some switch/flag for partial/PATCH updates?
There was a problem hiding this comment.
Will we need some switch/flag for partial/PATCH updates?
That will be a separate method.
and adding a default to representation Signed-off-by: Steve Hawkins <shawkins@redhat.com>
and adding a default to representation Signed-off-by: Steve Hawkins <shawkins@redhat.com>
and adding a default to representation Signed-off-by: Steve Hawkins <shawkins@redhat.com>
and adding a default to representation Signed-off-by: Steve Hawkins <shawkins@redhat.com>
and adding a default to representation Signed-off-by: Steve Hawkins <shawkins@redhat.com>
and adding a default to representation Signed-off-by: Steve Hawkins <shawkins@redhat.com>
Took the liberty of rebasing to main since I don't believe there is other pending work.
Re-opening the prior pull request, but this time it won't be a discussion about defaults.
Added a rough in of ClientService method refinements. In an ideal world this would all be pretty similar across types, possibly even allow for some generic base interfaces / classes.
Jackson notes:
I'd advocate for using NON_ABSENT (or NON_NULL) at a top level to allow for "empty" values such an empty string. However this has the side effect that any possibly fully empty child objects or collections will need to be specifically marked as NON_EMPTY (fabric8 does this).
Nullability:
Using not null / non absent heavily implies we will avoid using null as a value. If we want to use the existing representation classes for our own tests for example you can't do something like:
var client = new ClientRepresentation();
client.setSomeField(null);
Then call the endpoint and expect the serialized form to contain
"someField":null
This is how things are with the fabric8 kubernetes client built-in classes as well - and I don't think we've come across a case yet for simple put or post operations where this is a problem.
Kubernetes explains the relationship between their metadata, defaults, and nullability here: https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#defaulting-and-nullable - basically if a field is not explicitly nullable, then a null value or omitting the field are the same thing.
If we do need allow for some fields having explicit null values in get / put operations - and want to use our representation classes for that (which will be handy for our own tests), we'll need to be able to differentiate between null and unset. That could done internally with a wrapper, or a map. For example if wanted appUrl to be explicitly nullable:
We may end up wanting a map like this in general if we want to capture additionalProperties like we do in fabric8.
To be useful for creating patches, you have to do something like the above for all field, so I'll probably just show how we can approach patching (json patch and json merge) in the next pr by just avoiding using the representation class to deserialize / create json merge patches.
Defaults:
Added just one default, which is in ClientRepresentation / Jackson annotation, which is the protocol defaulting to OIDC - if that's not appropriate, I'll remove it.
@keycloak/cloud-native WDYT?