Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove run first column #2769

Merged
merged 11 commits into from
Jan 30, 2023
8 changes: 8 additions & 0 deletions docs/contributing/DEVELOPING.md
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,14 @@ run the following from `packages/graphql`:
yarn test:tck
```

### Verify TCK Tests

You can run all the TCK tests against the database to check that the Cypher generated is valid. This can be done with the env variable `VERIFY_TCK`

```bash
VERIFY_TCK yarn test:tck
```

### Testing using docker

```bash
Expand Down
1 change: 1 addition & 0 deletions examples/neo-place/typedefs.graphql
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ type Query {
ORDER BY p.position ASC
RETURN collect(color) as canvas
"""
columnName: "canvas"
)
@auth(rules: [{ isAuthenticated: true }])
}
Expand Down
2 changes: 2 additions & 0 deletions examples/neo-push/server/src/gql/Blog.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ export const typeDefs = gql`
WITH creator IS NOT NULL AS isCreator
RETURN isCreator
"""
columnName: "isCreator"
)
isAuthor: Boolean
@cypher(
Expand All @@ -22,6 +23,7 @@ export const typeDefs = gql`
WITH author IS NOT NULL AS isAuthor
RETURN isAuthor
"""
columnName: "isAuthor"
)
createdAt: DateTime @timestamp(operations: [CREATE])
updatedAt: DateTime @timestamp(operations: [UPDATE])
Expand Down
1 change: 1 addition & 0 deletions examples/neo-push/server/src/gql/Comment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export const typeDefs = gql`
) AS canDelete
RETURN canDelete
"""
columnName: "canDelete"
)
createdAt: DateTime @timestamp(operations: [CREATE])
updatedAt: DateTime @timestamp(operations: [UPDATE])
Expand Down
2 changes: 2 additions & 0 deletions examples/neo-push/server/src/gql/Post.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export const typeDefs = gql`
) AS canEdit
RETURN canEdit
"""
columnName: "canEdit"
)
canDelete: Boolean
@cypher(
Expand All @@ -35,6 +36,7 @@ export const typeDefs = gql`
) AS canDelete
RETURN canDelete
"""
columnName: "canDelete"
)
createdAt: DateTime @timestamp(operations: [CREATE])
updatedAt: DateTime @timestamp(operations: [UPDATE])
Expand Down
2 changes: 1 addition & 1 deletion examples/subscriptions/apollo_rabbitmq/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ const plugin = new Neo4jGraphQLSubscriptionsAMQPPlugin({
});

//Alternatively, we can remove the AMQP server if we are only using a development server
// const plugin = new new Neo4jGraphQLSubscriptionsSingleInstancePlugin();
// const plugin = new Neo4jGrapghQLSubscriptionsSingleInstancePlugin();

if (!process.argv[2]) throw new Error("Usage node server.js [port]");

Expand Down
7 changes: 1 addition & 6 deletions packages/graphql/src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,7 @@ const DEBUG_PREFIX = "@neo4j/graphql";
export const AUTH_FORBIDDEN_ERROR = "@neo4j/graphql/FORBIDDEN";
export const AUTH_UNAUTHENTICATED_ERROR = "@neo4j/graphql/UNAUTHENTICATED";
export const MIN_VERSIONS = [{ majorMinor: "4.3", neo4j: "4.3.2" }];
export const REQUIRED_APOC_FUNCTIONS = [
"apoc.util.validatePredicate",
"apoc.cypher.runFirstColumnSingle",
"apoc.cypher.runFirstColumnMany",
"apoc.date.convertFormat",
];
export const REQUIRED_APOC_FUNCTIONS = ["apoc.util.validatePredicate", "apoc.date.convertFormat"];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥳

export const REQUIRED_APOC_PROCEDURES = ["apoc.util.validate", "apoc.do.when", "apoc.cypher.doIt"];
export const DEBUG_ALL = `${DEBUG_PREFIX}:*`;
export const DEBUG_AUTH = `${DEBUG_PREFIX}:auth`;
Expand Down
5 changes: 2 additions & 3 deletions packages/graphql/src/graphql/directives/cypher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,8 @@ export const cypherDirective = new GraphQLDirective({
type: new GraphQLNonNull(GraphQLString),
},
columnName: {
description:
"[Experimental] Name of the returned variable from the Cypher statement, if provided, the query will be optimized to improve performance.",
type: GraphQLString,
description: "Name of the returned variable from the Cypher statement.",
type: new GraphQLNonNull(GraphQLString),
},
},
});
6 changes: 3 additions & 3 deletions packages/graphql/src/schema/make-augmented-schema.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ describe("makeAugmentedSchema", () => {
}

type Query {
movies: [Movie!]! @cypher(statement: "")
movies: [Movie!]! @cypher(statement: "RETURN 5 as a", columnName: "a")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The examples in this file upset me, I really wish there was a way we could have Cypher queries like this without the aliasing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but none we could find :(

}
`;

Expand Down Expand Up @@ -225,7 +225,7 @@ describe("makeAugmentedSchema", () => {
name: String
}

interface ActedIn @cypher(statement: "RETURN rand()") {
interface ActedIn @cypher(statement: "RETURN rand() as rand", columnName: "rand") {
screenTime: Int
}
`;
Expand Down Expand Up @@ -282,7 +282,7 @@ describe("makeAugmentedSchema", () => {
}

interface ActedIn {
id: ID @cypher(statement: "RETURN id(this)")
id: ID @cypher(statement: "RETURN id(this) as id", columnName: "id")
roles: [String]
}
`;
Expand Down
19 changes: 16 additions & 3 deletions packages/graphql/src/schema/validation/validate-document.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@ describe("validateDocument", () => {
post.id = randomUUID()
RETURN post
"""
columnName: "post"
)
}

Expand Down Expand Up @@ -277,6 +278,7 @@ describe("validateDocument", () => {
s.salaryReviewDate = salary.salaryReviewDate
RETURN s
"""
columnName: "s"
)

mergeEmploymentRecords(employmentRecords: [EmpRecord]): [EmploymentRecord]
Expand All @@ -303,6 +305,7 @@ describe("validateDocument", () => {
MERGE (er)-[:PAYS_SALARY]->(s)
RETURN er
"""
columnName: "er"
)
}
`;
Expand All @@ -319,7 +322,7 @@ describe("validateDocument", () => {
}

type Query {
nodes: [Test] @cypher(statement: "")
nodes: [Test] @cypher(statement: "", columnName: "")
}
`;

Expand Down Expand Up @@ -374,10 +377,15 @@ describe("validateDocument", () => {
}

extend type Order {
subTotal: Float @cypher(statement: "MATCH (this)-[:CONTAINS]->(b:Book) RETURN sum(b.price)")
subTotal: Float
@cypher(
statement: "MATCH (this)-[:CONTAINS]->(b:Book) RETURN sum(b.price) as result"
columnName: "result"
)
shippingCost: Float
@cypher(
statement: "MATCH (this)-[:SHIPS_TO]->(a:Address) RETURN round(0.01 * distance(a.location, Point({latitude: 40.7128, longitude: -74.0060})) / 1000, 2)"
statement: "MATCH (this)-[:SHIPS_TO]->(a:Address) RETURN round(0.01 * distance(a.location, Point({latitude: 40.7128, longitude: -74.0060})) / 1000, 2) as result"
columnName: "resul"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
columnName: "resul"
columnName: "result"

)
estimatedDelivery: DateTime @customResolver
}
Expand All @@ -389,6 +397,7 @@ describe("validateDocument", () => {
recommended(limit: Int = 3): [Book]
@cypher(
statement: "MATCH (this)-[:PLACED]->(:Order)-[:CONTAINS]->(:Book)<-[:CONTAINS]-(:Order)<-[:PLACED]-(c:Customer) MATCH (c)-[:PLACED]->(:Order)-[:CONTAINS]->(rec:Book) WHERE NOT exists((this)-[:PLACED]->(:Order)-[:CONTAINS]->(rec)) RETURN rec LIMIT $limit"
columnName: "rec"
)
}

Expand All @@ -402,6 +411,7 @@ describe("validateDocument", () => {
currentWeather: Weather
@cypher(
statement: "CALL apoc.load.json('https://www.7timer.info/bin/civil.php?lon=' + this.location.longitude + '&lat=' + this.location.latitude + '&ac=0&unit=metric&output=json&tzshift=0') YIELD value WITH value.dataseries[0] as weather RETURN {temperature: weather.temp2m, windSpeed: weather.wind10m.speed, windDirection: weather.wind10m.direction, precipitation: weather.prec_type, summary: weather.weather} AS conditions"
columnName: "condition"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
columnName: "condition"
columnName: "conditions"

)
}

Expand Down Expand Up @@ -435,6 +445,7 @@ describe("validateDocument", () => {
ORDER BY jaccard DESC
RETURN b LIMIT 1
"""
columnName: "jaccard"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
columnName: "jaccard"
columnName: "b"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!

)
}

Expand Down Expand Up @@ -467,6 +478,7 @@ describe("validateDocument", () => {
MERGE (t)-[:ABOUT]->(s)
RETURN s
"""
columnName: "s"
)
}

Expand All @@ -477,6 +489,7 @@ describe("validateDocument", () => {
CALL db.index.fulltext.queryNodes('bookIndex', $searchString+'~')
YIELD node RETURN node
"""
columnName: "node"
)
}
`;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,6 @@ export function translateCypherDirectiveProjection({
}
}

let customCypherClause: Cypher.Clause | undefined;
const nodeRef = new Cypher.NamedNode(chainStr);

// Null default argument values are not passed into the resolve tree therefore these are not being passed to
Expand All @@ -161,23 +160,12 @@ export function translateCypherDirectiveProjection({
);
const extraArgs = { ...nullArgumentValues, ...field.args };

if (!cypherField.columnName) {
const runCypherInApocClause = createCypherDirectiveApocProcedure({
nodeRef,
expectMultipleValues,
context,
cypherField,
extraArgs,
});
customCypherClause = new Cypher.Unwind([runCypherInApocClause, param]);
} else {
customCypherClause = createCypherDirectiveSubquery({
cypherField,
nodeRef,
resultVariable: param,
extraArgs,
});
}
const customCypherClause = createCypherDirectiveSubquery({
cypherField,
nodeRef,
resultVariable: param,
extraArgs,
});

const unionExpression = hasUnionLabelsPredicate ? new Cypher.With("*").where(hasUnionLabelsPredicate) : undefined;

Expand Down Expand Up @@ -210,40 +198,6 @@ export function translateCypherDirectiveProjection({
return res;
}

function createCypherDirectiveApocProcedure({
cypherField,
expectMultipleValues,
context,
nodeRef,
extraArgs,
}: {
cypherField: CypherField;
expectMultipleValues: boolean;
context: Context;
nodeRef: Cypher.Node;
extraArgs: Record<string, any>;
}): Cypher.apoc.RunFirstColumn {
const rawApocParams = Object.entries(extraArgs);

const apocParams: Record<string, Cypher.Param> = rawApocParams.reduce((acc, [key, value]) => {
acc[key] = new Cypher.Param(value);
return acc;
}, {});

const apocParamsMap = new Cypher.Map({
...apocParams,
this: nodeRef,
...(context.auth && { auth: new Cypher.NamedParam("auth") }),
...(Boolean(context.cypherParams) && { cypherParams: new Cypher.NamedParam("cypherParams") }),
});
const apocClause = new Cypher.apoc.RunFirstColumn(
cypherField.statement,
apocParamsMap,
Boolean(expectMultipleValues)
);
return apocClause;
}

function createCypherDirectiveSubquery({
cypherField,
nodeRef,
Expand Down
37 changes: 4 additions & 33 deletions packages/graphql/src/translate/translate-top-level-cypher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -158,18 +158,10 @@ export function translateTopLevelCypher({
const apocParamsStr = `{${apocParams.strs.length ? `${apocParams.strs.join(", ")}` : ""}}`;

if (type === "Query") {
if (field.columnName) {
const experimentalCypherStatement = createCypherDirectiveSubquery({
field,
});
cypherStrs.push(...experimentalCypherStatement);
} else {
const legacyCypherStatement = createCypherDirectiveApocProcedure({
field,
apocParams: apocParamsStr,
});
cypherStrs.push(...legacyCypherStatement);
}
const experimentalCypherStatement = createCypherDirectiveSubquery({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably shouldn't be called experimental anymore

Suggested change
const experimentalCypherStatement = createCypherDirectiveSubquery({
const cypherStatement = createCypherDirectiveSubquery({

field,
});
cypherStrs.push(...experimentalCypherStatement);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cypherStrs.push(...experimentalCypherStatement);
cypherStrs.push(...cypherStatement);

} else {
cypherStrs.push(`
CALL apoc.cypher.doIt("${statement}", ${apocParamsStr}) YIELD value
Expand Down Expand Up @@ -207,27 +199,6 @@ export function translateTopLevelCypher({
}).build();
}

function createCypherDirectiveApocProcedure({
field,
apocParams,
}: {
field: CypherField;
apocParams: string;
}): string[] {
const isArray = field.typeMeta.array;
const expectMultipleValues = !field.isScalar && !field.isEnum && isArray;
const cypherStrs: string[] = [];

if (expectMultipleValues) {
cypherStrs.push(`WITH apoc.cypher.runFirstColumnMany("${field.statement}", ${apocParams}) as x`);
} else {
cypherStrs.push(`WITH apoc.cypher.runFirstColumnSingle("${field.statement}", ${apocParams}) as x`);
}

cypherStrs.push("UNWIND x as this\nWITH this");
return cypherStrs;
}

function createCypherDirectiveSubquery({ field }: { field: CypherField }): string[] {
const cypherStrs: string[] = [];
cypherStrs.push("CALL {", field.statement, "}");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ describe("connections sort", () => {
title: String!
runtime: Int!
actors: [${Actor}!]! @relationship(type: "ACTED_IN", direction: IN, properties: "ActedIn")
numberOfActors: Int! @cypher(statement: "MATCH (actor:${Actor})-[:ACTED_IN]->(this) RETURN count(actor)")
numberOfActors: Int! @cypher(statement: "MATCH (actor:${Actor})-[:ACTED_IN]->(this) RETURN count(actor) as count", columnName: "count")
}

type ${Series} implements Production {
Expand All @@ -66,8 +66,9 @@ describe("connections sort", () => {
@cypher(
statement: """
MATCH (this)-[r:ACTED_IN]->(:${Movie})
RETURN sum(r.screenTime)
"""
RETURN sum(r.screenTime) as sum
""",
columnName: "sum"
)
}
interface ActedIn @relationshipProperties {
Expand Down
Loading