Skip to content

Allow relative URL being used as base URL #330

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

Merged
merged 5 commits into from
Feb 9, 2022
Merged

Allow relative URL being used as base URL #330

merged 5 commits into from
Feb 9, 2022

Conversation

yifanbian-msft
Copy link
Contributor

@yifanbian-msft yifanbian-msft commented Dec 2, 2021

Based on the specification, the Server URL might be relative; and when consumer tries to customize the server list, this might block Swagger UI from rendering.

Use the following configuration to repro:

    internal class OpenApiConfigurationOptions : DefaultOpenApiConfigurationOptions
    {
        public override OpenApiInfo Info { get; set; } = new OpenApiInfo()
        {
            Title = "Some REST API",
            Version = "1.0.0",
        };

        public override OpenApiVersionType OpenApiVersion { get; set; } = OpenApiVersionType.V3;

        public override bool IncludeRequestingHostName { get; set; } = false;

        public override List<OpenApiServer> Servers { get; set; } = new List<OpenApiServer>()
        {
            new OpenApiServer()
            {
                Url = "/api",
            },
        };
    }

The Uri(string) constructor assumes that the URI string is absolute.

@yifanbian-msft
Copy link
Contributor Author

yifanbian-msft commented Dec 2, 2021

@Azure/azure-functions-team @Azure/dotnet-cda to review.

Copy link
Contributor

@justinyoo justinyoo left a comment

Choose a reason for hiding this comment

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

@yifanbian-msft Thanks for your PR! I found that there's another line 68 for you to take a look. Would you please find and update the line, please?

@yifanbian-msft
Copy link
Contributor Author

@justinyoo Actually, for the one on L68, it only gets executed when user didn't specify custom configuration, the this._baseUrl comes from baseUrl, which is an absolute one. It doesn't need UriKind to be specified.

@justinyoo justinyoo added enhancement New feature or request v1.1.0 labels Dec 22, 2021
Copy link
Contributor

@justinyoo justinyoo left a comment

Choose a reason for hiding this comment

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

I left a comment on your change. Could you take a look?

@@ -89,7 +89,7 @@ public ISwaggerUI AddServer(IHttpRequestDataObject req, string routePrefix, IOpe

this._baseUrl = servers.First().Url;

absolutePath = new Uri(this._baseUrl).AbsolutePath.TrimEnd('/');
absolutePath = new Uri(this._baseUrl, UriKind.RelativeOrAbsolute).AbsolutePath.TrimEnd('/');
Copy link
Contributor

Choose a reason for hiding this comment

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

I cloned your repo and run it on my local machine. Unfortunately, this doesn't render the Swagger UI properly. Would you please take a further look?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this! I just took a closer look at the code, maybe we just don't need to parse that URL? I have updated the code, could you take a look?

@yifanbian-msft yifanbian-msft marked this pull request as draft December 23, 2021 02:31
@yifanbian-msft yifanbian-msft marked this pull request as ready for review December 23, 2021 03:04
Copy link
Contributor

@justinyoo justinyoo left a comment

Choose a reason for hiding this comment

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

Thanks for the long wait! I found that the issue was actually on the other line. Could you take a look and make an appropriate change on your side?

Copy link
Contributor

@justinyoo justinyoo left a comment

Choose a reason for hiding this comment

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

Thanks for clarifying your change. They now make sense to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants