From ad42fedda5f8a1d398d5d9cf6114f85635b49998 Mon Sep 17 00:00:00 2001 From: Jonathan Amsterdam Date: Tue, 19 Sep 2023 07:54:09 -0400 Subject: [PATCH] net/http: explain why two patterns conflict It can be difficult to tell at a glance why two patterns conflict, so explain it with example paths. Change-Id: Ie384f0a4ef64f30e6e6898bce4b88027bc81034b Reviewed-on: https://go-review.googlesource.com/c/go/+/529122 Run-TryBot: Jonathan Amsterdam Reviewed-by: Damien Neil TryBot-Result: Gopher Robot --- src/net/http/pattern.go | 129 +++++++++++++++++++++++++++++++++++ src/net/http/pattern_test.go | 112 ++++++++++++++++++++++++++++++ src/net/http/server.go | 5 +- 3 files changed, 244 insertions(+), 2 deletions(-) diff --git a/src/net/http/pattern.go b/src/net/http/pattern.go index 6b9e535bee..eca179180f 100644 --- a/src/net/http/pattern.go +++ b/src/net/http/pattern.go @@ -388,3 +388,132 @@ func isLitOrSingle(seg segment) bool { } return seg.s != "/" } + +// describeConflict returns an explanation of why two patterns conflict. +func describeConflict(p1, p2 *pattern) string { + mrel := p1.compareMethods(p2) + prel := p1.comparePaths(p2) + rel := combineRelationships(mrel, prel) + if rel == equivalent { + return fmt.Sprintf("%s matches the same requests as %s", p1, p2) + } + if rel != overlaps { + panic("describeConflict called with non-conflicting patterns") + } + if prel == overlaps { + return fmt.Sprintf(`%[1]s and %[2]s both match some paths, like %[3]q. +But neither is more specific than the other. +%[1]s matches %[4]q, but %[2]s doesn't. +%[2]s matches %[5]q, but %[1]s doesn't.`, + p1, p2, commonPath(p1, p2), differencePath(p1, p2), differencePath(p2, p1)) + } + if mrel == moreGeneral && prel == moreSpecific { + return fmt.Sprintf("%s matches more methods than %s, but has a more specific path pattern", p1, p2) + } + if mrel == moreSpecific && prel == moreGeneral { + return fmt.Sprintf("%s matches fewer methods than %s, but has a more general path pattern", p1, p2) + } + return fmt.Sprintf("bug: unexpected way for two patterns %s and %s to conflict: methods %s, paths %s", p1, p2, mrel, prel) +} + +// writeMatchingPath writes to b a path that matches the segments. +func writeMatchingPath(b *strings.Builder, segs []segment) { + for _, s := range segs { + writeSegment(b, s) + } +} + +func writeSegment(b *strings.Builder, s segment) { + b.WriteByte('/') + if !s.multi && s.s != "/" { + b.WriteString(s.s) + } +} + +// commonPath returns a path that both p1 and p2 match. +// It assumes there is such a path. +func commonPath(p1, p2 *pattern) string { + var b strings.Builder + var segs1, segs2 []segment + for segs1, segs2 = p1.segments, p2.segments; len(segs1) > 0 && len(segs2) > 0; segs1, segs2 = segs1[1:], segs2[1:] { + if s1 := segs1[0]; s1.wild { + writeSegment(&b, segs2[0]) + } else { + writeSegment(&b, s1) + } + } + if len(segs1) > 0 { + writeMatchingPath(&b, segs1) + } else if len(segs2) > 0 { + writeMatchingPath(&b, segs2) + } + return b.String() +} + +// differencePath returns a path that p1 matches and p2 doesn't. +// It assumes there is such a path. +func differencePath(p1, p2 *pattern) string { + var b strings.Builder + + var segs1, segs2 []segment + for segs1, segs2 = p1.segments, p2.segments; len(segs1) > 0 && len(segs2) > 0; segs1, segs2 = segs1[1:], segs2[1:] { + s1 := segs1[0] + s2 := segs2[0] + if s1.multi && s2.multi { + // From here the patterns match the same paths, so we must have found a difference earlier. + b.WriteByte('/') + return b.String() + + } + if s1.multi && !s2.multi { + // s1 ends in a "..." wildcard but s2 does not. + // A trailing slash will distinguish them, unless s2 ends in "{$}", + // in which case any segment will do; prefer the wildcard name if + // it has one. + b.WriteByte('/') + if s2.s == "/" { + if s1.s != "" { + b.WriteString(s1.s) + } else { + b.WriteString("x") + } + } + return b.String() + } + if !s1.multi && s2.multi { + writeSegment(&b, s1) + } else if s1.wild && s2.wild { + // Both patterns will match whatever we put here; use + // the first wildcard name. + writeSegment(&b, s1) + } else if s1.wild && !s2.wild { + // s1 is a wildcard, s2 is a literal. + // Any segment other than s2.s will work. + // Prefer the wildcard name, but if it's the same as the literal, + // tweak the literal. + if s1.s != s2.s { + writeSegment(&b, s1) + } else { + b.WriteByte('/') + b.WriteString(s2.s + "x") + } + } else if !s1.wild && s2.wild { + writeSegment(&b, s1) + } else { + // Both are literals. A precondition of this function is that the + // patterns overlap, so they must be the same literal. Use it. + if s1.s != s2.s { + panic(fmt.Sprintf("literals differ: %q and %q", s1.s, s2.s)) + } + writeSegment(&b, s1) + } + } + if len(segs1) > 0 { + // p1 is longer than p2, and p2 does not end in a multi. + // Anything that matches the rest of p1 will do. + writeMatchingPath(&b, segs1) + } else if len(segs2) > 0 { + writeMatchingPath(&b, segs2) + } + return b.String() +} diff --git a/src/net/http/pattern_test.go b/src/net/http/pattern_test.go index 7c51897907..f67a2b5135 100644 --- a/src/net/http/pattern_test.go +++ b/src/net/http/pattern_test.go @@ -392,3 +392,115 @@ func TestConflictsWith(t *testing.T) { } } } + +func TestRegisterConflict(t *testing.T) { + mux := NewServeMux() + pat1 := "/a/{x}/" + if err := mux.registerErr(pat1, NotFoundHandler()); err != nil { + t.Fatal(err) + } + pat2 := "/a/{y}/{z...}" + err := mux.registerErr(pat2, NotFoundHandler()) + var got string + if err == nil { + got = "" + } else { + got = err.Error() + } + want := "matches the same requests as" + if !strings.Contains(got, want) { + t.Errorf("got\n%s\nwant\n%s", got, want) + } +} + +func TestDescribeConflict(t *testing.T) { + for _, test := range []struct { + p1, p2 string + want string + }{ + {"/a/{x}", "/a/{y}", "the same requests"}, + {"/", "/{m...}", "the same requests"}, + {"/a/{x}", "/{y}/b", "both match some paths"}, + {"/a", "GET /{x}", "matches more methods than GET /{x}, but has a more specific path pattern"}, + {"GET /a", "HEAD /", "matches more methods than HEAD /, but has a more specific path pattern"}, + {"POST /", "/a", "matches fewer methods than /a, but has a more general path pattern"}, + } { + got := describeConflict(mustParsePattern(t, test.p1), mustParsePattern(t, test.p2)) + if !strings.Contains(got, test.want) { + t.Errorf("%s vs. %s:\ngot:\n%s\nwhich does not contain %q", + test.p1, test.p2, got, test.want) + } + } +} + +func TestCommonPath(t *testing.T) { + for _, test := range []struct { + p1, p2 string + want string + }{ + {"/a/{x}", "/{x}/a", "/a/a"}, + {"/a/{z}/", "/{z}/a/", "/a/a/"}, + {"/a/{z}/{m...}", "/{z}/a/", "/a/a/"}, + {"/{z}/{$}", "/a/", "/a/"}, + {"/{z}/{$}", "/a/{x...}", "/a/"}, + {"/a/{z}/{$}", "/{z}/a/", "/a/a/"}, + {"/a/{x}/b/{y...}", "/{x}/c/{y...}", "/a/c/b/"}, + {"/a/{x}/b/", "/{x}/c/{y...}", "/a/c/b/"}, + {"/a/{x}/b/{$}", "/{x}/c/{y...}", "/a/c/b/"}, + {"/a/{z}/{x...}", "/{z}/b/{y...}", "/a/b/"}, + } { + pat1 := mustParsePattern(t, test.p1) + pat2 := mustParsePattern(t, test.p2) + if pat1.comparePaths(pat2) != overlaps { + t.Fatalf("%s does not overlap %s", test.p1, test.p2) + } + got := commonPath(pat1, pat2) + if got != test.want { + t.Errorf("%s vs. %s: got %q, want %q", test.p1, test.p2, got, test.want) + } + } +} + +func TestDifferencePath(t *testing.T) { + for _, test := range []struct { + p1, p2 string + want string + }{ + {"/a/{x}", "/{x}/a", "/a/x"}, + {"/{x}/a", "/a/{x}", "/x/a"}, + {"/a/{z}/", "/{z}/a/", "/a/z/"}, + {"/{z}/a/", "/a/{z}/", "/z/a/"}, + {"/{a}/a/", "/a/{z}/", "/ax/a/"}, + {"/a/{z}/{x...}", "/{z}/b/{y...}", "/a/z/"}, + {"/{z}/b/{y...}", "/a/{z}/{x...}", "/z/b/"}, + {"/a/b/", "/a/b/c", "/a/b/"}, + {"/a/b/{x...}", "/a/b/c", "/a/b/"}, + {"/a/b/{x...}", "/a/b/c/d", "/a/b/"}, + {"/a/b/{x...}", "/a/b/c/d/", "/a/b/"}, + {"/a/{z}/{m...}", "/{z}/a/", "/a/z/"}, + {"/{z}/a/", "/a/{z}/{m...}", "/z/a/"}, + {"/{z}/{$}", "/a/", "/z/"}, + {"/a/", "/{z}/{$}", "/a/x"}, + {"/{z}/{$}", "/a/{x...}", "/z/"}, + {"/a/{foo...}", "/{z}/{$}", "/a/foo"}, + {"/a/{z}/{$}", "/{z}/a/", "/a/z/"}, + {"/{z}/a/", "/a/{z}/{$}", "/z/a/x"}, + {"/a/{x}/b/{y...}", "/{x}/c/{y...}", "/a/x/b/"}, + {"/{x}/c/{y...}", "/a/{x}/b/{y...}", "/x/c/"}, + {"/a/{c}/b/", "/{x}/c/{y...}", "/a/cx/b/"}, + {"/{x}/c/{y...}", "/a/{c}/b/", "/x/c/"}, + {"/a/{x}/b/{$}", "/{x}/c/{y...}", "/a/x/b/"}, + {"/{x}/c/{y...}", "/a/{x}/b/{$}", "/x/c/"}, + } { + pat1 := mustParsePattern(t, test.p1) + pat2 := mustParsePattern(t, test.p2) + rel := pat1.comparePaths(pat2) + if rel != overlaps && rel != moreGeneral { + t.Fatalf("%s vs. %s are %s, need overlaps or moreGeneral", pat1, pat2, rel) + } + got := differencePath(pat1, pat2) + if got != test.want { + t.Errorf("%s vs. %s: got %q, want %q", test.p1, test.p2, got, test.want) + } + } +} diff --git a/src/net/http/server.go b/src/net/http/server.go index 629d8d3c62..b9f4a6b448 100644 --- a/src/net/http/server.go +++ b/src/net/http/server.go @@ -2655,8 +2655,9 @@ func (mux *ServeMux) registerErr(patstr string, handler Handler) error { // Check for conflict. if err := mux.index.possiblyConflictingPatterns(pat, func(pat2 *pattern) error { if pat.conflictsWith(pat2) { - return fmt.Errorf("pattern %q (registered at %s) conflicts with pattern %q (registered at %s)", - pat, pat.loc, pat2, pat2.loc) + d := describeConflict(pat, pat2) + return fmt.Errorf("pattern %q (registered at %s) conflicts with pattern %q (registered at %s):\n%s", + pat, pat.loc, pat2, pat2.loc, d) } return nil }); err != nil { -- 2.48.1