-
Notifications
You must be signed in to change notification settings - Fork 252
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix find blob by tag #1622
base: legacy
Are you sure you want to change the base?
Fix find blob by tag #1622
Conversation
There were 2 issues in here, one is that the API version got updated recently and that caused the response to be different than the one expected so querying blobs by tag wasn't working. Another item was when we add multiple tags to the where expression, that was failing because of the way the url was being encoded and that doesn't seem to be accepted by Azure REST API
@seanmcc-msft is this a known breaking change to the format? |
I took the chance to hook up the MaxResults into FindBlobsByTagsBuilder as it wasn't doing anything, removed the next_marker on the input and change it to Marker to match the |
@seanmcc-msft is there anyone on your team that can review this? It's mostly service-related code. |
@jaschrep-msft @vincenttran-msft @jalauzon-msft please take a look. |
fn make_url_compatible_with_api(url: &mut Url) { | ||
let compatible_query = url.query().map(|q| q.replace("+", "%20")); | ||
|
||
url.set_query(compatible_query.as_deref()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General note, requiring %20
instead of +
as the proper encoding for a space character is service-wide. Any URL encoding done by the library for storage requests needs to do this. Probably beyond the scope of this PR, but good future reference.
Updates the requirements on [base64](https://github.com/marshallpierce/rust-base64) to permit the latest version. - [Changelog](https://github.com/marshallpierce/rust-base64/blob/master/RELEASE-NOTES.md) - [Commits](marshallpierce/rust-base64@v0.21.0...v0.22.0) --- updated-dependencies: - dependency-name: base64 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
dcc3aea
to
1b750e2
Compare
|
There were 2 issues in here, one is that the API version got updated recently and that caused the response to be different than the one expected so querying blobs by tag wasn't working. I believe the version that the current
main
code has is the response for when we query withx-ms-version: 2019-12-12
, however, we're now sending our requests withx-ms-version: 2022-11-02
which caused this to completely break.Another item was when we add multiple tags to the where expression, that was failing because of the way the URL was being encoded and that doesn't seem to be accepted by Azure REST API.
I've been using a workaround branch that was still using the old API for quite a while without any issues with this URL fix, related to this issue I raised a while ago #1284
The error seems to have changed on the new API version as without the
make_url_compatible_with_api
if we query anything with more than 1 tag in the filter we get this error:One question I have regarding the extra structs I had to add to have the response matching the new format of the XML for the tag content, do we need that at all? I was wondering if we could just drop the tag content as it doesn't seem like it's being used anywhere. Happy to keep it though in case there's any intention of using it in the future or if we want it for information purposes.