Repository navigation
implementing json patch - #40904
implementing json patch#40904
Conversation
Signed-off-by: Steve Hawkins <shawkins@redhat.com>
There was a problem hiding this comment.
The changes look good, straightforward enough. My only concern would be the zjsonpatch dependency. It seems more like an internal utility lib for Fabric8 rather than something truly standalone (e.g. there doesn't seem to be any docs etc.). I would be worried if we could truly rely on it (in terms of breaking changes, or if it simply doesn't disappear with the next Fabric8 release). Or it's not that case?
It's just a fork of the original zjsonpatch to strip it of additional dependencies and to always be aligned with our version of jackson fabric8io/kubernetes-client#6276 It has not been promoted for external usage, but it's highly unlikely to disappear. The biggest downside to the usage of parsson is that it requires an additional serialization / deserialization from string. That is: object -> string (or char stream) -> apply patch -> string (or char stream) -> result object The initial conversion from object to string will be handled by an ObjectMapper, which we can ultimately tweak if needed. However the JsonValue -> string conversion will be handled by a parsson JsonWriter which is not based upon Jackson and it's not clear how we would implement customizations - at the very least any Jackson annotations would not be used for this conversion step. |
Signed-off-by: Steve Hawkins <shawkins@redhat.com>
|
@vmuzikar are you good with merging? |
* implementing json patch Signed-off-by: Steve Hawkins <shawkins@redhat.com> * adding zjsonpatch to the dist and adding basic tests Signed-off-by: Steve Hawkins <shawkins@redhat.com> --------- Signed-off-by: Steve Hawkins <shawkins@redhat.com>
* implementing json patch Signed-off-by: Steve Hawkins <shawkins@redhat.com> * adding zjsonpatch to the dist and adding basic tests Signed-off-by: Steve Hawkins <shawkins@redhat.com> --------- Signed-off-by: Steve Hawkins <shawkins@redhat.com>
* implementing json patch Signed-off-by: Steve Hawkins <shawkins@redhat.com> * adding zjsonpatch to the dist and adding basic tests Signed-off-by: Steve Hawkins <shawkins@redhat.com> --------- Signed-off-by: Steve Hawkins <shawkins@redhat.com>
* implementing json patch Signed-off-by: Steve Hawkins <shawkins@redhat.com> * adding zjsonpatch to the dist and adding basic tests Signed-off-by: Steve Hawkins <shawkins@redhat.com> --------- Signed-off-by: Steve Hawkins <shawkins@redhat.com>
* implementing json patch Signed-off-by: Steve Hawkins <shawkins@redhat.com> * adding zjsonpatch to the dist and adding basic tests Signed-off-by: Steve Hawkins <shawkins@redhat.com> --------- Signed-off-by: Steve Hawkins <shawkins@redhat.com>
@keycloak/cloud-native this is roughly what json patch support looks like. We would also need to put the fabric8 zjsonpatch dependency in the lib.
If we don't want the additional dependency, the alternative is to support jakarta.json / parsson (coming from Quarkus) - that seems slightly less efficient as we'll need to convert back and forth from the JsonValue representation.
Let me know what you think.