-
Notifications
You must be signed in to change notification settings - Fork 303
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
Board Review: Support for JSON Merge Patch #3520
Comments
scheduled for 11/4 |
I see two ways to do this in C#. An
|
To avoid the "stomp" problem described above, and to not have to require protocol methods, our patchable models could provide the following APIs: public class Employee
{
public int? Id { get; set; }
public string Name { get; set; }
public int? Age { get; set; }
public string Title { get; set; }
public Employee CreatePatch();
public static explicit operator RequestContent(Employee value);
} The usage would be: e = client.GetEmployee(e.Id);
e = e.CreatePatch();
e.Title = "Bob II";
e = client.UpdateEmployee(e); // this casts e to RequestContent. The cast knows how to create JSON Patch payload
// sent { "title": "Bob II" } over the wire
// received back { "id": 17, "name": "Bob", "age": 42, "title": "Bob II" } from the service Possibly we could return "patchable" values, i.e. return value of e.CreatePath, from Get methods. In this case users would simply do: e = client.GetEmployee(e.Id); // this returns patchable value, i.e. returns e.CreatePatch() instead of returning e.
e.Title = "Bob II";
e = client.UpdateEmployee(e); // this casts e to RequestContent. The cast knows how to create JSON Patch payload
// sent { "title": "Bob II" } over the wire
// received back { "id": 17, "name": "Bob", "age": 42, "title": "Bob II" } from the service The main disadvantage of this idea (of "a patchable model") is that it has perf overhead, i.e. each model has to have an additional field that would store the original. |
Recording[MS INTERNAL ONLY] |
@lilyjma Could you please schedule a follow-on meeting for this topic? Thx! |
One thing/requirement that is missing from the examples above is the ability to update without doing a GET/read first. |
second discussion scheduled for 11/30 |
I experimented with the idea of "smart" models. Here is the result of the experiment: https://gist.github.com/KrzysztofCwalina/d5c1fba32eb4548f14bbf27184401731 I think smart models add a lot of complexity. Because of that, I would suggest we start with something like untyped JsonPatchBuilder, which users can use to create patch payloads stored in BinaryData or RequestContent, which in turn can be passed to "update" methods. We can think long term about simpler "smart models" if users complain. |
Recording[MS INTERNAL ONLY] |
SummaryJava and .NETWill implement a PatchBuilder.
JavascriptJavaScript has native support for "null" so it is covered. PythonPython will continue to use it's special sentinel to signify "null" GoGo will continue to use it's special sentinel to signify "null"
Swift ???Follow-on items
|
RFC 7386 is obsoleted by https://www.rfc-editor.org/rfc/rfc7396 |
Hi @mikekistler, we deeply appreciate your input into this project. Regrettably, this issue has remained inactive for over 2 years, leading us to the decision to close it. We've implemented this policy to maintain the relevance of our issue queue and facilitate easier navigation for new contributors. If you still believe this topic requires attention, please feel free to create a new issue, referencing this one. Thank you for your understanding and ongoing support. |
The recently updated Azure API Guidelines recommend that update operations be implement using the
PATCH
method and specifically with JSON Merge Patch. The preference for JSON Merge Patch is that it is naturally idempotent so no special support is required to ensure idempotency. JSON Merge Patch is also superior to PUT because PUT from a client that is not at the latest api-version can remove properties that were added in later api-versions.But JSON Merge Patch, as defined in RFC 7386, has special semantics, particularly for values passed as "null".
This is a subtle detail that is often overlooked, so it is worth taking a look at this to see if we have any gaps. There are at least 3 aspects to consider:
1) Server-side implementation
As many Azure services are implemented in ASP.NET Core, we should make sure there is proper support for JSON Merge-Patch provided in this framework and that it is well documented and easy to use.
2) Client-side support
Do our current generated (or hand-written) clients properly support passing "null" explicitly for a property value to an update operation that uses JSON Merge Patch? Apparently some languages do support this:
Go: has a NullValue() function that returns a reference to a T. This reference is to a singleton instance of T (our sentinel). When this particular T instance is serialized Go emits ‘null’. This works in Go because all properties are defined as reference types.
TypeScript/JavaScript: JavaScript supports this naturally because "null" is a distinct value from "undefined".
Java: Java does not have support for nulls in JSON Merge-Patch but the team has done some investigation on how it might be done:
https://gist.github.com/anuchandy/ca914f4a8dfaa49ea8e082de7e619c8f
The other languages may have support for this but I'm just not aware of it.
For languages that do have this support, does our testserver verify that it works correctly?
3) Client-side documentation
Even when our client libraries support passing an explicit "null", users need to know about it in order to use it properly. Does our SDK documentation describe this feature sufficiently and is that documentation easily discoverable by users?
The text was updated successfully, but these errors were encountered: