From 2e6de360c03c8eb678024c065a58875884f6a760 Mon Sep 17 00:00:00 2001 From: Darrell Warde Date: Mon, 2 Sep 2024 14:20:11 +0100 Subject: [PATCH] Simplified spatial property projection --- .changeset/nervous-masks-beam.md | 5 ++ .../src/graphql/objects/CartesianPoint.ts | 7 +- packages/graphql/src/graphql/objects/Point.ts | 10 ++- .../src/graphql/objects/utils/srid-to-crs.ts | 42 +++++++++++ .../src/schema/resolvers/field/numerical.ts | 4 +- .../attribute-fields/PointAttributeField.ts | 75 ------------------- .../queryAST/factory/FieldFactory.ts | 13 ---- .../graphql/tests/tck/array-methods.test.ts | 5 +- .../connections/filtering/node/points.test.ts | 5 +- .../filtering/relationship/points.test.ts | 5 +- packages/graphql/tests/tck/projection.test.ts | 5 +- .../graphql/tests/tck/types/point.test.ts | 55 +++----------- .../graphql/tests/tck/types/points.test.ts | 30 ++------ 13 files changed, 80 insertions(+), 181 deletions(-) create mode 100644 .changeset/nervous-masks-beam.md create mode 100644 packages/graphql/src/graphql/objects/utils/srid-to-crs.ts delete mode 100644 packages/graphql/src/translate/queryAST/ast/fields/attribute-fields/PointAttributeField.ts diff --git a/.changeset/nervous-masks-beam.md b/.changeset/nervous-masks-beam.md new file mode 100644 index 0000000000..7c41c54a16 --- /dev/null +++ b/.changeset/nervous-masks-beam.md @@ -0,0 +1,5 @@ +--- +"@neo4j/graphql": patch +--- + +Simplify the projection of spatial properties diff --git a/packages/graphql/src/graphql/objects/CartesianPoint.ts b/packages/graphql/src/graphql/objects/CartesianPoint.ts index 07f486bc79..8808e9d7c2 100644 --- a/packages/graphql/src/graphql/objects/CartesianPoint.ts +++ b/packages/graphql/src/graphql/objects/CartesianPoint.ts @@ -19,6 +19,7 @@ import { GraphQLFloat, GraphQLInt, GraphQLNonNull, GraphQLObjectType, GraphQLString } from "graphql"; import { numericalResolver } from "../../schema/resolvers/field/numerical"; +import { sridToCrs } from "./utils/srid-to-crs"; export const CartesianPoint = new GraphQLObjectType({ name: "CartesianPoint", @@ -27,22 +28,20 @@ export const CartesianPoint = new GraphQLObjectType({ fields: { x: { type: new GraphQLNonNull(GraphQLFloat), - resolve: (source) => source.point.x, }, y: { type: new GraphQLNonNull(GraphQLFloat), - resolve: (source) => source.point.y, }, z: { type: GraphQLFloat, - resolve: (source) => source.point.z, }, crs: { type: new GraphQLNonNull(GraphQLString), + resolve: (source) => sridToCrs(source.srid), }, srid: { type: new GraphQLNonNull(GraphQLInt), - resolve: (source, args, context, info) => numericalResolver(source.point, args, context, info), + resolve: (source, args, context, info) => numericalResolver(source, args, context, info), }, }, }); diff --git a/packages/graphql/src/graphql/objects/Point.ts b/packages/graphql/src/graphql/objects/Point.ts index e00749f847..795ada1fb2 100644 --- a/packages/graphql/src/graphql/objects/Point.ts +++ b/packages/graphql/src/graphql/objects/Point.ts @@ -19,6 +19,7 @@ import { GraphQLFloat, GraphQLInt, GraphQLNonNull, GraphQLObjectType, GraphQLString } from "graphql"; import { numericalResolver } from "../../schema/resolvers/field/numerical"; +import { sridToCrs } from "./utils/srid-to-crs"; export const Point = new GraphQLObjectType({ name: "Point", @@ -27,22 +28,23 @@ export const Point = new GraphQLObjectType({ fields: { longitude: { type: new GraphQLNonNull(GraphQLFloat), - resolve: (source) => source.point.x, + resolve: (source) => source.longitude || source.x, }, latitude: { type: new GraphQLNonNull(GraphQLFloat), - resolve: (source) => source.point.y, + resolve: (source) => source.latitude || source.y, }, height: { type: GraphQLFloat, - resolve: (source) => source.point.z, + resolve: (source) => source.height || source.z, }, crs: { type: new GraphQLNonNull(GraphQLString), + resolve: (source) => sridToCrs(source.srid), }, srid: { type: new GraphQLNonNull(GraphQLInt), - resolve: (source, args, context, info) => numericalResolver(source.point, args, context, info), + resolve: (source, args, context, info) => numericalResolver(source, args, context, info), }, }, }); diff --git a/packages/graphql/src/graphql/objects/utils/srid-to-crs.ts b/packages/graphql/src/graphql/objects/utils/srid-to-crs.ts new file mode 100644 index 0000000000..f5c2ef8926 --- /dev/null +++ b/packages/graphql/src/graphql/objects/utils/srid-to-crs.ts @@ -0,0 +1,42 @@ +/* + * 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 { integer } from "neo4j-driver"; +import { isIntegerable } from "../../../schema/resolvers/field/numerical"; + +export function sridToCrs(srid: unknown): string { + if (!isIntegerable(srid)) { + return ""; + } + + const integerSrid = integer.toNumber(srid); + + switch (integerSrid) { + case 4326: + return "wgs-84"; + case 4979: + return "wgs-84-3d"; + case 7203: + return "cartesian"; + case 9157: + return "cartesian-3d"; + default: + return ""; + } +} diff --git a/packages/graphql/src/schema/resolvers/field/numerical.ts b/packages/graphql/src/schema/resolvers/field/numerical.ts index dd10fc1b97..a69d93d74b 100644 --- a/packages/graphql/src/schema/resolvers/field/numerical.ts +++ b/packages/graphql/src/schema/resolvers/field/numerical.ts @@ -22,7 +22,9 @@ import type { Integer } from "neo4j-driver"; import { integer, isInt } from "neo4j-driver"; import { defaultFieldResolver } from "./defaultField"; -function isIntegerable(value: unknown): value is number | string | Integer | { low: number; high: number } | bigint { +export function isIntegerable( + value: unknown +): value is number | string | Integer | { low: number; high: number } | bigint { if (!value) { return false; } diff --git a/packages/graphql/src/translate/queryAST/ast/fields/attribute-fields/PointAttributeField.ts b/packages/graphql/src/translate/queryAST/ast/fields/attribute-fields/PointAttributeField.ts deleted file mode 100644 index 3838d6924d..0000000000 --- a/packages/graphql/src/translate/queryAST/ast/fields/attribute-fields/PointAttributeField.ts +++ /dev/null @@ -1,75 +0,0 @@ -/* - * 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 Cypher from "@neo4j/cypher-builder"; -import type { AttributeAdapter } from "../../../../../schema-model/attribute/model-adapters/AttributeAdapter"; -import { AttributeField } from "./AttributeField"; - -export class PointAttributeField extends AttributeField { - private crs: boolean; - - constructor({ attribute, alias, crs }: { attribute: AttributeAdapter; alias: string; crs: boolean }) { - super({ alias, attribute }); - this.crs = crs; - } - - public getProjectionField(variable: Cypher.Variable): Record { - const pointProjection = this.createPointProjection(variable); - return { [this.alias]: pointProjection }; - } - - protected getCypherExpr(target: Cypher.Variable): Cypher.Expr { - return this.createPointProjection(target); - } - - private createPointProjection(variable: Cypher.Variable): Cypher.Expr { - const pointProperty = variable.property(this.attribute.databaseName); - - const caseStatement = new Cypher.Case().when(Cypher.isNotNull(pointProperty)); - - // Sadly need to select the whole point object due to the risk of height/z - // being selected on a 2D point, to which the database will throw an error - if (this.attribute.typeHelper.isList()) { - const arrayProjection = this.createPointArrayProjection(pointProperty); - return caseStatement.then(arrayProjection).else(Cypher.Null); - } else { - const pointProjection = this.createPointProjectionMap(pointProperty); - return caseStatement.then(pointProjection).else(Cypher.Null); - } - } - - private createPointArrayProjection(pointProperty: Cypher.Property): Cypher.Expr { - const projectionVar = new Cypher.Variable(); - - const projectionMap = this.createPointProjectionMap(projectionVar); - - return new Cypher.ListComprehension(projectionVar).in(pointProperty).map(projectionMap); - } - - private createPointProjectionMap(variable: Cypher.Variable | Cypher.Property): Cypher.Map { - const projectionMap = new Cypher.Map(); - projectionMap.set({ point: variable }); - - if (this.crs) { - projectionMap.set({ crs: variable.property("crs") }); - } - - return projectionMap; - } -} diff --git a/packages/graphql/src/translate/queryAST/factory/FieldFactory.ts b/packages/graphql/src/translate/queryAST/factory/FieldFactory.ts index b0eb4f55fb..a4a0a580d0 100644 --- a/packages/graphql/src/translate/queryAST/factory/FieldFactory.ts +++ b/packages/graphql/src/translate/queryAST/factory/FieldFactory.ts @@ -37,7 +37,6 @@ import type { AggregationField } from "../ast/fields/aggregation-fields/Aggregat import { CountField } from "../ast/fields/aggregation-fields/CountField"; import { AttributeField } from "../ast/fields/attribute-fields/AttributeField"; import { DateTimeField } from "../ast/fields/attribute-fields/DateTimeField"; -import { PointAttributeField } from "../ast/fields/attribute-fields/PointAttributeField"; import type { ConnectionReadOperation } from "../ast/operations/ConnectionReadOperation"; import type { CompositeConnectionReadOperation } from "../ast/operations/composite/CompositeConnectionReadOperation"; import { isConcreteEntity } from "../utils/is-concrete-entity"; @@ -213,18 +212,6 @@ export class FieldFactory { }); } - if (attribute.typeHelper.isPoint() || attribute.typeHelper.isCartesianPoint()) { - const typeName = attribute.typeHelper.isList() - ? (attribute.type as ListType).ofType.name - : attribute.type.name; - const { crs } = field.fieldsByTypeName[typeName] as any; - return new PointAttributeField({ - attribute, - alias: field.alias, - crs: Boolean(crs), - }); - } - if (attribute.typeHelper.isDateTime()) { return new DateTimeField({ attribute, diff --git a/packages/graphql/tests/tck/array-methods.test.ts b/packages/graphql/tests/tck/array-methods.test.ts index bae2e2dc17..13e044c93c 100644 --- a/packages/graphql/tests/tck/array-methods.test.ts +++ b/packages/graphql/tests/tck/array-methods.test.ts @@ -156,10 +156,7 @@ describe("Arrays Methods", () => { WITH this WHERE apoc.util.validatePredicate(this.filmingLocations IS NULL, \\"Property %s cannot be NULL\\", ['filmingLocations']) SET this.filmingLocations = this.filmingLocations + [p in $this_update_filmingLocations_PUSH | point(p)] - RETURN collect(DISTINCT this { .title, filmingLocations: CASE - WHEN this.filmingLocations IS NOT NULL THEN [update_var0 IN this.filmingLocations | { point: update_var0 }] - ELSE NULL - END }) AS data" + RETURN collect(DISTINCT this { .title, .filmingLocations }) AS data" `); expect(formatParams(result.params)).toMatchInlineSnapshot(` diff --git a/packages/graphql/tests/tck/connections/filtering/node/points.test.ts b/packages/graphql/tests/tck/connections/filtering/node/points.test.ts index e65f2ab597..8f3c9ae5c6 100644 --- a/packages/graphql/tests/tck/connections/filtering/node/points.test.ts +++ b/packages/graphql/tests/tck/connections/filtering/node/points.test.ts @@ -97,10 +97,7 @@ describe("Cypher -> Connections -> Filtering -> Node -> Points", () => { WITH edges UNWIND edges AS edge WITH edge.node AS this1, edge.relationship AS this0 - RETURN collect({ properties: { screenTime: this0.screenTime, __resolveType: \\"ActedIn\\" }, node: { name: this1.name, currentLocation: CASE - WHEN this1.currentLocation IS NOT NULL THEN { point: this1.currentLocation } - ELSE NULL - END, __resolveType: \\"Actor\\" } }) AS var2 + RETURN collect({ properties: { screenTime: this0.screenTime, __resolveType: \\"ActedIn\\" }, node: { name: this1.name, currentLocation: this1.currentLocation, __resolveType: \\"Actor\\" } }) AS var2 } RETURN { edges: var2, totalCount: totalCount } AS var3 } diff --git a/packages/graphql/tests/tck/connections/filtering/relationship/points.test.ts b/packages/graphql/tests/tck/connections/filtering/relationship/points.test.ts index 1d27109630..93e61172e2 100644 --- a/packages/graphql/tests/tck/connections/filtering/relationship/points.test.ts +++ b/packages/graphql/tests/tck/connections/filtering/relationship/points.test.ts @@ -95,10 +95,7 @@ describe("Cypher -> Connections -> Filtering -> Relationship -> Points", () => { WITH edges UNWIND edges AS edge WITH edge.node AS this1, edge.relationship AS this0 - RETURN collect({ properties: { screenTime: this0.screenTime, location: CASE - WHEN this0.location IS NOT NULL THEN { point: this0.location } - ELSE NULL - END, __resolveType: \\"ActedIn\\" }, node: { name: this1.name, __resolveType: \\"Actor\\" } }) AS var2 + RETURN collect({ properties: { screenTime: this0.screenTime, location: this0.location, __resolveType: \\"ActedIn\\" }, node: { name: this1.name, __resolveType: \\"Actor\\" } }) AS var2 } RETURN { edges: var2, totalCount: totalCount } AS var3 } diff --git a/packages/graphql/tests/tck/projection.test.ts b/packages/graphql/tests/tck/projection.test.ts index 90499291c2..43fd2f0698 100644 --- a/packages/graphql/tests/tck/projection.test.ts +++ b/packages/graphql/tests/tck/projection.test.ts @@ -99,10 +99,7 @@ describe("Cypher Projection", () => { WITH create_this1 MATCH (create_this1)-[create_this2:HAS_PHOTO]->(create_this3:Photo) WHERE create_this3.url = $create_param1 - WITH create_this3 { .url, location: CASE - WHEN create_this3.location IS NOT NULL THEN { point: create_this3.location } - ELSE NULL - END } AS create_this3 + WITH create_this3 { .url, .location } AS create_this3 RETURN collect(create_this3) AS create_var4 } CALL { diff --git a/packages/graphql/tests/tck/types/point.test.ts b/packages/graphql/tests/tck/types/point.test.ts index 924bde41bc..5e501c9d30 100644 --- a/packages/graphql/tests/tck/types/point.test.ts +++ b/packages/graphql/tests/tck/types/point.test.ts @@ -55,10 +55,7 @@ describe("Cypher Points", () => { expect(formatCypher(result.cypher)).toMatchInlineSnapshot(` "MATCH (this:PointContainer) WHERE this.point = point($param0) - RETURN this { point: CASE - WHEN this.point IS NOT NULL THEN { point: this.point, crs: this.point.crs } - ELSE NULL - END } AS this" + RETURN this { .point } AS this" `); expect(formatParams(result.params)).toMatchInlineSnapshot(` @@ -88,10 +85,7 @@ describe("Cypher Points", () => { expect(formatCypher(result.cypher)).toMatchInlineSnapshot(` "MATCH (this:PointContainer) WHERE NOT (this.point = point($param0)) - RETURN this { point: CASE - WHEN this.point IS NOT NULL THEN { point: this.point } - ELSE NULL - END } AS this" + RETURN this { .point } AS this" `); expect(formatParams(result.params)).toMatchInlineSnapshot(` @@ -122,10 +116,7 @@ describe("Cypher Points", () => { expect(formatCypher(result.cypher)).toMatchInlineSnapshot(` "MATCH (this:PointContainer) WHERE this.point IN [var0 IN $param0 | point(var0)] - RETURN this { point: CASE - WHEN this.point IS NOT NULL THEN { point: this.point, crs: this.point.crs } - ELSE NULL - END } AS this" + RETURN this { .point } AS this" `); expect(formatParams(result.params)).toMatchInlineSnapshot(` @@ -158,10 +149,7 @@ describe("Cypher Points", () => { expect(formatCypher(result.cypher)).toMatchInlineSnapshot(` "MATCH (this:PointContainer) WHERE NOT (this.point IN [var0 IN $param0 | point(var0)]) - RETURN this { point: CASE - WHEN this.point IS NOT NULL THEN { point: this.point, crs: this.point.crs } - ELSE NULL - END } AS this" + RETURN this { .point } AS this" `); expect(formatParams(result.params)).toMatchInlineSnapshot(` @@ -194,10 +182,7 @@ describe("Cypher Points", () => { expect(formatCypher(result.cypher)).toMatchInlineSnapshot(` "MATCH (this:PointContainer) WHERE point.distance(this.point, point($param0.point)) < $param0.distance - RETURN this { point: CASE - WHEN this.point IS NOT NULL THEN { point: this.point } - ELSE NULL - END } AS this" + RETURN this { .point } AS this" `); expect(formatParams(result.params)).toMatchInlineSnapshot(` @@ -230,10 +215,7 @@ describe("Cypher Points", () => { expect(formatCypher(result.cypher)).toMatchInlineSnapshot(` "MATCH (this:PointContainer) WHERE point.distance(this.point, point($param0.point)) <= $param0.distance - RETURN this { point: CASE - WHEN this.point IS NOT NULL THEN { point: this.point } - ELSE NULL - END } AS this" + RETURN this { .point } AS this" `); expect(formatParams(result.params)).toMatchInlineSnapshot(` @@ -266,10 +248,7 @@ describe("Cypher Points", () => { expect(formatCypher(result.cypher)).toMatchInlineSnapshot(` "MATCH (this:PointContainer) WHERE point.distance(this.point, point($param0.point)) > $param0.distance - RETURN this { point: CASE - WHEN this.point IS NOT NULL THEN { point: this.point } - ELSE NULL - END } AS this" + RETURN this { .point } AS this" `); expect(formatParams(result.params)).toMatchInlineSnapshot(` @@ -302,10 +281,7 @@ describe("Cypher Points", () => { expect(formatCypher(result.cypher)).toMatchInlineSnapshot(` "MATCH (this:PointContainer) WHERE point.distance(this.point, point($param0.point)) >= $param0.distance - RETURN this { point: CASE - WHEN this.point IS NOT NULL THEN { point: this.point } - ELSE NULL - END } AS this" + RETURN this { .point } AS this" `); expect(formatParams(result.params)).toMatchInlineSnapshot(` @@ -340,10 +316,7 @@ describe("Cypher Points", () => { expect(formatCypher(result.cypher)).toMatchInlineSnapshot(` "MATCH (this:PointContainer) WHERE point.distance(this.point, point($param0.point)) = $param0.distance - RETURN this { point: CASE - WHEN this.point IS NOT NULL THEN { point: this.point } - ELSE NULL - END } AS this" + RETURN this { .point } AS this" `); expect(formatParams(result.params)).toMatchInlineSnapshot(` @@ -386,10 +359,7 @@ describe("Cypher Points", () => { create_this1.point = point(create_var0.point) RETURN create_this1 } - RETURN collect(create_this1 { point: CASE - WHEN create_this1.point IS NOT NULL THEN { point: create_this1.point, crs: create_this1.point.crs } - ELSE NULL - END }) AS data" + RETURN collect(create_this1 { .point }) AS data" `); expect(formatParams(result.params)).toMatchInlineSnapshot(` @@ -427,10 +397,7 @@ describe("Cypher Points", () => { "MATCH (this:PointContainer) WHERE this.id = $param0 SET this.point = point($this_update_point) - RETURN collect(DISTINCT this { point: CASE - WHEN this.point IS NOT NULL THEN { point: this.point, crs: this.point.crs } - ELSE NULL - END }) AS data" + RETURN collect(DISTINCT this { .point }) AS data" `); expect(formatParams(result.params)).toMatchInlineSnapshot(` diff --git a/packages/graphql/tests/tck/types/points.test.ts b/packages/graphql/tests/tck/types/points.test.ts index 051b2f2acb..856e62f76f 100644 --- a/packages/graphql/tests/tck/types/points.test.ts +++ b/packages/graphql/tests/tck/types/points.test.ts @@ -55,10 +55,7 @@ describe("Cypher Points", () => { expect(formatCypher(result.cypher)).toMatchInlineSnapshot(` "MATCH (this:PointContainer) WHERE this.points = [var0 IN $param0 | point(var0)] - RETURN this { points: CASE - WHEN this.points IS NOT NULL THEN [var1 IN this.points | { point: var1, crs: var1.crs }] - ELSE NULL - END } AS this" + RETURN this { .points } AS this" `); expect(formatParams(result.params)).toMatchInlineSnapshot(` @@ -90,10 +87,7 @@ describe("Cypher Points", () => { expect(formatCypher(result.cypher)).toMatchInlineSnapshot(` "MATCH (this:PointContainer) WHERE NOT (this.points = [var0 IN $param0 | point(var0)]) - RETURN this { points: CASE - WHEN this.points IS NOT NULL THEN [var1 IN this.points | { point: var1 }] - ELSE NULL - END } AS this" + RETURN this { .points } AS this" `); expect(formatParams(result.params)).toMatchInlineSnapshot(` @@ -126,10 +120,7 @@ describe("Cypher Points", () => { expect(formatCypher(result.cypher)).toMatchInlineSnapshot(` "MATCH (this:PointContainer) WHERE point($param0) IN this.points - RETURN this { points: CASE - WHEN this.points IS NOT NULL THEN [var0 IN this.points | { point: var0, crs: var0.crs }] - ELSE NULL - END } AS this" + RETURN this { .points } AS this" `); expect(formatParams(result.params)).toMatchInlineSnapshot(` @@ -160,10 +151,7 @@ describe("Cypher Points", () => { expect(formatCypher(result.cypher)).toMatchInlineSnapshot(` "MATCH (this:PointContainer) WHERE NOT (point($param0) IN this.points) - RETURN this { points: CASE - WHEN this.points IS NOT NULL THEN [var0 IN this.points | { point: var0, crs: var0.crs }] - ELSE NULL - END } AS this" + RETURN this { .points } AS this" `); expect(formatParams(result.params)).toMatchInlineSnapshot(` @@ -202,10 +190,7 @@ describe("Cypher Points", () => { create_this1.points = [create_var2 IN create_var0.points | point(create_var2)] RETURN create_this1 } - RETURN collect(create_this1 { points: CASE - WHEN create_this1.points IS NOT NULL THEN [create_var3 IN create_this1.points | { point: create_var3, crs: create_var3.crs }] - ELSE NULL - END }) AS data" + RETURN collect(create_this1 { .points }) AS data" `); expect(formatParams(result.params)).toMatchInlineSnapshot(` @@ -245,10 +230,7 @@ describe("Cypher Points", () => { "MATCH (this:PointContainer) WHERE this.id = $param0 SET this.points = [p in $this_update_points | point(p)] - RETURN collect(DISTINCT this { points: CASE - WHEN this.points IS NOT NULL THEN [update_var0 IN this.points | { point: update_var0, crs: update_var0.crs }] - ELSE NULL - END }) AS data" + RETURN collect(DISTINCT this { .points }) AS data" `); expect(formatParams(result.params)).toMatchInlineSnapshot(`