Fixed issues with resource token #13855
Conversation
srinathnarayanan
commented
Sep 18, 2020
srinathnarayanan
commented
- Earlier, the SDK was doing a comparison by id to check if the resource token was for a particular resource was passed to the client
- Now, we check for the entire path of the resource to be a key to the resource token passed to the client
- Changed existing tests to reflect this behavior.
- Modified existing tests
srinathnarayanan
requested review from
southpolesteve and
zfoster
as code owners
ghost
added
the
| resource_tokens[urllib.quote(all_collection.id)] = (all_permission.properties['_token']) | ||
| resource_tokens[urllib.quote(read_collection.id)] = (read_permission.properties['_token']) | ||
| resource_tokens["dbs/" + created_db.id + "/colls/" + all_collection.id] = (all_permission.properties['_token']) | ||
| resource_tokens["dbs/" + created_db.id + "/colls/" + read_collection.id] = (read_permission.properties['_token']) |
There was a problem hiding this comment.
Was the existing test invalid in some way?
Rather than alter existing tests, we should simply add new ones - we don't want to inadvertently break existing customers. All the existing tests should pass without modification (unless the tests were never correct in the first place).
There was a problem hiding this comment.
So the way in which resource tokens were implemented were faulty. resource tokens are supposed to be a map of resource link to token . i.e {"dbs/mydb/coll/mycoll1": "token1", "dbs/mydb/colls/mycoll2": "token2"} (this is the implementation in the .Net SDK and is the spec as well). But in the python sdk currently, we expect to get it to be a map of resource id to token. i.e {"mycoll1": "token1", "mycoll2": "token2"}. This is faulty since the same collection name can exist in 2 different databases.
These tests also follow the old pattern. They will break with the new changes, so I changed the tests as well.
There was a problem hiding this comment.
Thanks, but is it possible any customers were using the old faulty pattern? For example by manually building this dictionary? Or if they had done that would it not have worked?
There was a problem hiding this comment.
@annatisch It is possible that some cases would have worked but that is not intentional. We consider it a bug and fixed the same issue in JS SDK recently.
Previous versions of the Cosmos SDK were all based on rid which was a unique identifier resource. The transition to user supplied ids a couple years ago changed that, but we didn't fix resource tokens to account for it. Given all this, and the fact that resource tokens do not see a lot of usage, I am fine with merging this as a bugfix.
There was a problem hiding this comment.
I would still prefer this to be backwards compatible - but if you've rolled out the same change in other languages I guess that's okay.
|
/azp run python - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…into path_format_args_polling * 'master' of https://github.com/Azure/azure-sdk-for-python: (46 commits) Fix typo in track 2 migration guide (Azure#14006) commit metrics advisor (Azure#14072) [eventgrid] Update README.md (Azure#14030) Update Key Vault readmes (Azure#14068) Update CODEOWNERS (Azure#14054) Correct azure-eventhub example (Azure#14050) Replace docstring code blocks with literalincludes (Azure#13948) Added more docstring context and sample code for `receive_messages()` (Azure#13789) Form 2.1-preview.1 (Azure#13770) Sync eng/common directory with azure-sdk-tools repository for Tools PR 1029 (Azure#13896) Service Mapping Update for Communication (Azure#13979) update sample to show facet+count & collection type (Azure#13909) Increment package version after release of azure_eventgrid (Azure#14020) Update CHANGELOG.md (Azure#14014) fixed delete blob types (Azure#14001) Fixed issues with resource token (Azure#13855) update samples and readme (Azure#13999) update dependent version of azure-core (Azure#13986) Sync eng/common directory with azure-sdk-tools repository for Tools PR 1041 (Azure#14005) Track2-azure-mgmt-baremetalinfrastructure-2020-9-22 (Azure#13941) ...
Release microsoft.sql 2021 02 01 preview (Azure#14339) * Adds base for updating Microsoft.Sql from version preview/2020-11-01-preview to version 2021-02-01-preview * Updates readme * Updates API version in new specs and examples * Fix Integer format: S360 swagger lint issues in 2021-02-01 (Azure#13855) * Update integer format * update typo * Swagger Documentation for Outbound Firewall Rules (Azure#13820) * Swagger Documentation for Outbound Firewall Rules * Add the new file to v5 Co-authored-by: Vineet Mahadik <Vineet.Mahadik@microsoft.com> * Swagger Documentation For Database Resource With Ledger (Azure#13916) * adding database swagger files * modifying example * Fixing to be boolean * adding database extensions and database operations * adding other database examples * removing database_legacy, adding usages * removing usages * removing databases_legacy from readme * Swagger Documentation For Ledger Digest Upload (Azure#13871) * adding ledger api * Fixes from validation * removing required endpoint parameter, adding ledgerName (current) to id * removing 201 response from resource, renaming ledgerName * adding location to 202 results * fixing 202 response location * renaming files * updating files with new controller name + correct entity name * fixing readme * Dev brandong getrestorabledropped (Azure#14129) * Update RestorableDroppedDatabases API to add BackupStorageAccountType property * Update readme.md * Re-add elasticPoolId as a deprecated property * Update elasticPoolId description and remove trailing comma causing failures * Remove the unsupported deprecated property * Add 2021-02-01-preview minor changes (Azure#13942) * add 2021-02-01-preview for test * update with 2021-04-19 latest * update readme.md * update with latest master in DSMainDev * Carry IsInfraEncryptionEnabled to Database.json (Azure#14322) * carry latest minor changes. * re-format readme.md * remove 2020 11 01 RestorableDroppedManagedDatabases in V5 * Update readme file in 2021 02 01 dev branch to match the master branch (Azure#14336) * Carry IsInfraEncryptionEnabled to Database.json (Azure#14322) * carry latest minor changes. * re-format readme.md * remove 2020 11 01 RestorableDroppedManagedDatabases in V5 * update to match master branch readme.md Co-authored-by: Vineet Mahadik <44247873+VMMicrosoft@users.noreply.github.com> Co-authored-by: Vineet Mahadik <Vineet.Mahadik@microsoft.com> Co-authored-by: rewongmicrosoft <57964114+rewongmicrosoft@users.noreply.github.com> Co-authored-by: brandong-ms <44618010+brandong-ms@users.noreply.github.com> Co-authored-by: Arthur Ning <57385816+akning-ms@users.noreply.github.com>
