-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[Azure.Core] Add Public Model Serialization #35742
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
// Copyright (c) Microsoft Corporation. All rights reserved. | ||
// Licensed under the MIT License. | ||
|
||
using System; | ||
using System.IO; | ||
|
||
namespace Azure | ||
{ | ||
/// <summary> | ||
/// TODO | ||
/// </summary> | ||
public interface IJsonSerializable | ||
{ | ||
/// <summary> | ||
/// TODO | ||
/// </summary> | ||
/// <param name="stream"></param> | ||
/// <param name="bytesWritten"></param> | ||
/// <param name="options"></param> | ||
/// <returns></returns> | ||
bool TrySerialize(Stream stream, out long bytesWritten, SerializableOptions? options = default); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did we consider also taking There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could that handle a stream where position != 0 though? A There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See #35742 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the similar vein - DynamicJson /cc: @annelo-msft There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @pallavit - yes, it would be good to look at the interactions between DynamicData and the serialization interfaces and see if there's a good way to make it easy to go back and forth between them. @m-nash and I have discussed this a tiny bit - it potentially lights up scenarios around going back and forth between protocol and convenience methods, and could also have perf implications where model types are large and callers only want to access a few properties on them when it doesn't make sense to deserialize the entire model. For perf, I would expect us to want to use MutableJsonDocument rather than DynamicData, until there is lighter weight dynamic story. In the meantime, DynamicData/MutableJsonDocument can serialize to a Stream, so this interface doesn't prevent that interaction, just adds a hop. Does the motivation for taking There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 on why does the stream need to be seekable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the scenario of serialization the customer would want to read from the beginning after we have serialized into the stream if its not seekable how would they do this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The stream might be a non-seekable FileStream that writes to disk. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An underlying stream may be seekable when another stream wrapping some segment of bytes might not be, like a buffered stream over a network stream. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On the DynamicData pivot, is there a goal to keep the serialized-out format close enough to wire format that DynamicData could feasibly implement this interface? Which is related to the question - should protocol method-only APIs have serializable "models" as well? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I appreciate we don't want to use exceptions for code flow, but is there any expected scenario where we wouldn't serialize when we're told (via call) that we should serialize? If not, this just creates undue burden and an exception is warranted. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the suggestion here to have a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If an error is truly exceptional, yes. If you're just returning a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as long as its valid json there shouldn't be an error There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have an example usage scenario that would help us understand the value of using TryXx() vs. Non-try APIs? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 for the "no try, just do" As Anne mentioned, I'm having trouble figuring out a scenario where I would try. Are you thinking of something like "I don't know what type this is, so I'll try A and if it doesn't work, then B, and if that doesn't work, it must be C?" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ah, for clarification, I was asking my question quite honestly - I believe such a scenario may exist, I would just like to understand it more concretely, and I think it would be useful for all of us to look at for the purposes of evaluation. I was not trying to imply with my question that such a scenario does not exist. In general, TryXx() APIs tend to be provided to make it possible to call APIs without invoking the exception handling machinery, which is bad for performance. If Try APIs exist, it's easy to implement non-Try APIs around them, but not vice versa. So in principle I have no problem with using Try APIs (I tend to like them), but it does feel like a non-standard choice for an interface. And also, as @JoshLove-msft mentioned elsewhere, they don't work with async methods because of out parameters, so if that's a need here, we may not have that option. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Apologies if it came across otherwise; it was not my intent to imply that you didn't think there were scenarios. My assumption was that you were also having difficulty coming up with a real-world example. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The example usage is in the test case in this PR. For easy reference https://github.com/Azure/azure-sdk-for-net/pull/35742/files#diff-46013d0356c5137faeddc1f0b2c084532794fcea22ff8574fa4a46b824640855R46-R47 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see it's used, but I still don't understand why. If there is any exception writing object data to a stream of bytes, it seems exceptional and should throw an exception. Is there a case where any underlying exception - or even a higher-level condition - should simply return false? And if there is a case where the underlying exception is thrown - like failure to write to a stream - shouldn't the caller know the details instead of having to get back There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The options appearing after the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think because options are optional they would need to go at the end. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, that's why I'm proposing that we make them required and if we really need to make them optional, we overload. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh sorry missed your suggestion at the end. I think I want to settle on the out param existing at all and the Try vs Do before making adjustments here but will keep this in mind once we settle on that part. |
||
|
||
/// <summary> | ||
/// TODO | ||
/// </summary> | ||
/// <param name="stream"></param> | ||
/// <param name="bytesConsumed"></param> | ||
/// <param name="options"></param> | ||
/// <returns></returns> | ||
bool TryDeserialize(Stream stream, out long bytesConsumed, SerializableOptions? options = default); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
// Copyright (c) Microsoft Corporation. All rights reserved. | ||
// Licensed under the MIT License. | ||
|
||
namespace Azure | ||
{ | ||
/// <summary> | ||
/// TODO | ||
/// </summary> | ||
public class SerializableOptions | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be helpful to look at the System.Text.Json serialization-related options types, and understand if there are options we would want to support from those and/or if we would want to design the API to get those options from related types like @heaths suggested here: https://github.com/Azure/azure-sdk-for-net/pull/35742/files#r1176840779 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Our options are purposefully more restrictive here since the idea is we already have all the renaming / restructuring logic written internally and we simply want to expose this to the customers publicly. We don't really want to expose the ability for them to further override these settings since then they aren't getting a universally transferable format. It would even be difficult to serialize / deserialize within the same library if they could override these types of things. If there are any specific options you think should be included on top of what we have please let us know. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One extra thought I'm stashing here quickly - we're working on DynamicDataOptions which may have serialization implications -- wondering if there's any overlap/should work together pivots here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you mean differently-formatted JSON is not "universally transferrable"? JSON parsers are supposed to ignore insignificant whitespace by design. I've never seen one that doesn't across a multitude of languages and class libraries. It wouldn't be a true JSON parser otherwise. Our definition of "pretty print" will vary from person to person / use to use. We should make it at least as robust as STJ or consider not supporting it until there's need. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was referring to the other options which allowed you to define name changes etc through custom json converters. We want to avoid exposing these since we are pinning ourselves to Azure Wire Format where we can control the translation layer so that other languages can deserialize the objects we serialize. We have played with IAzureJsonSerializable and AzureJsonSerializableOptions to make this point clear but avoided that in this first draft. PrettyPrint I agree this is a universally accepted concept. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was referring to preferences like how many spaces to indent, or even to use tabs instead of spaces. These are things that STJ and even Newtonsoft.Json expose. |
||
{ | ||
/// <summary> | ||
/// TODO | ||
/// </summary> | ||
public bool IncludeReadOnlyProperties { get; set; } = true; | ||
|
||
/// <summary> | ||
/// TODO | ||
/// </summary> | ||
public bool HandleAdditionalProperties { get; set; } = true; | ||
Comment on lines
+14
to
+19
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: .NET guidelines are that properties should typically default to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. double negatives are always confusing, prefer if we keep these as positive statements instead of at all possible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It could be something like IgnoreAdditionalProperties and IgnoreReadOnlyProperties, but we definitely have some other public types where we use default of true for bool, e.g. https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/core/Azure.Core/src/DiagnosticsOptions.cs#L81 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have a crisp definition of "AdditionalProperties"? We might want to tune this naming for clarity. Is it a "Missing Member", per this definition? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 "IgnoreReadOnlyProperties", to be consistent with BCL naming There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @JoshLove-msft I like that suggestion @nisha-bhatia lets add a task to rename both to Ignore and default to false. @annelo-msft I think a missing member is the other way around, the class expected it but the payload didn't include. AddtionalProperties are extra things that the class doesn't know about. |
||
|
||
/// <summary> | ||
/// TODO | ||
/// </summary> | ||
public bool PrettyPrint { get; set; } = false; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did we consider mirroring SJT with WriteIndented? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Its interesting in the description of what you wrote they say "true if JSON is pretty printed on serialization" indicating that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But they chose not to use that term. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 that this property would be more helpful and consistent with other .NET APIs if it described its intent/specifically what it does. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can discuss the naming here for sure, but PrettyPrint = Indent + Newlines + spaces after There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What scenario motivates the need for this property? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Its a very common scenario to generate a user friendly readable format for logs vs the smallest possible format for wire transfer perf reasons. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is rather specific. Instead, should we may let users pass a Alternatively, what about serializing to a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One of the goals here is not pin us to a specific underlying serialization technology and give us the freedom to swap this out as needed under the covers so we wouldn't want to take these types on the public signature. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then at the very least we should probably replicate more of STJ's options. What does "pretty print" even mean? See my examples in my comment for ways this could vary. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We definitely don't want to duplicate SJT's options for the reasons listed here #35742 (comment) |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,159 @@ | ||
// Copyright (c) Microsoft Corporation. All rights reserved. | ||
// Licensed under the MIT License. | ||
|
||
using System; | ||
using System.Collections.Generic; | ||
using System.IO; | ||
using System.Text.Json; | ||
|
||
namespace Azure.Core.Tests.ModelSerializationTests | ||
{ | ||
public class Animal : IJsonSerializable, IUtf8JsonSerializable | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the intended goal that Azure SDK public models will implement both of these interfaces? Will There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe that the intended goal is to expose TrySerialize and TryDeserialize in IJsonSerializable to all users. I think the goal is to generate these two methods for all public models as currently users cannot serialize/ deserialize on their own as these methods are internal. IUtf8JsonSerializable would remain internal and needs to be updated - we are just using a copy for now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We didn't want to gut the underlying serialization implementation just yet. Our first step here is create this public interface into what is already happening and this gives us the flexibility to adjust the internal implementation whenever we want as long as functionally it performs the same. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. For building out our conceptual models, does this mean we should think of IJsonSerializable as an eventual replacement for IUtf8JsonSerializable where it appears in Core APIs (although the practical implementation will be staged in steps for convenience)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a great question. Having 2 interfaces gives us the flexibility but it also increases the burden of keeping them consistent. |
||
{ | ||
private Dictionary<string, BinaryData> RawData { get; set; } = new Dictionary<string, BinaryData>(); | ||
|
||
public bool IsHungry { get; set; } = false; | ||
public double Weight { get; set; } = 0; | ||
public string LatinName { get; private set; } = "Animalia"; | ||
public string Name { get; set; } = "Animal"; | ||
|
||
public Animal() | ||
{ | ||
} | ||
|
||
public Animal(double weight, string latinName, string name, bool isHungry) | ||
{ | ||
Weight = weight; | ||
LatinName = latinName; | ||
Name = name; | ||
IsHungry = isHungry; | ||
} | ||
|
||
internal Animal(double weight, string latinName, string name, bool isHungry, Dictionary<string, BinaryData> rawData) | ||
{ | ||
Weight = weight; | ||
LatinName = latinName; | ||
Name = name; | ||
IsHungry = isHungry; | ||
RawData = rawData; | ||
} | ||
|
||
#region Serialization | ||
void IUtf8JsonSerializable.Write(Utf8JsonWriter writer, SerializableOptions options) | ||
{ | ||
writer.WriteStartObject(); | ||
if (options.IncludeReadOnlyProperties) | ||
{ | ||
writer.WritePropertyName("latinName"u8); | ||
writer.WriteStringValue(LatinName); | ||
} | ||
writer.WritePropertyName("name"u8); | ||
writer.WriteStringValue(Name); | ||
writer.WritePropertyName("isHungry"u8); | ||
writer.WriteBooleanValue(IsHungry); | ||
writer.WritePropertyName("weight"u8); | ||
writer.WriteNumberValue(Weight); | ||
|
||
if (options.HandleAdditionalProperties) | ||
{ | ||
//write out the raw data | ||
foreach (var property in RawData) | ||
{ | ||
writer.WritePropertyName(property.Key); | ||
#if NET6_0_OR_GREATER | ||
writer.WriteRawValue(property.Value); | ||
#else | ||
JsonSerializer.Serialize(writer, JsonDocument.Parse(property.Value.ToString()).RootElement); | ||
#endif | ||
} | ||
} | ||
writer.WriteEndObject(); | ||
} | ||
|
||
internal static Animal DeserializeAnimal(JsonElement element, SerializableOptions options) | ||
{ | ||
double weight = default; | ||
string name = ""; | ||
string latinName = ""; | ||
bool isHungry = default; | ||
|
||
Dictionary<string, BinaryData> rawData = new Dictionary<string, BinaryData>(); | ||
foreach (var property in element.EnumerateObject()) | ||
{ | ||
if (property.NameEquals("weight"u8)) | ||
{ | ||
weight = property.Value.GetDouble(); | ||
continue; | ||
} | ||
if (property.NameEquals("name"u8)) | ||
{ | ||
name = property.Value.GetString(); | ||
continue; | ||
} | ||
if (property.NameEquals("latinName"u8)) | ||
{ | ||
latinName = property.Value.GetString(); | ||
continue; | ||
} | ||
if (property.NameEquals("isHungry"u8)) | ||
{ | ||
isHungry = property.Value.GetBoolean(); | ||
continue; | ||
} | ||
|
||
if (options.HandleAdditionalProperties) | ||
{ | ||
//this means it's an unknown property we got | ||
rawData.Add(property.Name, BinaryData.FromString(property.Value.GetRawText())); | ||
} | ||
} | ||
return new Animal(weight, latinName, name, isHungry, rawData); | ||
} | ||
#endregion | ||
|
||
#region InterfaceImplementation | ||
public bool TryDeserialize(Stream stream, out long bytesConsumed, SerializableOptions options = default) | ||
{ | ||
bytesConsumed = 0; | ||
try | ||
{ | ||
JsonDocument jsonDocument = JsonDocument.Parse(stream); | ||
var model = DeserializeAnimal(jsonDocument.RootElement, options ?? new SerializableOptions()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious: what is the thinking behind creating a separate instance of the type in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nisha-bhatia should have a task to measure the perf diff for both serialize and deserialize. Based on these results we may or may not pull that optimization into v1. The goal at a high level was to not change the internal implementation on v1. |
||
this.LatinName = model.LatinName; | ||
this.Weight = model.Weight; | ||
this.IsHungry = model.IsHungry; | ||
this.Name = model.Name; | ||
this.RawData = model.RawData; | ||
bytesConsumed = stream.Length; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will If it is always There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same comment on Serialize There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For deserialize I think you are right position is probably more accurate, on serialize length and position should always be the same? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But in both cases, are we adding any info that can't be gained by inspecting the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thats a good question we should consider this when we finalize the signature sets this was part of the original suggested strawman from @KrzysztofCwalina. |
||
return true; | ||
} | ||
catch | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we be specific about exceptions that should cause deserialization to fail? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nisha-bhatia can you create an issue for this we should be purposeful here. |
||
{ | ||
return false; | ||
} | ||
} | ||
|
||
public bool TrySerialize(Stream stream, out long bytesWritten, SerializableOptions options = default) | ||
{ | ||
bytesWritten = 0; | ||
try | ||
{ | ||
JsonWriterOptions jsonWriterOptions = new JsonWriterOptions(); | ||
if (options.PrettyPrint) | ||
{ | ||
jsonWriterOptions.Indented = true; | ||
} | ||
Utf8JsonWriter writer = new Utf8JsonWriter(stream, jsonWriterOptions); | ||
Comment on lines
+141
to
+146
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per my comment above, if we just write to a |
||
((IUtf8JsonSerializable)this).Write(writer, options ?? new SerializableOptions()); | ||
writer.Flush(); | ||
bytesWritten = stream.Length; | ||
return true; | ||
} | ||
catch | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is not typical to just handle catch any exception and return false, even in TryX methods. Typically there is a well-defined set of scenarios where we would return false, and other exceptions should be propagated. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I question whether we should return false at all. See my comments above. If being unable to read or write data is exceptional, we should throw exceptions. Returning true or false just pushes the problem upstream. If the underlying APIs throw, we shouldn't catch that (exceptions as code flow) and turn it into a bool. |
||
{ | ||
return false; | ||
} | ||
} | ||
#endregion | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,162 @@ | ||
// Copyright (c) Microsoft Corporation. All rights reserved. | ||
// Licensed under the MIT License. | ||
|
||
using System; | ||
using System.Collections.Generic; | ||
using System.IO; | ||
using System.Text.Json; | ||
|
||
namespace Azure.Core.Tests.ModelSerializationTests | ||
{ | ||
public class CatReadOnlyProperty : Animal, IJsonSerializable, IUtf8JsonSerializable | ||
{ | ||
private Dictionary<string, BinaryData> RawData { get; set; } = new Dictionary<string, BinaryData>(); | ||
|
||
public CatReadOnlyProperty(double weight, string latinName, string name, bool isHungry, bool hasWhiskers) : base(weight, "Felis catus", name, isHungry) | ||
{ | ||
CatLatinName = LatinName; | ||
CatIsHungry = IsHungry; | ||
CatWeight = Weight; | ||
CatName = Name; | ||
HasWhiskers = hasWhiskers; | ||
} | ||
|
||
internal CatReadOnlyProperty(double weight, string latinName, string name, bool isHungry, bool hasWhiskers, Dictionary<string, BinaryData> rawData) : this(weight, latinName, name, isHungry, hasWhiskers) | ||
{ | ||
RawData = rawData; | ||
} | ||
|
||
public bool HasWhiskers { get; set; } = true; | ||
|
||
private string CatLatinName; | ||
private bool CatIsHungry; | ||
private double CatWeight; | ||
private string CatName; | ||
|
||
#region Serialization | ||
void IUtf8JsonSerializable.Write(Utf8JsonWriter writer, SerializableOptions options) | ||
{ | ||
writer.WriteStartObject(); | ||
if (options.IncludeReadOnlyProperties) | ||
{ | ||
writer.WritePropertyName("latinName"u8); | ||
writer.WriteStringValue(LatinName); | ||
} | ||
writer.WritePropertyName("name"u8); | ||
writer.WriteStringValue(CatName); | ||
writer.WritePropertyName("isHungry"u8); | ||
writer.WriteBooleanValue(CatIsHungry); | ||
writer.WritePropertyName("weight"u8); | ||
writer.WriteNumberValue(CatWeight); | ||
writer.WritePropertyName("hasWhiskers"u8); | ||
writer.WriteBooleanValue(HasWhiskers); | ||
|
||
if (options.HandleAdditionalProperties) | ||
{ | ||
//write out the raw data | ||
foreach (var property in RawData) | ||
{ | ||
writer.WritePropertyName(property.Key); | ||
#if NET6_0_OR_GREATER | ||
writer.WriteRawValue(property.Value); | ||
#else | ||
JsonSerializer.Serialize(writer, JsonDocument.Parse(property.Value.ToString()).RootElement); | ||
#endif | ||
} | ||
} | ||
writer.WriteEndObject(); | ||
} | ||
|
||
internal static CatReadOnlyProperty DeserializeCatReadOnlyProperty(JsonElement element, SerializableOptions options) | ||
{ | ||
double weight = default; | ||
string name = ""; | ||
string latinName = ""; | ||
bool isHungry = default; | ||
bool hasWhiskers = default; | ||
|
||
Dictionary<string, BinaryData> rawData = new Dictionary<string, BinaryData>(); | ||
foreach (var property in element.EnumerateObject()) | ||
{ | ||
if (property.NameEquals("weight"u8)) | ||
{ | ||
weight = property.Value.GetDouble(); | ||
continue; | ||
} | ||
if (property.NameEquals("name"u8)) | ||
{ | ||
name = property.Value.GetString(); | ||
continue; | ||
} | ||
if (property.NameEquals("latinName"u8)) | ||
{ | ||
latinName = property.Value.GetString(); | ||
continue; | ||
} | ||
if (property.NameEquals("hasWhiskers"u8)) | ||
{ | ||
hasWhiskers = property.Value.GetBoolean(); | ||
continue; | ||
} | ||
if (property.NameEquals("isHungry"u8)) | ||
{ | ||
isHungry = property.Value.GetBoolean(); | ||
continue; | ||
} | ||
|
||
if (options.HandleAdditionalProperties) | ||
{ | ||
//this means its an unknown property we got | ||
rawData.Add(property.Name, BinaryData.FromString(property.Value.GetRawText())); | ||
} | ||
} | ||
return new CatReadOnlyProperty(weight, latinName, name, isHungry, hasWhiskers, rawData); | ||
} | ||
#endregion | ||
|
||
#region InterfaceImplementation | ||
public new bool TryDeserialize(Stream stream, out long bytesConsumed, SerializableOptions options = default) | ||
{ | ||
bytesConsumed = 0; | ||
try | ||
{ | ||
JsonDocument jsonDocument = JsonDocument.Parse(stream); | ||
var model = DeserializeCatReadOnlyProperty(jsonDocument.RootElement, options ?? new SerializableOptions()); | ||
this.CatLatinName = model.LatinName; | ||
this.CatWeight = model.Weight; | ||
this.CatIsHungry = model.IsHungry; | ||
this.HasWhiskers = model.HasWhiskers; | ||
this.CatIsHungry = model.CatIsHungry; | ||
bytesConsumed = stream.Length; | ||
return true; | ||
} | ||
catch | ||
{ | ||
return false; | ||
} | ||
} | ||
|
||
public new bool TrySerialize(Stream stream, out long bytesWritten, SerializableOptions options = default) | ||
{ | ||
bytesWritten = 0; | ||
try | ||
{ | ||
JsonWriterOptions jsonWriterOptions = new JsonWriterOptions(); | ||
if (options.PrettyPrint) | ||
{ | ||
jsonWriterOptions.Indented = true; | ||
} | ||
Utf8JsonWriter writer = new Utf8JsonWriter(stream, jsonWriterOptions); | ||
((IUtf8JsonSerializable)this).Write(writer, options ?? new SerializableOptions()); | ||
writer.Flush(); | ||
bytesWritten = (int)stream.Length; | ||
return true; | ||
} | ||
catch | ||
{ | ||
return false; | ||
} | ||
} | ||
#endregion | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
// Copyright (c) Microsoft Corporation. All rights reserved. | ||
// Licensed under the MIT License. | ||
|
||
using System.Text.Json; | ||
|
||
namespace Azure.Core.Tests.ModelSerializationTests | ||
{ | ||
internal interface IUtf8JsonSerializable | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious: why does this live in the test project? Would we make these changes to the shared source IUtf8JsonSerializable type? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IUtf8JsonSerializable would need to be changed to include the SerializableOptions param, for now we added a copy of the file. |
||
{ | ||
void Write(Utf8JsonWriter writer, SerializableOptions options); | ||
} | ||
} |
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.
We should consider if there's any value to splitting this up into
IJsonSerializable
andIJsonDeserializable
. Key Vault does this and it's easy to detect when one or both directions are implemented; though, in practice, I don't know if that's really necessary.