Fixes to Tables tests, as well as client documentation and exceptions. #23336
Conversation
…Cosmos endpoint. - Fixed tests that failed due to Cosmos not returning a `[email protected]` property in a `TableEntity` alongside the `Timestamp` property, like Storage does. This is apparently intended behavior, so we now treat `Timestamp` as a special case that needs to be always converted to `OffsetDateTime`. - Fixed the `setGetProperty()` tests in `TableServiceClientTest` and `TableServiceAsyncClientTest`, which failed for Storage because properties can take up to 30 seconds to be propagated on Storage accounts. - Updated `TableEntity` documentation to remove all mentions of using its subclasses. - Updated clients to properly map the implementation `TableServiceErrorException` to the public `TableServiceException` in operations such as `getAccessPolicies()`, `getProperties()`, `setProperties()` and `getStatistics()`, including their "withResponse" variants. - Updated client documentation to accurately reflect which exceptions are thrown by service methods. - Fixed batch operations to properly log exceptions other than `TableTransactionFailedException`.
vcolin7
requested review from
JonathanGiles,
annatisch,
christothes,
joheredi,
seankane-msft and
srnagar
annatisch
left a comment
annatisch
left a comment
There was a problem hiding this comment.
I can't speak to the Java - but the docs changes look good :)
|
|
||
| @Test | ||
| public void canUseSasTokenToCreateValidTableClient() { | ||
| Assumptions.assumeFalse(IS_COSMOS_TEST, "SAS Tokens are not supported for Cosmos endpoints."); |
There was a problem hiding this comment.
A Table-level SAS is supported by Cosmos - or are those separate tests?
There was a problem hiding this comment.
You are right, this is supported on the Table level by Cosmos. I will remove the assumption.
There was a problem hiding this comment.
Though I can't help but wonder why these tests fail for Cosmos and not for Storage, since we use the same logic to generate both SAS tokens and we don't really need to do anything special for Cosmos.
sdk/tables/azure-data-tables/src/main/java/com/azure/data/tables/TableAsyncClient.java
Show resolved
Hide resolved
| StepVerifier.create(tableClient.submitTransactionWithResponse(transactionalBatch)) | ||
| .expectErrorMatches(e -> e instanceof TableServiceException | ||
| && e.getMessage().contains("Status code 400") | ||
| && e.getMessage().contains("InvalidDuplicateRow") | ||
| && e.getMessage().contains("The batch request contains multiple changes with same row key.") | ||
| && e.getMessage().contains("An entity can appear only once in a batch request.")) | ||
| .verify(); | ||
| } else { | ||
| StepVerifier.create(tableClient.submitTransactionWithResponse(transactionalBatch)) | ||
| .expectErrorMatches(e -> e instanceof TableTransactionFailedException | ||
| && e.getMessage().contains("An action within the operation failed") | ||
| && e.getMessage().contains("The failed operation was") | ||
| && e.getMessage().contains("CreateEntity") | ||
| && e.getMessage().contains("partitionKey='" + partitionKeyValue) | ||
| && e.getMessage().contains("rowKey='" + rowKeyValue)) | ||
| .verify(); | ||
| } |
There was a problem hiding this comment.
Why do we return different exceptions for the same transaction batch? Whether the request is made to Cosmos or to Storage, the user should get the same exception.
There was a problem hiding this comment.
Because Cosmos is not consistent with Storage in this particular error scenario, it returns a 400 status code with the error message shown above instead of returning a 202 multipart response with the error message included within. I will talk to the Cosmos team about this and figure if it's intended or not.
There was a problem hiding this comment.
Either way, is this something that we can handle inside Tables SDK to map it to a common exception type for both Storage and Cosmos? It would be odd for users to get TableTransactionFailedException and TableServiceException for the same transaction depending on the source service.
There was a problem hiding this comment.
I think there is something I can do as a workaround in the meantime to return the same type of exception, but I would prefer to wait and see if this is a service bug.
| return; | ||
| } catch (TableServiceException e) { | ||
| assertTrue(IS_COSMOS_TEST); | ||
| assertTrue(e.getMessage().contains("Status code 400")); |
There was a problem hiding this comment.
Checking for exception messages is not reliable. Service can change the error message and these tests will start failing. Are there other fields in the exception that are part of the service contract that we can use to validate?
There was a problem hiding this comment.
I think we can check for the actual status code for now until we figure out if the Cosmos service not returning a multipart response is intended behavior.
- Removed assumption that prevented the `canUseSasTokenToCreateValidTableClient()` tests to run on Cosmos endpoints. - Changed `TableAsyncClient.submitTransactionWithResponse()` to wrap an `IllegalArgumentException` with a `Mono` when no transaction actions are provided. - Updated `submitTransactionAsyncWithSameRowKeys()` tests to check the error's status code instead of just the error message.
…endpoints to unblock nightly test runs. Will investigate further until this can be resolved.
3 participants
This PR contains the following changes:
Tests:
[email protected]property in aTableEntityalongside theTimestampproperty, like Storage does. This is intended behavior, so we now treatTimestampas a special case that needs to be always converted toOffsetDateTime.setGetProperty()tests inTableServiceClientTestandTableServiceAsyncClientTest, which failed for Storage because properties can take up to 30 seconds to be propagated on Storage accounts.Clients:
TableEntitydocumentation to remove all mentions subclassing it, as it is not currently supported.TableServiceErrorExceptionto the publicTableServiceExceptionin operations such asgetAccessPolicies(),getProperties(),setProperties()andgetStatistics(), including theirwithResponsevariants.TableTransactionFailedException.