From 1eb6467c9827d945f1f9a20995bd5c161ec8293a Mon Sep 17 00:00:00 2001 From: Mark Lee Date: Fri, 23 Oct 2026 08:47:18 -0700 Subject: [PATCH] fix(graphql): apply root field and base rules together in naming-convention naming-convention resolved a root field such as a Query field to its root selector alone, so an option like "FieldDefinition[parent.name.value=Query]" with only forbiddenPrefixes silently stopped the general "FieldDefinition" casing rule from running on that type. @graphql-eslint registers one listener per option key, so both rules apply to such a field, the less specific one first. Check a root field against its base rule and then its root rule, reporting each violation. Replace the test that expected the root rule to override the base rule, which @graphql-eslint 3.13.1 and 3.20.1 do not do. Assisted-By: claude-sonnet 5.5 via opencode --- internal/graphql/lint/naming_convention.go | 42 +++++------ .../graphql/lint/naming_convention_test.go | 69 ++++++++++++++++--- 2 files changed, 83 insertions(+), 28 deletions(-) diff --git a/internal/graphql/lint/naming_convention.go b/internal/graphql/lint/naming_convention.go index 5ff9bdf9..606a2ac4 100644 --- a/internal/graphql/lint/naming_convention.go +++ b/internal/graphql/lint/naming_convention.go @@ -224,24 +224,26 @@ func parseNamingConventionOptions(opts map[string]any) namingConventionOptions { return o } -// ruleFor resolves the namingRule that applies to s, if any: s's own -// root-field selector (if it has one) or base selector, whichever -// scripts/devbase.yaml actually configured, falling back to the -// shared "types" rule only for a type-definition site whose own kind -// key was not configured individually. -func (o namingConventionOptions) ruleFor(s *namingSite) (namingRule, bool) { +// rulesFor resolves the namingRules that apply to s, in check order: +// its base selector (falling back to the shared "types" rule only for +// a type-definition site whose own kind key was not configured +// individually), then its root-field selector, if configured. The +// root-field rule adds to the base rule instead of replacing it, because +// @graphql-eslint registers one listener per option key and runs the +// least specific first. +func (o namingConventionOptions) rulesFor(s *namingSite) []namingRule { + var rules []namingRule + if r, ok := o.selectors[s.baseSelector]; ok { + rules = append(rules, r) + } else if s.typeDefKindKey != "" && o.types != nil { + rules = append(rules, *o.types) + } if s.rootSelector != "" { if r, ok := o.selectors[s.rootSelector]; ok { - return r, true + rules = append(rules, r) } } - if r, ok := o.selectors[s.baseSelector]; ok { - return r, true - } - if s.typeDefKindKey != "" && o.types != nil { - return *o.types, true - } - return namingRule{}, false + return rules } // namingSite is one name in a schema file that naming-convention @@ -384,15 +386,15 @@ func namingRuleMessage(name string, rule *namingRule) string { } // namingConventionViolations reports a RuleNamingConvention violation -// for every site in sites that either has a configured namingRule it -// fails (per namingRuleMessage) or an underscore opts forbids, in -// document order. Both checks run for the same site independently, and -// neither one skips the other, matching @graphql-eslint's own behavior -// of registering them as separate listeners. +// for each configured namingRule a site fails (per namingRuleMessage) +// and for each underscore opts forbids, in document order. A site can +// fail several of these. The checks run independently, and none skips +// another, matching @graphql-eslint's own behavior of registering them +// as separate listeners. func namingConventionViolations(sites []namingSite, opts namingConventionOptions) []Violation { var violations []Violation for _, s := range sites { - if rule, ok := opts.ruleFor(&s); ok { + for _, rule := range opts.rulesFor(&s) { if msg := namingRuleMessage(s.name, &rule); msg != "" { violations = append(violations, Violation{ err: gqlerror.ErrorPosf(s.pos, "%s %q %s", s.kindLabel, s.name, msg), diff --git a/internal/graphql/lint/naming_convention_test.go b/internal/graphql/lint/naming_convention_test.go index 0c2e1155..e0178842 100644 --- a/internal/graphql/lint/naming_convention_test.go +++ b/internal/graphql/lint/naming_convention_test.go @@ -69,14 +69,6 @@ func TestNamingConventionValidCases(t *testing.T) { "EnumValueDefinition": map[string]any{"style": "UPPER_CASE", "prefix": "ENUM_VALUE_"}, }, }, - { - "Query field selector overrides the general FieldDefinition rule", - `type One { fieldA: String } type Query { QUERY_A(id: ID!): String }`, - map[string]any{ - "FieldDefinition[parent.name.value=Query]": map[string]any{"style": "UPPER_CASE", "prefix": "QUERY"}, - "FieldDefinition": map[string]any{"style": "camelCase", "prefix": "field"}, - }, - }, { "should ignore fields matching ignorePattern", `type Test { EU: ID EUIntlFlag: ID UPC: ID }`, @@ -202,6 +194,67 @@ func TestNamingConventionInvalidCases(t *testing.T) { `Field "getC" should not have "get" prefix`, }, }, + { + // @graphql-eslint registers one listener per option key, so + // the Query selector adds to the FieldDefinition rule + // instead of replacing it (verified against + // @graphql-eslint/eslint-plugin 3.13.1 and 3.20.1). + "Query field selector adds to the general FieldDefinition rule", + `type One { fieldA: String } type Query { QUERY_A(id: ID!): String }`, + opts(map[string]any{ + "FieldDefinition[parent.name.value=Query]": map[string]any{"style": "UPPER_CASE", "prefix": "QUERY"}, + "FieldDefinition": map[string]any{"style": "camelCase", "prefix": "field"}, + }), + []string{`Field "QUERY_A" should have "field" prefix`}, + }, + { + "a root field's casing is still checked when only forbiddenPrefixes is set on the Query selector", + `type Query { Bad_name: String, getThing: String }`, + opts(map[string]any{ + "FieldDefinition": map[string]any{"style": "camelCase"}, + "FieldDefinition[parent.name.value=Query]": map[string]any{"forbiddenPrefixes": []any{"get"}}, + }), + []string{ + `Field "Bad_name" should be in camelCase format`, + `Field "getThing" should not have "get" prefix`, + }, + }, + { + "Mutation and Subscription fields get both rules too", + `type Query { a: String } type Mutation { Bad_m: String, getM: String } type Subscription { Bad_s: String, getS: String }`, + opts(map[string]any{ + "FieldDefinition": map[string]any{"style": "camelCase"}, + "FieldDefinition[parent.name.value=Mutation]": map[string]any{"forbiddenPrefixes": []any{"get"}}, + "FieldDefinition[parent.name.value=Subscription]": map[string]any{"forbiddenPrefixes": []any{"get"}}, + }), + []string{ + `Field "Bad_m" should be in camelCase format`, + `Field "getM" should not have "get" prefix`, + `Field "Bad_s" should be in camelCase format`, + `Field "getS" should not have "get" prefix`, + }, + }, + { + "the types rule never applies to a field, only to type definitions", + `type Query { getX: String, lower: String }`, + opts(map[string]any{ + "types": "PascalCase", + "FieldDefinition[parent.name.value=Query]": map[string]any{"forbiddenPrefixes": []any{"get"}}, + }), + []string{`Field "getX" should not have "get" prefix`}, + }, + { + "a root field that breaks both rules is reported for each, general rule first", + `type Query { Get_x: String }`, + opts(map[string]any{ + "FieldDefinition": map[string]any{"style": "camelCase"}, + "FieldDefinition[parent.name.value=Query]": map[string]any{"forbiddenPrefixes": []any{"Get"}}, + }), + []string{ + `Field "Get_x" should be in camelCase format`, + `Field "Get_x" should not have "Get" prefix`, + }, + }, }) }