Proposal: Remove side-effects from hook package

This commit removes logging from the hook package and relies on returning errors
to convey...errors.  Encountered errors are returned immediately.  This commit
may alter the behavior of hook.  If errors were logged in the past but the given
function did not return immediately, this commit would change that behavior.

Uses named errors and custom error types.  You may be able to
consolidate ArgumentError and SourceError, but I need to think about it
some more.  These changes should be transparent to the caller if they're
expecting standard "error" types.

Tests have been updated to validate error return values and provide test
coverage for a few new lines of code introduced by this commit.

hook/hook.go

@@ -5,9 +5,9 @@ import (
 	"crypto/sha1"
 	"encoding/hex"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io/ioutil"
-	"log"
 	"reflect"
 	"regexp"
 	"strconv"
@@ -25,8 +25,48 @@ const (
 	SourceEntireHeaders string = "entire-headers"
 )
 
+var (
+	ErrInvalidPayloadSignature = errors.New("invalid payload signature")
+)
+
+// An ArgumentError describes an invalid argument passed to Hook.
+type ArgumentError struct {
+	Argument Argument
+}
+
+func (e *ArgumentError) Error() string {
+	if e == nil {
+		return "<nil>"
+	}
+	return fmt.Sprintf("couldn't retrieve argument for %+v", e.Argument)
+}
+
+// An SourceError describes an invalid source passed to Hook.
+type SourceError struct {
+	Argument Argument
+}
+
+func (e *SourceError) Error() string {
+	if e == nil {
+		return "<nil>"
+	}
+	return fmt.Sprintf("invalid source for argument %+v", e.Argument)
+}
+
+// A ParseError describes an error parsing user input.
+type ParseError struct {
+	Err error
+}
+
+func (e *ParseError) Error() string {
+	if e == nil {
+		return "<nil>"
+	}
+	return e.Err.Error()
+}
+
 // CheckPayloadSignature calculates and verifies SHA1 signature of the given payload
-func CheckPayloadSignature(payload []byte, secret string, signature string) (string, bool) {
+func CheckPayloadSignature(payload []byte, secret string, signature string) (string, error) {
 	if strings.HasPrefix(signature, "sha1=") {
 		signature = signature[5:]
 	}
@@ -35,7 +75,12 @@ func CheckPayloadSignature(payload []byte, secret string, signature string) (str
 	mac.Write(payload)
 	expectedMAC := hex.EncodeToString(mac.Sum(nil))
 
-	return expectedMAC, hmac.Equal([]byte(signature), []byte(expectedMAC))
+	var err error = nil
+	if !hmac.Equal([]byte(signature), []byte(expectedMAC)) {
+		err = ErrInvalidPayloadSignature
+	}
+
+	return expectedMAC, err
 }
 
 // ReplaceParameter replaces parameter value with the passed value in the passed map
@@ -201,7 +246,7 @@ type Hook struct {
 
 // ParseJSONParameters decodes specified arguments to JSON objects and replaces the
 // string with the newly created object
-func (h *Hook) ParseJSONParameters(headers, query, payload *map[string]interface{}) {
+func (h *Hook) ParseJSONParameters(headers, query, payload *map[string]interface{}) error {
 	for i := range h.JSONStringParameters {
 		if arg, ok := h.JSONStringParameters[i].Get(headers, query, payload); ok {
 			var newArg map[string]interface{}
@@ -212,7 +257,7 @@ func (h *Hook) ParseJSONParameters(headers, query, payload *map[string]interface
 			err := decoder.Decode(&newArg)
 
 			if err != nil {
-				log.Printf("error parsing argument as JSON payload %+v\n", err)
+				return &ParseError{err}
 			} else {
 				var source *map[string]interface{}
 
@@ -228,18 +273,20 @@ func (h *Hook) ParseJSONParameters(headers, query, payload *map[string]interface
 				if source != nil {
 					ReplaceParameter(h.JSONStringParameters[i].Name, source, newArg)
 				} else {
-					log.Printf("invalid source for argument %+v\n", h.JSONStringParameters[i])
+					return &SourceError{h.JSONStringParameters[i]}
 				}
 			}
 		} else {
-			log.Printf("couldn't retrieve argument for %+v\n", h.JSONStringParameters[i])
+			return &ArgumentError{h.JSONStringParameters[i]}
 		}
 	}
+
+	return nil
 }
 
 // ExtractCommandArguments creates a list of arguments, based on the
 // PassArgumentsToCommand property that is ready to be used with exec.Command()
-func (h *Hook) ExtractCommandArguments(headers, query, payload *map[string]interface{}) []string {
+func (h *Hook) ExtractCommandArguments(headers, query, payload *map[string]interface{}) ([]string, error) {
 	var args = make([]string, 0)
 
 	args = append(args, h.ExecuteCommand)
@@ -249,11 +296,11 @@ func (h *Hook) ExtractCommandArguments(headers, query, payload *map[string]inter
 			args = append(args, arg)
 		} else {
 			args = append(args, "")
-			log.Printf("couldn't retrieve argument for %+v\n", h.PassArgumentsToCommand[i])
+			return args, &ArgumentError{h.PassArgumentsToCommand[i]}
 		}
 	}
 
-	return args
+	return args, nil
 }
 
 // Hooks is an array of Hook objects
@@ -315,7 +362,7 @@ type Rules struct {
 
 // Evaluate finds the first rule property that is not nil and returns the value
 // it evaluates to
-func (r Rules) Evaluate(headers, query, payload *map[string]interface{}, body *[]byte) bool {
+func (r Rules) Evaluate(headers, query, payload *map[string]interface{}, body *[]byte) (bool, error) {
 	switch {
 	case r.And != nil:
 		return r.And.Evaluate(headers, query, payload, body)
@@ -327,49 +374,60 @@ func (r Rules) Evaluate(headers, query, payload *map[string]interface{}, body *[
 		return r.Match.Evaluate(headers, query, payload, body)
 	}
 
-	return false
+	return false, nil
 }
 
 // AndRule will evaluate to true if and only if all of the ChildRules evaluate to true
 type AndRule []Rules
 
 // Evaluate AndRule will return true if and only if all of ChildRules evaluate to true
-func (r AndRule) Evaluate(headers, query, payload *map[string]interface{}, body *[]byte) bool {
+func (r AndRule) Evaluate(headers, query, payload *map[string]interface{}, body *[]byte) (bool, error) {
 	res := true
 
 	for _, v := range r {
-		res = res && v.Evaluate(headers, query, payload, body)
+		rv, err := v.Evaluate(headers, query, payload, body)
+		if err != nil {
+			return false, err
+		}
+
+		res = res && rv
 		if res == false {
-			return res
+			return res, nil
 		}
 	}
 
-	return res
+	return res, nil
 }
 
 // OrRule will evaluate to true if any of the ChildRules evaluate to true
 type OrRule []Rules
 
 // Evaluate OrRule will return true if any of ChildRules evaluate to true
-func (r OrRule) Evaluate(headers, query, payload *map[string]interface{}, body *[]byte) bool {
+func (r OrRule) Evaluate(headers, query, payload *map[string]interface{}, body *[]byte) (bool, error) {
 	res := false
 
 	for _, v := range r {
-		res = res || v.Evaluate(headers, query, payload, body)
+		rv, err := v.Evaluate(headers, query, payload, body)
+		if err != nil {
+			return false, err
+		}
+
+		res = res || rv
 		if res == true {
-			return res
+			return res, nil
 		}
 	}
 
-	return res
+	return res, nil
 }
 
 // NotRule will evaluate to true if any and only if the ChildRule evaluates to false
 type NotRule Rules
 
 // Evaluate NotRule will return true if and only if ChildRule evaluates to false
-func (r NotRule) Evaluate(headers, query, payload *map[string]interface{}, body *[]byte) bool {
+func (r NotRule) Evaluate(headers, query, payload *map[string]interface{}, body *[]byte) (bool, error) {
-	return !Rules(r).Evaluate(headers, query, payload, body)
+	rv, err := Rules(r).Evaluate(headers, query, payload, body)
+	return !rv, err
 }
 
 // MatchRule will evaluate to true based on the type
@@ -389,29 +447,20 @@ const (
 )
 
 // Evaluate MatchRule will return based on the type
-func (r MatchRule) Evaluate(headers, query, payload *map[string]interface{}, body *[]byte) bool {
+func (r MatchRule) Evaluate(headers, query, payload *map[string]interface{}, body *[]byte) (bool, error) {
 	if arg, ok := r.Parameter.Get(headers, query, payload); ok {
 		switch r.Type {
 		case MatchValue:
-			return arg == r.Value
+			return arg == r.Value, nil
 		case MatchRegex:
-			ok, err := regexp.MatchString(r.Regex, arg)
+			return regexp.MatchString(r.Regex, arg)
-			if err != nil {
-				log.Printf("error while trying to evaluate regex: %+v", err)
-			}
-			return ok
 		case MatchHashSHA1:
-			expected, ok := CheckPayloadSignature(*body, r.Secret, arg)
+			_, err := CheckPayloadSignature(*body, r.Secret, arg)
-			if !ok {
+			return err == nil, err
-				log.Printf("payload signature mismatch, expected %s got %s", expected, arg)
+		}
 	}
 
-			return ok
+	return false, &ArgumentError{r.Parameter}
-		}
-	} else {
-		log.Printf("couldn't retrieve argument for %+v\n", r.Parameter)
-	}
-	return false
 }
 
 // CommandStatusResponse type encapsulates the executed command exit code, message, stdout and stderr

hook/hook_test.go

@@ -21,7 +21,8 @@ var checkPayloadSignatureTests = []struct {
 
 func TestCheckPayloadSignature(t *testing.T) {
 	for _, tt := range checkPayloadSignatureTests {
-		mac, ok := CheckPayloadSignature(tt.payload, tt.secret, tt.signature)
+		mac, err := CheckPayloadSignature(tt.payload, tt.secret, tt.signature)
+		ok := err == nil
 		if ok != tt.ok || mac != tt.mac {
 			t.Errorf("failed to check payload signature {%q, %q, %q}:\nexpected {mac:%#v, ok:%#v},\ngot {mac:%#v, ok:%#v}", tt.payload, tt.secret, tt.signature, tt.mac, tt.ok, mac, ok)
 		}
@@ -92,23 +93,25 @@ var hookParseJSONParametersTests = []struct {
 	params                     []Argument
 	headers, query, payload    *map[string]interface{}
 	rheaders, rquery, rpayload *map[string]interface{}
+	ok                         bool
 }{
-	{[]Argument{Argument{"header", "a"}}, &map[string]interface{}{"a": `{"b": "y"}`}, nil, nil, &map[string]interface{}{"a": map[string]interface{}{"b": "y"}}, nil, nil},
+	{[]Argument{Argument{"header", "a"}}, &map[string]interface{}{"a": `{"b": "y"}`}, nil, nil, &map[string]interface{}{"a": map[string]interface{}{"b": "y"}}, nil, nil, true},
-	{[]Argument{Argument{"url", "a"}}, nil, &map[string]interface{}{"a": `{"b": "y"}`}, nil, nil, &map[string]interface{}{"a": map[string]interface{}{"b": "y"}}, nil},
+	{[]Argument{Argument{"url", "a"}}, nil, &map[string]interface{}{"a": `{"b": "y"}`}, nil, nil, &map[string]interface{}{"a": map[string]interface{}{"b": "y"}}, nil, true},
-	{[]Argument{Argument{"payload", "a"}}, nil, nil, &map[string]interface{}{"a": `{"b": "y"}`}, nil, nil, &map[string]interface{}{"a": map[string]interface{}{"b": "y"}}},
+	{[]Argument{Argument{"payload", "a"}}, nil, nil, &map[string]interface{}{"a": `{"b": "y"}`}, nil, nil, &map[string]interface{}{"a": map[string]interface{}{"b": "y"}}, true},
-	{[]Argument{Argument{"header", "z"}}, &map[string]interface{}{"z": `{}`}, nil, nil, &map[string]interface{}{"z": map[string]interface{}{}}, nil, nil},
+	{[]Argument{Argument{"header", "z"}}, &map[string]interface{}{"z": `{}`}, nil, nil, &map[string]interface{}{"z": map[string]interface{}{}}, nil, nil, true},
 	// failures
-	{[]Argument{Argument{"header", "z"}}, &map[string]interface{}{"z": ``}, nil, nil, &map[string]interface{}{"z": ``}, nil, nil},     // empty string
+	{[]Argument{Argument{"header", "z"}}, &map[string]interface{}{"z": ``}, nil, nil, &map[string]interface{}{"z": ``}, nil, nil, false},     // empty string
-	{[]Argument{Argument{"header", "y"}}, &map[string]interface{}{"X": `{}`}, nil, nil, &map[string]interface{}{"X": `{}`}, nil, nil}, // missing parameter
+	{[]Argument{Argument{"header", "y"}}, &map[string]interface{}{"X": `{}`}, nil, nil, &map[string]interface{}{"X": `{}`}, nil, nil, false}, // missing parameter
-	{[]Argument{Argument{"string", "z"}}, &map[string]interface{}{"z": ``}, nil, nil, &map[string]interface{}{"z": ``}, nil, nil},     // invalid argument source
+	{[]Argument{Argument{"string", "z"}}, &map[string]interface{}{"z": ``}, nil, nil, &map[string]interface{}{"z": ``}, nil, nil, false},     // invalid argument source
 }
 
 func TestHookParseJSONParameters(t *testing.T) {
 	for _, tt := range hookParseJSONParametersTests {
 		h := &Hook{JSONStringParameters: tt.params}
-		h.ParseJSONParameters(tt.headers, tt.query, tt.payload)
+		err := h.ParseJSONParameters(tt.headers, tt.query, tt.payload)
-		if !reflect.DeepEqual(tt.headers, tt.rheaders) {
+		ok := err == nil
-			t.Errorf("failed to parse %v:\nexpected %#v,\ngot %#v", tt.params, *tt.rheaders, *tt.headers)
+		if ok != tt.ok || !reflect.DeepEqual(tt.headers, tt.rheaders) {
+			t.Errorf("failed to parse %v:\nexpected %#v, ok: %#v\ngot %#v, ok: %v", tt.params, *tt.rheaders, tt.ok, *tt.headers, ok)
 		}
 	}
 }
@@ -118,18 +121,20 @@ var hookExtractCommandArgumentsTests = []struct {
 	args                    []Argument
 	headers, query, payload *map[string]interface{}
 	value                   []string
+	ok                      bool
 }{
-	{"test", []Argument{Argument{"header", "a"}}, &map[string]interface{}{"a": "z"}, nil, nil, []string{"test", "z"}},
+	{"test", []Argument{Argument{"header", "a"}}, &map[string]interface{}{"a": "z"}, nil, nil, []string{"test", "z"}, true},
 	// failures
-	{"fail", []Argument{Argument{"payload", "a"}}, &map[string]interface{}{"a": "z"}, nil, nil, []string{"fail", ""}},
+	{"fail", []Argument{Argument{"payload", "a"}}, &map[string]interface{}{"a": "z"}, nil, nil, []string{"fail", ""}, false},
 }
 
 func TestHookExtractCommandArguments(t *testing.T) {
 	for _, tt := range hookExtractCommandArgumentsTests {
 		h := &Hook{ExecuteCommand: tt.exec, PassArgumentsToCommand: tt.args}
-		value := h.ExtractCommandArguments(tt.headers, tt.query, tt.payload)
+		value, err := h.ExtractCommandArguments(tt.headers, tt.query, tt.payload)
-		if !reflect.DeepEqual(value, tt.value) {
+		ok := err == nil
-			t.Errorf("failed to extract args {cmd=%q, args=%v}:\nexpected %#v,\ngot %#v", tt.exec, tt.args, tt.value, value)
+		if ok != tt.ok || !reflect.DeepEqual(value, tt.value) {
+			t.Errorf("failed to extract args {cmd=%q, args=%v}:\nexpected %#v, ok: %v\ngot %#v, ok: %v", tt.exec, tt.args, tt.value, tt.ok, value, ok)
 		}
 	}
 }
@@ -178,24 +183,26 @@ var matchRuleTests = []struct {
 	headers, query, payload   *map[string]interface{}
 	body                      []byte
 	ok                        bool
+	err                       bool
 }{
-	{"value", "", "", "z", Argument{"header", "a"}, &map[string]interface{}{"a": "z"}, nil, nil, []byte{}, true},
+	{"value", "", "", "z", Argument{"header", "a"}, &map[string]interface{}{"a": "z"}, nil, nil, []byte{}, true, false},
-	{"regex", "^z", "", "z", Argument{"header", "a"}, &map[string]interface{}{"a": "z"}, nil, nil, []byte{}, true},
+	{"regex", "^z", "", "z", Argument{"header", "a"}, &map[string]interface{}{"a": "z"}, nil, nil, []byte{}, true, false},
-	{"payload-hash-sha1", "", "secret", "", Argument{"header", "a"}, &map[string]interface{}{"a": "b17e04cbb22afa8ffbff8796fc1894ed27badd9e"}, nil, nil, []byte(`{"a": "z"}`), true},
+	{"payload-hash-sha1", "", "secret", "", Argument{"header", "a"}, &map[string]interface{}{"a": "b17e04cbb22afa8ffbff8796fc1894ed27badd9e"}, nil, nil, []byte(`{"a": "z"}`), true, false},
+	// negatives
+	{"regex", "^X", "", "", Argument{"header", "a"}, &map[string]interface{}{"a": "z"}, nil, nil, []byte{}, false, false},
+	{"value", "", "", "X", Argument{"header", "a"}, &map[string]interface{}{"a": "z"}, nil, nil, []byte{}, false, false},
 	// failures
-	{"value", "", "", "X", Argument{"header", "a"}, &map[string]interface{}{"a": "z"}, nil, nil, []byte{}, false},
+	{"value", "", "2", "X", Argument{"header", "a"}, &map[string]interface{}{"y": "z"}, nil, nil, []byte{}, false, true},                // reference invalid header
-	{"value", "", "", "X", Argument{"header", "a"}, &map[string]interface{}{"y": "z"}, nil, nil, []byte{}, false},
+	{"regex", "*", "", "", Argument{"header", "a"}, &map[string]interface{}{"a": "z"}, nil, nil, []byte{}, false, true},                 // invalid regex
-	{"regex", "^X", "", "", Argument{"header", "a"}, &map[string]interface{}{"a": "z"}, nil, nil, []byte{}, false},
+	{"payload-hash-sha1", "", "secret", "", Argument{"header", "a"}, &map[string]interface{}{"a": ""}, nil, nil, []byte{}, false, true}, // invalid hmac
-	{"regex", "*", "", "", Argument{"header", "a"}, &map[string]interface{}{"a": "z"}, nil, nil, []byte{}, false},
-	{"payload-hash-sha1", "", "secret", "", Argument{"header", "a"}, &map[string]interface{}{"a": ""}, nil, nil, []byte{}, false},
 }
 
 func TestMatchRule(t *testing.T) {
 	for _, tt := range matchRuleTests {
 		r := MatchRule{tt.typ, tt.regex, tt.secret, tt.value, tt.param}
-		ok := r.Evaluate(tt.headers, tt.query, tt.payload, &tt.body)
+		ok, err := r.Evaluate(tt.headers, tt.query, tt.payload, &tt.body)
-		if ok != tt.ok {
+		if ok != tt.ok || (err == nil) == tt.err {
-			t.Errorf("failed to match %#v:\nexpected %#v,\ngot %#v", r, tt.ok, ok)
+			t.Errorf("failed to match %#v:\nexpected ok: %#v, err: %#v,\ngot ok: %#v, err: %#v", r, tt.ok, tt.err, ok, err)
 		}
 	}
 }
@@ -206,6 +213,7 @@ var andRuleTests = []struct {
 	headers, query, payload *map[string]interface{}
 	body                    []byte
 	ok                      bool
+	err                     bool
 }{
 	{
 		"(a=z, b=y): a=z && b=y",
@@ -214,7 +222,7 @@ var andRuleTests = []struct {
 			{Match: &MatchRule{"value", "", "", "y", Argument{"header", "b"}}},
 		},
 		&map[string]interface{}{"a": "z", "b": "y"}, nil, nil, []byte{},
-		true,
+		true, false,
 	},
 	{
 		"(a=z, b=Y): a=z && b=y",
@@ -223,7 +231,7 @@ var andRuleTests = []struct {
 			{Match: &MatchRule{"value", "", "", "y", Argument{"header", "b"}}},
 		},
 		&map[string]interface{}{"a": "z", "b": "Y"}, nil, nil, []byte{},
-		false,
+		false, false,
 	},
 	// Complex test to cover Rules.Evaluate
 	{
@@ -249,16 +257,23 @@ var andRuleTests = []struct {
 			},
 		},
 		&map[string]interface{}{"a": "z", "b": "y", "c": "x", "d": "w", "e": "X", "f": "X"}, nil, nil, []byte{},
-		true,
+		true, false,
+	},
+	{"empty rule", AndRule{{}}, nil, nil, nil, nil, false, false},
+	// failures
+	{
+		"invalid rule",
+		AndRule{{Match: &MatchRule{"value", "", "", "X", Argument{"header", "a"}}}},
+		&map[string]interface{}{"y": "z"}, nil, nil, nil,
+		false, true,
 	},
-	{"empty rule", AndRule{{}}, nil, nil, nil, nil, false},
 }
 
 func TestAndRule(t *testing.T) {
 	for _, tt := range andRuleTests {
-		ok := tt.rule.Evaluate(tt.headers, tt.query, tt.payload, &tt.body)
+		ok, err := tt.rule.Evaluate(tt.headers, tt.query, tt.payload, &tt.body)
-		if ok != tt.ok {
+		if ok != tt.ok || (err != nil) != tt.err {
-			t.Errorf("failed to match %#v:\nexpected %#v,\ngot %#v", tt.desc, tt.ok, ok)
+			t.Errorf("failed to match %#v:\nexpected ok: %#v, err: %s,\ngot ok: %#v, err: %s", tt.desc, tt.ok, tt.err, ok, err != nil)
 		}
 	}
 }
@@ -269,6 +284,7 @@ var orRuleTests = []struct {
 	headers, query, payload *map[string]interface{}
 	body                    []byte
 	ok                      bool
+	err                     bool
 }{
 	{
 		"(a=z, b=X): a=z || b=y",
@@ -277,7 +293,7 @@ var orRuleTests = []struct {
 			{Match: &MatchRule{"value", "", "", "y", Argument{"header", "b"}}},
 		},
 		&map[string]interface{}{"a": "z", "b": "X"}, nil, nil, []byte{},
-		true,
+		true, false,
 	},
 	{
 		"(a=X, b=y): a=z || b=y",
@@ -286,7 +302,7 @@ var orRuleTests = []struct {
 			{Match: &MatchRule{"value", "", "", "y", Argument{"header", "b"}}},
 		},
 		&map[string]interface{}{"a": "X", "b": "y"}, nil, nil, []byte{},
-		true,
+		true, false,
 	},
 	{
 		"(a=Z, b=Y): a=z || b=y",
@@ -295,15 +311,24 @@ var orRuleTests = []struct {
 			{Match: &MatchRule{"value", "", "", "y", Argument{"header", "b"}}},
 		},
 		&map[string]interface{}{"a": "Z", "b": "Y"}, nil, nil, []byte{},
-		false,
+		false, false,
+	},
+	// failures
+	{
+		"invalid rule",
+		OrRule{
+			{Match: &MatchRule{"value", "", "", "z", Argument{"header", "a"}}},
+		},
+		&map[string]interface{}{"y": "Z"}, nil, nil, []byte{},
+		false, true,
 	},
 }
 
 func TestOrRule(t *testing.T) {
 	for _, tt := range orRuleTests {
-		ok := tt.rule.Evaluate(tt.headers, tt.query, tt.payload, &tt.body)
+		ok, err := tt.rule.Evaluate(tt.headers, tt.query, tt.payload, &tt.body)
-		if ok != tt.ok {
+		if ok != tt.ok || (err != nil) != tt.err {
-			t.Errorf("%#v:\nexpected %#v,\ngot %#v", tt.desc, tt.ok, ok)
+			t.Errorf("%#v:\nexpected ok: %#v, err: %s,\ngot ok: %#v, err: %s", tt.desc, tt.ok, tt.err, ok, err != nil)
 		}
 	}
 }
@@ -314,16 +339,17 @@ var notRuleTests = []struct {
 	headers, query, payload *map[string]interface{}
 	body                    []byte
 	ok                      bool
+	err                     bool
 }{
-	{"(a=z): !a=X", NotRule{Match: &MatchRule{"value", "", "", "X", Argument{"header", "a"}}}, &map[string]interface{}{"a": "z"}, nil, nil, []byte{}, true},
+	{"(a=z): !a=X", NotRule{Match: &MatchRule{"value", "", "", "X", Argument{"header", "a"}}}, &map[string]interface{}{"a": "z"}, nil, nil, []byte{}, true, false},
-	{"(a=z): !a=z", NotRule{Match: &MatchRule{"value", "", "", "z", Argument{"header", "a"}}}, &map[string]interface{}{"a": "z"}, nil, nil, []byte{}, false},
+	{"(a=z): !a=z", NotRule{Match: &MatchRule{"value", "", "", "z", Argument{"header", "a"}}}, &map[string]interface{}{"a": "z"}, nil, nil, []byte{}, false, false},
 }
 
 func TestNotRule(t *testing.T) {
 	for _, tt := range notRuleTests {
-		ok := tt.rule.Evaluate(tt.headers, tt.query, tt.payload, &tt.body)
+		ok, err := tt.rule.Evaluate(tt.headers, tt.query, tt.payload, &tt.body)
-		if ok != tt.ok {
+		if ok != tt.ok || (err != nil) != tt.err {
-			t.Errorf("failed to match %#v:\nexpected %#v,\ngot %#v", tt.rule, tt.ok, ok)
+			t.Errorf("failed to match %#v:\nexpected ok: %#v, err: %s,\ngot ok: %#v, err: %s", tt.rule, tt.ok, tt.err, ok, err != nil)
 		}
 	}
 }

webhook.go

@@ -185,8 +185,25 @@ func hookHandler(w http.ResponseWriter, r *http.Request) {
 
 		// handle hook
 		for _, h := range matchedHooks {
-			h.ParseJSONParameters(&headers, &query, &payload)
+			err := h.ParseJSONParameters(&headers, &query, &payload)
-			if h.TriggerRule == nil || h.TriggerRule != nil && h.TriggerRule.Evaluate(&headers, &query, &payload, &body) {
+			if err != nil {
+				log.Printf("error parsing JSON: %s", err)
+				return
+			}
+
+			var ok bool
+
+			if h.TriggerRule == nil {
+				ok = true
+			} else {
+				ok, err = h.TriggerRule.Evaluate(&headers, &query, &payload, &body)
+				if err != nil {
+					log.Printf("error evaluating hook: %s", err)
+					return
+				}
+			}
+
+			if ok && err == nil {
 				log.Printf("%s hook triggered successfully\n", h.ID)
 
 				if h.CaptureCommandOutput {
@@ -213,10 +230,17 @@ func hookHandler(w http.ResponseWriter, r *http.Request) {
 }
 
 func handleHook(h *hook.Hook, headers, query, payload *map[string]interface{}, body *[]byte) string {
+	var err error
+
 	cmd := exec.Command(h.ExecuteCommand)
-	cmd.Args = h.ExtractCommandArguments(headers, query, payload)
 	cmd.Dir = h.CommandWorkingDirectory
 
+	cmd.Args, err = h.ExtractCommandArguments(headers, query, payload)
+	if err != nil {
+		log.Printf("error extracting command arguments: %s", err)
+		return ""
+	}
+
 	log.Printf("executing %s (%s) with arguments %s using %s as cwd\n", h.ExecuteCommand, cmd.Path, cmd.Args, cmd.Dir)
 
 	out, err := cmd.CombinedOutput()