Skip to content
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

Guidance on handling ETags #638

Closed
annelo-msft opened this issue Sep 17, 2019 · 9 comments
Closed

Guidance on handling ETags #638

annelo-msft opened this issue Sep 17, 2019 · 9 comments
Assignees

Comments

@annelo-msft
Copy link
Member

annelo-msft commented Sep 17, 2019

The Azure SDK guidelines aren't currently prescriptive regarding the use of ETag in APIs. In storage libraries, ETag is alternately handled in a separate blob properties class in Azure Blob Storage, and as an entity property in Azure Storage Tables. It would be helpful for the board to characterize when to take one approach versus the other and give further guidance about patterns for conditional operations.

There are several key questions to this issue:

  • Should SDKs abstract the concept of ETag from the user?
  • Should ETag live as a property on the logical entity it represents, or a separate object holding response headers?
  • Should ETag be explicitly passed to a method, or read from the logical entity?
  • Should ETag be settable on the logical entity so that the user can clear the value?

In the following discussion, we'll look specifically at the use of ETag in the AppConfiguration client library, several approaches that answer these questions in different ways, and make a recommendation for a solution that addresses most of our concerns.

ETag in AppConfiguration

The .NET APIs currently provide the following options for CRUD operations:

image

For Delete, ETag is explicitly passed as a parameter which has the benefit of alerting the caller to its use in the method. The concerns come with the Set operation, where If-Match is set on the Request if the ETag property on ConfigurationSetting has a value, which hides its use from callers. It also means we must make ETag a settable property on ConfigurationSettting to enable clearing it, which can cause confusion to users creating an instance of ConfigurationSetting, where they might believe they should set this value.

Possible Approaches

  1. ETag is not a property of ConfigurationSetting, and passed as a parameter to operations.

If ETag isn't a property of ConfigurationSetting, users must obtain ETag from the Response object and implement management of a group of ETags in order to pass them into Delete, Get and Set methods. It might not be obvious to the user that it should be obtained from the response, and it would require many customers to implement similar logic that our library should provide.

  1. ETag is a read-write property of ConfigurationSetting, and ConfigurationSetting is passed as a parameter to operations (not ETag)

If If-Match is set when the ConfigurationSetting's ETag property has a value, the user must learn this pattern to implement optimistic concurrency. There is no ETag parameter in method signatures for Delete, Get or Set, so it is not explicitly called out to the user that the value is used in the method. In order to use last writer wins semantics for these methods, ETag must be a settable property of ConfigurationSetting, which potentially leads to confusion when creating instances of ConfigurationSetting -- the implication is that the user should set it to something which is misleading, since in almost all cases this value should be obtained from the server. Another concern with this approach is that Set and Delete will throw infrequently, so users may not realize they need to handle the exception until they get to production.

  1. ETag is a read-only property of ConfigurationSetting, and both ConfigurationSetting and a separate ETag parameter is passed to override the ConfigurationSetting property.

Having two ETags passed to the method causes confusion about which one takes precedence.

  1. ETag is a read-only property of ConfigurationSetting, and both ConfigurationSetting and an onlyIfMatches parameter is passed to the method.

The onlyIfMatches parameter indicates whether or not the ETag value should be added to the Request in an If-Match header. This approach gives the user the option to make the operation conditional or not, without causing confusion with multiple ETag values available in the same method. There is a parameter indicating that ETag will be in use which will help alert callers both to its use and the need to handle exceptions the method could throw. ETag is not settable, so it avoids confusion when a ConfigurationSetting object is created.

Proposed API

The fourth approach addresses most of our concerns surrounding ETag. In this approach, the CRUD operations become:

image

In addition, in this approach, the answers to our key questions become:

  • Should SDKs abstract the concept of ETag from the user?
    -> No, ETag is still visible, but only its functionality is described at the method level, and the defaults use LWW semantics so user not handling optimistic concurrency won't fail sporadically.
  • Should ETag live as a property on the logical entity it represents, or a separate object holding response headers?
    -> Property of logical entity
  • Should ETag be explicitly passed to a method, or read from the logical entity?
    -> Read from the logical entity, applied based on parameter that defaults to last writer wins.
  • Should ETag be settable on the logical entity so that the user can clear the value?
    -> Not settable, but applied based on method parameter.
@adrianhall
Copy link
Member

Scheduled for 9/24

@johanste
Copy link
Member

I would propose a slight alternative; instead of using a single boolean parameter name, use an enumeration to determine how the etag should be interpreted (if provided). Strawman implementation below.

class Condition(Enum):

    Unconditionally
    IfNotModified
    IfModified
    IfPresent
    IfMissing

