[BUG] TableClient SubmitTransactionAsync throws InvalidOperationExteption on empty arguments

[thomaseyde]

Library name and version

Azure.Data.Tables 12.9.1

Describe the bug

Call to table.ClientSubmitTransactionAsync(actions) throws an InvalidOperationException when actions is an empty collection.

There is nothing in the code documentation (tooltip in IDE) which hints that an empty collection is an exceptional case.

This behaviour is unfortunate, because one usually discover this in production logs or during debugging and the exceptions does not add value.

Expected behavior

Passing an empty collection to table.ClientSubmitTransactionAsync(actions) should be treated as a no-op so we do not have to add defensive code for something non-obvious.

Actual behavior

Call to table.ClientSubmitTransactionAsync(actions) throws an InvalidOperationException when actions is an empty collection.

Reproduction Steps

 await tableClient.SubmitTransactionAsync([]);

Environment

No response

[jsquire]

Hi @thomaseyde. Thank you for reaching out and we regret that you're experiencing difficulties. Unfortunately, the client behavior is by design in this scenario and not something that we can apply your suggestion to.

By invoking ClientSubmitTransactionAsync, you are requesting that the client perform a service operation on your behalf. The client does not have the insight to know whether you intended to invoke this with an empty set of arguments or whether that indicates a bug in your application. If we were to silently no-op for this scenario, it signals to callers would that the requested service operation was invoked successfully, which it was not.
This would, itself, be confusing behavior and open the door to another class of application errors. It would also constitute a behavioral breaking change to the existing API contract.

By convention, the Azure SDK clients in all packages perform only validation for client-focused parameters (such as the service endpoint) and basic sanity checks consistent with the REST API spec for a service (such as a null check on a parameter annotated as "required"). We intentionally do not perform deeper inspection of parameter values nor the relationship between them - the service is responsible for these, by contract. As a result, if you pass an empty set of actions, the client will ensure that it is not a null reference and, if so, defer to the service on what it considers valid.

[github-actions]

Hi @thomaseyde. Thank you for opening this issue and giving us the opportunity to assist. We believe that this has been addressed. If you feel that further discussion is needed, please add a comment with the text "/unresolve" to remove the "issue-addressed" label and continue the conversation.

[thomaseyde]

/unresolve

Thank you for your feedback.

But I fail to follow the logic:

By invoking ClientSubmitTransactionAsync, you are requesting that the client perform a service operation on your behalf.

That is true. But that is not what the client does. The requirements for the REST api are:

An entity group transaction must meet the following requirements:

• All entities subject to operations as part of the transaction must have the same PartitionKey value.
• An entity can appear only once in the transaction, and only one operation may be performed against it.
• The transaction can include at most 100 entities, and its total payload may be no more than 4 MiB in size.
• All entities are subject to the limitations described in Understanding the Table Service Data Model.

From Performing entity group transactions

Nothing here says an empty set is invalid input, yet the client decides up front that it is.

The client does not have the insight to know whether you intended to invoke this with an empty set of arguments or whether that indicates a bug in your application.

That is absolutely correct, the client cannot know my intensions. Yet it concludes an empty set is a bug.

If we were to silently no-op for this scenario, it signals to callers would that the requested service operation was invoked successfully, which it was not.

That is true. We can debate wether this is a practical statement or not, but from a user's point of view, this is better than having an exception thrown on innocent input.

This would, itself, be confusing behavior and open the door to another class of application errors. It would also constitute a behavioral breaking change to the existing API contract.

I can see why turning the empty set into a no-op may not be a good solution. But if you truly perform operations on my behalf, you should pass the empty set to the service and report the result.

By convention, the Azure SDK clients in all packages perform only validation for client-focused parameters (such as the service endpoint) and basic sanity checks consistent with the REST API spec for a service (such as a null check on a parameter annotated as "required").

An empty set is not null. According to the REST service requirement I refer to, an empty set is considered valid input.

We intentionally do not perform deeper inspection of parameter values nor the relationship between them - the service is responsible for these, by contract. As a result, if you pass an empty set of actions, the client will ensure that it is not a null reference and, if so, defer to the service on what it considers valid.

And yet, that is not what the client does. The client does not check on null, it explicitly checks if the set is empty.

From the source code

        internal virtual async Task<Response<IReadOnlyList<Response>>> SubmitTransactionInternalAsync(
            IEnumerable<TableTransactionAction> transactionalBatch,
            Guid batchId,
            Guid changesetId,
            bool async,
            CancellationToken cancellationToken = default)
        {
            // ...

                using (IEnumerator<TableTransactionAction> batchEnumerator = transactionalBatch.GetEnumerator())
                {
                    if (!batchEnumerator.MoveNext())
                    {
                        throw new InvalidOperationException(TableConstants.ExceptionMessages.BatchIsEmpty);
                    }

                    // ...
                }
            // ...
        }

The line throwing the exception seems to not comply with the REST api requirements.

[jsquire]

Hi @thomaseyde. Thanks for sharing your additional feedback. While we appreciate that you have different thoughts on some of the design of the library, and we could continue to exchange perspectives on the details, I want to refocus for a moment on the outcome.

Regardless of whether or not we agree on the "correct" behavior, making the suggested change would constitute a behavioral breaking change to the existing API contract. This is not something that we would consider doing, as it violates our strong backwards compatibility guarantees. Likewise, the additional API complexity of a new parameter or overload for this scenario outweighs the benefits.

As a result, we won't be moving forward with the suggestion. However, in taking a second look, I did notice that the <exception> documentation for InvalidOperationException on these methods doesn't explicitly call out this scenario. We will add that to ensure that the behavior is properly documented.

[thomaseyde]

Well, better documentation is better than nothing.

You know more than me on how the client actually works, but for me I see no changes at all:

• No new parameters.
• No signature change.
• The documented exception can still be thrown when necessary.

There is only an internal change:

• Pass the empty set to the service and report back the result.
• If the service agrees with you that an empty set is an error, then throw the exception.

Existing code may have a defensive check, which is no longer needed, but it will not break.

I appreciate your promise to not break backwards compatibility, but that is not an issue in this case.

But you made your standpoint very clear, this will be my last comment on the case.