diff --git a/pkg/go/utils/line-numbers.go b/pkg/go/utils/line-numbers.go index 3e842a78..7a65e803 100644 --- a/pkg/go/utils/line-numbers.go +++ b/pkg/go/utils/line-numbers.go @@ -5,28 +5,118 @@ import ( "strings" ) -func GetConditionLineNumber(conditionName string, lines []string) int { +// nameBytes marks every byte that can appear inside a declaration name, keyed by +// the bytes themselves so each entry is visibly correct. IDENTIFIER admits letters, +// digits, `_` and MINUS, and EXTENDED_IDENTIFIER adds SLASH and DOT +// (OpenFGALexer.g4), so `core.doc` is one name rather than `core` and a terminator. +var nameBytes = [256]bool{ + 'a': true, 'b': true, 'c': true, 'd': true, 'e': true, 'f': true, 'g': true, + 'h': true, 'i': true, 'j': true, 'k': true, 'l': true, 'm': true, 'n': true, + 'o': true, 'p': true, 'q': true, 'r': true, 's': true, 't': true, 'u': true, + 'v': true, 'w': true, 'x': true, 'y': true, 'z': true, + 'A': true, 'B': true, 'C': true, 'D': true, 'E': true, 'F': true, 'G': true, + 'H': true, 'I': true, 'J': true, 'K': true, 'L': true, 'M': true, 'N': true, + 'O': true, 'P': true, 'Q': true, 'R': true, 'S': true, 'T': true, 'U': true, + 'V': true, 'W': true, 'X': true, 'Y': true, 'Z': true, + '0': true, '1': true, '2': true, '3': true, '4': true, + '5': true, '6': true, '7': true, '8': true, '9': true, + '_': true, '-': true, '/': true, '.': true, +} + +// IsNameByte reports whether b can appear inside a declaration name. +func IsNameByte(b byte) bool { + return nameBytes[b] +} + +// declarationIndex returns the index of the first line beginning with prefix whose +// name ends there, or -1. prefix always spans whole words (`type `, `define +// `, `condition `), so requiring the next byte to end the name is what +// stops `document` matching a declaration of `documentation`. +func declarationIndex(lines []string, prefix string) int { return slices.IndexFunc(lines, func(line string) bool { - return strings.HasPrefix(strings.TrimSpace(line), "condition "+conditionName) + trimmed := NormalizeWhitespace(strings.TrimSpace(line)) + if !strings.HasPrefix(trimmed, prefix) { + return false + } + + rest := trimmed[len(prefix):] + + return rest == "" || !IsNameByte(rest[0]) }) } +// isInlineWhitespace reports whether b is a byte the lexer's WHITESPACE rule +// accepts between tokens on a line: space, tab or form feed (OpenFGALexer.g4). +func isInlineWhitespace(b byte) bool { + return b == ' ' || b == '\t' || b == '\f' +} + +// NormalizeWhitespace collapses every run of inline whitespace into one space, so +// `define\towner:` and `define owner:` are matched the same as `define owner:`. +// Only space, tab and form feed are folded — exactly the lexer's WHITESPACE +// alphabet; anything else (nbsp, vertical tab) fails to lex and can never reach a +// line lookup. Lines that are already normal are returned unchanged. +func NormalizeWhitespace(line string) string { + for i := range len(line) { + if b := line[i]; b == '\t' || b == '\f' || (b == ' ' && i > 0 && line[i-1] == ' ') { + return foldInlineWhitespace(line) + } + } + + return line +} + +func foldInlineWhitespace(line string) string { + var sb strings.Builder + + sb.Grow(len(line)) + + inRun := false + + for i := range len(line) { + if isInlineWhitespace(line[i]) { + inRun = true + + continue + } + + if inRun { + sb.WriteByte(' ') + + inRun = false + } + + sb.WriteByte(line[i]) + } + + if inRun { + sb.WriteByte(' ') + } + + return sb.String() +} + +// GetConditionLineNumber returns the index of the line declaring conditionName, or +// -1. `less` does not match a declaration of `less_than`. +func GetConditionLineNumber(conditionName string, lines []string) int { + return declarationIndex(lines, "condition "+conditionName) +} + +// GetTypeLineNumber returns the index of the line declaring typeName, or -1. +// `document` does not match a declaration of `documentation`. func GetTypeLineNumber(typeName string, lines []string) int { - return slices.IndexFunc(lines, func(line string) bool { - return strings.HasPrefix(strings.TrimSpace(line), "type "+typeName) - }) + return declarationIndex(lines, "type "+typeName) } +// GetExtendedTypeLineNumber returns the index of the line extending typeName, or -1. func GetExtendedTypeLineNumber(typeName string, lines []string) int { - return slices.IndexFunc(lines, func(line string) bool { - return strings.HasPrefix(strings.TrimSpace(line), "extend type "+typeName) - }) + return declarationIndex(lines, "extend type "+typeName) } +// GetRelationLineNumber returns the index of the line defining relation, or -1. +// `owner` does not match a definition of `owner_group`. func GetRelationLineNumber(relation string, lines []string) int { - return slices.IndexFunc(lines, func(line string) bool { - return strings.HasPrefix(strings.TrimSpace(line), "define "+relation) - }) + return declarationIndex(lines, "define "+relation) } type StartEnd struct { diff --git a/pkg/go/utils/line-numbers_test.go b/pkg/go/utils/line-numbers_test.go new file mode 100644 index 00000000..d5184c90 --- /dev/null +++ b/pkg/go/utils/line-numbers_test.go @@ -0,0 +1,389 @@ +package utils + +import "testing" + +func TestIsNameByte(t *testing.T) { + t.Parallel() + + // IDENTIFIER: (LETTER | '_') (LETTER | DIGIT | '_' | MINUS)*, and + // EXTENDED_IDENTIFIER adds SLASH and DOT (OpenFGALexer.g4), so the name bytes + // are exactly the ASCII letters, digits, `_`, `-`, `/` and `.`. Checking every + // byte pins the table to those ranges, so a mistyped entry cannot survive. + isName := func(b byte) bool { + switch { + case b >= 'a' && b <= 'z', b >= 'A' && b <= 'Z', b >= '0' && b <= '9': + return true + case b == '_', b == '-', b == '/', b == '.': + return true + } + + return false + } + + for i := range 256 { + b := byte(i) + if got, want := IsNameByte(b), isName(b); got != want { + t.Errorf("IsNameByte(0x%02X) = %v, want %v", b, got, want) + } + } +} + +func TestNormalizeWhitespace(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + in string + want string + }{ + {name: "already normal returns unchanged", in: "define owner: [user]", want: "define owner: [user]"}, + {name: "collapses repeated spaces", in: "define owner:", want: "define owner:"}, + {name: "folds a tab", in: "define\towner:", want: "define owner:"}, + {name: "folds a form feed", in: "define\fowner:", want: "define owner:"}, + {name: "folds a mixed run", in: "extend \t type\f\forg", want: "extend type org"}, + {name: "folds leading and trailing runs", in: "\t define owner \f", want: " define owner "}, + {name: "leaves other bytes alone", in: "a\vb", want: "a\vb"}, + {name: "empty line", in: "", want: ""}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + if got := NormalizeWhitespace(tt.in); got != tt.want { + t.Errorf("NormalizeWhitespace(%q) = %q, want %q", tt.in, got, tt.want) + } + }) + } +} + +func TestGetTypeLineNumber(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + typeName string + lines []string + want int + }{ + { + name: "finds the declaration", + typeName: "user", + lines: []string{"model", " schema 1.2", "type user", " relations"}, + want: 2, + }, + { + name: "does not match a type whose name extends the one asked for", + typeName: "document", + lines: []string{"type documentation", " relations", "type document", " relations"}, + want: 2, + }, + { + name: "allows a trailing module comment", + typeName: "other", + lines: []string{"type user", "type other # module: core, file: core.fga"}, + want: 1, + }, + { + // `#`, `/`, `.` and `-` are all name bytes or name terminators per the + // grammar's identifier rules; `#` terminates, so this resolves to the + // first line. `type doc#x` is not valid DSL, so it cannot reach here + // from a parsed model — the case is pinned only to document the choice. + name: "treats a glued hash as ending the name", + typeName: "doc", + lines: []string{"type doc#x", "type doc"}, + want: 0, + }, + { + // EXTENDED_IDENTIFIER admits `.`, `/` and `-` inside a name, so a + // module-qualified name must not match a longer one that shares its + // prefix. + name: "does not match a dotted name that extends the one asked for", + typeName: "core.doc", + lines: []string{"type core.doc2", "type core.doc"}, + want: 1, + }, + { + name: "does not match a hyphenated name that extends the one asked for", + typeName: "my-doc", + lines: []string{"type my-doc-archive", "type my-doc"}, + want: 1, + }, + { + name: "does not match a slashed name that extends the one asked for", + typeName: "internal", + lines: []string{"type internal/doc", "type internal"}, + want: 1, + }, + { + name: "does not match an extend declaration", + typeName: "user", + lines: []string{"extend type user", "type user"}, + want: 1, + }, + { + // WHITESPACE admits \f, so it must separate a name from its trailing + // comment as a space would; trimming only " \t" collapsed this to 0:0. + name: "allows a form feed before a trailing comment", + typeName: "other", + lines: []string{"type user", "type other\f# module: core, file: core.fga"}, + want: 1, + }, + { + // WHITESPACE is ('\t' | ' ' | '\u000C')+, so any of the three can + // separate `type` from the name. + name: "allows a tab between type and the name", + typeName: "user", + lines: []string{"model", "type\tuser"}, + want: 1, + }, + { + name: "allows a form feed between type and the name", + typeName: "user", + lines: []string{"model", "type\fuser"}, + want: 1, + }, + { + name: "collapses a mixed run of whitespace between type and the name", + typeName: "user", + lines: []string{"model", "type \t user"}, + want: 1, + }, + { + name: "returns -1 when absent", + typeName: "missing", + lines: []string{"type user", "type org"}, + want: -1, + }, + { + name: "returns -1 for no lines", + typeName: "user", + lines: nil, + want: -1, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + if got := GetTypeLineNumber(tt.typeName, tt.lines); got != tt.want { + t.Errorf("GetTypeLineNumber(%q) = %d, want %d", tt.typeName, got, tt.want) + } + }) + } +} + +func TestGetExtendedTypeLineNumber(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + typeName string + lines []string + want int + }{ + { + name: "finds the extend declaration", + typeName: "user", + lines: []string{"type user", "extend type user", " relations"}, + want: 1, + }, + { + name: "does not match a type whose name extends the one asked for", + typeName: "document", + lines: []string{"extend type documentation", "extend type document"}, + want: 1, + }, + { + name: "allows a trailing module comment", + typeName: "org", + lines: []string{"extend type org # module: org, file: org.fga"}, + want: 0, + }, + { + name: "allows a form feed before a trailing comment", + typeName: "org", + lines: []string{"extend type org\f# module: org, file: org.fga"}, + want: 0, + }, + { + name: "allows tabs between extend, type and the name", + typeName: "org", + lines: []string{"type org", "extend\ttype\torg"}, + want: 1, + }, + { + name: "returns -1 when absent", + typeName: "user", + lines: []string{"type user"}, + want: -1, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + if got := GetExtendedTypeLineNumber(tt.typeName, tt.lines); got != tt.want { + t.Errorf("GetExtendedTypeLineNumber(%q) = %d, want %d", tt.typeName, got, tt.want) + } + }) + } +} + +func TestGetRelationLineNumber(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + relation string + lines []string + want int + }{ + { + name: "finds the definition", + relation: "owner", + lines: []string{"type doc", " relations", " define owner: [user]"}, + want: 2, + }, + { + name: "does not match a relation whose name extends the one asked for", + relation: "owner", + lines: []string{" define owner_group: [group]", " define owner: [user]"}, + want: 1, + }, + { + name: "tolerates repeated spaces after define", + relation: "owner", + lines: []string{" define owner: [user]"}, + want: 0, + }, + { + name: "allows whitespace before the colon", + relation: "owner", + lines: []string{" define owner : [user]"}, + want: 0, + }, + { + // `define owner\f: [user]` parses, so it must be findable. + name: "allows a form feed before the colon", + relation: "owner", + lines: []string{" define owner\f: [user]"}, + want: 0, + }, + { + // `define\towner: [user]` parses — WHITESPACE admits tabs — so the + // definition must be findable. + name: "allows a tab between define and the name", + relation: "owner", + lines: []string{" define\towner: [user]"}, + want: 0, + }, + { + name: "allows a form feed between define and the name", + relation: "owner", + lines: []string{" define\fowner: [user]"}, + want: 0, + }, + { + name: "collapses a mixed run of whitespace between define and the name", + relation: "owner", + lines: []string{" define \t owner: [user]"}, + want: 0, + }, + { + name: "returns -1 when absent", + relation: "missing", + lines: []string{" define owner: [user]"}, + want: -1, + }, + { + name: "returns -1 for no lines", + relation: "owner", + lines: nil, + want: -1, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + if got := GetRelationLineNumber(tt.relation, tt.lines); got != tt.want { + t.Errorf("GetRelationLineNumber(%q) = %d, want %d", tt.relation, got, tt.want) + } + }) + } +} + +func TestGetConditionLineNumber(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + conditionName string + lines []string + want int + }{ + { + name: "finds the declaration", + conditionName: "in_range", + lines: []string{"type user", "condition in_range(x: int) {"}, + want: 1, + }, + { + name: "does not match a condition whose name extends the one asked for", + conditionName: "less", + lines: []string{"condition less_than(x: int) {", "condition less(x: int) {"}, + want: 1, + }, + { + name: "allows whitespace before the parameter list", + conditionName: "less", + lines: []string{"condition less (x: int) {"}, + want: 0, + }, + { + // `condition less\f(x: int) {` parses, so it must be findable. + name: "allows a form feed before the parameter list", + conditionName: "less", + lines: []string{"condition less\f(x: int) {"}, + want: 0, + }, + { + name: "allows a tab between condition and the name", + conditionName: "less", + lines: []string{"condition\tless(x: int) {"}, + want: 0, + }, + { + name: "collapses a mixed run of whitespace between condition and the name", + conditionName: "less", + lines: []string{"condition \t less(x: int) {"}, + want: 0, + }, + { + name: "returns -1 when absent", + conditionName: "missing", + lines: []string{"condition less(x: int) {"}, + want: -1, + }, + { + name: "returns -1 for no lines", + conditionName: "less", + lines: nil, + want: -1, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + if got := GetConditionLineNumber(tt.conditionName, tt.lines); got != tt.want { + t.Errorf("GetConditionLineNumber(%q) = %d, want %d", tt.conditionName, got, tt.want) + } + }) + } +} diff --git a/pkg/go/validation/name_validation.go b/pkg/go/validation/name_validation.go index 4b9a4150..0d5bd4c9 100644 --- a/pkg/go/validation/name_validation.go +++ b/pkg/go/validation/name_validation.go @@ -6,10 +6,12 @@ import ( "strings" openfgav1 "github.com/openfga/api/proto/openfga/v1" + + "github.com/openfga/language/pkg/go/utils" ) -// ValidationRegexRules contains the regex rules for validation -// These match the Rules from the JS implementation +// ValidationRegexRules contains the regex rules for validation. +// These match the Rules from the JS implementation. var ValidationRegexRules = struct { Type string Relation string @@ -40,8 +42,8 @@ var ( } ) -// ValidateTypeName validates a type name with both regex and reserved keyword checking -// This enhances the basic regex validation with semantic checks +// ValidateTypeName validates a type name with both regex and reserved keyword +// checking. This enhances the basic regex validation with semantic checks. func ValidateTypeName(typeName string, collector *ErrorCollector, lineIndex *int, meta *Meta) bool { // First check if it's a reserved keyword if IsReservedTypeName(typeName) { @@ -59,8 +61,8 @@ func ValidateTypeName(typeName string, collector *ErrorCollector, lineIndex *int return true } -// ValidateRelationName validates a relation name with both regex and reserved keyword checking -// This enhances the basic regex validation with semantic checks +// ValidateRelationName validates a relation name with both regex and reserved +// keyword checking. This enhances the basic regex validation with semantic checks. func ValidateRelationName(relationName, typeName string, collector *ErrorCollector, lineIndex *int, meta *Meta) bool { // First check if it's a reserved keyword if IsReservedRelationName(relationName) { @@ -101,8 +103,8 @@ func validateFieldValue(rule, value string) bool { return regex.MatchString(value) } -// GetTypeLineNumber finds the line number where a type is defined -// This is equivalent to the getTypeLineNumber function in JS +// GetTypeLineNumber finds the line number where a type is defined. +// This is equivalent to the getTypeLineNumber function in JS. func GetTypeLineNumber(typeName string, lines []string, skipIndex *int) *int { if len(lines) == 0 { return nil @@ -114,8 +116,9 @@ func GetTypeLineNumber(typeName string, lines []string, skipIndex *int) *int { continue } - // Look for "type typeName" pattern - trimmedLine := strings.TrimSpace(line) + // Look for "type typeName" pattern. Runs of inline whitespace are folded + // first so `type\tuser` passes the "type " prefix gate like `type user`. + trimmedLine := utils.NormalizeWhitespace(strings.TrimSpace(line)) if strings.HasPrefix(trimmedLine, "type ") { parts := strings.Fields(trimmedLine) if len(parts) >= 2 && parts[1] == typeName { @@ -144,8 +147,9 @@ func GetRelationLineNumber(relationName string, lines []string, skipIndex *int) } for i := start; i < len(lines); i++ { - // Look for "define relationName:" pattern - trimmedLine := strings.TrimSpace(lines[i]) + // Look for "define relationName:" pattern. Runs of inline whitespace are + // folded first so `define\towner:` is found like `define owner:`. + trimmedLine := utils.NormalizeWhitespace(strings.TrimSpace(lines[i])) if strings.HasPrefix(trimmedLine, "define ") { // Extract relation name from "define relationName:" definePart := strings.TrimPrefix(trimmedLine, "define ") @@ -162,8 +166,8 @@ func GetRelationLineNumber(relationName string, lines []string, skipIndex *int) return nil } -// GetConditionLineNumber finds the line number where a condition is defined -// This is equivalent to the geConditionLineNumber function in JS +// GetConditionLineNumber finds the line number where a condition is defined. +// This is equivalent to the geConditionLineNumber function in JS. func GetConditionLineNumber(conditionName string, lines []string, skipIndex *int) *int { if len(lines) == 0 { return nil @@ -176,16 +180,16 @@ func GetConditionLineNumber(conditionName string, lines []string, skipIndex *int conditionPrefix := "condition " + conditionName for i := start; i < len(lines); i++ { - // Match the condition declaration itself, mirroring the reference's - // `condition ` prefix check, so we don't match an unrelated line - // that merely contains the condition name as a substring. The parameter - // list's `(` must follow the name so a condition whose name is a prefix - // of another (e.g. `less` vs `less_than`) cannot match the wrong line. - trimmedLine := strings.TrimSpace(lines[i]) + // The name must end at the prefix, so `less` does not match a declaration + // of `less_than`. Inline whitespace is folded first so `condition\tless(` + // is found like `condition less(`. + trimmedLine := utils.NormalizeWhitespace(strings.TrimSpace(lines[i])) if !strings.HasPrefix(trimmedLine, conditionPrefix) { continue } - if strings.HasPrefix(strings.TrimLeft(trimmedLine[len(conditionPrefix):], " \t"), "(") { + + rest := trimmedLine[len(conditionPrefix):] + if rest == "" || !utils.IsNameByte(rest[0]) { return &i } } @@ -193,8 +197,8 @@ func GetConditionLineNumber(conditionName string, lines []string, skipIndex *int return nil } -// ValidateNameRules validates naming rules for types and relations in a model -// This is equivalent to the populateRelations function's naming validation in JS +// ValidateNameRules validates naming rules for types and relations in a model. +// This is equivalent to the populateRelations function's naming validation in JS. func ValidateNameRules(collector *ErrorCollector, typeName string, relationNames []string, typeLineIndex *int, meta *Meta, lines []string) { // Validate type name diff --git a/pkg/go/validation/name_validation_test.go b/pkg/go/validation/name_validation_test.go index 61520757..39714c09 100644 --- a/pkg/go/validation/name_validation_test.go +++ b/pkg/go/validation/name_validation_test.go @@ -6,7 +6,6 @@ import ( "github.com/stretchr/testify/assert" ) - func TestValidationRegexRules(t *testing.T) { // Test that regex rules match the JS implementation assert.Equal(t, "[^:#@\\*\\s]{1,254}", ValidationRegexRules.Type) @@ -341,6 +340,17 @@ func TestGetTypeLineNumber(t *testing.T) { skipIndex: ptrInt(0), expected: ptrInt(2), }, + { + // Tabs are folded before the `type ` prefix gate, so a tab after + // `type` works. + name: "finds type separated by a tab", + typeName: "user", + lines: []string{ + "model", + "type\tuser", + }, + expected: ptrInt(1), + }, { name: "empty lines", typeName: "document", @@ -421,6 +431,28 @@ func TestGetRelationLineNumber(t *testing.T) { skipIndex: ptrInt(1), expected: ptrInt(2), }, + { + // WHITESPACE is ('\t' | ' ' | '\u000C')+, so `define\tviewer:` parses + // and must be findable. + name: "finds relation separated by a tab", + relationName: "viewer", + lines: []string{ + "type document", + " relations", + " define\tviewer: [user]", + }, + expected: ptrInt(2), + }, + { + name: "finds relation separated by a mixed whitespace run", + relationName: "viewer", + lines: []string{ + "type document", + " relations", + " define \t viewer: [user]", + }, + expected: ptrInt(2), + }, { name: "empty lines", relationName: "viewer", @@ -489,6 +521,15 @@ func TestGetConditionLineNumber(t *testing.T) { skipIndex: ptrInt(1), expected: ptrInt(2), }, + { + name: "finds condition separated by a tab", + conditionName: "is_owner", + lines: []string{ + "type document", + "condition\tis_owner(x: int) {", + }, + expected: ptrInt(1), + }, { name: "empty lines", conditionName: "is_owner", diff --git a/pkg/go/validation/schema_validation.go b/pkg/go/validation/schema_validation.go index 9e2995fa..d80f0462 100644 --- a/pkg/go/validation/schema_validation.go +++ b/pkg/go/validation/schema_validation.go @@ -5,6 +5,8 @@ import ( "strings" openfgav1 "github.com/openfga/api/proto/openfga/v1" + + "github.com/openfga/language/pkg/go/utils" ) const ( @@ -17,10 +19,6 @@ var SupportedSchemaVersions = map[string]bool{ SchemaVersion12: true, } -// multiSpaceRegex collapses runs of whitespace when normalizing a DSL line for -// schema-version matching. Hoisted so it is compiled once, not per line. -var multiSpaceRegex = regexp.MustCompile(`\s{2,}`) - func IsValidSchemaVersion(version string) bool { return SupportedSchemaVersions[version] } @@ -29,11 +27,14 @@ func GetSchemaLineNumber(schemaVersion string, lines []string) *int { if len(lines) == 0 { return nil } - pattern := `^\s*schema\s+` + regexp.QuoteMeta(schemaVersion) + `\s*$` + // A trailing comment may follow the version (`schema 1.5 # note`), matching the + // reference's `(\s+#.*)?`. + pattern := `^\s*schema\s+` + regexp.QuoteMeta(schemaVersion) + `(\s+#.*)?$` regex := regexp.MustCompile(pattern) for i, line := range lines { - normalizedLine := strings.TrimSpace(line) - normalizedLine = multiSpaceRegex.ReplaceAllString(normalizedLine, " ") + // Fold inline whitespace with the helper the other line-number lookups + // use, so `schema\t1.5` resolves like `schema 1.5`. + normalizedLine := utils.NormalizeWhitespace(strings.TrimSpace(line)) if regex.MatchString(normalizedLine) { return &i } diff --git a/pkg/go/validation/yaml_test_integration_test.go b/pkg/go/validation/yaml_test_integration_test.go index e04b3cb1..16f7ad15 100644 --- a/pkg/go/validation/yaml_test_integration_test.go +++ b/pkg/go/validation/yaml_test_integration_test.go @@ -235,16 +235,17 @@ func (runner *YAMLTestRunner) errorsMatch(expected YAMLExpectedError, actual *Va } } - // Check line numbers if specified - if expected.Line.Start > 0 && actual.Line != nil { - if actual.Line.Start != expected.Line.Start { + // Check line numbers if specified. A case that pins a line requires the error + // to carry one, so an error with no position at all is a mismatch. + if expected.Line.Start > 0 { + if actual.Line == nil || actual.Line.Start != expected.Line.Start { return false } } // Check column numbers if specified - if expected.Column.Start > 0 && actual.Column != nil { - if actual.Column.Start != expected.Column.Start { + if expected.Column.Start > 0 { + if actual.Column == nil || actual.Column.Start != expected.Column.Start { return false } } diff --git a/pkg/java/src/main/java/dev/openfga/language/validation/Dsl.java b/pkg/java/src/main/java/dev/openfga/language/validation/Dsl.java index 47de85c9..77b1f263 100644 --- a/pkg/java/src/main/java/dev/openfga/language/validation/Dsl.java +++ b/pkg/java/src/main/java/dev/openfga/language/validation/Dsl.java @@ -31,6 +31,14 @@ private int findLine(Predicate predicate, int skipIndex) { .orElse(-1); } + // Collapse every run of inline whitespace into one space — space, tab and form + // feed are exactly what the lexer's WHITESPACE rule admits between tokens + // (OpenFGALexer.g4), so `define\towner:` must be found like `define owner:`. + // Anything else (nbsp, vertical tab) fails to lex and can never reach a lookup. + private static String normalizeWhitespace(String line) { + return line.trim().replaceAll("[ \\t\\f]+", " "); + } + public int getConditionLineNumber(String conditionName) { return getConditionLineNumber(conditionName, 0); } @@ -39,16 +47,15 @@ public int getConditionLineNumber(String conditionName, int skipIndex) { // Require `(` after the name so a condition name that is a prefix of // another (e.g. `less` vs `less_than`) cannot match the wrong line. return findLine( - line -> line.trim().matches("condition " + Pattern.quote(conditionName) + "\\s*\\(.*"), skipIndex); + line -> normalizeWhitespace(line).matches("condition " + Pattern.quote(conditionName) + "\\s*\\(.*"), + skipIndex); } public int getRelationLineNumber(String relationName, int skipIndex) { // Require `:` after the name so a relation name that is a prefix of // another (e.g. `writer` vs `writers`) cannot match the wrong line. return findLine( - line -> line.trim() - .replaceAll(" {2,}", " ") - .matches("define " + Pattern.quote(relationName) + "\\s*:.*"), + line -> normalizeWhitespace(line).matches("define " + Pattern.quote(relationName) + "\\s*:.*"), skipIndex); } @@ -57,10 +64,7 @@ public int getSchemaLineNumber(String schemaVersion) { // e.g. `1.1` cannot match `schema 1.10`. A comment must be preceded by // whitespace so a `#` glued to the version isn't treated as a comment. return findLine( - line -> line.trim() - .replaceAll(" {2,}", " ") - .matches("schema " + Pattern.quote(schemaVersion) + "(\\s+#.*)?"), - 0); + line -> normalizeWhitespace(line).matches("schema " + Pattern.quote(schemaVersion) + "(\\s+#.*)?"), 0); } public int getTypeLineNumber(String typeName) { @@ -71,7 +75,8 @@ public int getTypeLineNumber(String typeName, int skipIndex) { // Allow an optional trailing comment (e.g. `type page # module: ...`) after the type name. // The comment must be preceded by whitespace so a `#` glued to the name isn't treated as a comment. // Quote the type name so regex metacharacters (e.g. `.`) are matched literally. - return findLine(line -> line.trim().matches("type " + Pattern.quote(typeName) + "(\\s+#.*)?"), skipIndex); + return findLine( + line -> normalizeWhitespace(line).matches("type " + Pattern.quote(typeName) + "(\\s+#.*)?"), skipIndex); } public static String getRelationDefName(Userset userset) { diff --git a/pkg/js/util/line-numbers.ts b/pkg/js/util/line-numbers.ts index 4411534c..396eba15 100644 --- a/pkg/js/util/line-numbers.ts +++ b/pkg/js/util/line-numbers.ts @@ -1,3 +1,9 @@ +// Collapse every run of inline whitespace into one space — space, tab and form +// feed are exactly what the lexer's WHITESPACE rule admits between tokens +// (OpenFGALexer.g4), so `define\towner:` must be found like `define owner:`. +// Anything else (nbsp, vertical tab) fails to lex and can never reach a lookup. +const normalizeWhitespace = (line: string) => line.trim().replace(/[ \t\f]+/g, " "); + export const getConditionLineNumber = (conditionName: string, lines?: string[], skipIndex?: number) => { if (!skipIndex || skipIndex < 0) { skipIndex = 0; @@ -9,8 +15,8 @@ export const getConditionLineNumber = (conditionName: string, lines?: string[], // (e.g. `less` vs `less_than`) cannot match the wrong line. const conditionPrefix = `condition ${conditionName}`; const index = lines.slice(skipIndex).findIndex((line: string) => { - const trimmed = line.trim(); - return trimmed.startsWith(conditionPrefix) && /^\s*\(/.test(trimmed.slice(conditionPrefix.length)); + const normalized = normalizeWhitespace(line); + return normalized.startsWith(conditionPrefix) && /^\s*\(/.test(normalized.slice(conditionPrefix.length)); }); return index === -1 ? -1 : index + skipIndex; }; @@ -27,8 +33,8 @@ export const getTypeLineNumber = (typeName: string, lines?: string[], skipIndex? // Match the type name literally (it may contain regex metacharacters like `.`). const typePrefix = `${extension ? "extend " : ""}type ${typeName}`; const index = lines.slice(skipIndex).findIndex((line: string) => { - const trimmed = line.trim(); - return trimmed.startsWith(typePrefix) && /^(\s+#.*)?$/.test(trimmed.slice(typePrefix.length)); + const normalized = normalizeWhitespace(line); + return normalized.startsWith(typePrefix) && /^(\s+#.*)?$/.test(normalized.slice(typePrefix.length)); }); return index === -1 ? -1 : index + skipIndex; }; @@ -43,7 +49,7 @@ export const getRelationLineNumber = (relation: string, lines?: string[], skipIn // Match the relation name literally (it may contain regex metacharacters like `.`). const relationPrefix = `define ${relation}`; const index = lines.slice(skipIndex).findIndex((line: string) => { - const normalized = line.trim().replace(/ {2,}/g, " "); + const normalized = normalizeWhitespace(line); return normalized.startsWith(relationPrefix) && /^\s*:/.test(normalized.slice(relationPrefix.length)); }); return index === -1 ? -1 : index + skipIndex; diff --git a/pkg/js/validator/validate-dsl.ts b/pkg/js/validator/validate-dsl.ts index a52f90ac..74bad1dc 100644 --- a/pkg/js/validator/validate-dsl.ts +++ b/pkg/js/validator/validate-dsl.ts @@ -291,6 +291,12 @@ function hasEntryPointOrLoop( return { hasEntry: false, loop: false }; } +// Collapse every run of inline whitespace into one space — space, tab and form +// feed are exactly what the lexer's WHITESPACE rule admits between tokens +// (OpenFGALexer.g4), so `define\towner:` must be found like `define owner:`. +// Anything else (nbsp, vertical tab) fails to lex and can never reach a lookup. +const normalizeWhitespace = (line: string) => line.trim().replace(/[ \t\f]+/g, " "); + const geConditionLineNumber = (conditionName: string, lines?: string[], skipIndex?: number) => { if (!skipIndex || skipIndex < 0) { skipIndex = 0; @@ -302,8 +308,8 @@ const geConditionLineNumber = (conditionName: string, lines?: string[], skipInde // (e.g. `less` vs `less_than`) cannot match the wrong line. const conditionPrefix = `condition ${conditionName}`; const index = lines.slice(skipIndex).findIndex((line: string) => { - const trimmed = line.trim(); - return trimmed.startsWith(conditionPrefix) && /^\s*\(/.test(trimmed.slice(conditionPrefix.length)); + const normalized = normalizeWhitespace(line); + return normalized.startsWith(conditionPrefix) && /^\s*\(/.test(normalized.slice(conditionPrefix.length)); }); return index === -1 ? -1 : index + skipIndex; }; @@ -320,8 +326,8 @@ const getTypeLineNumber = (typeName: string, lines?: string[], skipIndex?: numbe // Match the type name literally (it may contain regex metacharacters like `.`). const typePrefix = `type ${typeName}`; const index = lines.slice(skipIndex).findIndex((line: string) => { - const trimmed = line.trim(); - return trimmed.startsWith(typePrefix) && /^(\s+#.*)?$/.test(trimmed.slice(typePrefix.length)); + const normalized = normalizeWhitespace(line); + return normalized.startsWith(typePrefix) && /^(\s+#.*)?$/.test(normalized.slice(typePrefix.length)); }); return index === -1 ? -1 : index + skipIndex; }; @@ -336,7 +342,7 @@ const getRelationLineNumber = (relation: string, lines?: string[], skipIndex?: n // Match the relation name literally (it may contain regex metacharacters like `.`). const relationPrefix = `define ${relation}`; const index = lines.slice(skipIndex).findIndex((line: string) => { - const normalized = line.trim().replace(/ {2,}/g, " "); + const normalized = normalizeWhitespace(line); return normalized.startsWith(relationPrefix) && /^\s*:/.test(normalized.slice(relationPrefix.length)); }); return index === -1 ? -1 : index + skipIndex; @@ -352,7 +358,7 @@ const getSchemaLineNumber = (schema: string, lines?: string[]) => { // Match the schema version literally (it contains `.`). const schemaPrefix = `schema ${schema}`; const index = lines.slice(0).findIndex((line: string) => { - const normalized = line.trim().replace(/ {2,}/g, " "); + const normalized = normalizeWhitespace(line); return normalized.startsWith(schemaPrefix) && /^(\s+#.*)?$/.test(normalized.slice(schemaPrefix.length)); }); diff --git a/tests/data/dsl-semantic-validation-cases.yaml b/tests/data/dsl-semantic-validation-cases.yaml index d788997a..5c288733 100644 --- a/tests/data/dsl-semantic-validation-cases.yaml +++ b/tests/data/dsl-semantic-validation-cases.yaml @@ -2275,3 +2275,85 @@ metadata: symbol: "less" errorType: condition-not-used + +# The four cases below contain literal tab characters (shown as \t in the names) +# between a declaration keyword and its name. Tabs and form feeds are valid +# separators per the lexer's WHITESPACE rule, so every SDK must fold them when +# locating declaration lines. Keep the tabs when editing: replacing them with +# spaces would stop these cases from guarding that behaviour. +- name: tab separator in define\treader is folded when locating the error line + dsl: | + model + schema 1.1 + type user + type group + relations + define member: [user] + define reader: member but not allowed + expected_errors: + - msg: "the relation `allowed` does not exist." + line: + start: 6 + end: 6 + column: + start: 34 + end: 41 + metadata: + symbol: "allowed" + errorType: missing-definition +- name: tab separator in type\tself is folded when locating the error line + dsl: | + model + schema 1.1 + type user + type self + relations + define member: [user] + expected_errors: + - msg: "a type cannot be named 'self' or 'this'." + line: + start: 3 + end: 3 + column: + start: 5 + end: 9 + metadata: + symbol: "self" + errorType: reserved-type-keywords +- name: tab separator in condition\tallowed_ip is folded when locating the error line + dsl: | + model + schema 1.1 + type user + type document + relations + define viewer: [user] + condition allowed_ip(current_ip: ipaddress) { + current_ip.in_cidr("192.168.0.0/24") + } + expected_errors: + - msg: "`allowed_ip` condition is not used in the model." + line: + start: 6 + end: 6 + column: + start: 10 + end: 20 + metadata: + symbol: "allowed_ip" + errorType: condition-not-used +- name: tab separator in schema\t1.5 is folded when locating the error line + dsl: | + model + schema 1.5 + type user + expected_errors: + - msg: "invalid schema 1.5" + line: + start: 1 + end: 1 + column: + start: 9 + end: 12 + metadata: + errorType: invalid-schema diff --git a/tests/data/transformer-module/07-prefix-collisions/expected_errors.json b/tests/data/transformer-module/07-prefix-collisions/expected_errors.json new file mode 100644 index 00000000..6f9cf718 --- /dev/null +++ b/tests/data/transformer-module/07-prefix-collisions/expected_errors.json @@ -0,0 +1,14 @@ +[ + { + "msg": "duplicate condition less", + "file": "dup.fga", + "line": { + "start": 10, + "end": 10 + }, + "column": { + "start": 10, + "end": 14 + } + } +] diff --git a/tests/data/transformer-module/07-prefix-collisions/module/core.fga b/tests/data/transformer-module/07-prefix-collisions/module/core.fga new file mode 100644 index 00000000..869bdcb8 --- /dev/null +++ b/tests/data/transformer-module/07-prefix-collisions/module/core.fga @@ -0,0 +1,11 @@ +module core + +type user + +type documentation + relations + define viewer: [user with less] + +condition less(val: int) { + val < 5 +} diff --git a/tests/data/transformer-module/07-prefix-collisions/module/dup.fga b/tests/data/transformer-module/07-prefix-collisions/module/dup.fga new file mode 100644 index 00000000..01af4422 --- /dev/null +++ b/tests/data/transformer-module/07-prefix-collisions/module/dup.fga @@ -0,0 +1,13 @@ +module dup + +type document + relations + define viewer: [user with less_than] + +condition less_than(val: int) { + val < 10 +} + +condition less(val: int) { + val < 3 +} diff --git a/tests/data/transformer-module/08-tab-separated-declarations/expected_errors.json b/tests/data/transformer-module/08-tab-separated-declarations/expected_errors.json new file mode 100644 index 00000000..fdac24ab --- /dev/null +++ b/tests/data/transformer-module/08-tab-separated-declarations/expected_errors.json @@ -0,0 +1,14 @@ +[ + { + "msg": "relation owner already exists on type document", + "file": "dup.fga", + "line": { + "start": 4, + "end": 4 + }, + "column": { + "start": 11, + "end": 16 + } + } +] diff --git a/tests/data/transformer-module/08-tab-separated-declarations/module/core.fga b/tests/data/transformer-module/08-tab-separated-declarations/module/core.fga new file mode 100644 index 00000000..0d11eddb --- /dev/null +++ b/tests/data/transformer-module/08-tab-separated-declarations/module/core.fga @@ -0,0 +1,7 @@ +module core + +type user + +type document + relations + define owner: [user] diff --git a/tests/data/transformer-module/08-tab-separated-declarations/module/dup.fga b/tests/data/transformer-module/08-tab-separated-declarations/module/dup.fga new file mode 100644 index 00000000..831d3f7e --- /dev/null +++ b/tests/data/transformer-module/08-tab-separated-declarations/module/dup.fga @@ -0,0 +1,5 @@ +module dup + +extend type document + relations + define owner: [user]