Skip to content

Storage: Use RequestConditions and return exploding responses#8275

Merged
tg-msft merged 4 commits into
Azure:masterfrom
tg-msft:etags
Oct 22, 2019
Merged

Storage: Use RequestConditions and return exploding responses#8275
tg-msft merged 4 commits into
Azure:masterfrom
tg-msft:etags

Conversation

@tg-msft
Copy link
Copy Markdown
Member

@tg-msft tg-msft commented Oct 21, 2019

It's probably best to review this by commit:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed #8292

Comment thread sdk/storage/Azure.Storage.Blobs/src/Models/BlobRequestConditions.cs Outdated
Comment thread sdk/storage/Azure.Storage.Common/src/StorageExceptionExtensions.cs Outdated
Comment thread sdk/storage/Azure.Storage.Blobs.Batching/src/DelayedResponse.cs Outdated
Comment thread sdk/storage/Azure.Storage.Common/swagger/Generator/src/generator.ts Outdated
Comment thread sdk/storage/Azure.Storage.Blobs/src/LeaseClient.cs Outdated
Comment thread sdk/storage/Azure.Storage.Blobs/src/LeaseClient.cs Outdated
Comment thread sdk/storage/Azure.Storage.Blobs/src/LeaseClient.cs Outdated
Comment thread sdk/storage/Azure.Storage.Blobs/src/LeaseClient.cs Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is an ExplodingResponse?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an affectionate name for Response<T>s that will throw if you try to access Value. It's the pattern we've settled on for 304s per Azure/azure-sdk#638.

We want to avoid throwing exceptions if we can because they're not great for performance and clutter up logs. If you add an HTTP precondition on a GET/HEAD operation, Storage will return a 304. Instead of throwing, we'll give you back a Response<T> that has a Status == 304 and will still throw if you try to access Response<T>.Value. This gives us the best of both worlds because it'll still throw for customers who don't check, but there's a way for customers who know better to avoid the exception. All other HTTP verbs will return a Status == 412 and we always throw for those.

We just need to be careful in our code that we don't accidentally touch Response<T>.Value if it might explode.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where are these AddConditions() methods used?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are used by the base ToString. Instead of having [BlobRequestConditions:IfMatch=null;IfNoneMatch=null;...] we'll just list the conditions that are actually present so it'll be [BlobRequestConditions:IfMatch=...;LeaseId=...] if you're only using IfMatch/LeaseId.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wildcard makes the semantics here "update if the blob (however identified from its unique identifier) is not present."
This means it will fail if you try to upload a blob that is the same version as the one you're holding, which technically is not overwriting it. I would have expected IfNoneMatch to take the Etag of the blob, so it would fail if you're trying the update with a different version, which would overwrite existing data. If theses are the semantics the service defines, though, I'm not concerned.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, 99% of the time this API is called we won't have an ETag to use. That's part of why we called this overwrite instead of onlyIfUnchanged like you're doing in AppConfig since it's not relative to an existing ETag. Any customer who wants that behavior could the overload that allows them to pass BlobAccessConditions.

Copy link
Copy Markdown
Contributor

@annelo-msft annelo-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tg-msft tg-msft merged commit 640488d into Azure:master Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

5 participants