Skip to content
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

feat(common): support null value for reference type collection elements #3163

Merged
merged 20 commits into from
Mar 2, 2023

Conversation

archerzz
Copy link
Member

@archerzz archerzz commented Feb 21, 2023

  • update SerializationBuilder to set ValueSerialization.IsNullable : true for collection elements of reference type
    • that will cause null value handling in serialization & deserialization logic to be added for collection elements of reference type
  • update SerializationWriter so that in generated model deserialization methods, null value handling (return null if pass in null json value) is added at the beginning
  • update Utf8JsonWriterExtension.WriteBase64StringValue() to handle null json value in serialization
  • update JsonElementExtension.GetBytesFromBase64() to handle null json value in deserialization

update JsonCodeWriterExtension, for collection items:

  • In list serialization, instead of generating null value handling logic for all nullable value serialization, only generating the logic for nullable value types. That can eliminate a lot null handling codes in serialization method. And for those without such logic:
    • string type can be handled by Utf8JsonWriter.WriteStringValue() which has null value support
    • byte[] is handled by the modified Utf8JsonWriterExtension.WriteBase64StringValue() (see the change above)
    • model classes and other reference types are handled by Utf8JsonWriterExtension.WriteObjectValue() which has null value support
  • In dictionary serialization, add the missing null value handling for values
  • In list & dictionary deserialization, change the generated codes to:
    • model class null value check is handled by their own deserialization method (see the change above)
    • string null value check in handled by JsonElement.GetString()
    • byte[] is handled by JsonElementExtension.GetBytesFromBase64()
    • nullable value type, list/dictionary, other framework reference will have generated null value check codes

  • fix 3 test cases about null value in collection elements in deserialization

resolve #3159

Serialization

Serialization Null check in example comments
nullable value type generated codes in parent serialization method if (value==null) { writer.WriteNullValue()}
SDK model Utf8JsonWriterExtensions.WriteObjectValue()
string Utf8JsonWriterExtensions.WriteStringValue()
byte[] Utf8JsonWriterExtensions.WriteBase64StringValue() need to add null check in WriteBase64StringValue
array, dictionary generated codes in parent serialization method if (value==null) { writer.WriteNullValue()}
other reference type generated codes in parent serialization method if (value==null) { writer.WriteNullValue()}

Deserialization

Serialization Null Check in example comments
nullable value type generated codes in parent deserialization method int? if (element.Value.ValueKind == JsonValueKind.Null) { array.Add(null)}
SDK model model deserialization method IPAddress need to add null check in deserialization method
string JsonElement.GetString()
byte[] JsonElementExtension.GetBytesFromBase64() need to add null check in GetBytesFromBase64
array, dictionary generated codes in parent deserialization method if (element.Value.ValueKind == JsonValueKind.Null) { array.Add(null)}
other framework reference type generated codes in parent deserialization method if (element.Value.ValueKind == JsonValueKind.Null) { array.Add(null)}

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

@m-nash
Copy link
Member

m-nash commented Feb 22, 2023

I believe we want to get your parallel work in before uploading the regen PR? If so lets mark this as draft until we get ready for that.

@archerzz archerzz force-pushed the common/null-collection-element branch from 3bba737 to 5cc8130 Compare March 1, 2023 05:46
archerzz added 19 commits March 1, 2023 13:47
- update `SerializationBuilder` to set `ValueSerialization.IsNullable : true` for reference type collection elements
  - that will add null value handling in deserialization logic for collection elements of reference type
- generate null check in generated serialization/deserialization codes for:
  - nullable value type
  - list/dictionary types
  - some framework reference types
- optimize null value processing so that we don't generate null check codes
  - for custom model types, put null value processing logic into the deserialization method of each model
  - for string, use default serialization/deserialization methods which can handle null value
  - update `byte[]` serializatoin/deserialization methods so that they can handle null value, so that we don't generate null check codes externally
- fix 3 test cases previously do not accept null value in collections in response, now we align with other language implementaion on that

resolve Azure#3159
`WriteObjectValue` cannot cover all cases, so add the defensive null check just in case
@archerzz archerzz marked this pull request as ready for review March 1, 2023 06:17
@archerzz archerzz force-pushed the common/null-collection-element branch from 5cc8130 to 6a9da34 Compare March 1, 2023 06:30
@archerzz
Copy link
Member Author

archerzz commented Mar 1, 2023

Here is the generated preview PR: Azure/azure-sdk-for-net#34624

@archerzz archerzz merged commit 936d073 into Azure:feature/v3 Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(common): support null value of reference type collection element in deserialization
3 participants