Skip to content

Commit 7cc0fdd

Browse files
authored
fix(protographic): enforce using __typename in composite types for requires (#2680)
1 parent f0aa414 commit 7cc0fdd

File tree

11 files changed

+926
-53
lines changed

11 files changed

+926
-53
lines changed

protographic/.gitignore

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,5 @@ dist
22
out
33
node_modules
44
.env
5-
.eslintcache
5+
.eslintcache
6+
coverage

protographic/SDL_PROTO_RULES.md

Lines changed: 109 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,8 +43,6 @@ Rules should follow [Proto Best Practices](https://protobuf.dev/best-practices/d
4343
#### Federation Features
4444

4545
- ✗ Federation entity lookups with nested keys
46-
- ✗ Abstract types (interfaces/unions) in @requires field selections
47-
- ✗ Inline fragments in @requires field selections
4846

4947
#### GraphQL Features
5048

@@ -289,6 +287,115 @@ message RequireProductNameByIdFields {
289287
- Only the selected fields from nested types are included (e.g., `description` and `reviewSummary` from `ProductDetails`, not `id` or `title`)
290288
- Field order in the proto matches the normalized selection order
291289

290+
#### Composite Types in Required Fields (Interfaces & Unions)
291+
292+
When `@requires` references a field whose type is an interface or union, the required field selection must include inline fragments to specify which fields to extract from each concrete type.
293+
294+
##### The `__typename` Requirement
295+
296+
When using inline fragments on an interface or union field in `@requires`, `__typename` must be present in the selection set. This is required for the engine to determine which concrete type to deserialize at runtime. Validation produces errors with field paths (e.g., `in "pet.friend"`) for nested selections missing `__typename`.
297+
298+
Example:
299+
300+
```graphql
301+
@requires(fields: "primaryItem { __typename ... on PalletItem { name } ... on ContainerItem { name } }")
302+
```
303+
304+
##### Selection Normalization
305+
306+
Before proto generation, selections are normalized by distributing parent-level fields into each inline fragment. For example:
307+
308+
```graphql
309+
media { id ... on Book { author } ... on Movie { director } }
310+
```
311+
312+
Normalizes to:
313+
314+
```graphql
315+
media { ... on Book { id author } ... on Movie { id director } }
316+
```
317+
318+
This ensures each fragment is self-contained. Nested inline fragments on sub-interfaces are also flattened to concrete types. For example, given `Employee` (interface) with implementors `Manager`, `Engineer`, `Contractor`, and `Intern` — where `Engineer` and `Contractor` also implement `Managed`:
319+
320+
```graphql
321+
# Before normalization:
322+
members {
323+
id
324+
... on Managed { supervisor }
325+
}
326+
327+
# After normalization — expanded to all concrete types,
328+
# with `supervisor` only on types that implement Managed:
329+
members {
330+
... on Manager { id }
331+
... on Engineer { id supervisor }
332+
... on Contractor { id supervisor }
333+
... on Intern { id }
334+
}
335+
```
336+
337+
##### Proto Mapping — Interface/Union Becomes `oneof`
338+
339+
The interface or union type maps to a message with `oneof instance` containing each concrete type. `__typename` is consumed during validation only — it does **not** appear in the generated proto.
340+
341+
```graphql
342+
type Storage @key(fields: "id") {
343+
id: ID!
344+
primaryItem: StorageItem! @external
345+
itemInfo: String!
346+
@requires(
347+
fields: "primaryItem { __typename ... on PalletItem { name palletCount } ... on ContainerItem { name containerSize } }"
348+
)
349+
}
350+
351+
interface StorageItem {
352+
name: String!
353+
}
354+
355+
type PalletItem implements StorageItem {
356+
name: String!
357+
palletCount: Int!
358+
}
359+
360+
type ContainerItem implements StorageItem {
361+
name: String!
362+
containerSize: String!
363+
}
364+
```
365+
366+
Maps to (Fields message only):
367+
368+
```protobuf
369+
message RequireStorageItemInfoByIdFields {
370+
message PalletItem {
371+
string name = 1;
372+
int32 pallet_count = 2;
373+
}
374+
375+
message ContainerItem {
376+
string name = 1;
377+
string container_size = 2;
378+
}
379+
380+
message StorageItem {
381+
oneof instance {
382+
ContainerItem container_item = 1;
383+
PalletItem pallet_item = 2;
384+
}
385+
}
386+
387+
StorageItem primary_item = 1;
388+
}
389+
```
390+
391+
**Key Points**:
392+
393+
- `__typename` is absent from the proto — it is consumed during validation only
394+
- The interface type becomes a message with `oneof instance`
395+
- Each implementing type gets its own message with only the fields selected in its fragment
396+
- All type messages (`StorageItem`, `PalletItem`, `ContainerItem`) are **nested inside** the `Fields` message. This scoping ensures they don't collide with the root-level messages of the same name, which contain all fields of the type — while the nested versions contain only the subset selected in `@requires`
397+
- The same pattern applies to union types (union member types instead of implementing types)
398+
292399
## Field Resolvers
293400

294401
Field resolvers allow you to define custom resolution logic for specific fields within a GraphQL type. Using the `@connect__fieldResolver` directive, you can specify which fields should be resolved through dedicated RPC methods, enabling lazy loading, computed fields, or integration with external data sources.

protographic/src/abstract-selection-rewriter.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -630,7 +630,7 @@ export class AbstractSelectionRewriter {
630630
// the wrong inline fragments.
631631
const fieldsToAdd = fields
632632
.filter((field) => !existingFields.has(field.name.value))
633-
.filter((field) => inlineFragmentType.getFields()[field.name.value]);
633+
.filter((field) => field.name.value === '__typename' || inlineFragmentType.getFields()[field.name.value]);
634634

635635
// Add the interface fields to the fragment. We always prepend them for now.
636636
// TODO: Check if fields should be inserted in the order of appearance in the selection set.

protographic/src/required-fields-visitor.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -365,7 +365,7 @@ export class RequiredFieldsVisitor {
365365
* @throws Error if the field definition is not found on the current type
366366
*/
367367
private onEnterField(ctx: VisitContext<FieldNode>): void {
368-
if (!this.current) {
368+
if (!this.current || ctx.node.name.value === '__typename') {
369369
return;
370370
}
371371

protographic/src/sdl-validation-visitor.ts

Lines changed: 67 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,13 @@ import {
1313
NamedTypeNode,
1414
GraphQLID,
1515
ConstArgumentNode,
16+
GraphQLObjectType,
17+
GraphQLSchema,
18+
buildASTSchema,
1619
} from 'graphql';
17-
import { CONNECT_FIELD_RESOLVER, CONTEXT } from './string-constants.js';
20+
import { safeParse } from '@wundergraph/composition';
21+
import { CONNECT_FIELD_RESOLVER, CONTEXT, FIELDS, REQUIRES_DIRECTIVE_NAME } from './string-constants.js';
22+
import { SelectionSetValidationVisitor } from './selection-set-validation-visitor.js';
1823

1924
/**
2025
* Type mapping from Kind enum values to their corresponding AST node types
@@ -48,8 +53,6 @@ interface LintingRule<K extends keyof KindToNodeTypeMap = keyof KindToNodeTypeMa
4853
name: string;
4954
/** Human-readable description of what this rule validates */
5055
description?: string;
51-
/** Whether this validation rule is currently active */
52-
enabled: boolean;
5356
/** The AST node kind this rule applies to */
5457
nodeKind: K;
5558
/** The validation function to execute for matching nodes */
@@ -89,6 +92,7 @@ export class SDLValidationVisitor {
8992
private readonly validationResult: ValidationResult;
9093
private lintingRules: LintingRule<any>[] = [];
9194
private visitor: ASTVisitor;
95+
private parsedSchema: GraphQLSchema;
9296

9397
/**
9498
* Creates a new SDL validation visitor for the given GraphQL schema
@@ -101,6 +105,8 @@ export class SDLValidationVisitor {
101105
warnings: [],
102106
};
103107

108+
this.parsedSchema = buildASTSchema(parse(schema), { assumeValid: true, assumeValidSDL: true });
109+
104110
this.initializeLintingRules();
105111
this.visitor = this.createASTVisitor();
106112
}
@@ -114,36 +120,88 @@ export class SDLValidationVisitor {
114120
const objectTypeRule: LintingRule<Kind.OBJECT_TYPE_DEFINITION> = {
115121
name: 'nested-key-directives',
116122
description: 'Validates that @key directives do not contain nested field selections',
117-
enabled: true,
118123
nodeKind: Kind.OBJECT_TYPE_DEFINITION,
119124
validationFunction: (ctx) => this.validateObjectTypeKeyDirectives(ctx),
120125
};
121126

122127
const listTypeRule: LintingRule<Kind.LIST_TYPE> = {
123128
name: 'nullable-items-in-list-types',
124129
description: 'Validates that list types do not contain nullable items',
125-
enabled: true,
126130
nodeKind: Kind.LIST_TYPE,
127131
validationFunction: (ctx) => this.validateListTypeNullability(ctx),
128132
};
129133

130134
const providesRule: LintingRule<Kind.FIELD_DEFINITION> = {
131135
name: 'use-of-provides',
132136
description: 'Validates usage of @provides directive which is not yet supported',
133-
enabled: true,
134137
nodeKind: Kind.FIELD_DEFINITION,
135138
validationFunction: (ctx) => this.validateProvidesDirective(ctx),
136139
};
137140

138141
const resolverContextRule: LintingRule<Kind.FIELD_DEFINITION> = {
139142
name: 'use-of-invalid-resolver-context',
140143
description: 'Validates whether a resolver context can be extracted from a type',
141-
enabled: true,
142144
nodeKind: Kind.FIELD_DEFINITION,
143145
validationFunction: (ctx) => this.validateInvalidResolverContext(ctx),
144146
};
145147

146-
this.lintingRules = [objectTypeRule, listTypeRule, providesRule, resolverContextRule];
148+
const compositeTypeReflectionRule: LintingRule<Kind.FIELD_DEFINITION> = {
149+
name: 'use-of-typename',
150+
description: 'Validates that __typename is present for composite types in @requires selections',
151+
nodeKind: Kind.FIELD_DEFINITION,
152+
validationFunction: (ctx) => this.validateCompositeTypeReflection(ctx),
153+
};
154+
155+
this.lintingRules = [objectTypeRule, listTypeRule, providesRule, resolverContextRule, compositeTypeReflectionRule];
156+
}
157+
158+
private validateCompositeTypeReflection(ctx: VisitContext<FieldDefinitionNode>): void {
159+
const directive = ctx.node.directives?.find((directive) => directive.name.value === REQUIRES_DIRECTIVE_NAME);
160+
if (!directive) {
161+
return;
162+
}
163+
164+
const fieldSet = directive.arguments?.find((arg) => arg.name.value === FIELDS);
165+
if (!fieldSet) {
166+
return;
167+
}
168+
169+
if (fieldSet.value.kind !== Kind.STRING) {
170+
this.addError('Invalid @requires directive: fields argument must be a string', fieldSet.loc);
171+
return;
172+
}
173+
174+
const fieldSetValue = fieldSet.value.value;
175+
const { error, documentNode } = safeParse('{' + fieldSetValue + '}');
176+
if (error || !documentNode) {
177+
this.addError('Invalid @requires directive: fields argument must be a valid GraphQL selection set', fieldSet.loc);
178+
return;
179+
}
180+
181+
const parentNode = ctx.ancestors.at(-1);
182+
if (!parentNode || !this.isASTObjectTypeNode(parentNode)) {
183+
this.addError('Invalid @requires directive: fields argument must be a valid GraphQL selection set', fieldSet.loc);
184+
return;
185+
}
186+
187+
const objectType = this.parsedSchema.getType(parentNode.name.value) as GraphQLObjectType;
188+
if (!objectType) {
189+
this.addError('Invalid @requires directive: parent type not found', fieldSet.loc);
190+
return;
191+
}
192+
193+
const visitor = new SelectionSetValidationVisitor(documentNode, objectType, this.parsedSchema);
194+
195+
visitor.visit();
196+
197+
const { errors, warnings } = visitor.getValidationResult();
198+
for (const error of errors) {
199+
this.addError(error, fieldSet.loc);
200+
}
201+
202+
for (const warning of warnings) {
203+
this.addWarning(warning, fieldSet.loc);
204+
}
147205
}
148206

149207
/**
@@ -213,7 +271,7 @@ export class SDLValidationVisitor {
213271
* @private
214272
*/
215273
private executeValidationRules(ctx: VisitContext<ASTNode>): void {
216-
const applicableRules = this.lintingRules.filter((rule) => rule.nodeKind === ctx.node.kind && rule.enabled);
274+
const applicableRules = this.lintingRules.filter((rule) => rule.nodeKind === ctx.node.kind);
217275

218276
for (const rule of applicableRules) {
219277
// Type assertion is safe here because we've filtered by nodeKind
@@ -572,45 +630,6 @@ export class SDLValidationVisitor {
572630
return !Array.isArray(node) && 'kind' in node && node.kind === Kind.OBJECT_TYPE_DEFINITION;
573631
}
574632

575-
/**
576-
* Enable or disable a specific validation rule by name
577-
* @param ruleName - The name of the rule to configure
578-
* @param enabled - Whether the rule should be enabled
579-
* @returns true if the rule was found and configured, false otherwise
580-
*/
581-
public configureRule(ruleName: string, enabled: boolean): boolean {
582-
const rule = this.lintingRules.find((gate) => gate.name === ruleName);
583-
if (rule) {
584-
rule.enabled = enabled;
585-
return true;
586-
}
587-
return false;
588-
}
589-
590-
/**
591-
* Get information about all available validation rules
592-
* @returns Array of rule configurations
593-
*/
594-
public getAvailableRules(): Readonly<LintingRule<any>[]> {
595-
return Object.freeze([...this.lintingRules]);
596-
}
597-
598-
/**
599-
* Check if the validation found any critical errors
600-
* @returns true if errors were found, false otherwise
601-
*/
602-
public hasErrors(): boolean {
603-
return this.validationResult.errors.length > 0;
604-
}
605-
606-
/**
607-
* Check if the validation found any warnings
608-
* @returns true if warnings were found, false otherwise
609-
*/
610-
public hasWarnings(): boolean {
611-
return this.validationResult.warnings.length > 0;
612-
}
613-
614633
/**
615634
* Add a warning to the validation results
616635
* @param message - The warning message

0 commit comments

Comments
 (0)