Skip to content

Commit 8aeb2a2

Browse files
CopilotjsquireCopilot
authored
[Tables] Fix RequestFailedException contract in GetEntityAsync/GetEntityIfExistsAsync when response body is missing or malformed (Azure#58305)
* Initial plan * Fix ArgumentNullException in GetEntityAsync/GetEntityIfExistsAsync when ContentStream is null Add null check for response.ContentStream before calling ResponseToDictionary in GetEntityInternalAsync. When ContentStream is null on a non-404 response, throw RequestFailedException instead of letting ArgumentNullException bubble up from JsonDocument.Parse. Add regression tests and changelog entry. Fixes Azure#58303 Agent-Logs-Url: https://github.com/Azure/azure-sdk-for-net/sessions/294e65c4-fa6f-4aba-b3f7-eace4eea30cf Co-authored-by: jsquire <913445+jsquire@users.noreply.github.com> * Use await in ThrowsAsync assertions in new tests Agent-Logs-Url: https://github.com/Azure/azure-sdk-for-net/sessions/294e65c4-fa6f-4aba-b3f7-eace4eea30cf Co-authored-by: jsquire <913445+jsquire@users.noreply.github.com> * Set ContentStream to empty MemoryStream before throwing RequestFailedException When response.ContentStream is null, assign an empty MemoryStream before throwing RequestFailedException(response). This ensures: 1. The exception message includes the HTTP status code and reason phrase (context for callers to diagnose the error) 2. GetRawResponse().ContentStream is non-null after catching the exception (protects callers from NullReferenceException when reading raw response) Also update NullContentStreamMockResponse mock to properly propagate the setter so the regression tests can verify that ContentStream is non-null on the thrown exception's raw response. Agent-Logs-Url: https://github.com/Azure/azure-sdk-for-net/sessions/b5206032-2deb-4830-a756-5f1d4f5590a4 Co-authored-by: jsquire <913445+jsquire@users.noreply.github.com> * Use explicit message in RequestFailedException when ContentStream is null Replace RequestFailedException(response) with RequestFailedException(status, message) to avoid the misleading "Service request succeeded." text that was produced when ContentStream was null on a 200 OK response. The new message explicitly says "Response body was null or empty. HTTP {status} ({reason})" which clearly communicates what went wrong and includes the status code for context. Also simplify NullContentStreamMockResponse back to a read-only getter, and update test assertions to verify the status code and message content. Agent-Logs-Url: https://github.com/Azure/azure-sdk-for-net/sessions/1362d0e0-26c9-4cd8-8fa8-1bac42cdf036 Co-authored-by: jsquire <913445+jsquire@users.noreply.github.com> * Apply suggestion from @jsquire * Update sdk/tables/Azure.Data.Tables/src/TableClient.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Handle empty/malformed ContentStream: catch JsonException and rethrow as RequestFailedException The null ContentStream check only covered one scenario. An empty or malformed (but non-null) stream causes JsonDocument.Parse to throw JsonException, which would leak an internal parsing exception to callers. Fix: wrap SerializationHelpers.ResponseToDictionary in a try/catch for JsonException and rethrow as RequestFailedException with a descriptive message. Also: - Add System.Text.Json using for JsonException - Add 4 regression tests covering empty MemoryStream scenario - Update CHANGELOG wording to match the null ContentStream scope of the fix Agent-Logs-Url: https://github.com/Azure/azure-sdk-for-net/sessions/4598458f-3f67-467c-97d0-2afbb1022424 Co-authored-by: jsquire <913445+jsquire@users.noreply.github.com> * Eagerly detect empty ContentStream at the null guard (L797) Agent-Logs-Url: https://github.com/Azure/azure-sdk-for-net/sessions/9108391f-f04d-409a-bd1c-6c2d9ff906f7 Co-authored-by: jsquire <913445+jsquire@users.noreply.github.com> * De-duplicate RequestFailedException throws into a single catch block Agent-Logs-Url: https://github.com/Azure/azure-sdk-for-net/sessions/67365fc6-8da0-4535-a223-9dcd09736569 Co-authored-by: jsquire <913445+jsquire@users.noreply.github.com> * Restore 'or malformed' in RequestFailedException message Agent-Logs-Url: https://github.com/Azure/azure-sdk-for-net/sessions/123bdfc7-8417-4dc3-a70a-962308357eab Co-authored-by: jsquire <913445+jsquire@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: jsquire <913445+jsquire@users.noreply.github.com> Co-authored-by: Jesse Squire <jesse.squire@gmail.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 parent 1f9b8a6 commit 8aeb2a2

3 files changed

Lines changed: 104 additions & 4 deletions

File tree

sdk/tables/Azure.Data.Tables/CHANGELOG.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,9 @@ Thank you to our developer community members who helped to make Azure Tables bet
1313
### Breaking Changes
1414

1515
### Bugs Fixed
16+
- Fixed an error handling issue where `GetEntityAsync` and `GetEntityIfExistsAsync` could throw an
17+
`ArgumentNullException` instead of `RequestFailedException` when the HTTP response had a null
18+
`ContentStream`. ([#58303](https://github.com/Azure/azure-sdk-for-net/issues/58303))
1619
- Fixed an `IndexOutOfRangeException` when constructing `TableServiceClient` or `TableClient` with a loopback URI that does not contain an account name in the path, such as the Cosmos DB emulator endpoint `http://localhost:8902/`.
1720
- Fixed an issue where `TimeSpan` properties in strongly typed table entities were not being deserialized.
1821
- Fixed an issue when deserializing strongly typed table entities with enum properties. Enum values that aren't defined in the enum type are now skipped during deserialization of the table entity.

sdk/tables/Azure.Data.Tables/src/TableClient.cs

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
using System.Linq;
99
using System.Linq.Expressions;
1010
using System.Net;
11+
using System.Text.Json;
1112
using System.Threading;
1213
using System.Threading.Tasks;
1314
using Azure.Core;
@@ -792,12 +793,22 @@ await _tableOperations.QueryEntityWithPartitionAndRowKeyAsync(
792793
{
793794
return new NoValueResponse<T>(response);
794795
}
795-
else
796+
797+
Dictionary<string, object> dictionary;
798+
try
796799
{
797-
var dictionary = SerializationHelpers.ResponseToDictionary(response);
798-
var result = dictionary.ToTableEntity<T>();
799-
return Response.FromValue(result, response);
800+
dictionary = SerializationHelpers.ResponseToDictionary(response);
800801
}
802+
catch (Exception ex) when (ex is ArgumentNullException or JsonException)
803+
{
804+
throw new RequestFailedException(
805+
response.Status,
806+
$"The response body was unexpectedly missing or malformed, so the entity could not be read from the response. HTTP {response.Status} ({response.ReasonPhrase}).",
807+
ex);
808+
}
809+
810+
var result = dictionary.ToTableEntity<T>();
811+
return Response.FromValue(result, response);
801812
}
802813
catch (Exception ex)
803814
{

sdk/tables/Azure.Data.Tables/tests/TableClientTests.cs

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -785,6 +785,78 @@ public async Task GetEntityIfExistsAsyncDoesNotThrowObjectDisposedException()
785785
Assert.AreEqual("world", result.Value.GetString("Value"));
786786
}
787787

788+
/// <summary>
789+
/// Regression test for https://github.com/Azure/azure-sdk-for-net/issues/58303.
790+
/// Verifies that GetEntityIfExistsAsync throws RequestFailedException instead of
791+
/// ArgumentNullException when the HTTP response has a null ContentStream, and that
792+
/// the exception message includes the HTTP status code.
793+
/// </summary>
794+
[Test]
795+
public async Task GetEntityIfExistsAsyncThrowsRequestFailedExceptionWhenContentStreamIsNull()
796+
{
797+
var response = new NullContentStreamMockResponse(200);
798+
var transport = new MockTransport(_ => response);
799+
var tableClient = new TableClient(_url, TableName, new MockCredential(), new TableClientOptions { Transport = transport });
800+
801+
RequestFailedException ex = Assert.ThrowsAsync<RequestFailedException>(async () => await tableClient.GetEntityIfExistsAsync<TableEntity>("pk", "rk-1"));
802+
Assert.AreEqual(200, ex.Status);
803+
StringAssert.Contains("200", ex.Message);
804+
}
805+
806+
/// <summary>
807+
/// Regression test for https://github.com/Azure/azure-sdk-for-net/issues/58303.
808+
/// Verifies that GetEntityAsync throws RequestFailedException instead of
809+
/// ArgumentNullException when the HTTP response has a null ContentStream, and that
810+
/// the exception message includes the HTTP status code.
811+
/// </summary>
812+
[Test]
813+
public async Task GetEntityAsyncThrowsRequestFailedExceptionWhenContentStreamIsNull()
814+
{
815+
var response = new NullContentStreamMockResponse(200);
816+
var transport = new MockTransport(_ => response);
817+
var tableClient = new TableClient(_url, TableName, new MockCredential(), new TableClientOptions { Transport = transport });
818+
819+
RequestFailedException ex = Assert.ThrowsAsync<RequestFailedException>(async () => await tableClient.GetEntityAsync<TableEntity>("pk", "rk-1"));
820+
Assert.AreEqual(200, ex.Status);
821+
StringAssert.Contains("200", ex.Message);
822+
}
823+
824+
/// <summary>
825+
/// Regression test for https://github.com/Azure/azure-sdk-for-net/issues/58303.
826+
/// Verifies that GetEntityIfExistsAsync throws RequestFailedException instead of
827+
/// JsonException when the HTTP response has an empty (non-null) ContentStream.
828+
/// </summary>
829+
[Test]
830+
public async Task GetEntityIfExistsAsyncThrowsRequestFailedExceptionWhenContentStreamIsEmpty()
831+
{
832+
var response = new MockResponse(200);
833+
response.ContentStream = new MemoryStream();
834+
var transport = new MockTransport(_ => response);
835+
var tableClient = new TableClient(_url, TableName, new MockCredential(), new TableClientOptions { Transport = transport });
836+
837+
RequestFailedException ex = Assert.ThrowsAsync<RequestFailedException>(async () => await tableClient.GetEntityIfExistsAsync<TableEntity>("pk", "rk-1"));
838+
Assert.AreEqual(200, ex.Status);
839+
StringAssert.Contains("200", ex.Message);
840+
}
841+
842+
/// <summary>
843+
/// Regression test for https://github.com/Azure/azure-sdk-for-net/issues/58303.
844+
/// Verifies that GetEntityAsync throws RequestFailedException instead of
845+
/// JsonException when the HTTP response has an empty (non-null) ContentStream.
846+
/// </summary>
847+
[Test]
848+
public async Task GetEntityAsyncThrowsRequestFailedExceptionWhenContentStreamIsEmpty()
849+
{
850+
var response = new MockResponse(200);
851+
response.ContentStream = new MemoryStream();
852+
var transport = new MockTransport(_ => response);
853+
var tableClient = new TableClient(_url, TableName, new MockCredential(), new TableClientOptions { Transport = transport });
854+
855+
RequestFailedException ex = Assert.ThrowsAsync<RequestFailedException>(async () => await tableClient.GetEntityAsync<TableEntity>("pk", "rk-1"));
856+
Assert.AreEqual(200, ex.Status);
857+
StringAssert.Contains("200", ex.Message);
858+
}
859+
788860
/// <summary>
789861
/// A mock response that disposes its ContentStream on Dispose(), simulating the
790862
/// behavior of the real HttpClientTransportResponse for non-MemoryStream seekable
@@ -801,6 +873,20 @@ public override void Dispose()
801873
}
802874
}
803875

876+
/// <summary>
877+
/// A mock response with a null ContentStream. Used by regression tests for issue #58303.
878+
/// </summary>
879+
private class NullContentStreamMockResponse : MockResponse
880+
{
881+
public NullContentStreamMockResponse(int status) : base(status) { }
882+
883+
public override Stream ContentStream
884+
{
885+
get => null;
886+
set { }
887+
}
888+
}
889+
804890
public class EnumEntity : ITableEntity
805891
{
806892
public string PartitionKey { get; set; }

0 commit comments

Comments
 (0)