Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions context.go
Original file line number Diff line number Diff line change
Expand Up @@ -133,11 +133,12 @@ func (x *Context) RoutePattern() string {
return routePattern
}

// replaceWildcards takes a route pattern and recursively replaces all
// occurrences of "/*/" to "/".
// replaceWildcards takes a route pattern and replaces all occurrences of
// "/*/" with "/". It iteratively runs until no wildcards remain to
// correctly handle consecutive wildcards.
func replaceWildcards(p string) string {
if strings.Contains(p, "/*/") {
return replaceWildcards(strings.Replace(p, "/*/", "/", -1))
for strings.Contains(p, "/*/") {
p = strings.ReplaceAll(p, "/*/", "/")
Comment on lines +140 to +141
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can this ever run more than two times?

Do we need the for loop?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I used a loop because the original implementation was recursive — I assumed /*/ could appear multiple times, so I kept the behavior but replaced recursion with a loop

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@srpvpn I hear you, it makes sense.

Can you please find out if this needs to be done once, twice or many more times please? I'd rather remove the for loop unless necessary.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@VojtechVitek
ReplaceAll doesn’t handle overlapping patterns — for example, /// becomes /*/, so we need to run it multiple times.
Since paths can have several wildcards in a row, a loop ensures they're all collapsed. The test with three wildcards confirms this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry. My question was if if it ever overlaps more than two times, since you introduced ReplaceAll()?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@VojtechVitek
Docs say “ReplaceAll returns a copy of the string s with all non-overlapping instances of old replaced by new”.
Because the scan resumes after the piece it just wrote, any overlap is skipped:

/// --1st pass--> /*/ --2nd pass--> /

So if a router tacks on three-plus /* segments, one call isn’t enough.

}
return p
}
Expand Down
11 changes: 11 additions & 0 deletions context_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,3 +91,14 @@ func TestRoutePattern(t *testing.T) {
t.Fatalf("unexpected non-empty route pattern for nil context: %q", p)
}
}

// TestReplaceWildcardsConsecutive ensures multiple consecutive wildcards are
// collapsed into a single slash.
func TestReplaceWildcardsConsecutive(t *testing.T) {
if p := replaceWildcards("/foo/*/*/*/bar"); p != "/foo/bar" {
t.Fatalf("unexpected wildcard replacement: %s", p)
}
if p := replaceWildcards("/foo/*/*/*/bar/*"); p != "/foo/bar/*" {
t.Fatalf("unexpected trailing wildcard behavior: %s", p)
}
}
Loading