Conformance Tests for Azure Table Storage #1159
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
CodeMonkeyLeet
left a comment
CodeMonkeyLeet
left a comment
There was a problem hiding this comment.
Nice, thanks for adding this and fixing up the component!
berndverst
force-pushed
the
tablestorage
branch
from
1d1356c to
f8456bc
Compare
Member
Author
|
@CodeMonkeyLeet thanks for the review. Take another look when you got a chance. |
Comment on lines
199
to
218
| err := entity.Insert(storage.FullMetadata, nil) | ||
| if err != nil { | ||
| if isNotFoundError(err) { | ||
| // When entity is not found (set state first time) create it | ||
| entity.OdataEtag = "" | ||
| if etag == "" { | ||
| if req.Options.Concurrency == state.FirstWrite { | ||
| return state.NewETagError(state.ETagMismatch, err) | ||
| } | ||
|
|
||
| return entity.Insert(storage.FullMetadata, nil) | ||
| return entity.Update(true, nil) | ||
| } | ||
| err := entity.Update(false, nil) | ||
| if err != nil { | ||
| if isNotFoundError(err) { | ||
| return state.NewETagError(state.ETagMismatch, err) | ||
| } | ||
| } | ||
|
|
||
| return err | ||
| } | ||
|
|
||
| return err | ||
| return nil |
Contributor
There was a problem hiding this comment.
The logic LGTM, good job working it out. I would suggest heavily commenting it because the why it works took me a while to unravel that from the structure of the code and the quirks of the conformance test expectations.
I would also suggest explicitly checking for why Insert fails and handling expected cases with Update only.
err := entity.Insert(storage.FullMetadata, nil)
if err != nil {
// If Insert failed because item already exists, try to Update instead per Upsert semantics
if isEntityAlreadyExistsError(err) {
// Always Update using the etag when provided even if Concurrency != FirstWrite.
// This behavior is enforced by test state.go:440 today, where we treat the presence of etag as trumping Concurrency.
// In the future, we should error if an etag is provided when not using FirstWrite (see #2739)
if etag != "" {
uerr := entity.Update(false, nil)
if uerr != nil {
if isNotFoundError(uerr) {
return state.NewETagError(state.ETagMismatch, uerr)
}
return uerr
}
} else if req.Options.Concurrency == state.FirstWrite {
// Otherwise, if FirstWrite was set, but no etag was provided for an Update operation
// explicitly flag it as an error. This is enforced by test state.go:541.
// entity.Update itself does not flag the test case as a mismatch as it does not distinguish
// between nil and "" etags, the initial etag will always be "", which would match on update.
return state.NewETagError(state.ETagMismatch, errors.New("update with Concurrency.FirstWrite without ETag"))
} else {
// Finally, last write semantics without ETag should always perform a force update.
return entity.Update(true, nil)
}
} else {
// Any other unexpected error on Insert is propagated to the caller
return err
}
}
return nilwhere
func isEntityAlreadyExistsError(err error) bool {
azureError, ok := err.(storage.AzureStorageServiceError)
return ok && azureError.Code == "EntityAlreadyExists"
}
Codecov Report
@@ Coverage Diff @@
## master #1159 +/- ##
==========================================
+ Coverage 33.70% 33.77% +0.06%
==========================================
Files 138 138
Lines 11635 11686 +51
==========================================
+ Hits 3922 3947 +25
- Misses 7308 7329 +21
- Partials 405 410 +5
Continue to review full report at Codecov.
|
amimimor
pushed a commit
to amimimor/components-contrib
that referenced
this pull request
Dec 9, 2021
Conformance Tests for Azure Table Storage
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Progress towards #950 : Adds conformance tests for Azure Table Storage Pass.
Fixes incorrect ETag Deletion behavior.