]> Cypherpunks repositories - gostls13.git/commitdiff
net/http: explain why two patterns conflict
authorJonathan Amsterdam <jba@google.com>
Tue, 19 Sep 2023 11:54:09 +0000 (07:54 -0400)
committerJonathan Amsterdam <jba@google.com>
Tue, 19 Sep 2023 18:35:44 +0000 (18:35 +0000)
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 <jba@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

src/net/http/pattern.go
src/net/http/pattern_test.go
src/net/http/server.go

index 6b9e535bee7a99638b328cd66d6a09dfae26a38f..eca179180faf81a1ae7f97fe841b88de204c9153 100644 (file)
@@ -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()
+}
index 7c5189790715160f6ee39f8012cbc37064f53547..f67a2b5135f422622a1e4cbab5baa4c9b94155d3 100644 (file)
@@ -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 = "<nil>"
+       } 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)
+               }
+       }
+}
index 629d8d3c627bb7556aa0ed87be2eb1358e19196e..b9f4a6b44844d2c9a0a6ade8a92cbe96cb1c22b8 100644 (file)
@@ -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 {