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

use chi router instead of ShiftPath #1986

Closed
butonic opened this issue Aug 11, 2021 · 4 comments · Fixed by #2006
Closed

use chi router instead of ShiftPath #1986

butonic opened this issue Aug 11, 2021 · 4 comments · Fixed by #2006

Comments

@butonic
Copy link
Contributor

butonic commented Aug 11, 2021

I stumbled over https://benhoyt.com/writings/go-routing/ which examines the different ways routing can be done in golang.

IMO implementing the ocs / ocdav and other http endpoints with the ShiftPath approach makes it hard to understand what code is going to be executed for a given URL. We are using chi in some ocis services, eg the graph and our ocs implementation. We plan to fully move the latter into reva. An overview of what only the additional routing part looks like with shift path can be found in https://github.com/cs3org/reva/pull/1969/files#diff-6462df320ad761b651f59bdd74b3bd9123b0630538b7af253378a039a822b7f6R72-R151.

For readability (and performance) I'd vote to switch to chi. It has no further dependencies, supports quite a few middlewares and wrapping things like the opentelemetry api is easy.

But I'd like to get an opinion from @labkode @ishank011 @refs @C0rby and @rhafer before moving the user provisioning part from ocis to reva. We could make the switch as part of that work as it will touch a lot of the code anyway.

@refs
Copy link
Member

refs commented Aug 14, 2021

I believe the most compelling arguments to adopt chi instead of a trim-based approach are readability (already mentioned) and navigation. When browsing the codebase I rely on + f and a combination of the endpoint I'm trying to access and case "<slug>". I always find what I look for but 90% of the times that is just an entry point and I have to navigate my way down.

With a router, finding an endpoint is trivial as they would all be in the same location, and so will their handlers. A single unidimensional location with little need to traverse more files to find the function you're trying to fix.

It would also make adding endpoints more trivial. Right now we have clear defined endpoints, but in the future, as more functionality is added, adding new routes means touching n files, where n is potentially as many parts the path has (i.e: /remote.php/webdav/spaces/list) -> case "spaces" -> case "list" inside the case "spaces" handler and so on, sort of like a russian doll problem.

I would however be interested in the performance gain / loss of using chi as opposed to the current routing, I wouldn't adventure to say chi is faster than this because I have not had a look at their code base.

All weighed in, I think bringing Chi to the mix would be a nice addition, with no dependencies.

@labkode
Copy link
Member

labkode commented Aug 16, 2021

@butonic can you prepare a PR for that, let's start with a small service to see how it behaves.

@rhafer
Copy link
Contributor

rhafer commented Aug 17, 2021

can you prepare a PR for that, let's start with a small service to see how it behaves.

I am working on that.

@labkode
Copy link
Member

labkode commented Aug 17, 2021

@rhafer awesome!

rhafer added a commit to rhafer/reva that referenced this issue Aug 18, 2021
This should help with code readability and make it easier to figure out
which code is executed for which URL.

Closes cs3org#1986
rhafer added a commit to rhafer/reva that referenced this issue Aug 20, 2021
This should help with code readability and make it easier to figure out
which code is executed for which URL.

Closes cs3org#1986
rhafer added a commit to rhafer/reva that referenced this issue Aug 20, 2021
This should help with code readability and make it easier to figure out
which code is executed for which URL.

Closes cs3org#1986
rhafer added a commit to rhafer/reva that referenced this issue Aug 20, 2021
This should help with code readability and make it easier to figure out
which code is executed for which URL.

Closes cs3org#1986
rhafer added a commit to rhafer/reva that referenced this issue Aug 24, 2021
This should help with code readability and make it easier to figure out
which code is executed for which URL.

Closes cs3org#1986
rhafer added a commit to rhafer/reva that referenced this issue Aug 30, 2021
This should help with code readability and make it easier to figure out
which code is executed for which URL.

Closes cs3org#1986
rhafer added a commit to rhafer/reva that referenced this issue Aug 30, 2021
This should help with code readability and make it easier to figure out
which code is executed for which URL.

Closes cs3org#1986
rhafer added a commit to rhafer/reva that referenced this issue Aug 31, 2021
This should help with code readability and make it easier to figure out
which code is executed for which URL.

Closes cs3org#1986
rhafer added a commit to rhafer/reva that referenced this issue Sep 1, 2021
This should help with code readability and make it easier to figure out
which code is executed for which URL.

Closes cs3org#1986
labkode pushed a commit that referenced this issue Sep 1, 2021
This should help with code readability and make it easier to figure out
which code is executed for which URL.

Closes #1986
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.

4 participants