Changed IfCond to store the condition function and the compiled regular expression.

Updated ifCondition test to deep test all fields.
Changed NewComplexRule to not return a pointer.
Corrected panic detection in formatting.
Fixed failing test cases.
Fixed review bug for test.
Fixes bug caused by Replacer running on the regular expressions in IfMatcher. We also now compile regular expressions up front to detect errors.
Fixes rewrite bugs that come from formatting a rule as a string and failing with nil dereference caused by embedding Regexp pointer in a Rule. Re: Issue #1794

caddyhttp/httpserver/condition.go

@@ -53,59 +53,8 @@ const (
 	matchOp      = "match"
 )
 
-func operatorError(operator string) error {
-	return fmt.Errorf("Invalid operator %v", operator)
-}
-
 // ifCondition is a 'if' condition.
-type ifCondition func(string, string) bool
-
-var ifConditions = map[string]ifCondition{
-	isOp:         isFunc,
-	notOp:        notFunc,
-	hasOp:        hasFunc,
-	startsWithOp: startsWithFunc,
-	endsWithOp:   endsWithFunc,
-	matchOp:      matchFunc,
-}
-
-// isFunc is condition for Is operator.
-// It checks for equality.
-func isFunc(a, b string) bool {
-	return a == b
-}
-
-// notFunc is condition for Not operator.
-// It checks for inequality.
-func notFunc(a, b string) bool {
-	return !isFunc(a, b)
-}
-
-// hasFunc is condition for Has operator.
-// It checks if b is a substring of a.
-func hasFunc(a, b string) bool {
-	return strings.Contains(a, b)
-}
-
-// startsWithFunc is condition for StartsWith operator.
-// It checks if b is a prefix of a.
-func startsWithFunc(a, b string) bool {
-	return strings.HasPrefix(a, b)
-}
-
-// endsWithFunc is condition for EndsWith operator.
-// It checks if b is a suffix of a.
-func endsWithFunc(a, b string) bool {
-	return strings.HasSuffix(a, b)
-}
-
-// matchFunc is condition for Match operator.
-// It does regexp matching of a against pattern in b
-// and returns if they match.
-func matchFunc(a, b string) bool {
-	matched, _ := regexp.MatchString(b, a)
-	return matched
-}
+type ifFunc func(a, b string) bool
 
 // ifCond is statement for a IfMatcher condition.
 type ifCond struct {
@@ -113,40 +62,79 @@ type ifCond struct {
 	op  string
 	b   string
 	neg bool
+	rex *regexp.Regexp
+	f   ifFunc
 }
 
 // newIfCond creates a new If condition.
-func newIfCond(a, operator, b string) (ifCond, error) {
-	neg := false
-	if strings.HasPrefix(operator, "not_") {
-		neg = true
-		operator = operator[4:]
+func newIfCond(a, op, b string) (ifCond, error) {
+	i := ifCond{a: a, op: op, b: b}
+	if strings.HasPrefix(op, "not_") {
+		i.neg = true
+		i.op = op[4:]
 	}
-	if _, ok := ifConditions[operator]; !ok {
-		return ifCond{}, operatorError(operator)
+
+	switch i.op {
+	case isOp:
+		// It checks for equality.
+		i.f = i.isFunc
+	case notOp:
+		// It checks for inequality.
+		i.f = i.notFunc
+	case hasOp:
+		// It checks if b is a substring of a.
+		i.f = strings.Contains
+	case startsWithOp:
+		// It checks if b is a prefix of a.
+		i.f = strings.HasPrefix
+	case endsWithOp:
+		// It checks if b is a suffix of a.
+		i.f = strings.HasSuffix
+	case matchOp:
+		// It does regexp matching of a against pattern in b and returns if they match.
+		var err error
+		if i.rex, err = regexp.Compile(i.b); err != nil {
+			return ifCond{}, fmt.Errorf("Invalid regular expression: '%s', %v", i.b, err)
+		}
+		i.f = i.matchFunc
+	default:
+		return ifCond{}, fmt.Errorf("Invalid operator %v", i.op)
 	}
-	return ifCond{
-		a:   a,
-		op:  operator,
-		b:   b,
-		neg: neg,
-	}, nil
+
+	return i, nil
+}
+
+// isFunc is condition for Is operator.
+func (i ifCond) isFunc(a, b string) bool {
+	return a == b
+}
+
+// notFunc is condition for Not operator.
+func (i ifCond) notFunc(a, b string) bool {
+	return a != b
+}
+
+// matchFunc is condition for Match operator.
+func (i ifCond) matchFunc(a, b string) bool {
+	return i.rex.MatchString(a)
 }
 
 // True returns true if the condition is true and false otherwise.
 // If r is not nil, it replaces placeholders before comparison.
 func (i ifCond) True(r *http.Request) bool {
-	if c, ok := ifConditions[i.op]; ok {
+	if i.f != nil {
 		a, b := i.a, i.b
 		if r != nil {
 			replacer := NewReplacer(r, nil, "")
 			a = replacer.Replace(i.a)
-			b = replacer.Replace(i.b)
+			if i.op != matchOp {
+				b = replacer.Replace(i.b)
+			}
 		}
 		if i.neg {
-			return !c(a, b)
+			return !i.f(a, b)
 		}
-		return c(a, b)
+		return i.f(a, b)
 	}
 	return i.neg // false if not negated, true otherwise
 }

caddyhttp/httpserver/condition_test.go

@@ -2,8 +2,8 @@ package httpserver
 
 import (
 	"context"
-	"fmt"
 	"net/http"
+	"regexp"
 	"strings"
 	"testing"
 
@@ -14,66 +14,72 @@ func TestConditions(t *testing.T) {
 	tests := []struct {
 		condition string
 		isTrue    bool
+		shouldErr bool
 	}{
-		{"a is b", false},
-		{"a is a", true},
-		{"a not b", true},
-		{"a not a", false},
-		{"a has a", true},
-		{"a has b", false},
-		{"ba has b", true},
-		{"bab has b", true},
-		{"bab has bb", false},
-		{"a not_has a", false},
-		{"a not_has b", true},
-		{"ba not_has b", false},
-		{"bab not_has b", false},
-		{"bab not_has bb", true},
-		{"bab starts_with bb", false},
-		{"bab starts_with ba", true},
-		{"bab starts_with bab", true},
-		{"bab not_starts_with bb", true},
-		{"bab not_starts_with ba", false},
-		{"bab not_starts_with bab", false},
-		{"bab ends_with bb", false},
-		{"bab ends_with bab", true},
-		{"bab ends_with ab", true},
-		{"bab not_ends_with bb", true},
-		{"bab not_ends_with ab", false},
-		{"bab not_ends_with bab", false},
-		{"a match *", false},
-		{"a match a", true},
-		{"a match .*", true},
-		{"a match a.*", true},
-		{"a match b.*", false},
-		{"ba match b.*", true},
-		{"ba match b[a-z]", true},
-		{"b0 match b[a-z]", false},
-		{"b0a match b[a-z]", false},
-		{"b0a match b[a-z]+", false},
-		{"b0a match b[a-z0-9]+", true},
-		{"a not_match *", true},
-		{"a not_match a", false},
-		{"a not_match .*", false},
-		{"a not_match a.*", false},
-		{"a not_match b.*", true},
-		{"ba not_match b.*", false},
-		{"ba not_match b[a-z]", false},
-		{"b0 not_match b[a-z]", true},
-		{"b0a not_match b[a-z]", true},
-		{"b0a not_match b[a-z]+", true},
-		{"b0a not_match b[a-z0-9]+", false},
+		{"a is b", false, false},
+		{"a is a", true, false},
+		{"a not b", true, false},
+		{"a not a", false, false},
+		{"a has a", true, false},
+		{"a has b", false, false},
+		{"ba has b", true, false},
+		{"bab has b", true, false},
+		{"bab has bb", false, false},
+		{"a not_has a", false, false},
+		{"a not_has b", true, false},
+		{"ba not_has b", false, false},
+		{"bab not_has b", false, false},
+		{"bab not_has bb", true, false},
+		{"bab starts_with bb", false, false},
+		{"bab starts_with ba", true, false},
+		{"bab starts_with bab", true, false},
+		{"bab not_starts_with bb", true, false},
+		{"bab not_starts_with ba", false, false},
+		{"bab not_starts_with bab", false, false},
+		{"bab ends_with bb", false, false},
+		{"bab ends_with bab", true, false},
+		{"bab ends_with ab", true, false},
+		{"bab not_ends_with bb", true, false},
+		{"bab not_ends_with ab", false, false},
+		{"bab not_ends_with bab", false, false},
+		{"a match *", false, true},
+		{"a match a", true, false},
+		{"a match .*", true, false},
+		{"a match a.*", true, false},
+		{"a match b.*", false, false},
+		{"ba match b.*", true, false},
+		{"ba match b[a-z]", true, false},
+		{"b0 match b[a-z]", false, false},
+		{"b0a match b[a-z]", false, false},
+		{"b0a match b[a-z]+", false, false},
+		{"b0a match b[a-z0-9]+", true, false},
+		{"bac match b[a-z]{2}", true, false},
+		{"a not_match *", false, true},
+		{"a not_match a", false, false},
+		{"a not_match .*", false, false},
+		{"a not_match a.*", false, false},
+		{"a not_match b.*", true, false},
+		{"ba not_match b.*", false, false},
+		{"ba not_match b[a-z]", false, false},
+		{"b0 not_match b[a-z]", true, false},
+		{"b0a not_match b[a-z]", true, false},
+		{"b0a not_match b[a-z]+", true, false},
+		{"b0a not_match b[a-z0-9]+", false, false},
+		{"bac not_match b[a-z]{2}", false, false},
 	}
 
 	for i, test := range tests {
 		str := strings.Fields(test.condition)
 		ifCond, err := newIfCond(str[0], str[1], str[2])
 		if err != nil {
-			t.Error(err)
+			if !test.shouldErr {
+				t.Error(err)
+			}
+			continue
 		}
 		isTrue := ifCond.True(nil)
 		if isTrue != test.isTrue {
-			t.Errorf("Test %d: expected %v found %v", i, test.isTrue, isTrue)
+			t.Errorf("Test %d: '%s' expected %v found %v", i, test.condition, test.isTrue, isTrue)
 		}
 	}
 
@@ -192,6 +198,7 @@ func TestIfMatcher(t *testing.T) {
 }
 
 func TestSetupIfMatcher(t *testing.T) {
+	rex_b, _ := regexp.Compile("b")
 	tests := []struct {
 		input     string
 		shouldErr bool
@@ -201,7 +208,7 @@ func TestSetupIfMatcher(t *testing.T) {
 			if	a match b
 		 }`, false, IfMatcher{
 			ifs: []ifCond{
-				{a: "a", op: "match", b: "b", neg: false},
+				{a: "a", op: "match", b: "b", neg: false, rex: rex_b},
 			},
 		}},
 		{`test {
@@ -209,7 +216,7 @@ func TestSetupIfMatcher(t *testing.T) {
 			if_op or
 		 }`, false, IfMatcher{
 			ifs: []ifCond{
-				{a: "a", op: "match", b: "b", neg: false},
+				{a: "a", op: "match", b: "b", neg: false, rex: rex_b},
 			},
 			isOr: true,
 		}},
@@ -255,6 +262,7 @@ func TestSetupIfMatcher(t *testing.T) {
 	for i, test := range tests {
 		c := caddy.NewTestController("http", test.input)
 		c.Next()
+
 		matcher, err := SetupIfMatcher(c)
 		if err == nil && test.shouldErr {
 			t.Errorf("Test %d didn't error, but it should have", i)
@@ -263,15 +271,60 @@ func TestSetupIfMatcher(t *testing.T) {
 		} else if err != nil && test.shouldErr {
 			continue
 		}
-		if _, ok := matcher.(IfMatcher); !ok {
+
+		test_if, ok := matcher.(IfMatcher)
+		if !ok {
 			t.Error("RequestMatcher should be of type IfMatcher")
 		}
+
 		if err != nil {
 			t.Errorf("Expected no error, but got: %v", err)
 		}
-		if fmt.Sprint(matcher) != fmt.Sprint(test.expected) {
-			t.Errorf("Test %v: Expected %v, found %v", i,
-				fmt.Sprint(test.expected), fmt.Sprint(matcher))
+
+		if len(test_if.ifs) != len(test.expected.ifs) {
+			t.Errorf("Test %d: Expected %d ifConditions, found %v", i,
+				len(test.expected.ifs), len(test_if.ifs))
+		}
+
+		for j, if_c := range test_if.ifs {
+			expected_c := test.expected.ifs[j]
+
+			if if_c.a != expected_c.a {
+				t.Errorf("Test %d, ifCond %d: Expected A=%s, got %s",
+					i, j, if_c.a, expected_c.a)
+			}
+
+			if if_c.op != expected_c.op {
+				t.Errorf("Test %d, ifCond %d: Expected Op=%s, got %s",
+					i, j, if_c.op, expected_c.op)
+			}
+
+			if if_c.b != expected_c.b {
+				t.Errorf("Test %d, ifCond %d: Expected B=%s, got %s",
+					i, j, if_c.b, expected_c.b)
+			}
+
+			if if_c.neg != expected_c.neg {
+				t.Errorf("Test %d, ifCond %d: Expected Neg=%v, got %v",
+					i, j, if_c.neg, expected_c.neg)
+			}
+
+			if expected_c.rex != nil && if_c.rex == nil {
+				t.Errorf("Test %d, ifCond %d: Expected Rex=%v, got <nil>",
+					i, j, expected_c.rex)
+			}
+
+			if expected_c.rex == nil && if_c.rex != nil {
+				t.Errorf("Test %d, ifCond %d: Expected Rex=<nil>, got %v",
+					i, j, if_c.rex)
+			}
+
+			if expected_c.rex != nil && if_c.rex != nil {
+				if if_c.rex.String() != expected_c.rex.String() {
+					t.Errorf("Test %d, ifCond %d: Expected Rex=%v, got %v",
+						i, j, if_c.rex, expected_c.rex)
+				}
+			}
 		}
 	}
 }

caddyhttp/rewrite/rewrite.go

@@ -84,19 +84,19 @@ type ComplexRule struct {
 	// Request matcher
 	httpserver.RequestMatcher
 
-	*regexp.Regexp
+	Regexp *regexp.Regexp
 }
 
 // NewComplexRule creates a new RegexpRule. It returns an error if regexp
 // pattern (pattern) or extensions (ext) are invalid.
-func NewComplexRule(base, pattern, to string, ext []string, matcher httpserver.RequestMatcher) (*ComplexRule, error) {
+func NewComplexRule(base, pattern, to string, ext []string, matcher httpserver.RequestMatcher) (ComplexRule, error) {
 	// validate regexp if present
 	var r *regexp.Regexp
 	if pattern != "" {
 		var err error
 		r, err = regexp.Compile(pattern)
 		if err != nil {
-			return nil, err
+			return ComplexRule{}, err
 		}
 	}
 
@@ -105,7 +105,7 @@ func NewComplexRule(base, pattern, to string, ext []string, matcher httpserver.R
 		if len(v) < 2 || (len(v) < 3 && v[0] == '!') {
 			// check if no extension is specified
 			if v != "/" && v != "!/" {
-				return nil, fmt.Errorf("invalid extension %v", v)
+				return ComplexRule{}, fmt.Errorf("invalid extension %v", v)
 			}
 		}
 	}
@@ -118,7 +118,7 @@ func NewComplexRule(base, pattern, to string, ext []string, matcher httpserver.R
 		httpserver.PathMatcher(base),
 	)
 
-	return &ComplexRule{
+	return ComplexRule{
 		Base:           base,
 		To:             to,
 		Exts:           ext,
@@ -128,13 +128,13 @@ func NewComplexRule(base, pattern, to string, ext []string, matcher httpserver.R
 }
 
 // BasePath satisfies httpserver.Config
-func (r *ComplexRule) BasePath() string { return r.Base }
+func (r ComplexRule) BasePath() string { return r.Base }
 
 // Match satisfies httpserver.Config.
 //
 // Though ComplexRule embeds a RequestMatcher, additional
 // checks are needed which requires a custom implementation.
-func (r *ComplexRule) Match(req *http.Request) bool {
+func (r ComplexRule) Match(req *http.Request) bool {
 	// validate RequestMatcher
 	// includes if and path
 	if !r.RequestMatcher.Match(req) {
@@ -155,7 +155,7 @@ func (r *ComplexRule) Match(req *http.Request) bool {
 }
 
 // Rewrite rewrites the internal location of the current request.
-func (r *ComplexRule) Rewrite(fs http.FileSystem, req *http.Request) (re Result) {
+func (r ComplexRule) Rewrite(fs http.FileSystem, req *http.Request) (re Result) {
 	replacer := newReplacer(req)
 
 	// validate regexp if present
@@ -189,7 +189,7 @@ func (r *ComplexRule) Rewrite(fs http.FileSystem, req *http.Request) (re Result)
 
 // matchExt matches rPath against registered file extensions.
 // Returns true if a match is found and false otherwise.
-func (r *ComplexRule) matchExt(rPath string) bool {
+func (r ComplexRule) matchExt(rPath string) bool {
 	f := filepath.Base(rPath)
 	ext := path.Ext(f)
 	if ext == "" {
@@ -216,14 +216,14 @@ func (r *ComplexRule) matchExt(rPath string) bool {
 	return !mustUse
 }
 
-func (r *ComplexRule) regexpMatches(rPath string) []string {
+func (r ComplexRule) regexpMatches(rPath string) []string {
 	if r.Regexp != nil {
 		// include trailing slash in regexp if present
 		start := len(r.Base)
 		if strings.HasSuffix(r.Base, "/") {
 			start--
 		}
-		return r.FindStringSubmatch(rPath[start:])
+		return r.Regexp.FindStringSubmatch(rPath[start:])
 	}
 	return nil
 }

caddyhttp/rewrite/setup_test.go

@@ -3,6 +3,7 @@ package rewrite
 import (
 	"fmt"
 	"regexp"
+	"strings"
 	"testing"
 
 	"github.com/mholt/caddy"
@@ -97,14 +98,14 @@ func TestRewriteParse(t *testing.T) {
 			r	.*
 			to	/to /index.php?
 		 }`, false, []Rule{
-			&ComplexRule{Base: "/", To: "/to /index.php?", Regexp: regexp.MustCompile(".*")},
+			ComplexRule{Base: "/", To: "/to /index.php?", Regexp: regexp.MustCompile(".*")},
 		}},
 		{`rewrite {
 			regexp	.*
 			to		/to
 			ext		/ html txt
 		 }`, false, []Rule{
-			&ComplexRule{Base: "/", To: "/to", Exts: []string{"/", "html", "txt"}, Regexp: regexp.MustCompile(".*")},
+			ComplexRule{Base: "/", To: "/to", Exts: []string{"/", "html", "txt"}, Regexp: regexp.MustCompile(".*")},
 		}},
 		{`rewrite /path {
 			r	rr
@@ -115,27 +116,27 @@ func TestRewriteParse(t *testing.T) {
 		 	to 		/to /to2
 		 }
 		 `, false, []Rule{
-			&ComplexRule{Base: "/path", To: "/dest", Regexp: regexp.MustCompile("rr")},
-			&ComplexRule{Base: "/", To: "/to /to2", Regexp: regexp.MustCompile("[a-z]+")},
+			ComplexRule{Base: "/path", To: "/dest", Regexp: regexp.MustCompile("rr")},
+			ComplexRule{Base: "/", To: "/to /to2", Regexp: regexp.MustCompile("[a-z]+")},
 		}},
 		{`rewrite {
 			r	.*
 		 }`, true, []Rule{
-			&ComplexRule{},
+			ComplexRule{},
 		}},
 		{`rewrite {
 
 		 }`, true, []Rule{
-			&ComplexRule{},
+			ComplexRule{},
 		}},
 		{`rewrite /`, true, []Rule{
-			&ComplexRule{},
+			ComplexRule{},
 		}},
 		{`rewrite {
 			if {path} match /
 			to		/to
 		 }`, false, []Rule{
-			&ComplexRule{Base: "/", To: "/to"},
+			ComplexRule{Base: "/", To: "/to"},
 		}},
 	}
 
@@ -156,8 +157,8 @@ func TestRewriteParse(t *testing.T) {
 		}
 
 		for j, e := range test.expected {
-			actualRule := actual[j].(*ComplexRule)
-			expectedRule := e.(*ComplexRule)
+			actualRule := actual[j].(ComplexRule)
+			expectedRule := e.(ComplexRule)
 
 			if actualRule.Base != expectedRule.Base {
 				t.Errorf("Test %d, rule %d: Expected Base=%s, got %s",
@@ -175,13 +176,15 @@ func TestRewriteParse(t *testing.T) {
 			}
 
 			if actualRule.Regexp != nil {
-				if actualRule.String() != expectedRule.String() {
+				if actualRule.Regexp.String() != expectedRule.Regexp.String() {
 					t.Errorf("Test %d, rule %d: Expected Pattern=%s, got %s",
-						i, j, expectedRule.String(), actualRule.String())
+						i, j, actualRule.Regexp.String(), expectedRule.Regexp.String())
 				}
 			}
+		}
 
+		if rules_fmt := fmt.Sprintf("%v", actual); strings.HasPrefix(rules_fmt, "%!") {
+			t.Errorf("Test %d: Failed to string encode: %#v", i, rules_fmt)
 		}
 	}
-
 }