ClientModel Prototype: Nullable annotations in clients#41952
ClientModel Prototype: Nullable annotations in clients#41952annelo-msft wants to merge 8 commits intoAzure:feature/core-experimentfrom
Conversation
|
API change check API changes are not detected in this pull request. |
sdk/core/System.ClientModel/tests/client/MapsClient/IPAddressCountryPair.cs
Show resolved
Hide resolved
| private readonly string _apiVersion; | ||
|
|
||
| public MapsClient(Uri endpoint, ApiKeyCredential credential, MapsClientOptions options = default) | ||
| public MapsClient(Uri endpoint, ApiKeyCredential credential, MapsClientOptions? options = default) |
There was a problem hiding this comment.
ClientOptions become nullable.
| } | ||
|
|
||
| public virtual async Task<ClientResult> GetCountryCodeAsync(string ipAddress, RequestOptions options = null) | ||
| public virtual async Task<ClientResult> GetCountryCodeAsync(string ipAddress, RequestOptions? options = null) |
There was a problem hiding this comment.
RequestOptions become nullable.
sdk/core/System.ClientModel/tests/client/ModelReaderWriter/Models/DiscriminatorSet/BaseModel.cs
Show resolved
Hide resolved
sdk/core/System.ClientModel/tests/client/ModelReaderWriter/Models/DiscriminatorSet/BaseModel.cs
Show resolved
Hide resolved
sdk/core/System.ClientModel/tests/client/ModelReaderWriter/Models/DiscriminatorSet/BaseModel.cs
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| internal ModelX(string kind, string name, int xProperty, int? nullProperty, IList<string> fields, IDictionary<string, string> keyValuePairs, Dictionary<string, BinaryData> rawData) | ||
| internal ModelX(string kind, string? name, int xProperty, int? nullProperty, IList<string> fields, IDictionary<string, string> keyValuePairs, Dictionary<string, BinaryData> rawData) |
There was a problem hiding this comment.
Optional reference type properties become nullable in the serialization constructor.
| public ModelX() : base(null) | ||
| { | ||
| Kind = "X"; | ||
| Fields = new List<string>(); |
There was a problem hiding this comment.
Non-nullable properties must either be initialized or follow the assert pattern.
| if (modelX == null) | ||
| { | ||
| return null; | ||
| throw new ArgumentNullException(nameof(modelX)); |
There was a problem hiding this comment.
Non-nullable models throw ArgumentNullException in methods that expect them to be non-null.
| { | ||
| writer.WritePropertyName("nullProperty"u8); | ||
| writer.WriteNumberValue(NullProperty.Value); | ||
| writer.WriteNumberValue(NullProperty!.Value); |
There was a problem hiding this comment.
We ignore the nullability annotation if we've already done a null check.
| } | ||
|
|
||
| internal static ModelX DeserializeModelX(JsonElement element, ModelReaderWriterOptions options = default) | ||
| internal static ModelX DeserializeModelX(JsonElement element, ModelReaderWriterOptions? options = default) |
There was a problem hiding this comment.
options are nullable.
| if (element.ValueKind == JsonValueKind.Null) | ||
| { | ||
| return null; | ||
| throw new JsonException($"Invalid JSON provided to deserialize type '{nameof(ModelX)}'"); |
There was a problem hiding this comment.
Non-nullable model deserialize routines throw JsonException if JSON contains null.
| } | ||
| string kind = default; | ||
|
|
||
| string? kind = default; |
There was a problem hiding this comment.
Required properties are declared as nullable and then later checked to make sure they have a value before passing them to the type's constructor.
| if (property.NameEquals("fields"u8)) | ||
| { | ||
| fields = property.Value.EnumerateArray().Select(element => element.GetString()).ToList(); | ||
| // TODO: May do something different depending on the semantics of expected values |
There was a problem hiding this comment.
Collections should be clear about whether they can contain nullable values or not. They would throw JsonException if values shouldn't be null but the JSON provides null for their values.
| } | ||
| } | ||
|
|
||
| if (kind is null) |
There was a problem hiding this comment.
We check that required property values have been populated from JSON, or throw if they have not.
sdk/core/System.ClientModel/tests/client/ModelReaderWriter/Models/DiscriminatorSet/ModelY.cs
Show resolved
Hide resolved
| //} | ||
|
|
||
| private int? _xProperty; | ||
| public int XProperty |
There was a problem hiding this comment.
Required property pattern for value types.
This will throw at serialization-time if the user creating the type doesn't set a value required for serialization.
|
|
||
| // <auto-generated/> | ||
|
|
||
| #nullable disable |
There was a problem hiding this comment.
These aren't needed one way or another because the <auto-generated/> comment opts-out of nullability annotations per: https://learn.microsoft.com/en-us/dotnet/csharp/nullable-migration-strategies
| private string? _name; | ||
| /// <summary> The name of the resource. </summary> | ||
| public string Name { get; } | ||
| public string Name |
There was a problem hiding this comment.
Assert pattern for required value types.
|
|
||
| /// <summary> Azure Resource Manager metadata containing createdBy and modifiedBy information. </summary> | ||
| public SystemData SystemData { get; } | ||
| public SystemData? SystemData { get; } |
There was a problem hiding this comment.
Example nullable nested model.
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| // <auto-generated/> |
There was a problem hiding this comment.
For ClientModel-based client models, we need to either remove this line or explicitly set #nullable enable per https://learn.microsoft.com/en-us/dotnet/csharp/nullable-migration-strategies
|
Closing in favor of #41968 |
2 participants
#41554