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

Add ability to map root of subgroup #25

Merged
merged 3 commits into from
Feb 17, 2016
Merged

Conversation

dbudworth
Copy link
Contributor

There was a bug in the initial implementation of subgroups where mapping the root of a subgroup ended up in adding the "add trailing slash" logic.

This code basically changes it so if you do subGroup.Handle("/",...) then we just use the subgroups original path.

This change maps:
router.NewGroup("/foo").GET("/") -> /foo (no trailing slash added)

before this change it did:
/foo (with add trailing slash enabled)

There was a subtle bug where

router.NewGroup("/foo").GET("/"), which should mean "/foo" was getting mapped with add trailing slash logic.

My usecase that caused uncovered this:
API := router.NewGroup("/api/v1")
posts := API.NewGroup("/post")
posts.POST("/",...)

This caused posts to "/api/v1/posts" to get redirected to "/api/v1/posts/" and, thus, not be found by the router.
Added a test case to ensure the functionality works as expected
Cleaned up code
@dimfeld
Copy link
Owner

dimfeld commented Feb 17, 2016

My only concern here is that this makes it impossible to actually define a '/' on a subgroup when you do want the trailing slash added.

Perhaps a better solution would be modify checkPath to allow passing in an empty string, changing it to check if len(path) > 0 && path[0] != '/'. Then, to get the behavior you desire, you could just call group.GET("", handler), while invalid calls such as group.GET("bar", handler) are still disallowed. What do you think?

@dbudworth
Copy link
Contributor Author

yep, that's probably a better idea

We are saying that to map the subgroup itself, we do:
NewGroup("/foo").GET("", handler)
and to get traling slash logic, we do:
NewGroup("/foo").GET("/", handler)

That gives you the option to do either with the only downside being that, I assume, the most common usage will be to pass an empty string which feels weird for an API.

That said, I don't really have an alternative to offer.

@dimfeld
Copy link
Owner

dimfeld commented Feb 17, 2016

Yeah, that's what I'm thinking. I agree it feels a little weird, but I also couldn't come up with a better alternative.

@dbudworth
Copy link
Contributor Author

ok, updated to reflect changes and added test cases for all 3 versions
NewGroup("/foo").GET("") + GET /foo results in 200
NewGroup("/foo").GET("/") + GET /foo results in 301 -> /foo/
NewGroup("/foo").GET("/") + GET /foo/ results in 200

dimfeld added a commit that referenced this pull request Feb 17, 2016
Add ability to map root of subgroup
@dimfeld dimfeld merged commit df19d8c into dimfeld:master Feb 17, 2016
@dimfeld
Copy link
Owner

dimfeld commented Feb 17, 2016

Thanks!

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.

None yet

2 participants