Fix table entities error checking #563
Conversation
Some functions were only checking the response code if a call to execTable returned an error, but in some cases calls to this can fail without returning errors. It is unclear if this behaviour is intentiona or not, so for now we just check the response code at all times.
|
@johnglover, It will cover your contributions to all Microsoft-managed open source projects. |
|
@johnglover, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request. |
There was a problem hiding this comment.
Nice catch. I would have hoped that code review would have caught this before the offending lines were ever committed.
We've moved the storage package into its own repository, and are primarily doing development there. Furthermore, in general we like to take pull-requests to the dev branch. However, given the nature of this bug, I'd like to get it in as a release patch ASAP. For that reason, I believe the best course of action is to accept this PR and cherry-pick this change into the new storage repository. Thoughts, @mcardosos and @jhendrixMSFT?
|
|
||
| tn := AzureTable(randTable()) | ||
|
|
||
| ce := &CustomEntity{Name: "Luke", Surname: "Skywalker", Number: 1543, PKey: "pkey", RKey: "5"} |
There was a problem hiding this comment.
Taken from an existing test case so I can't take credit :)
| } | ||
|
|
||
| // MergeEntity merges the contents of an entity with the | ||
| // one passed as parameter. | ||
| // The function fails if there is no entity | ||
| // with the same PartitionKey and RowKey in the table. | ||
| func (c *TableServiceClient) MergeEntity(table AzureTable, entity TableEntity) error { | ||
| if sc, err := c.execTable(table, entity, true, "MERGE"); err != nil { | ||
| return checkRespCode(sc, []int{http.StatusNoContent}) | ||
| sc, err := c.execTable(table, entity, true, "MERGE") |
There was a problem hiding this comment.
I'd like to pull "MERGE" out as a const. However, I suspect that this isn't the right package to host its definition. I'll file an issue to track this follow up item.
There was a problem hiding this comment.
I would like the http package to include it as a const 😢
|
Sure, I agree with merging here and then cherry-pick. |
|
BTW, the repo for storage is now https://github.com/Azure/azure-storage-go :) |
|
@mcardosos: thanks, I wasn't aware of the separate storage repo. |
|
Fixed in the other repo too! |
…cking (Azure#20) Fix table entities error checking
…#20) Fix table entities error checking
…#20) Fix table entities error checking
There is a nasty bug in the current TableService code that results in some errors being silently dropped.
The following functions are only checking the response code if a call to
c.execTablereturns an error:InsertEntityUpdateEntityInsertOrReplaceEntityInsertOrMergeEntityIn some cases calls to
c.execTablecan fail without returning errors. In particular, it callsclient.execInternalJSON, which does return an error for bad status codes with no response body (https://github.com/Azure/azure-sdk-for-go/blob/master/storage/client.go#L414). However if there is some response body, it then unmarshals it, and only returns an error if this unmarshalling fails (https://github.com/Azure/azure-sdk-for-go/blob/master/storage/client.go#L420).It is unclear if this behaviour (for
execInternalJSON) is intentional or not, so this PR just makes sure that the response code is checked at all times. It also adds a new test verifying that trying to insert entities into nonexistent tables should fail as expected.