SMILEY4 / ktor-swagger-ui

Kotlin Ktor plugin to generate OpenAPI and provide Swagger UI
Apache License 2.0
182 stars 33 forks source link

The RouteCollector is unable to handle custom RouteSelectors #149

Closed jens-brimfors-wolt closed 3 weeks ago

jens-brimfors-wolt commented 1 month ago

Problem

There is a ktor library, that has a RouteSelector, that is not public to a application. The Application gets the RouteSelector on the path, but is unable to specify it in the ignoredRouteSelectors, since it is not public.

In practice the RouteCollector#getPath func adds an extra / for the custom RouteSelector

Solution

Drop extra slashes no matter the source.

SMILEY4 commented 1 month ago

Hi, Thank you for the pr.

I think understand the problem, but maybe the underlying issue is not the trailing "/", but that it is not possible to add private routeSelectors to the "ignored"-set, i.e. if the "toString()" would return anything else than an empty string, this would then still cause a problem. Maybe it would make more sense to (also) be able to add the name of the classes to ignore as strings? This woulnd't be as clean as adding classes but it would be possible to also add private route-selectors.

I'm interested in hearing your thoughts though.

jens-brimfors-wolt commented 1 month ago

Hi, Thank you for the pr.

I think understand the problem, but maybe the underlying issue is not the trailing "/", but that it is not possible to add private routeSelectors to the "ignored"-set, i.e. if the "toString()" would return anything else than an empty string, this would then still cause a problem. Maybe it would make more sense to (also) be able to add the name of the classes to ignore as strings? This woulnd't be as clean as adding classes but it would be possible to also add private route-selectors.

I'm interested in hearing your thoughts though.

Thanks for taking the time to look at this 🙇

You have a good point there about non-empty toString output..

I've seen this practice in many other frameworks that work with class references, and I did go looking to see if you had built that already. It would make it possible to fix this when it happens. The downside is that it's not obvious that the user of this lib has to do something, or what is needed. I happened to stumble down this rabbit hole as a Swagger-evangelist at work, found some strange Swaggers and just dug way longer than I should have to end up here.

I think it makes sense to implement a String-based ignore in the short term, while we contemplate the possibility to add something more dev-ex-friendly in the long term.

As to how to implement the string-based version without breaking backwards compatability.. I would create a var ignoredRouteSelectorsClassNames Set<String> in addition to the ignoredRouteSelectors, to keep it simple, I would say.

jens-brimfors-wolt commented 1 month ago

I pushed an update with a naive implementation of having a string-version ignoredRouteSelectorsClassNames along-side the ignoredRouteSelectors, to see/share how that might look.

I always feel it's good to have something concrete to share/talk about =)

By no means see this as a push to have this swim-laned, or that you need to rush anything!

So when you have time, and will, I'd be happy if you gave this some thought <3

SMILEY4 commented 1 month ago

Thanks, i think it looks good. I'm not sure if there is a simpler solution to the problem of which route selector to show and which ones to ignore, but i think this should work for all (?) situations now.

jens-brimfors-wolt commented 3 weeks ago

Thanks, i think it looks good. I'm not sure if there is a simpler solution to the problem of which route selector to show and which ones to ignore, but i think this should work for all (?) situations now.

Thanks @SMILEY4

I've been away for a while. And now I'm back ^^ Do you think this is a good enough to release, or should be annotate it as experimental or something?

SMILEY4 commented 3 weeks ago

I think its good to release. I'll merge it and release it soon 👍