Skip to content

Prototype sketching JSON Merge Patch convenience APIs#37341

Closed
annelo-msft wants to merge 121 commits intoAzure:mainfrom
annelo-msft:core-mergepatch-proto2
Closed

Prototype sketching JSON Merge Patch convenience APIs#37341
annelo-msft wants to merge 121 commits intoAzure:mainfrom
annelo-msft:core-mergepatch-proto2

Conversation

@annelo-msft
Copy link
Member

@annelo-msft annelo-msft commented Jun 30, 2023

Prototype addressing #27402

Description

We plan to generate convenience APIs for PATCH operations for clients that use JSON Merge Patch. This PR provides a sketch of what that might look like using MutableJsonDocument to track changes to convenience-layer models, and generate the JSON Merge Patch request content from the MutableJsonDocument changelist.

The primary elements of the proposed design include:

Additional details include:

  • Optional properties use MutableJsonElement.TryGetProperty before getting a value.
  • Nested models follow the same pattern of storing a MutableJsonElement. Because this one, PassFailCriteria, is optional, it isn't created until it's accessed, and then it's added to the parent JSON document.
  • Where we need to set a value that is an Azure SDK model type on a MutableJsonDocument property, the model will need to implement IModelSerializable. (Note: this version of IModelSerializable is for convenience of this prototype, and not a proposal for the final API. We are also not blocked on that API being available in Azure.Core. If needed, we can use IUtf8JsonSerializable for serialization here, but we might need to roll something internal for deserialization as a stopgap.)
  • The addition of RequestContent.CreatePatch is to enable PATCH operations at the protocol layer, and not required for the convenience APIs. It feels like we should release the functionality at the same time, but the protocol layer implementation has more requirements, so if we get in a rush, we can remove this API and add it later.
  • I've added some helper types that implement IDictionary<TKey, TValue> and IReadOnlyList<T> for generated properties of these types. We can continue down this path for other interfaces that the generator uses for model properties if we like this approach.

Some auxiliary notes:

  • We don't want to keep MutableJsonDocument in internal shared source for very long, lest people take dependencies on it and lock us into functionality we want to change later. We can do it to unblock commitments in the short term, but it would be best if we can add an analyzer telling people not to use it in their libraries, so no one does this mistakenly. We plan to make the type public as soon as we can.
  • The current implementation of JSON Merge Patch request content generation is largely throwaway because it's non-performant and way too fragile. It is expected to work for convenience API cases, but not for the general DynamicData case. Tests showing where it is known to fail are included, but these should only fail for cases needed by the protocol layer, and not by the convenience layer.

TODOs:

  • Provide example of required property on model
  • Have IModelSerializable.Serialize() call through to MutableJsonElement.WriteTo('P') for PATCH
  • Address Batch APIs directly
  • Deserialize without reflection
  • Move Patch JSON implementation to Span APIs
  • Implement Patch JSON for arrays
  • Implement Patch JSON for object deletions
  • Implement Patch JSON for property deletions that happen when an object is replaced
  • Move load test models to IJsonModelSerializable
  • Address model constructors and serialization constructors
  • Check that we cover all the functionality of change tracking dictionary and change tracking list
  • Decide on whether MutableJsonElement should implement collection interfaces or use helper types
  • What is parent child relationship across doc/element for large models and/or collections of models

nisha-bhatia and others added 30 commits April 21, 2023 11:09
* wip

* wip

* wip

* wip

* wip
* add scaffolding for readme

* fix type on class name

* one more typo :-(

* make tests sync
* add use cases for explicit casts

* remove extra empty line

* add custom expected value for net462
* add use cases for stj

* address feedback

* add some clarification to readme
* wip

* wip

* wip

* Update StaticDeserializerTests.cs

* Update StaticDeserializerTests.cs

* Update StaticDeserializerTests.cs
* add use cases for stj

* address feedback

* add some clarification to readme

* add model serializer example

* move serialization classes into azure.core.serialization

* update after namespace move
* Update SerializableOptions.cs

* wip

* wip

* update

* wip

* wip
* wip

* Update ModelSerializer.cs

* wip

* Update ReadOnlyPropertyTests.cs

* wip

* Update SerializableOptions.cs

* Update SerializableOptions.cs

* wip

* wip

* Update SerializationSamples.cs
* demo of what it would look like for public surface

* wip

* update tests and more renames

* name tweaks

* update api

* add nullable

* adjust public api surface and move test cases to public project to avoid internalsvisibleto

* integrate feature/updateSerializer

* rename IModel to IModelSerializable since it now has the serialization methods on it

* share deserialization code between converter and modelserializer
* wip

* wip

* Update Serialization.md

* Update ModelXmlTests.cs

* wip

* wip

* update combined interface example

---------

Co-authored-by: m-nash <prognash@gmail.com>
* update to v1 api surface

* update api after merge
@annelo-msft
Copy link
Member Author

Closing in favor of #38324

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants