-
Notifications
You must be signed in to change notification settings - Fork 539
[Internal] DTS: Refactors casing in DTS response and adds tests #5686
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
Changes from all commits
c21155b
a9fb8c5
343b6fa
5565b90
de0db67
99555a6
d88c35b
3029e60
133dbfe
b84dc1e
82674e3
b59acf8
b5914dd
61b5591
d05b1ff
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 |
|---|---|---|
|
|
@@ -42,10 +42,15 @@ internal DistributedTransactionOperationResult(DistributedTransactionOperationRe | |
|
|
||
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="DistributedTransactionOperationResult"/> class. | ||
| /// This protected constructor is intended for use by derived classes. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// Must be <c>public</c> for System.Text.Json reflection-based deserialization. | ||
| /// System.Text.Json 6.x only scans <c>BindingFlags.Public</c> constructors when resolving | ||
| /// <see cref="JsonConstructorAttribute"/>; non-public constructors are not found. | ||
| /// Support for non-public constructors was added in System.Text.Json 7.0. | ||
| /// </remarks> | ||
| [JsonConstructor] | ||
| protected DistributedTransactionOperationResult() | ||
| public DistributedTransactionOperationResult() | ||
| { | ||
| } | ||
|
|
||
|
|
@@ -60,7 +65,7 @@ protected DistributedTransactionOperationResult() | |
| /// Gets the HTTP status code returned by the operation. | ||
| /// </summary> | ||
| [JsonInclude] | ||
| [JsonPropertyName("statuscode")] | ||
| [JsonPropertyName("statusCode")] | ||
|
Meghana-Palaparthi marked this conversation as resolved.
|
||
| public virtual HttpStatusCode StatusCode { get; internal set; } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -91,33 +96,27 @@ protected DistributedTransactionOperationResult() | |
| [JsonIgnore] | ||
| public virtual Stream ResourceStream { get; internal set; } | ||
|
|
||
| /// <summary> | ||
| /// Used for JSON deserialization of the base64-encoded resource body. | ||
| /// </summary> | ||
| [JsonInclude] | ||
| [JsonPropertyName("resourcebody")] | ||
| internal string ResourceBodyBase64 | ||
| { | ||
| get => null; // Write-only for deserialization | ||
| set | ||
| { | ||
| if (!string.IsNullOrEmpty(value)) | ||
| { | ||
| byte[] resourceBody = Convert.FromBase64String(value); | ||
| this.ResourceStream = new MemoryStream(resourceBody, 0, resourceBody.Length, writable: false, publiclyVisible: true); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Request charge in request units for the operation. | ||
| /// </summary> | ||
| [JsonInclude] | ||
| [JsonPropertyName("requestCharge")] | ||
| internal virtual double RequestCharge { get; set; } | ||
| public virtual double RequestCharge { get; internal set; } | ||
|
Meghana-Palaparthi marked this conversation as resolved.
|
||
|
|
||
| [JsonPropertyName("substatuscode")] | ||
| [JsonIgnore] | ||
| internal virtual SubStatusCodes SubStatusCode { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Gets the sub-status code value as an unsigned integer. | ||
| /// </summary> | ||
| [JsonInclude] | ||
| [JsonPropertyName("subStatusCode")] | ||
| public virtual uint SubStatusCodeValue | ||
|
Meghana-Palaparthi marked this conversation as resolved.
|
||
| { | ||
| get => (uint)this.SubStatusCode; | ||
| internal set => this.SubStatusCode = (SubStatusCodes)value; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// ActivityId related to the operation. | ||
| /// </summary> | ||
|
|
@@ -127,14 +126,35 @@ internal string ResourceBodyBase64 | |
| [JsonIgnore] | ||
| internal ITrace Trace { get; set; } | ||
|
|
||
| private static readonly JsonSerializerOptions CaseInsensitiveOptions = new JsonSerializerOptions | ||
| { | ||
| PropertyNameCaseInsensitive = true, | ||
| }; | ||
|
|
||
| /// <summary> | ||
| /// Creates a <see cref="DistributedTransactionOperationResult"/> from a JSON element. | ||
| /// </summary> | ||
| /// <param name="json">The JSON element containing the operation result.</param> | ||
| /// <returns>The deserialized operation result.</returns> | ||
| internal static DistributedTransactionOperationResult FromJson(JsonElement json) | ||
| { | ||
| return JsonSerializer.Deserialize<DistributedTransactionOperationResult>(json); | ||
| DistributedTransactionOperationResult result = JsonSerializer.Deserialize<DistributedTransactionOperationResult>(json, DistributedTransactionOperationResult.CaseInsensitiveOptions); | ||
|
|
||
| if (json.TryGetProperty("resourceBody", out JsonElement resourceBody) | ||
|
Contributor
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. 🟢 Suggestion · Maintainability: Mixed Deserialization Pattern
This method uses Consider a custom
Contributor
Author
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.
|
||
| && resourceBody.ValueKind != JsonValueKind.Undefined | ||
| && resourceBody.ValueKind != JsonValueKind.Null) | ||
| { | ||
| // resourceBody is expected to be a JSON object (Cosmos DB document) | ||
| if (resourceBody.ValueKind != JsonValueKind.Object) | ||
|
Meghana-Palaparthi marked this conversation as resolved.
|
||
| { | ||
| throw new JsonException($"The 'resourceBody' value must be a JSON object, but was '{resourceBody.ValueKind}'."); | ||
| } | ||
|
|
||
| byte[] bytes = JsonSerializer.SerializeToUtf8Bytes(resourceBody); | ||
| result.ResourceStream = new MemoryStream(bytes, 0, bytes.Length, writable: false, publiclyVisible: true); | ||
| } | ||
|
|
||
| return result; | ||
| } | ||
| } | ||
| } | ||
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.
🟡 Recommendation · API Surface: Constructor Visibility
[JsonConstructor]changed fromprotectedtopublicWhile the class is normally
internal(via#if INTERNAL), when compiled aspublic, this lets consumers construct result objects with default (zero) values, bypassing SDK construction logic. Result types are typically not meant to be user-constructed.If
[JsonConstructor]requires accessibility for deserialization, consider whetherinternalwould suffice (it does for System.Text.Json when the type itself is internal).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.
The project resolves System.Text.Json 6.0.10. In STJ 6.x, [JsonConstructor] is only honored on public constructors. internal would silently break deserialization.