Skip to content

Commit

Permalink
Merge pull request #4235 from a-alle/fix/4232
Browse files Browse the repository at this point in the history 
Fix incorrect directive combination for inherited fields
  • Loading branch information
@a-alle
a-alle authored Nov 2, 2023
2 parents 0f0c955 + 8c25cab commit a14d465
Show file tree
Hide file tree
Showing 3 changed files with 138 additions and 34 deletions.
5 changes: 5 additions & 0 deletions .changeset/flat-badgers-itch.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@neo4j/graphql": patch
---

Fix directive combination logic for inherited field in the directive combination validation rule
81 changes: 47 additions & 34 deletions ...ges/graphql/src/schema/validation/custom-rules/valid-types/valid-directive-combination.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,10 +113,10 @@ export function DirectiveCombinationValid(context: SDLValidationContext): ASTVis
typeToDirectivesPerFieldMap.set(parentOfTraversedDef.name.value, seenFields);
}
};
const getDirectives = function (
const getDirectiveCombinations = function (
node: ASTNodeWithDirectives,
parentOfTraversedDef: ObjectOrInterfaceWithExtensions | undefined
): DirectiveNode[] {
): DirectiveNode[][] {
const directivesToCheck: DirectiveNode[] = [...(node.directives || [])];
if (
node.kind === Kind.OBJECT_TYPE_DEFINITION ||
Expand All @@ -126,21 +126,33 @@ export function DirectiveCombinationValid(context: SDLValidationContext): ASTVis
) {
// might have been directives on extension
directivesToCheck.push(...(typeToDirectivesMap.get(node.name.value) || []));
return getInheritedTypeNames(node, interfaceToImplementingTypes).reduce((acc, i) => {
const inheritedDirectives = typeToDirectivesMap.get(i) || [];
return acc.concat(inheritedDirectives);
}, directivesToCheck);
const inheritedTypeNames = getInheritedTypeNames(node, interfaceToImplementingTypes);
if (inheritedTypeNames.length) {
return inheritedTypeNames.reduce<DirectiveNode[][]>((acc, i) => {
const inheritedDirectives = typeToDirectivesMap.get(i) || [];
acc.push(directivesToCheck.concat(inheritedDirectives));
return acc;
}, []);
} else {
return [directivesToCheck];
}
}
if (node.kind === Kind.FIELD_DEFINITION) {
if (!parentOfTraversedDef) {
return [];
return [[]];
}
const inheritedTypeNames = getInheritedTypeNames(parentOfTraversedDef, interfaceToImplementingTypes);
if (inheritedTypeNames.length) {
return inheritedTypeNames.reduce<DirectiveNode[][]>((acc, i) => {
const inheritedDirectives = typeToDirectivesPerFieldMap.get(i)?.get(node.name.value) || [];
acc.push(directivesToCheck.concat(inheritedDirectives));
return acc;
}, []);
} else {
return [directivesToCheck];
}
return getInheritedTypeNames(parentOfTraversedDef, interfaceToImplementingTypes).reduce((acc, i) => {
const inheritedDirectives = typeToDirectivesPerFieldMap.get(i)?.get(node.name.value) || [];
return acc.concat(inheritedDirectives);
}, directivesToCheck);
}
return directivesToCheck;
return [directivesToCheck];
};

return {
Expand All @@ -154,15 +166,10 @@ export function DirectiveCombinationValid(context: SDLValidationContext): ASTVis

hydrateInterfaceWithImplementedTypesMap(node, interfaceToImplementingTypes);
hydrateWithDirectives(node, parentOfTraversedDef);
const directivesToCheck = getDirectives(node, parentOfTraversedDef);

if (directivesToCheck.length < 2) {
// no combination to check
return;
}
const directiveCombinationsToCheck = getDirectiveCombinations(node, parentOfTraversedDef);

const { isValid, errorMsg, errorPath } = assertValid(() =>
assertValidDirectives(directivesToCheck, node.kind)
assertValidDirectives(directiveCombinationsToCheck, node.kind)
);
if (!isValid) {
context.reportError(
Expand Down Expand Up @@ -194,24 +201,30 @@ function getInvalidCombinations(kind: ASTNode["kind"]): Record<PropertyKey, Read
return {};
}

function assertValidDirectives(directives: DirectiveNode[], kind: ASTNode["kind"]) {
function assertValidDirectives(directiveCombinationsToCheck: DirectiveNode[][], kind: ASTNode["kind"]) {
const invalidCombinations = getInvalidCombinations(kind);

directives.forEach((directive) => {
if (invalidCombinations[directive.name.value]) {
directives.forEach((d) => {
if (d.name.value === directive.name.value) {
return;
}
if (invalidCombinations[directive.name.value]?.includes(d.name.value)) {
throw new DocumentValidationError(
`Invalid directive usage: Directive @${directive.name.value} cannot be used in combination with @${d.name.value}`,
[]
);
}
});
for (const directives of directiveCombinationsToCheck) {
if (directives.length < 2) {
// no combination to check
continue;
}
});
directives.forEach((directive) => {
if (invalidCombinations[directive.name.value]) {
directives.forEach((d) => {
if (d.name.value === directive.name.value) {
return;
}
if (invalidCombinations[directive.name.value]?.includes(d.name.value)) {
throw new DocumentValidationError(
`Invalid directive usage: Directive @${directive.name.value} cannot be used in combination with @${d.name.value}`,
[]
);
}
});
}
});
}
}

export function SchemaOrTypeDirectives(context: SDLValidationContext): ASTVisitor {
Expand Down
86 changes: 86 additions & 0 deletions packages/graphql/src/schema/validation/validate-document.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6530,6 +6530,92 @@ describe("validation 2.0", () => {
});
});

describe("https://github.com/neo4j/graphql/issues/4232", () => {
test("interface at the end", () => {
const doc = gql`
type Person {
name: String!
}
type Episode implements IProduct {
editorsInCharge: [Person!]!
@relationship(
type: "EDITORS_IN_CHARGE"
direction: OUT
nestedOperations: [CONNECT, DISCONNECT]
)
}
type Series implements IProduct {
editorsInCharge: [Person!]!
@cypher(
statement: """
MATCH (this)-[:HAS_PART]->()-[:EDITORS_IN_CHARGE]->(n)
RETURN distinct(n) as editorsInCharge
"""
columnName: "editorsInCharge"
)
}
interface IProduct {
editorsInCharge: [Person!]!
}
`;

const executeValidate = () =>
validateDocument({
document: doc,
additionalDefinitions,
features: {},
experimental: false,
});

expect(executeValidate).not.toThrow();
});

test("interface at the beginning", () => {
const doc = gql`
type Person {
name: String!
}
interface IProduct {
editorsInCharge: [Person!]!
}
type Episode implements IProduct {
editorsInCharge: [Person!]!
@relationship(
type: "EDITORS_IN_CHARGE"
direction: OUT
nestedOperations: [CONNECT, DISCONNECT]
)
}
type Series implements IProduct {
editorsInCharge: [Person!]!
@cypher(
statement: """
MATCH (this)-[:HAS_PART]->()-[:EDITORS_IN_CHARGE]->(n)
RETURN distinct(n) as editorsInCharge
"""
columnName: "editorsInCharge"
)
}
`;

const executeValidate = () =>
validateDocument({
document: doc,
additionalDefinitions,
features: {},
experimental: false,
});

expect(executeValidate).not.toThrow();
});
});

describe("https://github.com/neo4j/graphql/issues/442", () => {
test("should not throw error on validation of schema if MutationResponse used", () => {
const doc = gql`
Expand Down

0 comments on commit a14d465

Please sign in to comment.