Skip to content
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

[storage] Change generated code Uri.ToString to Uri.AbsoluteUri #12118

Merged
merged 2 commits into from
May 19, 2020

Conversation

amnguye
Copy link
Member

@amnguye amnguye commented May 16, 2020

Resolves #11368

Uri.ToString() unencodes the Uri before sending the request to storage. When using a source Uri that has non ascii characters, we will encode the Uri by putting it in the Uri object but unencode it when setting the header.

Switching to using Uri.AbsoluteUri fixes this problem. This fix applies to AppendBlobClient.AppendBlockFromUri. BlockBlobClient.StageBlockFromUri, PageBlobClient.UploadPagesFromUri, FileClient.StartCopy, FileClient.UploadRangeFromUri

This PR also has updates to DataLakeErrorDetails because of the regenerated code.

  • Updated generator to convert Uri.ToString to Uri.AbsoluteUri
  • Regenerated adding in Error to DataLakeErrorDetails
  • Added tests and recordings for non Ascii source blob/file names

…ated adding in Error to DataLakeErrorDetails; added tests for non Ascii source blob/file names; Added recordings
@amnguye amnguye marked this pull request as ready for review May 16, 2020 05:28
Copy link
Member

@seanmcc-msft seanmcc-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Why did we make the Data Lake error change?

@amnguye
Copy link
Member Author

amnguye commented May 18, 2020

Looks good.

Why did we make the Data Lake error change?

Hm. So I assumed someone from our team made the change to the storage swagger and we just haven't regenerated recently, but looks like the change was made by this PR

Azure/azure-rest-api-specs#8739

Should I make a separate PR to make updates to the regenerated code?

@kasobol-msft
Copy link
Contributor

Looks good.
Why did we make the Data Lake error change?

Hm. So I assumed someone from our team made the change to the storage swagger and we just haven't regenerated recently, but looks like the change was made by this PR

Azure/azure-rest-api-specs#8739

Should I make a separate PR to make updates to the regenerated code?

We shouldn't be touching generated code (otherwise process would become non-deterministic at some point). If we were to change this we'd edit readme with generator transforms. However, this is just internal type rename, so we're good.

On the side note. https://github.com/Azure/azure-rest-api-specs/pull/8739/files it seems that we don't have this in 2019-12-12 rest spec. Would be good to get it there as well.

@tg-msft
Copy link
Member

tg-msft commented May 18, 2020

Yes - we should make the same swagger change to 2019-12-12 as @kasobol-msft noted. The new Track 2 generators being developed are a lot pickier about correct swagger syntax instead of silently swallowing issues. It's a good thing overall, but will require small changes like this.

@amnguye amnguye merged commit c29aa1e into Azure:master May 19, 2020
@tg-msft tg-msft mentioned this pull request Jul 11, 2020
@amnguye amnguye deleted the bug/storage/uriencode branch July 31, 2023 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Storage Blobs - can't copy from URL with non-ASCII characters
4 participants