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

The RouteCollector is unable to handle custom RouteSelectors #149

Merged

Conversation

jens-brimfors-wolt
Copy link
Contributor

@jens-brimfors-wolt jens-brimfors-wolt commented Oct 22, 2024

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.

@jens-brimfors-wolt jens-brimfors-wolt changed the title The RouteCollector is unable to handle a custom route-scoped plugins The RouteCollector is unable to handle custom RouteSelectors Oct 23, 2024
@jens-brimfors-wolt jens-brimfors-wolt marked this pull request as ready for review October 23, 2024 08:10
@SMILEY4
Copy link
Owner

SMILEY4 commented Oct 23, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

SMILEY4 commented Oct 26, 2024

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
Copy link
Contributor Author

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
Copy link
Owner

SMILEY4 commented Nov 4, 2024

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

@SMILEY4 SMILEY4 merged commit f1b489d into SMILEY4:develop Nov 4, 2024
2 checks passed
@jens-brimfors-wolt jens-brimfors-wolt deleted the handle_route-scoped_plugins branch November 6, 2024 15:04
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.

2 participants