From 4c6c609437a460ea3640d082f38b1e9e0d580ff6 Mon Sep 17 00:00:00 2001 From: Anurag Bandyopadhyay Date: Mon, 3 Aug 2026 19:40:34 +0530 Subject: [PATCH 1/7] fix(pkg/go/utils): match declarations by full name, not by prefix The line-number helpers in pkg/go/utils matched a declaration with strings.HasPrefix on the name alone, so a name that is a prefix of another resolved to the wrong line: asking for `document` matched `type documentation`, `owner` matched `define owner_group:`, and `less` matched `condition less_than(`. Each helper now validates what follows the name, mirroring the JS reference in pkg/js/validator/validate-dsl.ts: - types and extended types accept only end-of-line or a trailing comment, which the module fixtures rely on (`type other # module: core, file: core.fga`). The `#` must be preceded by whitespace, so `type doc#x` is not a match for `doc`. - relations require a `:` after the name, and runs of spaces are collapsed first so `define owner:` matches as `define owner:`. - conditions require the parameter list's `(` after the name. Signatures, the `int` return and the `-1` miss sentinel are all unchanged, so the only visible difference is that the transformer's duplicate-type, duplicate-condition, extended-type and duplicate- relation errors now point at the correct line in these collision cases. No error identity or message text changes. This also brings the helpers into agreement with the equivalents in pkg/go/validation, which already matched exactly; a differential check over 540 comparisons finds no remaining divergence. That agreement is what lets the two copies be unified without a further behaviour change. --- pkg/go/utils/line-numbers.go | 78 +++++++++-- pkg/go/utils/line-numbers_test.go | 223 ++++++++++++++++++++++++++++++ 2 files changed, 292 insertions(+), 9 deletions(-) create mode 100644 pkg/go/utils/line-numbers_test.go diff --git a/pkg/go/utils/line-numbers.go b/pkg/go/utils/line-numbers.go index 3e842a78..9be7b87f 100644 --- a/pkg/go/utils/line-numbers.go +++ b/pkg/go/utils/line-numbers.go @@ -5,27 +5,87 @@ import ( "strings" ) -func GetConditionLineNumber(conditionName string, lines []string) int { +// declarationIndex returns the index of the first line that, once trimmed, begins +// with prefix and whose remainder satisfies rest. Requiring the caller to validate +// the remainder is what keeps a declaration whose name is a prefix of another +// (e.g. `type document` vs `type documentation`) from matching the wrong line. +func declarationIndex(lines []string, prefix string, rest func(string) bool) int { return slices.IndexFunc(lines, func(line string) bool { - return strings.HasPrefix(strings.TrimSpace(line), "condition "+conditionName) + trimmed := strings.TrimSpace(line) + if !strings.HasPrefix(trimmed, prefix) { + return false + } + + return rest(trimmed[len(prefix):]) }) } -func GetTypeLineNumber(typeName string, lines []string) int { - return slices.IndexFunc(lines, func(line string) bool { - return strings.HasPrefix(strings.TrimSpace(line), "type "+typeName) +// endOrComment reports whether the remainder of a declaration line is empty or is +// only a trailing comment, mirroring the reference's `^(\s+#.*)?$`. The `#` must be +// preceded by whitespace so a `#` glued to the name isn't treated as a comment — +// `type doc#x` is not a declaration of `doc`. +func endOrComment(rest string) bool { + if rest == "" { + return true + } + + trimmed := strings.TrimLeft(rest, " \t") + if trimmed == rest { + return false + } + + return strings.HasPrefix(trimmed, "#") +} + +// startsWith reports whether rest begins with sep, ignoring leading whitespace. +func startsWith(rest, sep string) bool { + return strings.HasPrefix(strings.TrimLeft(rest, " \t"), sep) +} + +// normalizeSpaces collapses runs of spaces into one, mirroring the reference's +// ` {2,}` normalization, so `define owner:` is matched the same as `define owner:`. +func normalizeSpaces(line string) string { + for strings.Contains(line, " ") { + line = strings.ReplaceAll(line, " ", " ") + } + + return line +} + +// GetConditionLineNumber returns the index of the line declaring conditionName, or +// -1. The parameter list's `(` must follow the name, so `less` does not match a +// declaration of `less_than`. +func GetConditionLineNumber(conditionName string, lines []string) int { + return declarationIndex(lines, "condition "+conditionName, func(rest string) bool { + return startsWith(rest, "(") }) } +// GetTypeLineNumber returns the index of the line declaring typeName, or -1. Only a +// trailing comment may follow the name, so `document` does not match a declaration +// of `documentation`. +func GetTypeLineNumber(typeName string, lines []string) int { + return declarationIndex(lines, "type "+typeName, endOrComment) +} + +// GetExtendedTypeLineNumber returns the index of the line extending typeName, or -1. +// The name must be followed only by a trailing comment, as in GetTypeLineNumber. 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, endOrComment) } +// GetRelationLineNumber returns the index of the line defining relation, or -1. The +// `:` must follow the name, so `owner` does not match a definition of `owner_group`. func GetRelationLineNumber(relation string, lines []string) int { + prefix := "define " + relation + return slices.IndexFunc(lines, func(line string) bool { - return strings.HasPrefix(strings.TrimSpace(line), "define "+relation) + normalized := normalizeSpaces(strings.TrimSpace(line)) + if !strings.HasPrefix(normalized, prefix) { + return false + } + + return startsWith(normalized[len(prefix):], ":") }) } diff --git a/pkg/go/utils/line-numbers_test.go b/pkg/go/utils/line-numbers_test.go new file mode 100644 index 00000000..c55b4638 --- /dev/null +++ b/pkg/go/utils/line-numbers_test.go @@ -0,0 +1,223 @@ +package utils + +import "testing" + +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, + }, + { + name: "does not treat a glued hash as a comment", + typeName: "doc", + lines: []string{"type doc#x", "type doc"}, + want: 1, + }, + { + name: "does not match an extend declaration", + typeName: "user", + lines: []string{"extend type user", "type 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: "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, + }, + { + 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, + }, + { + 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) + } + }) + } +} From a165beccf43f4e742647ae550916786a3c5a3928 Mon Sep 17 00:00:00 2001 From: Anurag Bandyopadhyay Date: Wed, 5 Aug 2026 17:51:48 +0530 Subject: [PATCH 2/7] fix: address review comments --- pkg/go/utils/line-numbers.go | 12 ++++++++++-- pkg/go/utils/line-numbers_test.go | 28 ++++++++++++++++++++++++++++ pkg/go/validation/name_validation.go | 4 +++- 3 files changed, 41 insertions(+), 3 deletions(-) diff --git a/pkg/go/utils/line-numbers.go b/pkg/go/utils/line-numbers.go index 9be7b87f..b7b076b3 100644 --- a/pkg/go/utils/line-numbers.go +++ b/pkg/go/utils/line-numbers.go @@ -5,6 +5,14 @@ import ( "strings" ) +// intraLineWhitespace is the set of characters the grammar's WHITESPACE token +// admits within a line: `WHITESPACE: ( '\t' | ' ' | '\u000C')+;` (OpenFGALexer.g4). +// `\f` (U+000C) belongs here because the lexer prefers WHITESPACE over NEWLINE for +// a bare `\f`, so `define owner\f: [user]` is a single valid line. Trimming a +// narrower set would make such a declaration unfindable and collapse its reported +// location to 0:0. +const intraLineWhitespace = " \t\f" + // declarationIndex returns the index of the first line that, once trimmed, begins // with prefix and whose remainder satisfies rest. Requiring the caller to validate // the remainder is what keeps a declaration whose name is a prefix of another @@ -29,7 +37,7 @@ func endOrComment(rest string) bool { return true } - trimmed := strings.TrimLeft(rest, " \t") + trimmed := strings.TrimLeft(rest, intraLineWhitespace) if trimmed == rest { return false } @@ -39,7 +47,7 @@ func endOrComment(rest string) bool { // startsWith reports whether rest begins with sep, ignoring leading whitespace. func startsWith(rest, sep string) bool { - return strings.HasPrefix(strings.TrimLeft(rest, " \t"), sep) + return strings.HasPrefix(strings.TrimLeft(rest, intraLineWhitespace), sep) } // normalizeSpaces collapses runs of spaces into one, mirroring the reference's diff --git a/pkg/go/utils/line-numbers_test.go b/pkg/go/utils/line-numbers_test.go index c55b4638..4a30f658 100644 --- a/pkg/go/utils/line-numbers_test.go +++ b/pkg/go/utils/line-numbers_test.go @@ -41,6 +41,14 @@ func TestGetTypeLineNumber(t *testing.T) { 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, + }, { name: "returns -1 when absent", typeName: "missing", @@ -93,6 +101,12 @@ func TestGetExtendedTypeLineNumber(t *testing.T) { 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: "returns -1 when absent", typeName: "user", @@ -145,6 +159,13 @@ func TestGetRelationLineNumber(t *testing.T) { 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, + }, { name: "returns -1 when absent", relation: "missing", @@ -197,6 +218,13 @@ func TestGetConditionLineNumber(t *testing.T) { 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: "returns -1 when absent", conditionName: "missing", diff --git a/pkg/go/validation/name_validation.go b/pkg/go/validation/name_validation.go index 4b9a4150..b85be14b 100644 --- a/pkg/go/validation/name_validation.go +++ b/pkg/go/validation/name_validation.go @@ -185,7 +185,9 @@ func GetConditionLineNumber(conditionName string, lines []string, skipIndex *int if !strings.HasPrefix(trimmedLine, conditionPrefix) { continue } - if strings.HasPrefix(strings.TrimLeft(trimmedLine[len(conditionPrefix):], " \t"), "(") { + // The cutset must match the grammar's WHITESPACE token, which admits `\f` + // as well as spaces and tabs; see utils.intraLineWhitespace. + if strings.HasPrefix(strings.TrimLeft(trimmedLine[len(conditionPrefix):], " \t\f"), "(") { return &i } } From 5569c1c246bd1dc3e9a5a843fe6f3ac82451eb16 Mon Sep 17 00:00:00 2001 From: Anurag Bandyopadhyay Date: Thu, 6 Aug 2026 11:25:37 +0530 Subject: [PATCH 3/7] refactor: assert a word terminator instead of a per-kind tail The declaration helpers took a rest predicate so each caller could validate what followed the name. prefix always spans whole words, so the invariant is simply that the name ends there: check the next byte against the grammar's identifier rules instead. Drops rest, endOrComment, startsWith and intraLineWhitespace. \f needs no special handling now, since it is a name terminator like any other non-identifier byte. normalizeSpaces stays on the relation helper only, matching the reference's ' {2,}' handling. The condition lookup in pkg/go/validation shares the same helper rather than keeping a second copy of the rule. --- pkg/go/utils/line-numbers.go | 98 +++++++++++----------------- pkg/go/utils/line-numbers_test.go | 21 +++++- pkg/go/validation/name_validation.go | 15 ++--- 3 files changed, 66 insertions(+), 68 deletions(-) diff --git a/pkg/go/utils/line-numbers.go b/pkg/go/utils/line-numbers.go index b7b076b3..a7719de4 100644 --- a/pkg/go/utils/line-numbers.go +++ b/pkg/go/utils/line-numbers.go @@ -5,51 +5,45 @@ import ( "strings" ) -// intraLineWhitespace is the set of characters the grammar's WHITESPACE token -// admits within a line: `WHITESPACE: ( '\t' | ' ' | '\u000C')+;` (OpenFGALexer.g4). -// `\f` (U+000C) belongs here because the lexer prefers WHITESPACE over NEWLINE for -// a bare `\f`, so `define owner\f: [user]` is a single valid line. Trimming a -// narrower set would make such a declaration unfindable and collapse its reported -// location to 0:0. -const intraLineWhitespace = " \t\f" - -// declarationIndex returns the index of the first line that, once trimmed, begins -// with prefix and whose remainder satisfies rest. Requiring the caller to validate -// the remainder is what keeps a declaration whose name is a prefix of another -// (e.g. `type document` vs `type documentation`) from matching the wrong line. -func declarationIndex(lines []string, prefix string, rest func(string) bool) int { - return slices.IndexFunc(lines, func(line string) bool { - trimmed := strings.TrimSpace(line) - if !strings.HasPrefix(trimmed, prefix) { - return false - } - - return rest(trimmed[len(prefix):]) - }) -} - -// endOrComment reports whether the remainder of a declaration line is empty or is -// only a trailing comment, mirroring the reference's `^(\s+#.*)?$`. The `#` must be -// preceded by whitespace so a `#` glued to the name isn't treated as a comment — -// `type doc#x` is not a declaration of `doc`. -func endOrComment(rest string) bool { - if rest == "" { +// IsNameByte reports whether b can appear inside a declaration name. 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. +func IsNameByte(b byte) bool { + switch { + case b >= 'a' && b <= 'z', b >= 'A' && b <= 'Z', b >= '0' && b <= '9': return true } - trimmed := strings.TrimLeft(rest, intraLineWhitespace) - if trimmed == rest { - return false + switch b { + case '_', '-', '/', '.': + return true } - return strings.HasPrefix(trimmed, "#") + return false } -// startsWith reports whether rest begins with sep, ignoring leading whitespace. -func startsWith(rest, sep string) bool { - return strings.HasPrefix(strings.TrimLeft(rest, intraLineWhitespace), sep) +// 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, normalize func(string) string) int { + return slices.IndexFunc(lines, func(line string) bool { + trimmed := normalize(strings.TrimSpace(line)) + if !strings.HasPrefix(trimmed, prefix) { + return false + } + + rest := trimmed[len(prefix):] + + return rest == "" || !IsNameByte(rest[0]) + }) } +// keepSpaces leaves a line as-is. Only the relation helper collapses repeated +// spaces, matching the reference, where ` {2,}` normalization is applied in +// getRelationLineNumber alone. +func keepSpaces(line string) string { return line } + // normalizeSpaces collapses runs of spaces into one, mirroring the reference's // ` {2,}` normalization, so `define owner:` is matched the same as `define owner:`. func normalizeSpaces(line string) string { @@ -61,40 +55,26 @@ func normalizeSpaces(line string) string { } // GetConditionLineNumber returns the index of the line declaring conditionName, or -// -1. The parameter list's `(` must follow the name, so `less` does not match a -// declaration of `less_than`. +// -1. `less` does not match a declaration of `less_than`. func GetConditionLineNumber(conditionName string, lines []string) int { - return declarationIndex(lines, "condition "+conditionName, func(rest string) bool { - return startsWith(rest, "(") - }) + return declarationIndex(lines, "condition "+conditionName, keepSpaces) } -// GetTypeLineNumber returns the index of the line declaring typeName, or -1. Only a -// trailing comment may follow the name, so `document` does not match a declaration -// of `documentation`. +// 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 declarationIndex(lines, "type "+typeName, endOrComment) + return declarationIndex(lines, "type "+typeName, keepSpaces) } // GetExtendedTypeLineNumber returns the index of the line extending typeName, or -1. -// The name must be followed only by a trailing comment, as in GetTypeLineNumber. func GetExtendedTypeLineNumber(typeName string, lines []string) int { - return declarationIndex(lines, "extend type "+typeName, endOrComment) + return declarationIndex(lines, "extend type "+typeName, keepSpaces) } -// GetRelationLineNumber returns the index of the line defining relation, or -1. The -// `:` must follow the name, so `owner` does not match a definition of `owner_group`. +// 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 { - prefix := "define " + relation - - return slices.IndexFunc(lines, func(line string) bool { - normalized := normalizeSpaces(strings.TrimSpace(line)) - if !strings.HasPrefix(normalized, prefix) { - return false - } - - return startsWith(normalized[len(prefix):], ":") - }) + return declarationIndex(lines, "define "+relation, normalizeSpaces) } type StartEnd struct { diff --git a/pkg/go/utils/line-numbers_test.go b/pkg/go/utils/line-numbers_test.go index 4a30f658..b0d7381b 100644 --- a/pkg/go/utils/line-numbers_test.go +++ b/pkg/go/utils/line-numbers_test.go @@ -30,9 +30,28 @@ func TestGetTypeLineNumber(t *testing.T) { want: 1, }, { - name: "does not treat a glued hash as a comment", + // `#`, `/`, `.` 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, }, { diff --git a/pkg/go/validation/name_validation.go b/pkg/go/validation/name_validation.go index b85be14b..03462800 100644 --- a/pkg/go/validation/name_validation.go +++ b/pkg/go/validation/name_validation.go @@ -6,6 +6,8 @@ import ( "strings" openfgav1 "github.com/openfga/api/proto/openfga/v1" + + "github.com/openfga/language/pkg/go/utils" ) // ValidationRegexRules contains the regex rules for validation @@ -176,18 +178,15 @@ 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. + // The name must end at the prefix, so `less` does not match a declaration + // of `less_than`. trimmedLine := strings.TrimSpace(lines[i]) if !strings.HasPrefix(trimmedLine, conditionPrefix) { continue } - // The cutset must match the grammar's WHITESPACE token, which admits `\f` - // as well as spaces and tabs; see utils.intraLineWhitespace. - if strings.HasPrefix(strings.TrimLeft(trimmedLine[len(conditionPrefix):], " \t\f"), "(") { + + rest := trimmedLine[len(conditionPrefix):] + if rest == "" || !utils.IsNameByte(rest[0]) { return &i } } From 0ac431c527abdd60a51f4cd3548bcf1cd0d08538 Mon Sep 17 00:00:00 2001 From: Anurag Bandyopadhyay Date: Thu, 6 Aug 2026 11:53:19 +0530 Subject: [PATCH 4/7] test: cover declaration prefix collisions in the shared corpus The prefix-matching fix had Go-only coverage, while the JS helpers carry the same rule. tests/data/transformer-module is read by both, so a case there pins the behaviour for each. dup.fga declares less_than before less, so a lookup that matches on prefix alone resolves the duplicate to the wrong line. Verified by reintroducing the bug in each implementation: Go and JS both report line 6 instead of 10. --- .../07-prefix-collisions/expected_errors.json | 14 ++++++++++++++ .../07-prefix-collisions/module/core.fga | 11 +++++++++++ .../07-prefix-collisions/module/dup.fga | 13 +++++++++++++ 3 files changed, 38 insertions(+) create mode 100644 tests/data/transformer-module/07-prefix-collisions/expected_errors.json create mode 100644 tests/data/transformer-module/07-prefix-collisions/module/core.fga create mode 100644 tests/data/transformer-module/07-prefix-collisions/module/dup.fga 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 +} From e81c90728812a5bbad1c02bdc172c6ca625c9550 Mon Sep 17 00:00:00 2001 From: SoulPancake Date: Mon, 17 Aug 2026 18:58:32 +0530 Subject: [PATCH 5/7] perf: look up name bytes in a keyed table instead of a switch --- pkg/go/utils/line-numbers.go | 32 ++++++++++++++++++------------- pkg/go/utils/line-numbers_test.go | 32 +++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 13 deletions(-) diff --git a/pkg/go/utils/line-numbers.go b/pkg/go/utils/line-numbers.go index a7719de4..6a040c6e 100644 --- a/pkg/go/utils/line-numbers.go +++ b/pkg/go/utils/line-numbers.go @@ -5,21 +5,27 @@ import ( "strings" ) -// IsNameByte reports whether b can appear inside a declaration name. IDENTIFIER -// admits letters, digits, `_` and MINUS, and EXTENDED_IDENTIFIER adds SLASH and DOT +// 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. -func IsNameByte(b byte) bool { - switch { - case b >= 'a' && b <= 'z', b >= 'A' && b <= 'Z', b >= '0' && b <= '9': - return true - } - - switch b { - case '_', '-', '/', '.': - return true - } +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, +} - return false +// 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 diff --git a/pkg/go/utils/line-numbers_test.go b/pkg/go/utils/line-numbers_test.go index b0d7381b..216424f1 100644 --- a/pkg/go/utils/line-numbers_test.go +++ b/pkg/go/utils/line-numbers_test.go @@ -2,6 +2,32 @@ 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 TestGetTypeLineNumber(t *testing.T) { t.Parallel() @@ -54,6 +80,12 @@ func TestGetTypeLineNumber(t *testing.T) { 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", From bf097776f31d0dceeb5fb34bf3e712bcfbf42bce Mon Sep 17 00:00:00 2001 From: SoulPancake Date: Mon, 24 Aug 2026 10:34:22 +0530 Subject: [PATCH 6/7] fix: fold runs of inline whitespace when locating declaration lines The lexer's WHITESPACE rule admits tab and form feed as well as space, so declarations like `defineowner:` parse but were not found by the line-number helpers, anchoring errors at 0:0. Collapse every run of [ \t\f] to a single space in all helpers (type, extend type, relation, condition, schema) across Go, JS and Java, and pin the behaviour with literal-tab cases in the shared semantic corpus and a tab-separated transformer-module fixture. --- pkg/go/utils/line-numbers.go | 69 +++++++++++---- pkg/go/utils/line-numbers_test.go | 87 +++++++++++++++++++ pkg/go/validation/name_validation.go | 39 ++++----- pkg/go/validation/name_validation_test.go | 43 ++++++++- .../dev/openfga/language/validation/Dsl.java | 23 +++-- pkg/js/util/line-numbers.ts | 16 ++-- pkg/js/validator/validate-dsl.ts | 18 ++-- tests/data/dsl-semantic-validation-cases.yaml | 82 +++++++++++++++++ .../expected_errors.json | 14 +++ .../module/core.fga | 7 ++ .../module/dup.fga | 5 ++ 11 files changed, 343 insertions(+), 60 deletions(-) create mode 100644 tests/data/transformer-module/08-tab-separated-declarations/expected_errors.json create mode 100644 tests/data/transformer-module/08-tab-separated-declarations/module/core.fga create mode 100644 tests/data/transformer-module/08-tab-separated-declarations/module/dup.fga diff --git a/pkg/go/utils/line-numbers.go b/pkg/go/utils/line-numbers.go index 6a040c6e..d4bc69b8 100644 --- a/pkg/go/utils/line-numbers.go +++ b/pkg/go/utils/line-numbers.go @@ -28,13 +28,12 @@ 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 +// 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, normalize func(string) string) int { +func declarationIndex(lines []string, prefix string) int { return slices.IndexFunc(lines, func(line string) bool { - trimmed := normalize(strings.TrimSpace(line)) + trimmed := NormalizeWhitespace(strings.TrimSpace(line)) if !strings.HasPrefix(trimmed, prefix) { return false } @@ -45,42 +44,78 @@ func declarationIndex(lines []string, prefix string, normalize func(string) stri }) } -// keepSpaces leaves a line as-is. Only the relation helper collapses repeated -// spaces, matching the reference, where ` {2,}` normalization is applied in -// getRelationLineNumber alone. -func keepSpaces(line string) string { return line } +// 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' +} -// normalizeSpaces collapses runs of spaces into one, mirroring the reference's -// ` {2,}` normalization, so `define owner:` is matched the same as `define owner:`. -func normalizeSpaces(line string) string { - for strings.Contains(line, " ") { - line = strings.ReplaceAll(line, " ", " ") +// 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, keepSpaces) + 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 declarationIndex(lines, "type "+typeName, keepSpaces) + return declarationIndex(lines, "type "+typeName) } // GetExtendedTypeLineNumber returns the index of the line extending typeName, or -1. func GetExtendedTypeLineNumber(typeName string, lines []string) int { - return declarationIndex(lines, "extend type "+typeName, keepSpaces) + 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 declarationIndex(lines, "define "+relation, normalizeSpaces) + 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 index 216424f1..d5184c90 100644 --- a/pkg/go/utils/line-numbers_test.go +++ b/pkg/go/utils/line-numbers_test.go @@ -28,6 +28,35 @@ func TestIsNameByte(t *testing.T) { } } +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() @@ -100,6 +129,26 @@ func TestGetTypeLineNumber(t *testing.T) { 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", @@ -158,6 +207,12 @@ func TestGetExtendedTypeLineNumber(t *testing.T) { 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", @@ -217,6 +272,26 @@ func TestGetRelationLineNumber(t *testing.T) { 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", @@ -276,6 +351,18 @@ func TestGetConditionLineNumber(t *testing.T) { 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", diff --git a/pkg/go/validation/name_validation.go b/pkg/go/validation/name_validation.go index 03462800..162366ec 100644 --- a/pkg/go/validation/name_validation.go +++ b/pkg/go/validation/name_validation.go @@ -10,8 +10,7 @@ import ( "github.com/openfga/language/pkg/go/utils" ) -// ValidationRegexRules contains the regex rules for validation -// These match the Rules from the JS implementation +// These match the Rules from the JS implementation. var ValidationRegexRules = struct { Type string Relation string @@ -26,8 +25,7 @@ var ValidationRegexRules = struct { Object: "[^\\s]{2,256}", } -// The anchored type, relation, and condition name rules are fixed, so compile -// them once. compiledNameRules caches them by their anchored rule string, which +// them once. CompiledNameRules caches them by their anchored rule string, which // is also the clause reported in the error, so validateFieldValue can look up // the compiled pattern without recompiling on every name. var ( @@ -42,8 +40,7 @@ var ( } ) -// ValidateTypeName validates a type name with both regex and reserved keyword checking -// This enhances the basic regex validation with semantic checks +// 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) { @@ -61,8 +58,7 @@ 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 +// 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) { @@ -103,8 +99,7 @@ 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 +// This is equivalent to the getTypeLineNumber function in JS. func GetTypeLineNumber(typeName string, lines []string, skipIndex *int) *int { if len(lines) == 0 { return nil @@ -116,8 +111,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 { @@ -129,8 +125,7 @@ func GetTypeLineNumber(typeName string, lines []string, skipIndex *int) *int { return nil } -// GetRelationLineNumber finds the line number where a relation is defined. -// skipIndex, when provided, is the index to begin searching from (inclusive) — +// SkipIndex, when provided, is the index to begin searching from (inclusive) — // matching the reference implementation's getRelationLineNumber, which slices // the lines from skipIndex onward. This lets callers anchor the search to a // specific type block so the correct occurrence is found when several types @@ -146,8 +141,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 ") @@ -164,8 +160,7 @@ 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 +// This is equivalent to the geConditionLineNumber function in JS. func GetConditionLineNumber(conditionName string, lines []string, skipIndex *int) *int { if len(lines) == 0 { return nil @@ -179,8 +174,9 @@ func GetConditionLineNumber(conditionName string, lines []string, skipIndex *int conditionPrefix := "condition " + conditionName for i := start; i < len(lines); i++ { // The name must end at the prefix, so `less` does not match a declaration - // of `less_than`. - trimmedLine := strings.TrimSpace(lines[i]) + // 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 } @@ -194,8 +190,7 @@ 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 +// 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/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/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] From d858df8449a77e18615c920f10b884fb300f15fc Mon Sep 17 00:00:00 2001 From: Anurag Bandyopadhyay Date: Mon, 24 Aug 2026 22:35:34 +0530 Subject: [PATCH 7/7] fix(pkg/go): compare pinned positions strictly and resolve the schema line The YAML corpus runner skipped its line and column comparison whenever an error carried no position, so a case pinning line 6 matched an error with no line at all. With whitespace folding removed from the declaration lookups, the three tab cases reported no position and the suite still passed. A case that pins a position now requires the error to carry one. That exposed `invalid schema version with a trailing comment on the schema line`, which had never resolved a line in Go: GetSchemaLineNumber anchored the version at end of line, so `schema 1.5 # a comment` matched nothing, where the reference allows `(\s+#.*)?`. Widen the pattern, and take the line through utils.NormalizeWhitespace in place of the local `\s{2,}` fold so the schema lookup folds inline whitespace like the other helpers. Restore eight doc comments in name_validation.go and line-numbers.go whose opening line had been dropped, which left them starting mid-sentence and naming compiledNameRules in the capitalized form of an identifier that does not exist. --- pkg/go/utils/line-numbers.go | 3 ++- pkg/go/validation/name_validation.go | 16 ++++++++++++---- pkg/go/validation/schema_validation.go | 15 ++++++++------- pkg/go/validation/yaml_test_integration_test.go | 11 ++++++----- 4 files changed, 28 insertions(+), 17 deletions(-) diff --git a/pkg/go/utils/line-numbers.go b/pkg/go/utils/line-numbers.go index d4bc69b8..7a65e803 100644 --- a/pkg/go/utils/line-numbers.go +++ b/pkg/go/utils/line-numbers.go @@ -28,7 +28,8 @@ func IsNameByte(b byte) bool { return nameBytes[b] } -// name ends there, or -1. Prefix always spans whole words (`type `, `define +// 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 { diff --git a/pkg/go/validation/name_validation.go b/pkg/go/validation/name_validation.go index 162366ec..0d5bd4c9 100644 --- a/pkg/go/validation/name_validation.go +++ b/pkg/go/validation/name_validation.go @@ -10,6 +10,7 @@ import ( "github.com/openfga/language/pkg/go/utils" ) +// ValidationRegexRules contains the regex rules for validation. // These match the Rules from the JS implementation. var ValidationRegexRules = struct { Type string @@ -25,7 +26,8 @@ var ValidationRegexRules = struct { Object: "[^\\s]{2,256}", } -// them once. CompiledNameRules caches them by their anchored rule string, which +// The anchored type, relation, and condition name rules are fixed, so compile +// them once. compiledNameRules caches them by their anchored rule string, which // is also the clause reported in the error, so validateFieldValue can look up // the compiled pattern without recompiling on every name. var ( @@ -40,7 +42,8 @@ var ( } ) -// 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) { @@ -58,7 +61,8 @@ func ValidateTypeName(typeName string, collector *ErrorCollector, lineIndex *int return true } -// 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) { @@ -99,6 +103,7 @@ 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. func GetTypeLineNumber(typeName string, lines []string, skipIndex *int) *int { if len(lines) == 0 { @@ -125,7 +130,8 @@ func GetTypeLineNumber(typeName string, lines []string, skipIndex *int) *int { return nil } -// SkipIndex, when provided, is the index to begin searching from (inclusive) — +// GetRelationLineNumber finds the line number where a relation is defined. +// skipIndex, when provided, is the index to begin searching from (inclusive) — // matching the reference implementation's getRelationLineNumber, which slices // the lines from skipIndex onward. This lets callers anchor the search to a // specific type block so the correct occurrence is found when several types @@ -160,6 +166,7 @@ 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. func GetConditionLineNumber(conditionName string, lines []string, skipIndex *int) *int { if len(lines) == 0 { @@ -190,6 +197,7 @@ 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. func ValidateNameRules(collector *ErrorCollector, typeName string, relationNames []string, typeLineIndex *int, meta *Meta, lines []string) { 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 } }