[storage-file-share] Fix empty continuation token with list methods #26676
Conversation
This file contains 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
xirzec
requested review from
EmmaZhu,
XiaoningLiu,
jeremymeng,
vinjiang,
jiacfan and
ljian3377
as code owners
xirzec
added
Client
|
API change check API changes are not detected in this pull request. |
jeremymeng
approved these changes
Aug 1, 2023
dgetu
pushed a commit
that referenced
this pull request
Sep 6, 2023
…26676) ### Packages impacted by this PR `@azure/storage-file-share` ### Describe the problem that is addressed by this PR When the empty string was passed as a continuationToken to the `byPage` method of any `list` operation, the constructed url would have an empty marker parameter `&marker=&`. This was causing issues with SharedKey authentication since we now share the same credential signing logic as `storage-blob` and the implementation of `getURLQueries` in storage-blob ignores query parameters without a value: https://github.com/Azure/azure-sdk-for-js/blob/b4e1a63cd2499016f91852c7208f0c4c569651d4/sdk/storage/storage-blob/src/utils/utils.common.ts#L371 However, the previous copy of this method in `storage-file-share` did not: https://github.com/Azure/azure-sdk-for-js/blob/b4e1a63cd2499016f91852c7208f0c4c569651d4/sdk/storage/storage-file-share/src/utils/utils.common.ts#L306 Notice the missing `&& lastIndexOfEqual < value.length - 1` in the second implementation. ### What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen? While it is possible to change the way storage-blob does this signing to match storage-file-share, given that storage-blob is a far more popular package, there is considerable risk to changing behavior there. Since existing storage-file-share customers (such as Storage Explorer) may be already passing the empty string today, there is a strong desire to maintain that contract. Rather than diverge the signing logic around SharedKeyCredential again, I believe the least invasive fix is to have the list methods in storage-file-share ignore the empty string as demonstrated in this PR.
azure-sdk
pushed a commit
to azure-sdk/azure-sdk-for-js
that referenced
this pull request
Nov 14, 2023
Update readme.python.md (Azure#26676)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Packages impacted by this PR
@azure/storage-file-shareDescribe the problem that is addressed by this PR
When the empty string was passed as a continuationToken to the
byPagemethod of anylistoperation, the constructed url would have an empty marker parameter&marker=&.This was causing issues with SharedKey authentication since we now share the same credential signing logic as
storage-bloband the implementation ofgetURLQueriesin storage-blob ignores query parameters without a value:azure-sdk-for-js/sdk/storage/storage-blob/src/utils/utils.common.ts
Line 371 in b4e1a63
However, the previous copy of this method in
storage-file-sharedid not:azure-sdk-for-js/sdk/storage/storage-file-share/src/utils/utils.common.ts
Line 306 in b4e1a63
Notice the missing
&& lastIndexOfEqual < value.length - 1in the second implementation.What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?
While it is possible to change the way storage-blob does this signing to match storage-file-share, given that storage-blob is a far more popular package, there is considerable risk to changing behavior there.
Since existing storage-file-share customers (such as Storage Explorer) may be already passing the empty string today, there is a strong desire to maintain that contract.
Rather than diverge the signing logic around SharedKeyCredential again, I believe the least invasive fix is to have the list methods in storage-file-share ignore the empty string as demonstrated in this PR.