[Internal] DTS: Refactors casing in DTS response and adds tests#5686
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Meghana-Palaparthi
changed the title
Meghana-Palaparthi
requested review from
FabianMeiswinkel,
Pilchie,
adityasa,
khdang,
kirankumarkolli,
kirillg,
neildsh and
sboshra
as code owners
…action-test-suite
…action-test-suite
…uite' of https://github.com/Azure/azure-cosmos-dotnet-v3 into users/Meghana-Palaparthi/distributed-transaction-test-suite
…action-test-suite
…action-test-suite
…action-test-suite
…ing and adds DoNotParallelize
Two bugs in DistributedTransactionTests.cs:
1. Mock JSON used all-lowercase property names ('statuscode', 'substatuscode',
'resourcebody') that do not match the model's JsonPropertyName attributes
('statusCode', 'subStatusCode') or the manual TryGetProperty lookup
('resourceBody'). System.Text.Json deserialization is case-sensitive, so
StatusCode was always 0 causing all status-code assertions to fail.
2. Missing [DoNotParallelize] attribute. The TestInitialize calls TestInit()
which runs DeleteAllDatabasesAsync; without [DoNotParallelize] concurrent
test execution causes emulator resource contention (same fix as #5711 for
the predecessor class DistributedTransactionE2ETests).
Also removes stray 'git' text from an inline comment.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
AI Code Review SummaryPR Intent: Refactors Distributed Transaction response model -- switches from base64-encoded resource bodies to direct JSON objects, standardizes JSON property casing to camelCase, removes SessionToken from response population, and replaces old E2E tests with comprehensive unit + emulator tests. Overall Assessment: The direction is sound. The new serialization approach and test decomposition are improvements. However, there are test quality issues (mock responses using wrong property casing) and missing test coverage for newly-exposed properties. Findings Overview
Existing comments cross-referenced: 3 found from @kirankumarkolli and @Praveen-Msft. Finding #3 reinforces @Praveen-Msft's unresolved error handling comment. @kirankumarkolli's JsonElement concern was acknowledged by author and addressed by moving to inline handling in FromJson. See inline comments for details. |
| /// </summary> | ||
| [JsonConstructor] | ||
| protected DistributedTransactionOperationResult() | ||
| public DistributedTransactionOperationResult() |
There was a problem hiding this comment.
🟡 Recommendation · API Surface: Constructor Visibility
[JsonConstructor] changed from protected to public
While the class is normally internal (via #if INTERNAL), when compiled as public, 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 whether internal would suffice (it does for System.Text.Json when the type itself is internal).
There was a problem hiding this comment.
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.
| return JsonSerializer.Deserialize<DistributedTransactionOperationResult>(json); | ||
| DistributedTransactionOperationResult result = JsonSerializer.Deserialize<DistributedTransactionOperationResult>(json.GetRawText()); | ||
|
|
||
| if (json.TryGetProperty("resourceBody", out JsonElement resourceBody) |
There was a problem hiding this comment.
🟢 Suggestion · Maintainability: Mixed Deserialization Pattern
FromJson mixes automatic and manual deserialization
This method uses JsonSerializer.Deserialize for most properties, then manually processes resourceBody via TryGetProperty. This dual pattern is fragile — any future property needing special handling requires updates in both the class attributes AND the manual code in FromJson, with no compile-time safety net.
Consider a custom JsonConverter<DistributedTransactionOperationResult> to centralize all deserialization logic in one place.
There was a problem hiding this comment.
resourcebody is the only property that requires custom handling. Changing deserialization of other properties to reflect the deserialization of resourcebody could introduce a lot of unnecessary manual handling
- Make [JsonConstructor] ctor internal (was public) -- result types should not be externally constructable; System.Text.Json honors [JsonConstructor] on non-public ctors via reflection on .NET 6+ - Use JsonSerializer.Deserialize<T>(JsonElement) overload directly in FromJson instead of json.GetRawText() to avoid unnecessary string allocation - Wrap JsonSerializer.Deserialize in try-catch(JsonException) in FromJson so a malformed individual operation result returns InternalServerError instead of crashing the entire response parse - Fix statuscode -> statusCode casing in BuildSuccessResponse/BuildErrorResponse helpers in DistributedWriteTransactionTests and DistributedTransactionSerializerTests - Add SubStatusCode and RequestCharge deserialization tests to DistributedTransactionResponseTests Co-Authored-By: Copilot <223556219+Copilot@users.noreply.github.com>
kirankumarkolli
left a comment
kirankumarkolli
left a comment
There was a problem hiding this comment.
Also make deserialzier case-insensitive?
…arthi/distributed-transaction-test-suite
- Moves unique coverage from DistributedTransactionE2ETests.cs into the appropriate unit test files instead of the emulator test suite: - Argument validation (null stream): DistributedWriteTransactionTests - Stream serialization + IfMatchEtag: DistributedTransactionSerializerTests - PreconditionFailed response: DistributedTransactionResponseTests - Removes DistributedTransactionE2ETests.cs (superseded). - Merges origin/master. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
kirankumarkolli
deleted the
users/Meghana-Palaparthi/distributed-transaction-test-suite
branch
4 participants
Description
This pull request introduces several important changes to the distributed transactions implementation in Cosmos DB. The main focus is on improving the serialization and deserialization of distributed transaction operation results, updating property names to follow consistent casing, and modernizing how resource bodies are handled in responses. Additionally, it removes the end-to-end tests for distributed transactions and adds other test files.
Key changes include:
Serialization and Deserialization Improvements
resourcebodywith a newresourceBodyproperty of typeJsonElementinDistributedTransactionOperationResult, allowing direct handling of JSON objects instead of base64 strings. The setter now serializes the JSON element to bytes and populates theResourceStream.FromJsonto usejson.GetRawText()for more accurate deserialization.Property Naming and API Consistency
statuscode→statusCode,substatuscode→subStatusCode) for consistency with common JSON conventions.subStatusCodeValueas a public property for serialization, mapping to the internalSubStatusCodeenum.Header and Operation Type Handling
OperationTypeandResourceTypeto use explicit conversion methods (ToOperationTypeString(),ToResourceTypeString()) instead of.ToString(), ensuring correct string representations in headers.Test Code Cleanup
DistributedTransactionE2ETests.csfile, which contained end-to-end tests for distributed transactions.Minor Cleanups
SessionTokenfrom response header and deserialization logicType of change
Please delete options that are not relevant.
Closing issues
To automatically close an issue: closes #IssueNumber