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

Update azure-autorust to append path and parameters via URL lib #1663

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

andras-pinter
Copy link

When returning a URL from Client::endpoint the URL contains a tailing slash in any case. This is handled by the URL lib which will append the tailing slash to the end upon calling Display::fmt. Therefore formatting the returned URL from Client::endpoint will result in a duplicated slash, which could cause some (not all) Azure endpoints to have some trouble.

For example:
Base URL: https://some-app-conf.azconfig.io/
AppConfigUrl: https://some-app-conf.azconfig.io//keys?api-version=2023-10-01 This resulted in a 404 response.

Removing the tailing slash from the base endpoint's URL manually does not solve the problem, due to the reason mentioned above.

This patch applies a fix to append (set) the formatted path via rust URL lib, which will handle the duplicated slash (or any other problem).

Although I tried to locally generate mgmt and svc crates, I encountered some problems, and autorust removed and added a bunch of new files and modules.
So I'd like to ask somebody to help me out with the generation.

When returning a URL from Client::endpoint the URL contains a tailing slash
in any case. This is handled by the URL lib which will append the tailing
slash to the end upon calling Display::fmt. Therefore formatting the returned
URL from Client::endpoint will result in a duplicated slash, which could cause
some (not all) Azure endpoints to have some trouble.

For example:
Base URL: https://some-app-conf.azconfig.io/
AppConfigUrl: https://some-app-conf.azconfig.io//keys?api-version=2023-10-01
This resulted in a 404 response.

Removing the tailing slash from the URL manually does not solve the problem,
due to the reason mentioned above.

This patch applies a fix to append (set) the formatted path via rust URL lib,
which will handle the duplicated slash (or any other problem).
@heaths
Copy link
Member

heaths commented May 15, 2024

Whether there's a trailing slash or not, any code formatting URLs using the endpoint should handle that. It's just as likely that some code erroneously expects a trailing slash, doesn't append on, and ends up with an invalid URL. Besides, in most cases services should canonicalize paths.

I'm reluctant to take this one. Is there some common issue you're seeing with code not appending paths to endpoints with a trialing slash? Can those be fixed? That seems the crux of the problem.

@andras-pinter
Copy link
Author

andras-pinter commented May 15, 2024

Hey @heaths,

Yes, there is a particular error, what I recently encountered. Please see: #1664

@heaths
Copy link
Member

heaths commented May 15, 2024

@ctaggart could you review this? If you're good with it, I can regenerate the service crates.

@andras-pinter
Copy link
Author

My goal was to let azure_core::Url handle URL creation :)

@@ -58,7 +57,8 @@ impl ToTokens for RequestBuilderSendCode {
let api_version = &self.request_builder.api_version;
quote! {
fn url(&self) -> azure_core::Result<azure_core::Url> {
let mut url = azure_core::Url::parse(&format!(#fpath, self.client.endpoint(), #url_str_args))?;
let mut url = self.client.endpoint().clone();
url.set_path(&format!(#fpath, #url_str_args));
Copy link
Member

Choose a reason for hiding this comment

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

This change looks good to me.

@cataggar
Copy link
Member

@ctaggart could you review this? If you're good with it, I can regenerate the service crates.

Sounds good. Please go ahead.

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.

None yet

3 participants