From ed62916d92db7da93d2f6ad67b9fd85547ebc7f1 Mon Sep 17 00:00:00 2001 From: Tit Petric Date: Thu, 24 Aug 2023 14:53:00 +0200 Subject: [PATCH 1/5] Optimize allocations in newRouteRegexp --- regexp.go | 53 +++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 37 insertions(+), 16 deletions(-) diff --git a/regexp.go b/regexp.go index 8a1ed86e..081aa9e8 100644 --- a/regexp.go +++ b/regexp.go @@ -65,35 +65,48 @@ func newRouteRegexp(tpl string, typ regexpType, options routeRegexpOptions) (*ro } varsN := make([]string, len(idxs)/2) varsR := make([]*regexp.Regexp, len(idxs)/2) - pattern := bytes.NewBufferString("") + + var pattern, reverse strings.Builder pattern.WriteByte('^') - reverse := bytes.NewBufferString("") - var end int + + var end, colonIdx, groupIdx int var err error + var patt, param, name string for i := 0; i < len(idxs); i += 2 { // Set all values we are interested in. + groupIdx = i/2 + raw := tpl[end:idxs[i]] end = idxs[i+1] - parts := strings.SplitN(tpl[idxs[i]+1:end-1], ":", 2) - name := parts[0] - patt := defaultPattern - if len(parts) == 2 { - patt = parts[1] + + param = tpl[idxs[i]+1 : end-1] + colonIdx = strings.Index(param, ":") + if colonIdx == -1 { + name = param + patt = defaultPattern + } else { + name = param[0:colonIdx] + patt = param[colonIdx+1:] + } + if patt == "" { + patt = defaultPattern } + // Name or pattern can't be empty. if name == "" || patt == "" { - return nil, fmt.Errorf("mux: missing name or pattern in %q", - tpl[idxs[i]:end]) + return nil, fmt.Errorf("mux: missing name or pattern in %q", param) } // Build the regexp pattern. - fmt.Fprintf(pattern, "%s(?P<%s>%s)", regexp.QuoteMeta(raw), varGroupName(i/2), patt) + groupName := varGroupName(groupIdx) + + pattern.WriteString(regexp.QuoteMeta(raw)+"(?P<" + groupName + ">" + patt + ")") // Build the reverse template. - fmt.Fprintf(reverse, "%s%%s", raw) + reverse.WriteString(raw+"%s") // Append variable name and compiled pattern. - varsN[i/2] = name - varsR[i/2], err = RegexpCompileFunc(fmt.Sprintf("^%s$", patt)) + varsN[groupIdx] = name + varsR[groupIdx], err = RegexpCompileFunc("^" + patt + "$") if err != nil { return nil, err } @@ -101,31 +114,39 @@ func newRouteRegexp(tpl string, typ regexpType, options routeRegexpOptions) (*ro // Add the remaining. raw := tpl[end:] pattern.WriteString(regexp.QuoteMeta(raw)) + if options.strictSlash { pattern.WriteString("[/]?") } + if typ == regexpTypeQuery { // Add the default pattern if the query value is empty if queryVal := strings.SplitN(template, "=", 2)[1]; queryVal == "" { pattern.WriteString(defaultPattern) } } + if typ != regexpTypePrefix { pattern.WriteByte('$') } + patternStr := pattern.String() + var wildcardHostPort bool if typ == regexpTypeHost { - if !strings.Contains(pattern.String(), ":") { + if !strings.Contains(patternStr, ":") { wildcardHostPort = true } } + reverse.WriteString(raw) + if endSlash { reverse.WriteByte('/') } + // Compile full regexp. - reg, errCompile := RegexpCompileFunc(pattern.String()) + reg, errCompile := RegexpCompileFunc(patternStr) if errCompile != nil { return nil, errCompile } From eaec0e96630d59efb7e28cdf129f97139646e9f2 Mon Sep 17 00:00:00 2001 From: Tit Petric Date: Tue, 14 May 2024 14:50:01 +0200 Subject: [PATCH 2/5] Print full param, add test to cover error expectations --- regexp.go | 11 ++++++----- regexp_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 5 deletions(-) diff --git a/regexp.go b/regexp.go index 081aa9e8..a3906ea6 100644 --- a/regexp.go +++ b/regexp.go @@ -74,10 +74,11 @@ func newRouteRegexp(tpl string, typ regexpType, options routeRegexpOptions) (*ro var patt, param, name string for i := 0; i < len(idxs); i += 2 { // Set all values we are interested in. - groupIdx = i/2 + groupIdx = i / 2 raw := tpl[end:idxs[i]] end = idxs[i+1] + tag := tpl[idxs[i]:end] param = tpl[idxs[i]+1 : end-1] colonIdx = strings.Index(param, ":") @@ -94,21 +95,21 @@ func newRouteRegexp(tpl string, typ regexpType, options routeRegexpOptions) (*ro // Name or pattern can't be empty. if name == "" || patt == "" { - return nil, fmt.Errorf("mux: missing name or pattern in %q", param) + return nil, fmt.Errorf("mux: missing name or pattern in %q", tag) } // Build the regexp pattern. groupName := varGroupName(groupIdx) - pattern.WriteString(regexp.QuoteMeta(raw)+"(?P<" + groupName + ">" + patt + ")") + pattern.WriteString(regexp.QuoteMeta(raw) + "(?P<" + groupName + ">" + patt + ")") // Build the reverse template. - reverse.WriteString(raw+"%s") + reverse.WriteString(raw + "%s") // Append variable name and compiled pattern. varsN[groupIdx] = name varsR[groupIdx], err = RegexpCompileFunc("^" + patt + "$") if err != nil { - return nil, err + return nil, fmt.Errorf("mux: error compiling regex for %q: %w", tag, err) } } // Add the remaining. diff --git a/regexp_test.go b/regexp_test.go index d7518f3e..6813e170 100644 --- a/regexp_test.go +++ b/regexp_test.go @@ -4,9 +4,33 @@ import ( "net/url" "reflect" "strconv" + "strings" "testing" ) +func Test_newRouteRegexp_Errors(t *testing.T) { + tests := []struct { + in, out string + }{ + {"/{}", `mux: missing name or pattern in "{}"`}, + {"/{id:abc(}", `mux: error compiling regex for "{id:abc(}":`}, + } + + for _, tc := range tests { + t.Run("Test case for "+tc.in, func(t *testing.T) { + _, err := newRouteRegexp(tc.in, 0, routeRegexpOptions{}) + if err != nil { + if strings.HasPrefix(err.Error(), tc.out) { + return + } + t.Errorf("Resulting error does not contain %q as expected, error: %s", tc.out, err) + } else { + t.Error("Expected error, got nil") + } + }) + } +} + func Test_findFirstQueryKey(t *testing.T) { tests := []string{ "a=1&b=2", From 6304463140deb7ac26130a280637302192d04c52 Mon Sep 17 00:00:00 2001 From: Tit Petric Date: Tue, 14 May 2024 15:00:02 +0200 Subject: [PATCH 3/5] Fix slight behaviour change --- regexp.go | 7 +++---- regexp_test.go | 2 ++ 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/regexp.go b/regexp.go index a3906ea6..4b0051d2 100644 --- a/regexp.go +++ b/regexp.go @@ -80,7 +80,9 @@ func newRouteRegexp(tpl string, typ regexpType, options routeRegexpOptions) (*ro end = idxs[i+1] tag := tpl[idxs[i]:end] - param = tpl[idxs[i]+1 : end-1] + // trim braces from tag + param = tag[1 : len(tag)-1] + colonIdx = strings.Index(param, ":") if colonIdx == -1 { name = param @@ -89,9 +91,6 @@ func newRouteRegexp(tpl string, typ regexpType, options routeRegexpOptions) (*ro name = param[0:colonIdx] patt = param[colonIdx+1:] } - if patt == "" { - patt = defaultPattern - } // Name or pattern can't be empty. if name == "" || patt == "" { diff --git a/regexp_test.go b/regexp_test.go index 6813e170..f7fce81c 100644 --- a/regexp_test.go +++ b/regexp_test.go @@ -13,6 +13,8 @@ func Test_newRouteRegexp_Errors(t *testing.T) { in, out string }{ {"/{}", `mux: missing name or pattern in "{}"`}, + {"/{:.}", `mux: missing name or pattern in "{:.}"`}, + {"/{a:}", `mux: missing name or pattern in "{a:}"`}, {"/{id:abc(}", `mux: error compiling regex for "{id:abc(}":`}, } From cca81cba2d03bb86c11fd34dc47009a99200409d Mon Sep 17 00:00:00 2001 From: Tit Petric Date: Tue, 14 May 2024 15:03:02 +0200 Subject: [PATCH 4/5] Revert some whitespace for smaller change --- regexp.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/regexp.go b/regexp.go index 4b0051d2..ffa7a753 100644 --- a/regexp.go +++ b/regexp.go @@ -114,18 +114,15 @@ func newRouteRegexp(tpl string, typ regexpType, options routeRegexpOptions) (*ro // Add the remaining. raw := tpl[end:] pattern.WriteString(regexp.QuoteMeta(raw)) - if options.strictSlash { pattern.WriteString("[/]?") } - if typ == regexpTypeQuery { // Add the default pattern if the query value is empty if queryVal := strings.SplitN(template, "=", 2)[1]; queryVal == "" { pattern.WriteString(defaultPattern) } } - if typ != regexpTypePrefix { pattern.WriteByte('$') } @@ -138,9 +135,7 @@ func newRouteRegexp(tpl string, typ regexpType, options routeRegexpOptions) (*ro wildcardHostPort = true } } - reverse.WriteString(raw) - if endSlash { reverse.WriteByte('/') } From b684b385378110d4dea7a16ad4653cb9a4476f65 Mon Sep 17 00:00:00 2001 From: Tit Petric Date: Tue, 14 May 2024 15:20:12 +0200 Subject: [PATCH 5/5] Move regexp compilation up for early error return --- regexp.go | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/regexp.go b/regexp.go index ffa7a753..e0bcff6a 100644 --- a/regexp.go +++ b/regexp.go @@ -127,7 +127,18 @@ func newRouteRegexp(tpl string, typ regexpType, options routeRegexpOptions) (*ro pattern.WriteByte('$') } + // Compile full regexp. patternStr := pattern.String() + reg, errCompile := RegexpCompileFunc(patternStr) + if errCompile != nil { + return nil, errCompile + } + + // Check for capturing groups which used to work in older versions + if reg.NumSubexp() != len(idxs)/2 { + panic(fmt.Sprintf("route %s contains capture groups in its regexp. ", template) + + "Only non-capturing groups are accepted: e.g. (?:pattern) instead of (pattern)") + } var wildcardHostPort bool if typ == regexpTypeHost { @@ -140,18 +151,6 @@ func newRouteRegexp(tpl string, typ regexpType, options routeRegexpOptions) (*ro reverse.WriteByte('/') } - // Compile full regexp. - reg, errCompile := RegexpCompileFunc(patternStr) - if errCompile != nil { - return nil, errCompile - } - - // Check for capturing groups which used to work in older versions - if reg.NumSubexp() != len(idxs)/2 { - panic(fmt.Sprintf("route %s contains capture groups in its regexp. ", template) + - "Only non-capturing groups are accepted: e.g. (?:pattern) instead of (pattern)") - } - // Done! return &routeRegexp{ template: template,