Resolves #26 - Make a distinction between parsing and validation erro…

…rs. (#49)

* add parsing and validation error types (#29)

* Tweaks for error messaging and comments

* Resolves #26 - Improves error handling so that consumers can return 400 and not 500 on unrecognized operators

---------

Co-authored-by: Carl Markham <[email protected]>

README.md

@@ -27,6 +27,9 @@ func Example(headerValue string) (*epsearchast_v3.AstNode, error) {
 
 ```
 
+If the error that comes back is a ValidationErr you should treat it as a 400 to the caller.
+
+
 
 ### Aliases
 

docker-compose.yml

@@ -31,6 +31,7 @@ services:
       - '20003:9200'
     environment:
       discovery.type: single-node
+      ES_JAVA_OPTS: "-Xms1024m -Xmx1024m"
     healthcheck:
       test: [ "CMD-SHELL", "curl -f http://localhost:9200/_cat/health" ]
       interval: 10s

external/epsearchast/v3/ast.go

@@ -41,6 +41,8 @@ func (a *AstNode) AsFilter() string {
 }
 
 // GetAst converts the JSON to an AstNode if possible, returning an error otherwise.
+// If the Error is a ParsingErr it largely means you should treat the error as a 5xx.
+// If the Error is a ValidationErr it largely means you should treat the error as a 4xx.
 func GetAst(jsonTxt string) (*AstNode, error) {
 	astNode := &AstNode{}
 
@@ -54,15 +56,19 @@ func GetAst(jsonTxt string) (*AstNode, error) {
 			urlDecodingError = json.Unmarshal([]byte(decoded), astNode)
 
 			if urlDecodingError != nil {
-				return nil, fmt.Errorf("could not parse filter:%w, error parsing decoded filter: %v", err, urlDecodingError)
+				return nil, NewParsingErr(fmt.Errorf("error parsing decoded filter: %w %v", err, urlDecodingError))
 			}
 		} else {
-			return nil, fmt.Errorf("could not parse filter:%w, error decoding: %v", err, urlDecodingError)
+			return nil, NewParsingErr(fmt.Errorf("%w, error decoding: %v", err, urlDecodingError))
 		}
 	}
 
 	if err := astNode.checkValid(); err != nil {
-		return nil, fmt.Errorf("error validating filter (%s) :%w", astNode.AsFilter(), err)
+		// It might not be obvious why an invalid AST should be a validation error, especially if
+		// we receive something that doesn't make any sense like ge(a). The main argument case where we should
+		// treat this as a validation is an unknown operator. However, in theory the upstream generator
+		// passing us something likely means we should treat it as unsupported if we are out of date.
+		return nil, NewValidationErr(fmt.Errorf("(%s): %w", astNode.AsFilter(), err))
 	} else {
 		return astNode, nil
 	}
@@ -207,7 +213,7 @@ func (a *AstNode) checkValid() error {
 
 		}
 	default:
-		return fmt.Errorf("unknown operator %s", a.NodeType)
+		return fmt.Errorf("unsupported operator %s()", strings.ToLower(a.NodeType))
 	}
 
 	return nil

external/epsearchast/v3/ast_test.go

@@ -25,6 +25,19 @@ func TestEmptyObjectReturnsError(t *testing.T) {
 
 	// Verify
 	require.ErrorContains(t, err, "error validating filter")
+	require.ErrorAs(t, err, &ValidationErr{})
+	require.Nil(t, astNode)
+}
+
+func TestInvalidQueryReturnsParsingError(t *testing.T) {
+	// Fixture Setup
+
+	// Execute SUT
+	astNode, err := GetAst("{!@")
+
+	// Verify
+	require.ErrorContains(t, err, "could not parse filter")
+	require.ErrorAs(t, err, &ParsingErr{})
 	require.Nil(t, astNode)
 }
 
@@ -36,7 +49,35 @@ func TestInvalidObjectReturnsError(t *testing.T) {
 
 	// Verify
 	require.Error(t, err)
-	require.ErrorContains(t, err, "unknown operator FOO")
+	require.EqualError(t, err, "error validating filter: (foo()): unsupported operator foo()")
+	require.ErrorAs(t, err, &ValidationErr{})
+	require.Nil(t, astNode)
+}
+
+func TestUnrecognizedOperatorWithChildReturnsError(t *testing.T) {
+	// Fixture Setup
+
+	//language=JSON
+	jsonTxt := `{
+	  "type": "NOT",
+	  "children": [
+		{
+		  "type": "EQ",
+		  "args": [
+			"status",
+			"paid"
+		  ]
+		}
+	  ]
+	}
+`
+	// Execute SUT
+	astNode, err := GetAst(jsonTxt)
+
+	// Verify
+	require.Error(t, err)
+	require.EqualError(t, err, "error validating filter: (not()): unsupported operator not()")
+	require.ErrorAs(t, err, &ValidationErr{})
 	require.Nil(t, astNode)
 }
 
@@ -212,6 +253,7 @@ func TestEqWithChildReturnsError(t *testing.T) {
 	// Verify
 	require.Error(t, err)
 	require.ErrorContains(t, err, "should not have any children")
+	require.ErrorAs(t, err, &ValidationErr{})
 	require.Nil(t, astNode)
 }
 
@@ -247,6 +289,7 @@ func TestOneArgumentToInReturnsError(t *testing.T) {
 	// Verify
 	require.Error(t, err)
 	require.ErrorContains(t, err, "insufficient number of arguments to in")
+	require.ErrorAs(t, err, &ValidationErr{})
 	require.Nil(t, astNode)
 }
 
@@ -264,7 +307,8 @@ func TestInvalidOperatorReturnsError(t *testing.T) {
 
 	// Verify
 	require.Error(t, err)
-	require.ErrorContains(t, err, "unknown operator FOO")
+	require.ErrorContains(t, err, "unsupported operator foo()")
+	require.ErrorAs(t, err, &ValidationErr{})
 	require.Nil(t, astNode)
 }
 
@@ -287,6 +331,7 @@ func TestInWithChildReturnsError(t *testing.T) {
 	// Verify
 	require.Error(t, err)
 	require.ErrorContains(t, err, "should not have any children")
+	require.ErrorAs(t, err, &ValidationErr{})
 	require.Nil(t, astNode)
 }
 
@@ -308,6 +353,7 @@ func TestAndReturnsErrorWithOneChildren(t *testing.T) {
 	// Verify
 	require.Error(t, err)
 	require.ErrorContains(t, err, "and should have at least two children")
+	require.ErrorAs(t, err, &ValidationErr{})
 	require.Nil(t, astNode)
 }
 
@@ -332,7 +378,8 @@ func TestAndReturnsErrorWithAnInvalidChild(t *testing.T) {
 
 	// Verify
 	require.Error(t, err)
-	require.ErrorContains(t, err, "unknown operator FOO")
+	require.ErrorContains(t, err, "unsupported operator foo()")
+	require.ErrorAs(t, err, &ValidationErr{})
 	require.Nil(t, astNode)
 }
 

external/epsearchast/v3/errors.go

@@ -0,0 +1,31 @@
+package epsearchast_v3
+
+import "fmt"
+
+type ParsingErr struct {
+	err error
+}
+
+func (pe ParsingErr) Error() string {
+	return pe.err.Error()
+}
+
+func NewParsingErr(err error) ParsingErr {
+	return ParsingErr{
+		err: fmt.Errorf("could not parse filter: %w", err),
+	}
+}
+
+type ValidationErr struct {
+	err error
+}
+
+func (ve ValidationErr) Error() string {
+	return ve.err.Error()
+}
+
+func NewValidationErr(err error) ValidationErr {
+	return ValidationErr{
+		err: fmt.Errorf("error validating filter: %w", err),
+	}
+}

external/epsearchast/v3/validate.go

@@ -45,5 +45,11 @@ func ValidateAstFieldAndOperatorsWithAliasesAndValueValidationAndFieldTypes(astN
 		return err
 	}
 
-	return astNode.Accept(visitor)
+	err = astNode.Accept(visitor)
+
+	if err != nil {
+		return NewValidationErr(err)
+	}
+
+	return nil
 }

external/epsearchast/v3/validate_test.go

@@ -61,7 +61,7 @@ func TestValidationReturnsErrorForBinaryOperatorsWhenAstUsesUnknownField(t *test
 			err = ValidateAstFieldAndOperators(ast, map[string][]string{"other_field": {otherBinOp}, "another_field": {otherBinOp}})
 
 			// Verification
-			require.EqualError(t, err, fmt.Sprintf("unknown field [amount] specified in search filter, allowed fields are [another_field other_field]"))
+			require.EqualError(t, err, fmt.Sprintf("error validating filter: unknown field [amount] specified in search filter, allowed fields are [another_field other_field]"))
 		})
 	}