class AzureAppConfigurationClient(object):

    def get_configuration_setting(
            self, key, label=None, *, condition: Condition=Condition.Unconditionally, etag=None, **kwargs
        ):
        pass

    def set_configuration_setting(
        self, configuration_setting, *, condition:Condition=Condition.Unconditionally, **kwargs
    ):
        pass
    
    def delete_configuration_setting(
        self, configuration_setting, label=None, *, condition:Condition=Condition.Unconditionally, etag=None**kwargs
    ):
        pass
    
    # Convenience methods
    def add_configuration_setting(self, configuration_setting, *, **kwargs):
        return self.set_configuration_setting(configuration_setting, condition=Condition.IfMissing, **kwargs)
        
    def update_configuration_setting(self, configuration_setting_or_name, label=None, *, value=None, tags=None, etag=None, **kwargs):
        return self.set_configuration_setting(configuration_setting, condition=Condition.IfPresent, **kwargs)
  • The etag is still a property on the returned model/logical entity.
  • For methods that accept a model instance as a parameter, use the etag from the value passed in.
  • For methods that take the primary key of the resource, provide a specific etag parameter.
  • The etag property is read-only (but you should be able to provide a value in the constructor/deserialize a previously persisted resource instance.
  • You cannot clear the etag property on the model instance.

@johanste
Copy link
Member

Also, please note that, in the general case, not all services support If-Match/conditional headers. Which begs the question as to what the default should be (consistent across all services/unconditional or safe if the service supports it).

@KrzysztofCwalina
Copy link
Member

I like the proposal with explicit (optional enum). But I am not thrilled with get/set ETag property. Should we add an API to clear the ETag instead? Another thing I worry a bit is that some combination of the enum value and the method don't make sense, e.g. delete if missing.

Another option to consider: instead of using an enum, have dedicated method names

  • Add: IfMissing
  • Set: Unconditionally
  • Update: IfNotModified
  • Delete: Unconditionally

@johanste
Copy link
Member

If we were to just use method names, then we would need additional names (what should we call partial updates for services that support PATCH - and what would the conditional vs. unconditional partial updates - as opposed to replace/set - be called?)

I would not make the ETag property get/set. I would make it read-only.

And, yes, some combinations most likely don't make sense. One option would be to have different enum types for different methods. Alternatively, since I believe the API will do exactly what you told it to if you try to delete something only if it does not exist, we can rely on users simply not calling the API in that way. Unless they have come up with a novel idea as to why that would be a good idea :).

@tg-msft
Copy link
Member

tg-msft commented Sep 24, 2019

For Storage we've got HttpAccessConditions (review and source) that aligns more with @johanste's Condition enum than @KrzysztofCwalina's multiple methods suggestion. That's largely because we don't have many input/output models with an ETag on them.

@annelo-msft
Copy link
Member Author

annelo-msft commented Sep 29, 2019

Here is the outcome of conversations with the arch board.

A candidate implementation can be found here:

Handling Conditional Requests

We consider the broader issue of Conditional Requests in HTTP. Generally speaking, there are two types of conditional requests.

  1. Safe (e.g. GET)

    These are typically used to save bandwidth in an "update cache" scenario, i.e. I have a cached value, only send me the data if what the service has is newer than my copy. These return either a 200 or a 304 status code, indicating the value was not modified, which tells the caller that their cached value is up to date.

  2. Unsafe (e.g. PUT, DELETE)

    These are typically used to prevent losing updates in an optimistic concurrency scenario, i.e. I've modified the cached value I'm holding, but don't update the service version unless it hass the same copy I've got. These return either a success or a 412 error status code, indicating the value was modified, to indicate to the caller that they'll need to retry their update if they want it to succeed.

We handle these two cases differently, since 304 is essentially a success case (you have the latest value), whereas 412 is a failure case (your PUT request didn't succeed).

Return Types for Conditional Requests

Safe

For safe operations, we return Response<T> and callers must check the response status code to determine whether or not they received an updated value. If the response was 304, it means there is no updated value in the response content, so it is an error to reference response.Value. We throw a ResourceModifiedException to indicate this when response.Value is referenced on a 304 response.

Unsafe

For unsafe operations sent as conditional requests, we throw a RequestFailedException to indicate that the operation failed to complete. The caller must catch this exception, and take appropriate action, e.g. retrieving the latest value and retrying the update.

Method Signatures and Overloads

In order to prevent callers from unwittingly calling a method with a value that can throw, we require them to explicitly indicate that their request is conditional, by means of a parameter in the method signature.

Safe

For safe methods, the client may provide an onlyIfChanged parameter which defaults to false (unconditional). If the caller wants to make the method conditional, they must set this value to true.

Unsafe

For unsafe methods, the client may provide an onlyIfUnchanged parameter, which defaults to false (unconditional). If the caller wants to make the method conditional, they must set this to true.

While this is an unsafe default, it is consistent with the semantics of set in Dictionary and ConcurrentDictionary, which overwrite whatever value is present if called. It also means that callers opt-in to optimistic concurrency, which clarifies the need to handle exceptions that could be thrown as a result of a prior update. We consider this an uncommon case that might not be discovered during application development, leading to crashes in production that are hard to diagnose.

Flexibility: HttpRequestOptions

In order to support all varieties of conditional requests, including IfModifiedSince and IfUnmodifiedSince, and to accommodate the possibility of services introducing conditional requests in later versions, we provide overloads that take an HttpRequestOptions object, which currently provides properties for four of the five precondition headers, and could be extended to include the IfRange header if a service used it.

HttpRequestOptions gives us a place to add support for request scenarios we discover later. If we add something that's universal to requests, it will be added to the type. If we want to add something that is specific to a given operation, we can provide this functionality by inheriting from HttpRequestOptions and passing in a subtype to the base type.

Methods with a HttpRequestOptions parameter are considered "kitchen sink" methods that require users to specify all possible parameters to the given operation.

Convenience Methods

Convenience methods are provided which do not take conditional parameters and send the request unconditionally.

adrianhall added a commit that referenced this issue Oct 15, 2019
* Added requirements for etag handling

* Updated based on feedback from board

* Updates after review by @annelo-msft

* More comments from @annelo-msft

* Address JR concerns

* Anne's updates
@adrianhall
Copy link
Member

Closed on the request, per arch board, in PR #646 (merged)

@adrianhall
Copy link
Member

bterlson pushed a commit to bterlson/azure-sdk that referenced this issue Apr 16, 2020
* Added requirements for etag handling

* Updated based on feedback from board

* Updates after review by @annelo-msft

* More comments from @annelo-msft

* Address JR concerns

* Anne's updates
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants