Merge pull request #981 from tw4452852/redir

redir: loading block arguments before parsing matcher

caddyhttp/redirect/setup.go

@@ -33,15 +33,49 @@ func redirParse(c *caddy.Controller) ([]Rule, error) {
 
 	cfg := httpserver.GetConfig(c)
 
-	// setRedirCode sets the redirect code for rule if it can, or returns an error
-	setRedirCode := func(code string, rule *Rule) error {
+	initRule := func(rule *Rule, defaultCode string, args []string) error {
+		if cfg.TLS.Enabled {
+			rule.FromScheme = "https"
+		} else {
+			rule.FromScheme = "http"
+		}
+
+		var (
+			from = "/"
+			to   string
+			code = defaultCode
+		)
+		switch len(args) {
+		case 1:
+			// To specified (catch-all redirect)
+			// Not sure why user is doing this in a table, as it causes all other redirects to be ignored.
+			// As such, this feature remains undocumented.
+			to = args[0]
+		case 2:
+			// From and To specified
+			from = args[0]
+			to = args[1]
+		case 3:
+			// From, To, and Code specified
+			from = args[0]
+			to = args[1]
+			code = args[2]
+		default:
+			return c.ArgErr()
+		}
+
+		rule.FromPath = from
+		rule.To = to
 		if code == "meta" {
 			rule.Meta = true
-		} else if codeNumber, ok := httpRedirs[code]; ok {
+			code = defaultCode
+		}
+		if codeNumber, ok := httpRedirs[code]; ok {
 			rule.Code = codeNumber
 		} else {
 			return c.Errf("Invalid redirect code '%v'", code)
 		}
+
 		return nil
 	}
 
@@ -62,12 +96,14 @@ func redirParse(c *caddy.Controller) ([]Rule, error) {
 		return nil
 	}
 
+	const initDefaultCode = "301"
+
 	for c.Next() {
+		args := c.RemainingArgs()
 		matcher, err := httpserver.SetupIfMatcher(c)
 		if err != nil {
 			return nil, err
 		}
-		args := c.RemainingArgs()
 
 		var hadOptionalBlock bool
 		for c.NextBlock() {
@@ -77,103 +113,40 @@ func redirParse(c *caddy.Controller) ([]Rule, error) {
 
 			hadOptionalBlock = true
 
-			var rule = Rule{
+			rule := Rule{
 				RequestMatcher: matcher,
 			}
 
-			if cfg.TLS.Enabled {
-				rule.FromScheme = "https"
-			} else {
-				rule.FromScheme = "http"
-			}
-
+			defaultCode := initDefaultCode
 			// Set initial redirect code
-			// BUG: If the code is specified for a whole block and that code is invalid,
-			// the line number will appear on the first line inside the block, even if that
-			// line overwrites the block-level code with a valid redirect code. The program
-			// still functions correctly, but the line number in the error reporting is
-			// misleading to the user.
 			if len(args) == 1 {
-				err := setRedirCode(args[0], &rule)
-				if err != nil {
-					return redirects, err
-				}
-			} else {
-				rule.Code = http.StatusMovedPermanently // default code
+				defaultCode = args[0]
 			}
 
 			// RemainingArgs only gets the values after the current token, but in our
 			// case we want to include the current token to get an accurate count.
 			insideArgs := append([]string{c.Val()}, c.RemainingArgs()...)
-
-			switch len(insideArgs) {
-			case 1:
-				// To specified (catch-all redirect)
-				// Not sure why user is doing this in a table, as it causes all other redirects to be ignored.
-				// As such, this feature remains undocumented.
-				rule.FromPath = "/"
-				rule.To = insideArgs[0]
-			case 2:
-				// From and To specified
-				rule.FromPath = insideArgs[0]
-				rule.To = insideArgs[1]
-			case 3:
-				// From, To, and Code specified
-				rule.FromPath = insideArgs[0]
-				rule.To = insideArgs[1]
-				err := setRedirCode(insideArgs[2], &rule)
-				if err != nil {
-					return redirects, err
-				}
-			default:
-				return redirects, c.ArgErr()
+			err := initRule(&rule, defaultCode, insideArgs)
+			if err != nil {
+				return redirects, err
 			}
 
-			err := checkAndSaveRule(rule)
+			err = checkAndSaveRule(rule)
 			if err != nil {
 				return redirects, err
 			}
 		}
 
 		if !hadOptionalBlock {
-			var rule = Rule{
+			rule := Rule{
 				RequestMatcher: matcher,
 			}
-
-			if cfg.TLS.Enabled {
-				rule.FromScheme = "https"
-			} else {
-				rule.FromScheme = "http"
-			}
-
-			rule.Code = http.StatusMovedPermanently // default
-
-			switch len(args) {
-			case 1:
-				// To specified (catch-all redirect)
-				rule.FromPath = "/"
-				rule.To = args[0]
-			case 2:
-				// To and Code specified (catch-all redirect)
-				rule.FromPath = "/"
-				rule.To = args[0]
-				err := setRedirCode(args[1], &rule)
-				if err != nil {
-					return redirects, err
-				}
-			case 3:
-				// From, To, and Code specified
-				rule.FromPath = args[0]
-				rule.To = args[1]
-				err := setRedirCode(args[2], &rule)
-				if err != nil {
-					return redirects, err
-				}
-			default:
-				return redirects, c.ArgErr()
+			err := initRule(&rule, initDefaultCode, args)
+			if err != nil {
+				return redirects, err
 			}
 
-			err := checkAndSaveRule(rule)
+			err = checkAndSaveRule(rule)
 			if err != nil {
 				return redirects, err
 			}

caddyhttp/redirect/setup_test.go

 package redirect
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/mholt/caddy"
@@ -15,7 +16,7 @@ func TestSetup(t *testing.T) {
 		expectedRules []Rule
 	}{
 		// test case #0 tests the recognition of a valid HTTP status code defined outside of block statement
-		{"redir 300 {\n/ /foo\n}", false, []Rule{{FromPath: "/", To: "/foo", Code: 300}}},
+		{"redir 300 {\n/ /foo\n}", false, []Rule{{FromPath: "/", To: "/foo", Code: 300, RequestMatcher: httpserver.IfMatcher{}}}},
 
 		// test case #1 tests the recognition of an invalid HTTP status code defined outside of block statement
 		{"redir 9000 {\n/ /foo\n}", true, []Rule{{}}},
@@ -27,22 +28,43 @@ func TestSetup(t *testing.T) {
 		{"redir 9000 {\n/ /foo 300\n}", true, []Rule{{}}},
 
 		// test case #4 tests the recognition of a TO redirection in a block statement.The HTTP status code is set to the default of 301 - MovedPermanently
-		{"redir 302 {\n/foo\n}", false, []Rule{{FromPath: "/", To: "/foo", Code: 302}}},
+		{"redir 302 {\n/foo\n}", false, []Rule{{FromPath: "/", To: "/foo", Code: 302, RequestMatcher: httpserver.IfMatcher{}}}},
 
 		// test case #5 tests the recognition of a TO and From redirection in a block statement
-		{"redir {\n/bar /foo 303\n}", false, []Rule{{FromPath: "/bar", To: "/foo", Code: 303}}},
+		{"redir {\n/bar /foo 303\n}", false, []Rule{{FromPath: "/bar", To: "/foo", Code: 303, RequestMatcher: httpserver.IfMatcher{}}}},
 
 		// test case #6 tests the recognition of a TO redirection in a non-block statement. The HTTP status code is set to the default of 301 - MovedPermanently
-		{"redir /foo", false, []Rule{{FromPath: "/", To: "/foo", Code: 301}}},
+		{"redir /foo", false, []Rule{{FromPath: "/", To: "/foo", Code: 301, RequestMatcher: httpserver.IfMatcher{}}}},
 
 		// test case #7 tests the recognition of a TO and From redirection in a non-block statement
-		{"redir /bar /foo 303", false, []Rule{{FromPath: "/bar", To: "/foo", Code: 303}}},
+		{"redir /bar /foo 303", false, []Rule{{FromPath: "/bar", To: "/foo", Code: 303, RequestMatcher: httpserver.IfMatcher{}}}},
 
 		// test case #8 tests the recognition of multiple redirections
-		{"redir {\n / /foo 304 \n} \n redir {\n /bar /foobar 305 \n}", false, []Rule{{FromPath: "/", To: "/foo", Code: 304}, {FromPath: "/bar", To: "/foobar", Code: 305}}},
+		{"redir {\n / /foo 304 \n} \n redir {\n /bar /foobar 305 \n}", false,
+			[]Rule{{FromPath: "/", To: "/foo", Code: 304, RequestMatcher: httpserver.IfMatcher{}},
+				{FromPath: "/bar", To: "/foobar", Code: 305, RequestMatcher: httpserver.IfMatcher{}}}},
 
 		// test case #9 tests the detection of duplicate redirections
 		{"redir {\n /bar /foo 304 \n} redir {\n /bar /foo 304 \n}", true, []Rule{{}}},
+
+		// test case #10 tests the detection of a valid HTTP status code outside of a block statement being overridden by an valid HTTP status code inside statement of a block statement
+		{"redir 300 {\n/ /foo 301\n}", false, []Rule{{FromPath: "/", To: "/foo", Code: 301, RequestMatcher: httpserver.IfMatcher{}}}},
+
+		// test case #11 tests the recognition of a matcher
+		{"redir {\n if {port} is 80\n/ /foo\n}", false, []Rule{{FromPath: "/", To: "/foo", Code: 301,
+			RequestMatcher: func() httpserver.IfMatcher {
+				c := caddy.NewTestController("http", "{\n if {port} is 80\n}")
+				matcher, _ := httpserver.SetupIfMatcher(c)
+				return matcher.(httpserver.IfMatcher)
+			}()}}},
+
+		// test case #12 tests the detection of a valid HTTP status code outside of a block statement with a matcher
+		{"redir 300 {\n if {port} is 80\n/ /foo\n}", false, []Rule{{FromPath: "/", To: "/foo", Code: 300,
+			RequestMatcher: func() httpserver.IfMatcher {
+				c := caddy.NewTestController("http", "{\n if {port} is 80\n}")
+				matcher, _ := httpserver.SetupIfMatcher(c)
+				return matcher.(httpserver.IfMatcher)
+			}()}}},
 	} {
 		c := caddy.NewTestController("http", test.input)
 		err := setup(c)
@@ -64,6 +86,9 @@ func TestSetup(t *testing.T) {
 			if recievedRule.Code != test.expectedRules[i].Code {
 				t.Errorf("Test case #%d.%d expected a HTTP status code of %d, but recieved a code of %d", j, i, test.expectedRules[i].Code, recievedRule.Code)
 			}
+			if gotMatcher, expectMatcher := fmt.Sprint(recievedRule.RequestMatcher), fmt.Sprint(test.expectedRules[i].RequestMatcher); gotMatcher != expectMatcher {
+				t.Errorf("Test case #%d.%d expected a Matcher %s, but recieved a Matcher %s", j, i, expectMatcher, gotMatcher)
+			}
 		}
 	}