-
Notifications
You must be signed in to change notification settings - Fork 294
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.
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
Enable JSON Patch in AOT #1588
base: master
Are you sure you want to change the base?
Enable JSON Patch in AOT #1588
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: hwoodiwiss The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Welcome @hwoodiwiss! |
fb6c551
to
08159fe
Compare
08159fe
to
e840243
Compare
@@ -91,6 +91,11 @@ public static TValue Deserialize<TValue>(Stream json, JsonSerializerOptions json | |||
|
|||
public static string Serialize(object value, JsonSerializerOptions jsonSerializerOptions = null) | |||
{ | |||
if (value is V1Patch { Content: string jsonValue }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i did not get it, is it still some dynamic thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand the question?
In the usages I've seen, the Content property of the V1Patch object is always set as the json string value of the patch request body. This pattern, broken down, checks:
value is of type V1Patch, and is not null
value.Content is of type string
Then it assigns the value of value.Content to jsonValue, however this is all essentially just syntactic sugar, so all of that is resolved at compile time into a slightly uglier string of conditions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me ask this way?
is it a breaking change or different behavior from non-aot version?
the assumption is that V1Patch must be json string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh, got you, this isn't a breaking behaviour, and I would say it's restoring some functionality that exists in the non-aot version to the aot version.
This only restores the ability to use json string in the json patch, however I don't think full parity can be achieved without changing the API surface.
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1588 +/- ##
=========================================
Coverage ? 61.47%
=========================================
Files ? 103
Lines ? 3071
Branches ? 640
=========================================
Hits ? 1888
Misses ? 1183
Partials ? 0 ☔ View full report in Codecov by Sentry. |
Updates
KubernetesJson.Serialize
with a special case for seializingV1Patch
when the content is string, as based on the examples and the converter implementation, the expectation in this case is that the conent already be JSON.fixes #1587