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

[Bug]: SwaggerUIMiddleware is now internal #3144

Open
davesmits opened this issue Nov 13, 2024 · 8 comments · May be fixed by #3150
Open

[Bug]: SwaggerUIMiddleware is now internal #3144

davesmits opened this issue Nov 13, 2024 · 8 comments · May be fixed by #3150

Comments

@davesmits
Copy link

Describe the bug

With the release of Swashbuckle 7 SwaggerUIMiddleware became internal. This is blocking us from upgrade.

We have a multi tenant solution and to make swagger run a little bit different for each tenant we build a custom routing to end up the swagger ui. The url became /tenantname/swagger.

We achieved that by a Post Configure on the SwaggerUIOptions and have our own scoped middleware that passed this settings object to the SwaggerUIMiddleware. But now the middleware became internal this is no longer possible.

We are not really aware of other approaches that let us run multiple a swagger UI per tenant

Expected behavior

SwaggerUIMiddleware is public

Actual behavior

SwaggerUIMiddleware is internal

Steps to reproduce

No response

Exception(s) (if any)

No response

Swashbuckle.AspNetCore version

No response

.NET Version

No response

Anything else?

No response

@martincostello
Copy link
Collaborator

This isn't a bug as it was an intentional change as having it public made it harder to change implementation details in previous minor releases, and it had no virtual members so the need to use/derive from it seemed to be non-existent. Shipping a new major version also gave us the opportunity to make the breaking change.

However, I'm open to adding functionality/API that enables your use case of having multiple middlewares for different APIs.

Could you share some more details about exactly what it is that you do in your application with 6.x? That will give us more idea about how we could enable this in an easier to maintain way than the previous design.

@davesmits
Copy link
Author

davesmits commented Nov 14, 2024

Each tenant has its own OAuth authorization server and we wanted to provide useable swaggers to the users of different tenants.

So in order to make the Swagger UI be able to deal with this:

  • Wrapped the middleware in our own middleware so we could make it work for urls that had the follow pattern /[tenantname]/swagger
  • Injected some javascript that took the [tenantname] and based on that knew to find the right authorization server

(sorry for slow reply; I was just ready updating to .NET 9 and was 1 am here. Didn't expected a reply would came that quickly)

@martincostello
Copy link
Collaborator

Off the top of my head without trying it, it sounds like you could achieve the same thing with UseSwaggerUI() by providing a different options object for each URL combination you have (assuming it's something known in advance and isn't a "large" number) and changing the RoutePrefix:

string[] prefixes = ["foo", "bar"];
foreach (var prefix in prefixes)
{
    app.UseSwaggerUI(new() { RoutePrefix = prefix });
}

Would this work for you?

@davesmits
Copy link
Author

we have now about 5000 tenants where it needs to work? is that large? For me feels quiet some overhead in the number of middlewares running; but I have no idea on the impact of that

@martincostello
Copy link
Collaborator

5,000 does sound "large" - if this approach doesn't work, then I think the path forward would be to allow more configurability, rather than just re-expose the class.

It was made internal as it created issues when we wanted to change constructor parameters to add new dependencies, and I'd rather not reintroduce that problem. IMHO, the middleware class itself is an implementation detail.

@davesmits
Copy link
Author

davesmits commented Nov 14, 2024

Do you have an approach in mind? (maybe i can help implement it). Last night I tried to use RoutePrefix with a regex in it. But noticed that RoutePrefix is Escaped so that didnt work. And I am not sure about the implications to the rest of the code of this approach.

Another approach might be to extract the logic of the middleware and expose it as a Render Util class that external ones can use instead of the standard middleware?

@martincostello
Copy link
Collaborator

I was thinking about something like extracting the "is this a request I should respond to" logic here into its own callback:

var httpMethod = httpContext.Request.Method;
if (HttpMethods.IsGet(httpMethod))
{
var path = httpContext.Request.Path.Value;
// If the RoutePrefix is requested (with or without trailing slash), redirect to index URL
if (Regex.IsMatch(path, $"^/?{Regex.Escape(_options.RoutePrefix)}/?$", RegexOptions.IgnoreCase))
{
// Use relative redirect to support proxy environments
var relativeIndexUrl = string.IsNullOrEmpty(path) || path.EndsWith("/")
? "index.html"
: $"{path.Split('/').Last()}/index.html";
RespondWithRedirect(httpContext.Response, relativeIndexUrl);
return;
}
var match = Regex.Match(path, $"^/{Regex.Escape(_options.RoutePrefix)}/?(index.(html|js))$", RegexOptions.IgnoreCase);
if (match.Success)
{
await RespondWithFile(httpContext.Response, match.Groups[1].Value);
return;
}
}
await _staticFileMiddleware.Invoke(httpContext);
}

Then we could add a new options property that allows you to provide a custom implementation of that, which would then in theory mean you're effectively injecting the logic of your custom wrapping class into our middleware itself and the end result is the same as what you were doing with v6.

@davesmits
Copy link
Author

Yea think I got it. let me give it a try

@davesmits davesmits linked a pull request Nov 16, 2024 that will close this issue
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 a pull request may close this issue.

2 participants