Skip to content

Commit

Permalink
Fix non-existing relationships for 1 to 1 relationship. (#4762)
Browse files Browse the repository at this point in the history
* reintroduce legacy support for null on relationship filter 1..1

* fix flaky test

* better test name

* Create eleven-laws-shout.md
  • Loading branch information
MacondoExpress committed May 29, 2024
1 parent 8e332d2 commit a77109e
Show file tree
Hide file tree
Showing 5 changed files with 208 additions and 26 deletions.
5 changes: 5 additions & 0 deletions .changeset/eleven-laws-shout.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@neo4j/graphql": patch
---

Fix non-existing relationships for 1 to 1 relationship.
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import type { InterfaceEntityAdapter } from "../../../../schema-model/entity/mod
import type { RelationshipAdapter } from "../../../../schema-model/relationship/model-adapters/RelationshipAdapter";
import { filterTruthy } from "../../../../utils/utils";
import type { RelationshipWhereOperator } from "../../../where/types";
import { hasTarget } from "../../utils/context-has-target";
import { createNodeFromEntity } from "../../utils/create-node-from-entity";
import { wrapSubqueriesInCypherCalls } from "../../utils/wrap-subquery-in-calls";
import type { QueryASTContext } from "../QueryASTContext";
Expand Down Expand Up @@ -66,7 +65,7 @@ export class RelationshipFilter extends Filter {
}

public getChildren(): QueryASTNode[] {
return [...this.targetNodeFilters];
return this.targetNodeFilters;
}

public addTargetNodeFilter(...filter: Filter[]): void {
Expand Down Expand Up @@ -98,7 +97,9 @@ export class RelationshipFilter extends Filter {

const nestedSelection = filterTruthy(
this.targetNodeFilters.map((f) => {
if (!hasTarget(context)) throw new Error("No parent node found!");
if (!context.hasTarget()) {
throw new Error("No parent node found!");
}
const selection = f.getSelection(context);
if (selection.length === 0) return undefined;

Expand Down Expand Up @@ -177,7 +178,9 @@ export class RelationshipFilter extends Filter {
case "NONE":
case "SOME":
case "SINGLE": {
if (!hasTarget(context)) throw new Error("No parent node found!");
if (!context.hasTarget()) {
throw new Error("No parent node found!");
}
const match = new Cypher.Match(pattern);

const returnVar = new Cypher.Variable();
Expand Down Expand Up @@ -237,7 +240,9 @@ export class RelationshipFilter extends Filter {
const match = new Cypher.Match(pattern);

const subqueries = this.targetNodeFilters.map((f) => {
if (!hasTarget(context)) throw new Error("No parent node found!");
if (!context.hasTarget()) {
throw new Error("No parent node found!");
}
const returnVar = new Cypher.Variable();
returnVariables.push(returnVar);
const nestedSubqueries = f.getSubqueries(context).map((sq) => {
Expand Down Expand Up @@ -283,7 +288,9 @@ export class RelationshipFilter extends Filter {
public getSelection(queryASTContext: QueryASTContext): Array<Cypher.Match | Cypher.With> {
if (this.shouldCreateOptionalMatch() && !this.subqueryPredicate) {
const nestedContext = this.getNestedContext(queryASTContext);
if (!hasTarget(nestedContext)) throw new Error("No parent node found!");
if (!nestedContext.hasTarget()) {
throw new Error("No parent node found!");
}

const pattern = new Cypher.Pattern(nestedContext.source!)
.withoutLabels()
Expand All @@ -299,7 +306,9 @@ export class RelationshipFilter extends Filter {
}

public getPredicate(queryASTContext: QueryASTContext): Cypher.Predicate | undefined {
if (this.subqueryPredicate) return this.subqueryPredicate;
if (this.subqueryPredicate) {
return this.subqueryPredicate;
}
const nestedContext = this.getNestedContext(queryASTContext);

if (this.shouldCreateOptionalMatch()) {
Expand All @@ -316,8 +325,9 @@ export class RelationshipFilter extends Filter {
.to(nestedContext.target);

const predicate = this.createRelationshipOperation(pattern, nestedContext);
if (!predicate) return undefined;
return this.wrapInNotIfNeeded(predicate);
if (predicate) {
return this.wrapInNotIfNeeded(predicate);
}
}

protected getSingleRelationshipOperation({
Expand All @@ -329,7 +339,9 @@ export class RelationshipFilter extends Filter {
queryASTContext: QueryASTContext;
innerPredicate: Cypher.Predicate;
}): Cypher.Predicate {
if (!hasTarget(queryASTContext)) throw new Error("No parent node found!");
if (!queryASTContext.hasTarget()) {
throw new Error("No parent node found!");
}
const patternComprehension = new Cypher.PatternComprehension(pattern, new Cypher.Literal(1)).where(
innerPredicate
);
Expand All @@ -345,14 +357,18 @@ export class RelationshipFilter extends Filter {

switch (this.operator) {
case "ALL": {
if (!innerPredicate) return undefined;
if (!innerPredicate) {
return;
}
const match = new Cypher.Match(pattern).where(innerPredicate);
const negativeMatch = new Cypher.Match(pattern).where(Cypher.not(innerPredicate));
// Testing "ALL" requires testing that at least one element exists and that no elements not matching the filter exists
return Cypher.and(new Cypher.Exists(match), Cypher.not(new Cypher.Exists(negativeMatch)));
}
case "SINGLE": {
if (!innerPredicate) return undefined;
if (!innerPredicate) {
return;
}

return this.getSingleRelationshipOperation({
pattern,
Expand All @@ -362,27 +378,27 @@ export class RelationshipFilter extends Filter {
}
case "NONE":
case "SOME": {
if (!this.relationship.isList && this.relationship.isNullable) {
if (!innerPredicate) return undefined;

return this.getSingleRelationshipOperation({
pattern,
queryASTContext,
innerPredicate,
});
}

const match = new Cypher.Match(pattern);
if (innerPredicate) {
if (!this.relationship.isList) {
return this.getSingleRelationshipOperation({
pattern,
queryASTContext,
innerPredicate,
});
}
return new Cypher.Exists(match.where(innerPredicate));
}

return new Cypher.Exists(match);
}
}
}

protected wrapInNotIfNeeded(predicate: Cypher.Predicate): Cypher.Predicate {
if (this.isNot) return Cypher.not(predicate);
else return predicate;
if (this.isNot) {
return Cypher.not(predicate);
}
return predicate;
}
}
4 changes: 2 additions & 2 deletions packages/graphql/tests/integration/issues/2697.int.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,14 +90,14 @@ describe("https://github.com/neo4j/graphql/issues/2697", () => {
});

expect(gqlResult.errors).toBeUndefined();
expect((gqlResult as any).data[typeActor.plural]).toEqual([
expect((gqlResult as any).data[typeActor.plural]).toEqual(expect.toIncludeAllMembers([
{
name: "Arnold",
},
{
name: "Linda",
},
]);
]));
});

test("Aggregate on edge duration", async () => {
Expand Down
102 changes: 102 additions & 0 deletions packages/graphql/tests/integration/issues/4667.int.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
/*
* Copyright (c) "Neo4j"
* Neo4j Sweden AB [http://neo4j.com]
*
* This file is part of Neo4j.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import { graphql } from "graphql";
import { type Driver } from "neo4j-driver";
import { Neo4jGraphQL } from "../../../src";
import { cleanNodes } from "../../utils/clean-nodes";
import { UniqueType } from "../../utils/graphql-types";
import Neo4j from "../neo4j";

describe("https://github.com/neo4j/graphql/issues/4667", () => {
let driver: Driver;
let neo4j: Neo4j;
let neoSchema: Neo4jGraphQL;

let MyThing: UniqueType;
let MyStuff: UniqueType;

beforeAll(async () => {
neo4j = new Neo4j();
driver = await neo4j.getDriver();
});

beforeEach(async () => {
MyThing = new UniqueType("MyThing");
MyStuff = new UniqueType("MyStuff");

const session = await neo4j.getSession();
try {
await session.run(`
CREATE (:${MyThing} {id: "A"})-[:THE_STUFF]->(b1:${MyStuff} {id: "C"})
CREATE (:${MyThing} {id: "B"})
`);
} finally {
await session.close();
}
});

afterEach(async () => {
await cleanNodes(driver, [MyThing, MyStuff]);
});

afterAll(async () => {
await driver.close();
});

test("when passed null as an argument of a relationship filter should check that a relationship does not exist", async () => {
const typeDefs = /* GraphQL */ `
type ${MyThing} {
id: ID! @id
stuff: ${MyStuff} @relationship(type: "THE_STUFF", direction: OUT)
}
type ${MyStuff} {
id: ID! @id
thing: ${MyThing} @relationship(type: "THE_STUFF", direction: IN)
}
`;
neoSchema = new Neo4jGraphQL({
typeDefs,
driver,
});
const query = /* GraphQL */ `
query {
${MyThing.plural}(where: { stuff: null }) {
id
stuff {
id
}
}
}
`;

const result = await graphql({
schema: await neoSchema.getSchema(),
source: query,
contextValue: neo4j.getContextValues(),
});

expect(result.errors).toBeUndefined();
expect(result.data).toEqual({
[MyThing.plural]: expect.toIncludeSameMembers([expect.objectContaining({ id: "B" })]),
});
});
});
59 changes: 59 additions & 0 deletions packages/graphql/tests/tck/issues/4667.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/*
* Copyright (c) "Neo4j"
* Neo4j Sweden AB [http://neo4j.com]
*
* This file is part of Neo4j.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import { Neo4jGraphQL } from "../../../src";
import { formatCypher, formatParams, translateQuery } from "../utils/tck-test-utils";

describe("https://github.com/neo4j/graphql/issues/4667", () => {
test("when passed null as an argument of a relationship filter should check that a relationship does not exist", async () => {
const typeDefs = /* GraphQL */ `
type MyThing {
id: ID! @id
stuff: MyStuff @relationship(type: "THE_STUFF", direction: OUT)
}
type MyStuff {
id: ID! @id
thing: MyThing @relationship(type: "THE_STUFF", direction: IN)
}
`;

const neoSchema = new Neo4jGraphQL({ typeDefs });

const query = /* GraphQL */ `
query {
myThings(where: { stuff: null }) {
id
}
}
`;

const result = await translateQuery(neoSchema, query);

expect(formatCypher(result.cypher)).toMatchInlineSnapshot(`
"MATCH (this:MyThing)
WHERE NOT (EXISTS {
MATCH (this)-[:THE_STUFF]->(this0:MyStuff)
})
RETURN this { .id } AS this"
`);

expect(formatParams(result.params)).toMatchInlineSnapshot(`"{}"`);
});
});

0 comments on commit a77109e

Please sign in to comment.