Skip to content

tree: collapse consecutive subrouter wildcards in Walk - #1240

Open
NgoQuocViet2001 wants to merge 1 commit into
go-chi:masterfrom
NgoQuocViet2001:fix-walk-consecutive-wildcards
Open

NgoQuocViet2001 wants to merge 1 commit into
go-chi:masterfrom
NgoQuocViet2001:fix-walk-consecutive-wildcards

Conversation

@NgoQuocViet2001

Copy link
Copy Markdown

Walk removes the internal "//" subrouter connector with a single strings.ReplaceAll, which misses overlapping matches. When a Route("/") or Mount("/") sits directly inside another subrouter (the layout in the TestRoutePattern comment), the joined path has "///" and one "//" survives. Walk then disagrees with RoutePattern(), r.Pattern and Mux.Find:

resources := chi.NewRouter()
resources.Route("/", func(r chi.Router) { r.Get("/{resource_id}", h) })
r := chi.NewRouter()
r.Route("/v1", func(r chi.Router) { r.Mount("/resources", resources) })
// Walk before: GET /v1/resources/*/{resource_id}
// Walk after:  GET /v1/resources/{resource_id}

This makes walk() use replaceWildcards, which RoutePattern() has used for this since #515, so the two agree. A trailing "/*" from a real catch-all is kept. See also #310.

  • Ran: go test ./... (root and middleware) passes; go vet . ./middleware and gofmt -l . are clean.
  • Checked: the new TestWalkConsecutiveSubrouterWildcards covers the layout above, Mount("/") inside Mount("/") (was "GET /*/x") and a trailing catch-all. All three cases fail on master and pass with this change. TestWalker output is unchanged.

Walk joined the parent route and pattern and removed "/*/" with a single
strings.ReplaceAll. When two subrouter wildcards are adjacent, for example
a Route("/") or Mount("/") directly inside another subrouter, the joined
path contains "/*/*/" and one "/*/" survives because ReplaceAll does not
match overlapping occurrences. Walk then reported paths such as
"/v1/resources/*/{resource_id}" while RoutePattern() gives
"/v1/resources/{resource_id}".

Reuse replaceWildcards, which RoutePattern() already uses, so the
connector is removed until none remain.

@agammann agammann left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The route pattern normalization now uses the same iterative helper as Context.RoutePattern, so adjacent subrouter connectors are removed while a terminal catch-all remains. I checked the three regression layouts in TestWalkConsecutiveSubrouterWildcards and ran both that test and the full root package on this head; both pass.

This branch has not been deployed

No deployments
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