jdaugherty commented on code in PR #16836:
URL: https://github.com/apache/grails-core/pull/16836#discussion_r4476702248
##########
grails-data-neo4j/core/src/main/groovy/org/grails/datastore/gorm/neo4j/engine/Neo4jQuery.groovy:
##########
@@ -654,6 +662,22 @@ class Neo4jQuery extends Query {
RelationshipUtils.matchForAssociation(association, var, attributes)
}
+ /**
+ * The pattern that holds when the node has a relationship for the
association, or {@code null} when the
+ * property is not an association stored as a relationship. An association
is not a property of the node, so
+ * a null test on it has to look for the relationship and not for a node
property.
+ */
+ private static String associationRelationshipPattern(GraphPersistentEntity
entity, String propertyName, String prefix) {
+ if (entity.isRelationshipEntity()) {
+ return null
+ }
+ PersistentProperty property = entity.getPropertyByName(propertyName)
Review Comment:
`entity` is not always the entity the criterion belongs to.
`AssociationQueryHandler` dispatches the criteria nested in an association with
the entity it was given and only switches the prefix, so inside `owner { ... }`
this lookup runs against the class the query started from. With `Pet { Owner
owner; Kind kind }` and `Owner { String kind; Address address }`:
- `Pet.createCriteria().list { owner { isNull('kind') } }` returned the pets
whose owner has no `kind` on 8.0.x (`m_0.kind IS NULL`). Here `kind` resolves
to the association `Pet.kind`, the condition becomes `NOT
(m_0)-[:KIND]->(:Kind)`, and every pet with an owner comes back;
`isNotNull('kind')` returns nothing. A null test on a plain property that
worked is now wrong whenever the root class has an association of the same name.
- `owner { isNull('address') }` and `Pet.where { owner.address == null }`
still render `m_0.address IS NULL` and return every pet with an owner, because
`Pet` has no `address`. That is #16584 one level down, and this change does not
reach it.
One way to fix both: in `AssociationQueryHandler`, dispatch the nested
criteria with `association.associatedEntity` instead of `entity`, keeping
`entity` when the association targets a relationship entity. With that, both
queries return the right rows and the Neo4j core suite still passes. Please add
a feature for each of the two cases to `IsNullAssociationSpec`.
##########
grails-data-neo4j/core/src/main/groovy/org/grails/datastore/gorm/neo4j/engine/Neo4jQuery.groovy:
##########
@@ -359,6 +363,10 @@ class Neo4jQuery extends Query {
@Override
@CompileStatic
CypherExpression handle(GraphPersistentEntity entity, Query.IsNotNull
criterion, CypherBuilder builder, String prefix) {
+ String relationship = associationRelationshipPattern(entity,
criterion.property, prefix)
+ if (relationship != null) {
+ return new CypherExpression("${relationship}")
Review Comment:
Nit: `new CypherExpression(relationship)`. The GString adds nothing.
##########
grails-data-neo4j/core/src/test/groovy/grails/gorm/tests/IsNullAssociationSpec.groovy:
##########
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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
+ *
+ * https://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.
+ */
+
+package grails.gorm.tests
+
+import grails.gorm.annotation.Entity
+import org.apache.grails.data.neo4j.core.Neo4jGormDatastoreSpec
+import spock.lang.Issue
+
+/**
+ * An association is stored as a relationship, so isNull and isNotNull on it
test for the absence and presence of
+ * the relationship and not for a property of the node.
+ */
+@Issue('16584')
+class IsNullAssociationSpec extends Neo4jGormDatastoreSpec {
Review Comment:
The spec exercises one shape, a unidirectional to-one to the same class. The
null test now goes through `formatAssociationPatternFromExisting`, and these
branches have no feature:
- the reversed pattern of a bidirectional many-to-one
(`(n)<-[:ITEMS]-(:Shelf)`),
- a to-many,
- an association to a relationship entity (`(n)<-[]-(:Celeb)` for
`Movie.cast`),
- the `Basic` exclusion in `associationRelationshipPattern`.
The first three return the right rows when I run them, so this is missing
coverage and not a defect. Please add features for them.
##########
grails-data-neo4j/core/src/main/groovy/org/grails/datastore/gorm/neo4j/engine/Neo4jQuery.groovy:
##########
@@ -654,6 +662,22 @@ class Neo4jQuery extends Query {
RelationshipUtils.matchForAssociation(association, var, attributes)
}
+ /**
+ * The pattern that holds when the node has a relationship for the
association, or {@code null} when the
+ * property is not an association stored as a relationship. An association
is not a property of the node, so
+ * a null test on it has to look for the relationship and not for a node
property.
+ */
+ private static String associationRelationshipPattern(GraphPersistentEntity
entity, String propertyName, String prefix) {
+ if (entity.isRelationshipEntity()) {
+ return null
+ }
Review Comment:
No test reaches this branch: the Neo4j core suite passes with these three
lines removed. Without them `CastMember.createCriteria().list { isNull('from')
}` fails with `Type mismatch: expected Node but was Relationship`, because the
pattern starts at the relationship variable (`NOT (r)-[]->(:Movie)`). Please
add a feature that runs `isNull` and `isNotNull` on `from` and `to` of a
relationship entity, for example in `RelationshipMappingSpec`.
##########
grails-data-neo4j/core/src/main/groovy/org/grails/datastore/gorm/neo4j/engine/Neo4jQuery.groovy:
##########
@@ -335,6 +335,10 @@ class Neo4jQuery extends Query {
@Override
@CompileStatic
CypherExpression handle(GraphPersistentEntity entity, Query.IsNull
criterion, CypherBuilder builder, String prefix) {
+ String relationship = associationRelationshipPattern(entity,
criterion.property, prefix)
+ if (relationship != null) {
+ return new CypherExpression("NOT ${relationship}")
Review Comment:
This changes the rows existing queries return, so it needs a section in the
Grails 8 upgrade guide (`grails-doc/src/en/guide/upgrading/upgrading80x.adoc`),
also when the earlier result was a bug. For GORM for Neo4j the note has to
cover:
- `isNull` on an association returned every instance and now returns only
those without the relationship; `isNotNull` returned none and now returns those
with it.
- The same holds for `findAllBy<Association>IsNull` / `IsNotNull`,
`findAllBy<Association>(null)`, the `countBy` forms, and `where { association
== null }` / `!= null`.
- On a to-many association, `isNull` now means the collection has no element.
- `deleteAll()` and `updateAll()` follow the query: `Pet.where { owner ==
null }.deleteAll()` deleted every pet on 8.0.x and now deletes only the pets
without an owner.
It should also say how to get the earlier rows back, which is to remove the
criterion.
##########
grails-data-neo4j/core/src/main/groovy/org/grails/datastore/gorm/neo4j/engine/Neo4jQuery.groovy:
##########
@@ -654,6 +662,22 @@ class Neo4jQuery extends Query {
RelationshipUtils.matchForAssociation(association, var, attributes)
}
+ /**
+ * The pattern that holds when the node has a relationship for the
association, or {@code null} when the
+ * property is not an association stored as a relationship. An association
is not a property of the node, so
+ * a null test on it has to look for the relationship and not for a node
property.
+ */
+ private static String associationRelationshipPattern(GraphPersistentEntity
entity, String propertyName, String prefix) {
+ if (entity.isRelationshipEntity()) {
+ return null
+ }
+ PersistentProperty property = entity.getPropertyByName(propertyName)
+ if (property instanceof Association && !(property instanceof Basic)) {
Review Comment:
Not blocking: `Embedded` is an `Association` and not a `Basic`, so a null
test on an embedded property now renders `NOT (n)-[:HOME]->(:Address)`. An
embedded component is flattened onto the owner's node and never has a
relationship, so by the javadoc above this method should return `null` for it.
The rows are the same as before (all for `isNull`, none for `isNotNull`), so
nothing regresses, but please add `Embedded` to the exclusion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
