Skip to content

Comments

Update serialization code to use new signatures#3838

Merged
ArcturusZhang merged 47 commits intoAzure:feature/v3from
ArcturusZhang:public-serialization
Jan 4, 2024
Merged

Update serialization code to use new signatures#3838
ArcturusZhang merged 47 commits intoAzure:feature/v3from
ArcturusZhang:public-serialization

Conversation

@ArcturusZhang
Copy link
Member

@ArcturusZhang ArcturusZhang commented Oct 14, 2023

Supersedes #3434

Fixes #3181
Fixes #3353
Fixes #2351
Fixes Azure/azure-sdk-for-net#16977
Fixes #2833

Description

This feature literally changes every model, therefore this PR will have a lot of files changed. Details of features included in this PR are listed below.

In summary, now a model's serialization class will look like this:

public partial class Foo : IUtf8JsonSerializable, IJsonModel<Foo>
{
    void IUtf8JsonSerializable(Utf8JsonWriter writer) => ((IJsonModel<Model>)this).Write(writer, ModelReaderWriterOptions.DefaultWireOptions);

    void IJsonModel<Foo>.Write(Utf8JsonWriter writer, ModelReaderWriterOptions options)
	{
		/* previous serialization implementation with some minor changes around Json and Wire format and raw data field */
	}

    Foo IJsonModel<Foo>.Create(ref Utf8JsonReader reader, ModelReaderWriterOptions options)
	{
		/* validates the format and call DeserializeFoo method to return a model as result */
	}

	internal static Foo DeserializeFoo(JsonElement element, ModelReaderWriterOptions options = null)
	{
		/* previous deserialization implementation with some minor changes around Json and Wire format and raw data field*/
	}

	BinaryData IPersistableModel<Foo>.Write(ModelReaderWriterOptions options)
    {
		/* call ModelReaderWriter to write the model into a Binrary data */
        return ModelReaderWriter.Write(this, options);
    }

	Foo IPersistableModel<Foo>.Create(BinaryData data, ModelReaderWriterOptions options)
	{
		/* validates the format and call DeserializeFoo method to return a model as result */
	}

	string IPersistableModel<Foo>.GetFormatFromOptions(ModelReaderWriterOptions options) => "J"; // return json when the model ONLY supports Json, return Xml when the model supports Xml

	/* ToRequestContent and FromResponse method unchanged. */
}

For serialization partial class

  • All model have to implement IJsonModel<T> where T is itself
    • If the model is struct, it needs to implement IJsonModel<object> as well, otherwise compilation will fail
    • If the model is the "unknown" model in a discriminated set, it needs to implement IJsonModel<BaseType>
  • A few new implementations have to be added to implement the IJsonModel<T> interface.
  • The main logic of serializing lives in the explicit implementation of IJsonModel<T>, and IUtf8JsonSerializable just calls the implementation
  • The DeserializeXXX method remains mostly unchanged, a new optional parameter ModelReaderWriterOptions options is added.
  • When the model is abstract, it should have an attribute [DeserializationProxy(typeof(<the unknown internal type of it>))]
  • Write readonly properties into JSON when options.Format == ModelReaderWriterFormat.Json
  • Write the private "additional properties" into JSON when options.Format == ModelReaderWriterFormat.Json.
    (question) should we change this to options.Format != ModelReaderWriterFormat.Wire?
  • Read the private "additional properties" from JSON when options.Format == ModelReaderWriterFormat.Json
  • every model needs to implement IUtf8JsonSerializable (this might change in the future)

We leave the ToRequestContent method and FromResponse method unchanged, until the corresponding APIs in Azure.Core get into its next stable release.

For the model itself

  • Every model should have a new private "AdditionallProperties" property:
private Dictionary<string, BinaryData> _serializedAdditionalRawData; // when this model does not have any derived classes
protected internal Dictionary<string, BinaryData> _serializedAdditionalRawData; // when this model does not have any derived classes
  • This private/internal property automatically becomes AdditionalProperties when the model gets the flag on. Therefore the _serializedAdditionalRawData change does not happen on the models that already have additional properties.
    Since _serializedAdditionalRawData is just a private/protected internal version of AdditionalProperties, we will reuse the AdditionalProperties functionalities to implement it.
  • The internal ctor for deserialization will always accept the additional properties property as a property, no matter it is private or public.
  • The model factory entries do not add the private additional properties property as a parameter, but the model factory will include the public additional properties as a parameter. This is the same as previous behavior of model factory, no API changes of the existing entries are expected in model factory.
  • Every model now should have a new internal empty ctor if it does not have public empty ctor (for deserialization).
  • All models now will have both initialization ctor and serialization ctor (the internal ctor)
  • Model factory now includes those models without a deserializer previously, meaning new methods will be added to the model factory.

Checklist

To ensure a quick review and merge, please ensure:

  • The PR has a understandable title and description explaining the why and what.
  • The PR is opened in draft if not ready for review yet.
    • If opened in draft, please allocate sufficient time (24 hours) after moving out of draft for review
  • The branch is recent enough to not have merge conflicts upon creation.

Ready to Land?

  • Build is completely green
    • Submissions with test failures require tracking issue and approval of a CODEOWNER
  • At least one +1 review by a CODEOWNER
  • All -1 reviews are confirmed resolved by the reviewer
    • Override/Marking reviews stale must be discussed with CODEOWNERS first

@ArcturusZhang ArcturusZhang added Mgmt This issue is related to a management-plane library. Client This issue points to a problem in the data-plane of the library. DPG labels Oct 16, 2023
@ArcturusZhang ArcturusZhang changed the title [WIP] Update serialization code to use new signatures Update serialization code to use new signatures Nov 4, 2023
@ArcturusZhang ArcturusZhang marked this pull request as ready for review November 6, 2023 01:34
@chunyu3
Copy link
Member

chunyu3 commented Nov 9, 2023

LGTM, just a simple comment. I approved it.
And now, we will always generate serialize/deserialize method for models ignore its usage?

@ArcturusZhang ArcturusZhang merged commit 833f753 into Azure:feature/v3 Jan 4, 2024
@ArcturusZhang ArcturusZhang deleted the public-serialization branch January 4, 2024 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Client This issue points to a problem in the data-plane of the library. DPG Mgmt This issue is related to a management-plane library.

Projects

None yet

7 participants