Skip to content

Commit 6ea4e78

Browse files
meistermeiermeistermeier
committed
GH-2879 - Use elementId in derive queries.
There is a convenient function if the property in derived queries refers to the internal id to use the `id` / `elementId` function. This was never taken into consideration when the elementId support got introduced. The fix is straight forward in line with the existing call to `Cypher.call("id")` to avoid introducing more changes to the infrastructure. Also includes fixture for the logging capture based tests around elementId/id to catch the right log output again. Closes #2879
1 parent af1e40c commit 6ea4e78

File tree

5 files changed

+83
-20
lines changed

5 files changed

+83
-20
lines changed

‎src/main/java/org/springframework/data/neo4j/repository/query/CypherQueryCreator.java

+10-4
Original file line numberDiff line numberDiff line change
@@ -483,11 +483,17 @@ private Expression toCypherProperty(PersistentPropertyPath<Neo4jPersistentProper
483483

484484
String containerName = getContainerName(path, owner);
485485
if (owner.equals(this.nodeDescription) && path.getLength() == 1) {
486-
expression = leafProperty.isInternalIdProperty() ?
487-
Cypher.call("id").withArgs(Constants.NAME_OF_TYPED_ROOT_NODE.apply(nodeDescription)).asFunction() :
488-
Cypher.property(containerName, leafProperty.getPropertyName());
489-
} else if (leafProperty.isInternalIdProperty()) {
486+
if (leafProperty.isInternalIdProperty() && owner.isUsingDeprecatedInternalId()) {
487+
expression = Cypher.call("id").withArgs(Constants.NAME_OF_TYPED_ROOT_NODE.apply(nodeDescription)).asFunction();
488+
} else if (leafProperty.isInternalIdProperty()) {
489+
expression = Cypher.call("elementId").withArgs(Constants.NAME_OF_TYPED_ROOT_NODE.apply(nodeDescription)).asFunction();
490+
} else {
491+
expression = Cypher.property(containerName, leafProperty.getPropertyName());
492+
}
493+
} else if (leafProperty.isInternalIdProperty() && owner.isUsingDeprecatedInternalId()) {
490494
expression = Cypher.call("id").withArgs(Cypher.name(containerName)).asFunction();
495+
} else if (leafProperty.isInternalIdProperty()) {
496+
expression = Cypher.call("elementId").withArgs(Cypher.name(containerName)).asFunction();
491497
} else {
492498
expression = Cypher.property(containerName, leafProperty.getPropertyName());
493499
}

‎src/test/java/org/springframework/data/neo4j/integration/issues/pure_element_id/AbstractElementIdTestBase.java

+13-1
Original file line numberDiff line numberDiff line change
@@ -15,26 +15,32 @@
1515
*/
1616
package org.springframework.data.neo4j.integration.issues.pure_element_id;
1717

18+
import java.util.List;
1819
import java.util.Objects;
1920
import java.util.function.Predicate;
2021
import java.util.regex.Pattern;
2122

23+
import ch.qos.logback.classic.Level;
2224
import org.junit.jupiter.api.BeforeEach;
2325
import org.junit.jupiter.api.Tag;
2426
import org.neo4j.driver.Driver;
2527
import org.neo4j.driver.Session;
2628
import org.springframework.beans.factory.annotation.Autowired;
2729
import org.springframework.data.neo4j.test.BookmarkCapture;
30+
import org.springframework.data.neo4j.test.LogbackCapture;
2831
import org.springframework.data.neo4j.test.Neo4jExtension;
2932

33+
import static org.assertj.core.api.Assertions.assertThat;
34+
3035
@Tag(Neo4jExtension.NEEDS_VERSION_SUPPORTING_ELEMENT_ID)
3136
abstract class AbstractElementIdTestBase {
3237

3338
protected static Neo4jExtension.Neo4jConnectionSupport neo4jConnectionSupport;
3439

3540
@BeforeEach
36-
void setupData(@Autowired Driver driver, @Autowired BookmarkCapture bookmarkCapture) {
41+
void setupData(LogbackCapture logbackCapture, @Autowired Driver driver, @Autowired BookmarkCapture bookmarkCapture) {
3742

43+
logbackCapture.addLogger("org.springframework.data.neo4j.cypher.deprecation", Level.WARN);
3844
try (Session session = driver.session()) {
3945
session.run("MATCH (n) DETACH DELETE n").consume();
4046
bookmarkCapture.seedWith(session.lastBookmarks());
@@ -68,4 +74,10 @@ static String adaptQueryTo44IfNecessary(String query) {
6874
return query;
6975
}
7076

77+
static void assertThatLogMessageDoNotIndicateIDUsage(LogbackCapture logbackCapture) {
78+
List<String> formattedMessages = logbackCapture.getFormattedMessages();
79+
assertThat(formattedMessages)
80+
.noneMatch(s -> s.contains("Neo.ClientNotification.Statement.FeatureDeprecationWarning") ||
81+
s.contains("The query used a deprecated function. ('id' is no longer supported)"));
82+
}
7183
}

‎src/test/java/org/springframework/data/neo4j/integration/issues/pure_element_id/ImperativeElementIdIT.java

+29-6
Original file line numberDiff line numberDiff line change
@@ -50,9 +50,13 @@
5050
public class ImperativeElementIdIT extends AbstractElementIdTestBase {
5151

5252
interface Repo1 extends Neo4jRepository<NodeWithGeneratedId1, String> {
53+
54+
NodeWithGeneratedId1 findByIdIn(List<String> ids);
5355
}
5456

5557
interface Repo2 extends Neo4jRepository<NodeWithGeneratedId2, String> {
58+
59+
NodeWithGeneratedId2 findByRelatedNodesIdIn(List<String> ids);
5660
}
5761

5862
interface Repo3 extends Neo4jRepository<NodeWithGeneratedId3, String> {
@@ -61,6 +65,31 @@ interface Repo3 extends Neo4jRepository<NodeWithGeneratedId3, String> {
6165
interface Repo4 extends Neo4jRepository<NodeWithGeneratedId4, String> {
6266
}
6367

68+
@Test
69+
void dontCallIdForDerivedQueriesWithInClause(LogbackCapture logbackCapture, @Autowired Repo1 repo1) {
70+
71+
var node = repo1.save(new NodeWithGeneratedId1("testValue"));
72+
String id = node.getId();
73+
74+
repo1.findByIdIn(List.of(id));
75+
76+
assertThatLogMessageDoNotIndicateIDUsage(logbackCapture);
77+
}
78+
79+
@Test
80+
void dontCallIdForDerivedQueriesWithRelatedInClause(LogbackCapture logbackCapture, @Autowired Repo2 repo2) {
81+
var node1 = new NodeWithGeneratedId1("testValue");
82+
var node2 = new NodeWithGeneratedId2("testValue");
83+
node2.setRelatedNodes(List.of(node1));
84+
var savedNode2 = repo2.save(node2);
85+
86+
String id = savedNode2.getRelatedNodes().get(0).getId();
87+
88+
repo2.findByRelatedNodesIdIn(List.of(id));
89+
90+
assertThatLogMessageDoNotIndicateIDUsage(logbackCapture);
91+
}
92+
6493
@Test
6594
void simpleNodeCreationShouldFillIdAndNotUseIdFunction(LogbackCapture logbackCapture, @Autowired Repo1 repo1) {
6695

@@ -322,12 +351,6 @@ RETURN count(*)"""),
322351
}
323352
}
324353

325-
private static void assertThatLogMessageDoNotIndicateIDUsage(LogbackCapture logbackCapture) {
326-
assertThat(logbackCapture.getFormattedMessages())
327-
.noneMatch(s -> s.contains("Neo.ClientNotification.Statement.FeatureDeprecationWarning") ||
328-
s.contains("The query used a deprecated function. ('id' is no longer supported)"));
329-
}
330-
331354
@Configuration
332355
@EnableTransactionManagement
333356
@EnableNeo4jRepositories(considerNestedRepositories = true)

‎src/test/java/org/springframework/data/neo4j/integration/issues/pure_element_id/ReactiveElementIdIT.java

+29-8
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,6 @@
1515
*/
1616
package org.springframework.data.neo4j.integration.issues.pure_element_id;
1717

18-
import static org.assertj.core.api.Assertions.assertThat;
19-
2018
import java.util.List;
2119
import java.util.Map;
2220
import java.util.Optional;
@@ -40,6 +38,9 @@
4038
import org.springframework.lang.NonNull;
4139
import org.springframework.transaction.ReactiveTransactionManager;
4240
import org.springframework.transaction.annotation.EnableTransactionManagement;
41+
import reactor.core.publisher.Mono;
42+
43+
import static org.assertj.core.api.Assertions.assertThat;
4344

4445
/**
4546
* Assertions that no {@code id()} calls are generated when no deprecated id types are present.
@@ -55,9 +56,11 @@ public class ReactiveElementIdIT extends AbstractElementIdTestBase {
5556

5657

5758
interface Repo1 extends ReactiveNeo4jRepository<NodeWithGeneratedId1, String> {
59+
Mono<NodeWithGeneratedId1> findByIdIn(List<String> ids);
5860
}
5961

6062
interface Repo2 extends ReactiveNeo4jRepository<NodeWithGeneratedId2, String> {
63+
Mono<NodeWithGeneratedId2> findByRelatedNodesIdIn(List<String> ids);
6164
}
6265

6366
interface Repo3 extends ReactiveNeo4jRepository<NodeWithGeneratedId3, String> {
@@ -66,6 +69,30 @@ interface Repo3 extends ReactiveNeo4jRepository<NodeWithGeneratedId3, String> {
6669
interface Repo4 extends ReactiveNeo4jRepository<NodeWithGeneratedId4, String> {
6770
}
6871

72+
@Test
73+
void dontCallIdForDerivedQueriesWithInClause(LogbackCapture logbackCapture, @Autowired Repo1 repo1) {
74+
75+
var node = repo1.save(new NodeWithGeneratedId1("testValue")).block();
76+
String id = node.getId();
77+
78+
repo1.findByIdIn(List.of(id)).block();
79+
80+
assertThatLogMessageDoNotIndicateIDUsage(logbackCapture);
81+
}
82+
83+
@Test
84+
void dontCallIdForDerivedQueriesWithRelatedInClause(LogbackCapture logbackCapture, @Autowired Repo2 repo2) {
85+
var node1 = new NodeWithGeneratedId1("testValue");
86+
var node2 = new NodeWithGeneratedId2("testValue");
87+
node2.setRelatedNodes(List.of(node1));
88+
var savedNode2 = repo2.save(node2).block();
89+
90+
String id = savedNode2.getRelatedNodes().get(0).getId();
91+
92+
repo2.findByRelatedNodesIdIn(List.of(id)).block();
93+
94+
assertThatLogMessageDoNotIndicateIDUsage(logbackCapture);
95+
}
6996

7097
@Test
7198
void simpleNodeCreationShouldFillIdAndNotUseIdFunction(LogbackCapture logbackCapture, @Autowired Repo1 repo1) {
@@ -336,12 +363,6 @@ RETURN count(*)"""),
336363
}
337364
}
338365

339-
private static void assertThatLogMessageDoNotIndicateIDUsage(LogbackCapture logbackCapture) {
340-
assertThat(logbackCapture.getFormattedMessages())
341-
.noneMatch(s -> s.contains("Neo.ClientNotification.Statement.FeatureDeprecationWarning") ||
342-
s.contains("The query used a deprecated function. ('id' is no longer supported)"));
343-
}
344-
345366
@Configuration
346367
@EnableTransactionManagement
347368
@EnableReactiveNeo4jRepositories(considerNestedRepositories = true)

‎src/test/java/org/springframework/data/neo4j/test/LogbackCapture.java

+2-1
Original file line numberDiff line numberDiff line change
@@ -63,12 +63,13 @@ void start() {
6363
}
6464

6565
void clear() {
66+
this.resetLogLevel();
6667
this.listAppender.list.clear();
6768
}
6869

6970
@Override
7071
public void close() {
71-
resetLogLevel();
72+
this.resetLogLevel();
7273
this.listAppender.stop();
7374
this.logger.detachAppender(listAppender);
7475
}

0 commit comments

Comments
 (0)