test: refactor schema tests

schema/backlinks.go

@@ -69,6 +69,14 @@ func (scm *Builder) insertBackLinkField(
 		backlinkName = relationValue.ToString()
 	}
 
+	// If the field already exists don't add another one as this will just create a
+	// duplicate field name error that is confusing. This is will be an error but will
+	// be caught by relationship validation since it is not possible for Identity
+	// to have any fields which use a user-defined model
+	if query.Field(identityModel, backlinkName) != nil {
+		return nil
+	}
+
 	backLinkField := &parser.FieldNode{
 		Name: parser.NameNode{
 			Value: backlinkName,

schema/schema_test.go

@@ -1,113 +1,143 @@
 package schema_test
 
 import (
-	"encoding/json"
+	"errors"
 	"fmt"
 	"os"
+	"path/filepath"
+	"regexp"
+	"strconv"
+	"strings"
 	"testing"
 
 	"github.com/nsf/jsondiff"
+	"github.com/samber/lo"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"github.com/teamkeel/keel/schema"
+	"github.com/teamkeel/keel/schema/validation/errorhandling"
 	"google.golang.org/protobuf/encoding/protojson"
 )
 
-type Error struct {
-	Code string `json:"code"`
-}
-
-type Errors struct {
-	Errors []Error `json:"Errors"`
-}
-
-func TestSchema(t *testing.T) {
-	testdataDir := "./testdata"
+func TestProto(t *testing.T) {
+	testdataDir := "./testdata/proto"
 	testCases, err := os.ReadDir(testdataDir)
 	require.NoError(t, err)
 
 	for _, testCase := range testCases {
 		if !testCase.IsDir() {
+			t.Errorf("proto test data directory should only contain directories - file found: %s", testCase.Name())
 			continue
 		}
 
-		testCaseDir := testdataDir + "/" + testCase.Name()
+		testCaseDir := filepath.Join(testdataDir, testCase.Name())
 
 		t.Run(testCase.Name(), func(t *testing.T) {
 
-			files, err := os.ReadDir(testCaseDir)
+			expected, err := os.ReadFile(filepath.Join(testCaseDir, "proto.json"))
 			require.NoError(t, err)
 
-			s2m := schema.Builder{}
+			builder := schema.Builder{}
+			protoSchema, err := builder.MakeFromDirectory(testCaseDir)
+			require.NoError(t, err)
 
-			filesByName := map[string][]byte{}
-			for _, f := range files {
-				if f.IsDir() {
-					continue
-				}
-				b, err := os.ReadFile(testCaseDir + "/" + f.Name())
-				require.NoError(t, err)
-				filesByName[f.Name()] = b
-			}
+			actual, err := protojson.Marshal(protoSchema)
+			require.NoError(t, err)
+
+			opts := jsondiff.DefaultConsoleOptions()
 
-			protoSchema, err := s2m.MakeFromDirectory(testCaseDir)
-
-			var expectedJSON []byte
-			var actualJSON []byte
-
-			var actualProtoJSONPretty string
-
-			// This is used when expected error json differs from actual error json,
-			// and provides something you can copy and paste into your errors.json file,
-			// once you've got it looking right.
-			var prettyJSONErr string
-
-			if expectedProto, ok := filesByName["proto.json"]; ok {
-				require.NoError(t, err)
-				expectedJSON = expectedProto
-				actualJSON, err = protojson.Marshal(protoSchema)
-				require.NoError(t, err)
-				actualProtoJSONPretty = protojson.Format(protoSchema)
-				_ = actualProtoJSONPretty
-
-			} else if expectedErrors, ok := filesByName["errors.json"]; ok {
-				require.NotNil(t, err, "expected there to be validation errors")
-				expectedJSON = expectedErrors
-				capturedErr := err
-				actualJSON, err = json.Marshal(capturedErr)
-				require.NoError(t, err)
-				q, err := json.MarshalIndent(capturedErr, "", "  ")
-				prettyJSONErr = string(q)
-				require.NoError(t, err)
-			} else {
-				// if no proto.json file or errors.json file is provided then we assume this
-				// is a test case that is just expected to parse and validate with no errors
-				require.NoError(t, err)
+			diff, explanation := jsondiff.Compare(expected, actual, &opts)
+			if diff == jsondiff.FullMatch {
 				return
 			}
 
-			opts := jsondiff.DefaultConsoleOptions()
+			assert.Fail(t, "actual proto JSON does not match expected", explanation)
+		})
+	}
+}
+
+var expectErrorCommentRegex = regexp.MustCompile(`^\s*\/\/\s{0,1}expect-error:`)
+
+func TestValidation(t *testing.T) {
+	dir := "./testdata/errors"
+	testCases, err := os.ReadDir(dir)
+	require.NoError(t, err)
+
+	for _, testCase := range testCases {
+		if testCase.IsDir() {
+			t.Errorf("errors test data directory should only contain keel schema files - directory found: %s", testCase.Name())
+			continue
+		}
 
-			diff, explanation := jsondiff.Compare(expectedJSON, actualJSON, &opts)
-
-			switch diff {
-			case jsondiff.FullMatch:
-				// success
-			case jsondiff.SupersetMatch, jsondiff.NoMatch:
-				// These printfs produce output you can copy and paste to rectify
-				// errors during development.
-				fmt.Printf("Pretty json error (%s): \n%s\n", testCase.Name(), prettyJSONErr)
-				fmt.Printf("Pretty actual proto json (%s): \n%s\n", testCase.Name(), actualProtoJSONPretty)
-				assert.Fail(t, "actual result does not match expected", explanation)
-			case jsondiff.FirstArgIsInvalidJson:
-				assert.Fail(t, "expected JSON is invalid")
-			case jsondiff.SecondArgIsInvalidJson:
-				// highly unlikely (almost impossible) to happen
-				assert.Fail(t, "actual JSON (proto or errors) is invalid")
-			case jsondiff.BothArgsAreInvalidJson:
-				// also highly unlikely (almost impossible) to happen
-				assert.Fail(t, "both expected and actual JSON are invalid")
+		testCaseDir := filepath.Join(dir, testCase.Name())
+
+		t.Run(testCase.Name(), func(t *testing.T) {
+			b, err := os.ReadFile(testCaseDir)
+			require.NoError(t, err)
+
+			builder := &schema.Builder{}
+			_, err = builder.MakeFromString(string(b))
+
+			verrs := &errorhandling.ValidationErrors{}
+			if !errors.As(err, &verrs) {
+				t.Errorf("no validation errors returned")
+			}
+
+			expectedErrors := []*errorhandling.ValidationError{}
+			lines := strings.Split(string(b), "\n")
+			for i, line := range lines {
+				if !expectErrorCommentRegex.MatchString(line) {
+					continue
+				}
+
+				line := expectErrorCommentRegex.ReplaceAllString(line, "")
+				parts := strings.SplitN(line, ":", 4)
+
+				column, err := strconv.Atoi(parts[0])
+				require.NoError(t, err, "unable to parse start column from //expect-error comment")
+
+				endColumn, err := strconv.Atoi(parts[1])
+				require.NoError(t, err, "unable to parse end column from //expect-eror comment")
+
+				code := parts[2]
+				message := parts[3]
+
+				// A line can have multiple expected errors - so we find the next line that is not an "expect-error" comment
+				errorLine := i + 2
+				for j, l := range lines[i+1:] {
+					if !expectErrorCommentRegex.MatchString(l) {
+						errorLine += j
+						break
+					}
+				}
+
+				expectedErrors = append(expectedErrors, &errorhandling.ValidationError{
+					ErrorDetails: &errorhandling.ErrorDetails{
+						Message: message,
+					},
+					Code: code,
+					Pos: errorhandling.LexerPos{
+						Line:   errorLine,
+						Column: column,
+					},
+					EndPos: errorhandling.LexerPos{
+						Line:   errorLine,
+						Column: endColumn,
+					},
+				})
+			}
+
+			missing, unexpected := lo.Difference(lo.Map(expectedErrors, errorToString), lo.Map(verrs.Errors, errorToString))
+			for _, v := range missing {
+				t.Errorf("  Expected:   %s", v)
+			}
+			for _, v := range unexpected {
+				t.Errorf("  Unexpected: %s", v)
 			}
 		})
 	}
 }
+
+func errorToString(err *errorhandling.ValidationError, _ int) string {
+	return fmt.Sprintf("%d:%d:%d:%s:%s", err.Pos.Line, err.Pos.Column, err.EndPos.Column, err.Code, err.Message)
+}

schema/testdata/errors/action_types.keel

@@ -0,0 +1,10 @@
+model Person {
+    fields {
+        name Text
+    }
+
+    actions {
+        //expect-error:9:12:TypeError:foo is not a valid action type. Valid types are get, create, update, list, or delete
+        foo something()
+    }
+}

schema/testdata/errors/actions_no_bare_model_as_input.keel

@@ -0,0 +1,26 @@
+model Book {
+    fields {
+        author Author
+    }
+
+    actions {
+        //expect-error:36:42:ActionInputError:'author' refers to a model which cannot used as an input
+        create createBooks() with (author)
+        //expect-error:38:44:ActionInputError:'author' refers to a model which cannot used as an input
+        update updateBooks(id) with (author)
+        //expect-error:24:30:ActionInputError:'author' refers to a model which cannot used as an input
+        list listBooks(author)
+    }
+
+    actions {
+        //expect-error:44:50:ActionInputError:'author' refers to a model which cannot used as an input
+        create createBooksFunction() with (author)
+        //expect-error:46:52:ActionInputError:'author' refers to a model which cannot used as an input
+        update updateBooksFunction(id) with (author)
+        //expect-error:32:38:ActionInputError:'author' refers to a model which cannot used as an input
+        list listBooksFunction(author)
+    }
+}
+
+model Author {
+}

schema/testdata/validation_actions_update_invalid_where_operator/schema.keel → schema/testdata/errors/actions_update_invalid_where_operator.keel

@@ -3,9 +3,11 @@ model Person {
         name Text
         niNumber Number @unique
     }
+
     actions {
+        //expect-error:16:26:ActionInputError:The action 'updateName' can only update a single record and therefore must be filtered by unique fields
         update updateName() with (name) {
             @where(person.niNumber > 100)
         }
     }
-}
+}

schema/testdata/errors/actions_update_no_unique_input.keel

@@ -0,0 +1,10 @@
+model Person {
+    fields {
+        name Text
+    }
+
+    actions {
+        //expect-error:16:26:ActionInputError:The action 'updateName' can only update a single record and therefore must be filtered by unique fields
+        update updateName() with (name)
+    }
+}

schema/testdata/errors/actions_update_non_unique_input.keel

@@ -0,0 +1,11 @@
+model Person {
+    fields {
+        name Text
+        age Number
+    }
+
+    actions {
+        //expect-error:16:26:ActionInputError:The action 'updateName' can only update a single record and therefore must be filtered by unique fields
+        update updateName(age) with (name)
+    }
+}

schema/testdata/validation_actions_update_non_unique_where/schema.keel → schema/testdata/errors/actions_update_non_unique_where.keel

@@ -3,9 +3,11 @@ model Person {
         name Text
         age Number
     }
+
     actions {
+        //expect-error:16:26:ActionInputError:The action 'updateName' can only update a single record and therefore must be filtered by unique fields
         update updateName() with (name) {
             @where(person.age == 21)
         }
     }
-}
+}

schema/testdata/errors/api_duplicate_definitions.keel

@@ -0,0 +1,6 @@
+api Web {
+}
+
+//expect-error:5:8:E017:You have a duplicate definition for 'api Web'
+api Web {
+}

schema/testdata/errors/api_names_are_models.keel

@@ -0,0 +1,10 @@
+model Post {
+}
+
+api Web {
+    models {
+        Post
+        //expect-error:9:21:E047:api 'Web' has an unrecognised model UnknownModel
+        UnknownModel
+    }
+}

schema/testdata/errors/api_unsupported_attribute.keel

@@ -0,0 +1,4 @@
+api Web {
+    //expect-error:5:10:E011:api 'Web' has an unrecognised attribute @what
+    @what
+}

schema/testdata/errors/arbitrary_function_action_types.keel

@@ -0,0 +1,10 @@
+message Foo {
+    bar Text
+}
+
+model Person {
+    actions {
+        //expect-error:9:12:TypeError:The 'returns' keyword can only be used with 'read' or 'write' actions
+        get getPerson(id) returns (foo) @function
+    }
+}

schema/testdata/errors/arbitrary_function_multiple_inputs.keel

@@ -0,0 +1,19 @@
+message Foo {
+    bar Text
+}
+
+message Baz {
+    bar Text
+}
+
+message Bar {
+    id ID
+}
+
+model Person {
+    actions {
+        //expect-error:24:27:ActionInputError:read and write functions must receive exactly one message-based input
+        //expect-error:29:32:ActionInputError:read and write functions must receive exactly one message-based input
+        read getPerson(Foo, Baz) returns (Bar) @function
+    }
+}

schema/testdata/errors/arbitrary_functions_unknown_message_return.keel

@@ -0,0 +1,6 @@
+model Person {
+    actions {
+        //expect-error:46:60:ActionInputError:read and write functions must return a message-based response, or Any
+        write createBulkPeople(Any) returns (UnknownMessage) @function
+    }
+}