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.

Sign up for GitHub

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

Fix var tests dev #2650

Merged
merged 7 commits into from
Mar 29, 2022
Merged

Fix var tests dev #2650

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a737717
fix ExportCsvTest
vga91 Feb 24, 2022
9b7c3c0
trigger fix tests
vga91 Feb 24, 2022
4dad982
fixed schemas tests
vga91 Feb 28, 2022
96d3439
fixed metrics test
vga91 Feb 28, 2022
e048108
trigger fix tests
vga91 Feb 24, 2022
71096fe
changed TriggerTest
vga91 Mar 17, 2022
fea4aa9
removed Ignore from test
conker84 Mar 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions core/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,6 @@ dependencies {
testImplementation group: 'com.amazonaws', name: 'aws-java-sdk-comprehend', version: '1.11.683'

testImplementation group: 'org.codehaus.jackson', name: 'jackson-mapper-asl', version: '1.9.7'
implementation group: 'com.fasterxml.jackson.core', name: 'jackson-databind', version: '2.10.5.1'
implementation group: 'commons-beanutils', name: 'commons-beanutils', version: '1.9.4'
implementation group: 'com.opencsv', name: 'opencsv', version: '4.6'
implementation group: 'commons-beanutils', name: 'commons-beanutils', version: '1.9.4'
Expand All @@ -119,7 +118,7 @@ dependencies {
testImplementation group: 'org.xmlunit', name: 'xmlunit-core', version: '2.2.1'
testImplementation group: 'com.github.adejanovski', name: 'cassandra-jdbc-wrapper', version: '3.1.0'

testImplementation group: 'com.fasterxml.jackson.dataformat', name: 'jackson-dataformat-csv', version: '2.9.7'
testImplementation group: 'com.fasterxml.jackson.dataformat', name: 'jackson-dataformat-csv', version: '2.13.1'
testImplementation group: 'org.skyscreamer', name: 'jsonassert', version: '1.5.0'
testImplementation group: 'org.assertj', name: 'assertj-core', version: '3.13.2'
testImplementation group: 'org.mockito', name: 'mockito-core', version: '4.2.0'
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/apoc/util/FileUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ public static File getMetricsDirectory() {
// "dbms.directories.metrics", // metrics is only in EE
"dbms.directories.plugins",
"dbms.directories.run",
"dbms.directories.tx_log",
"dbms.directories.transaction.logs.root", // in Neo4j 5.0 GraphDatabaseSettings.transaction_logs_root_path changed from tx_log to this config
"unsupported.dbms.directories.neo4j_home"
);

Expand Down
13 changes: 7 additions & 6 deletions core/src/test/java/apoc/schema/SchemasEnterpriseFeaturesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import org.apache.commons.lang3.StringUtils;
import org.junit.After;
import org.junit.AfterClass;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Test;
import org.neo4j.driver.Record;
Expand Down Expand Up @@ -93,7 +94,7 @@ public void testKeptNodeKeyAndUniqueConstraintIfExists() {
assertEquals(1, result.size());
Map<String, Object> firstResult = result.get(0).asMap();
assertThat( (String) firstResult.get( "createStatement" ) )
.contains( "CREATE CONSTRAINT", "FOR (foo:`Foo`) REQUIRE (foo.`foo`, foo.`bar`) IS NODE KEY" );
.contains( "CREATE CONSTRAINT", "FOR (n:`Foo`) REQUIRE (n.`foo`, n.`bar`) IS NODE KEY" );
tx.commit();
return null;
});
Expand Down Expand Up @@ -131,8 +132,8 @@ public void testKeptNodeKeyAndUniqueConstraintIfExistsAndDropExistingIsFalse() {
.map(record -> (String) record.asMap().get("createStatement"))
.collect(Collectors.toList());
List<String> expectedDescriptions = List.of(
"FOR (foo:`Foo`) REQUIRE (foo.`foo`, foo.`bar`) IS NODE KEY",
"FOR (foo:`Foo`) REQUIRE (foo.`bar`, foo.`foo`) IS NODE KEY" );
"FOR (n:`Foo`) REQUIRE (n.`foo`, n.`bar`) IS NODE KEY",
"FOR (n:`Foo`) REQUIRE (n.`bar`, n.`foo`) IS NODE KEY" );
assertMatchesAll( expectedDescriptions, actualDescriptions );
tx.commit();
return null;
Expand Down Expand Up @@ -162,8 +163,8 @@ public void testCreateUniqueAndIsNodeKeyConstraintInSameLabel() {
.map(record -> (String) record.asMap().get("createStatement"))
.collect(Collectors.toList());
List<String> expectedDescriptions = List.of(
"FOR (galileo:`Galileo`) REQUIRE (galileo.`newton`, galileo.`tesla`) IS NODE KEY",
"FOR ( galileo:`Galileo` ) REQUIRE (galileo.`curie`) IS UNIQUE");
"FOR (n:`Galileo`) REQUIRE (n.`newton`, n.`tesla`) IS NODE KEY",
"FOR (n:`Galileo`) REQUIRE (n.`curie`) IS UNIQUE");
Comment on lines -165 to +167
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why we had to change this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Because previously there was call db.constraints (), now there is SHOW CONSTRAINTS YIELD createStatement (line 160). The latter returns a different result.

assertMatchesAll( expectedDescriptions, actualDescriptions );
tx.commit();
return null;
Expand Down Expand Up @@ -206,7 +207,7 @@ public void testDropNodeKeyConstraintAndCreateNodeKeyConstraintWhenUsingDropExis
assertEquals(1, result.size());
Map<String, Object> firstResult = result.get(0).asMap();
assertThat( (String) firstResult.get( "createStatement" ) )
.contains( "CREATE CONSTRAINT", "FOR (foo:`Foo`) REQUIRE (foo.`baa`, foo.`baz`) IS NODE KEY" );
.contains( "CREATE CONSTRAINT", "FOR (n:`Foo`) REQUIRE (n.`baa`, n.`baz`) IS NODE KEY" );
tx.commit();
return null;
});
Expand Down
45 changes: 32 additions & 13 deletions core/src/test/java/apoc/trigger/TriggerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,40 +129,59 @@ public void testTimeStampTrigger() throws Exception {
});
}

// TODO NC:Why is this failing?
@Ignore
@Test
public void testTxId() throws Exception {
db.executeTransactionally("CALL apoc.trigger.add('txinfo','UNWIND $createdNodes AS n SET n.txId = $transactionId, n.txTime = $commitTime',{phase:'after'})");
public void testTxIdAfterAsync() throws Exception {
db.executeTransactionally("CALL apoc.trigger.add('txinfo','UNWIND $createdNodes AS n SET n.txId = $transactionId, n.txTime = $commitTime',{phase:'afterAsync'})");
db.executeTransactionally("CREATE (f:Bar)");
TestUtil.testCall(db, "MATCH (f:Bar) RETURN f", (row) -> {
org.neo4j.test.assertion.Assert.assertEventually(() ->
db.executeTransactionally("MATCH (n:Bar) RETURN n", Map.of(),
result -> {
final Node node = result.<Node>columnAs("n").next();
return (long) node.getProperty("txId", -1L) > -1L
&& (long) node.getProperty("txTime") > start;
Comment on lines +140 to +141
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference between checking this and that the properties are not null (and have been assigned)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If I understand correctly, in this case I left the previous assertions on lines 154 and 155:

assertEquals (true, (Long) ((Node) row.get ("f")). getProperty ("txId")> -1L);
assertEquals (true, (Long) ((Node) row.get ("f")). getProperty ("txTime")> start);

I just changed the test from "after" to "afterAsync" (by adding assertEventually as well).

I think that verifying only the existence of txId and txTime is not enough, I could get wrong results because, for example, the txTime is lower than the start one, or am I wrong?

Copy link
Contributor

Choose a reason for hiding this comment

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

But the node was created without properties in the first place, so if they are present, that should be enough to know the trigger was fired, isn't it?

Anyway, this is not a biggie

})
, (value) -> value, 30L, TimeUnit.SECONDS);
}

@Test
public void testTxId() {
db.executeTransactionally("CREATE (f:Another)");
db.executeTransactionally("CALL apoc.trigger.add('txinfo','UNWIND $createdNodes AS n \n" +
"MATCH (a:Another) WITH a, n\n" +
"SET a.txId = $transactionId, a.txTime = $commitTime',{phase:'after'})");
db.executeTransactionally("CREATE (f:Bar)");
TestUtil.testCall(db, "MATCH (f:Another) RETURN f", (row) -> {
assertEquals(true, (Long) ((Node) row.get("f")).getProperty("txId") > -1L);
assertEquals(true, (Long) ((Node) row.get("f")).getProperty("txTime") > start);
});
db.executeTransactionally("MATCH (n:Another) DELETE n");
}

@Test
public void testMetaDataBefore() {
testMetaData("before");
db.executeTransactionally("CALL apoc.trigger.add('txinfo','UNWIND $createdNodes AS n SET n.label = labels(n)[0], n += $metaData', {phase: 'before'})");
testMetaData("MATCH (n:Bar) RETURN n");
}

// TODO NC:Why is this failing?
@Ignore
@Test
public void testMetaDataAfter() {
testMetaData("after");
db.executeTransactionally("CREATE (n:Another)");
db.executeTransactionally("CALL apoc.trigger.add('txinfo', 'UNWIND $createdNodes AS n MATCH (a:Another) SET a.label = labels(n)[0], a += $metaData', {phase: 'after'})");
Copy link
Contributor

Choose a reason for hiding this comment

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

How come we can modify the node here adding things if it is phase after?

Copy link
Collaborator

Choose a reason for hiding this comment

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

As above, because the node (a:Another) is not present in $createdNodes, but is created before the triggered query.

testMetaData("MATCH (n:Another) RETURN n");
db.executeTransactionally("MATCH (n:Another) DELETE n");
}

private void testMetaData(String phase) {
db.executeTransactionally("CALL apoc.trigger.add('txinfo','UNWIND $createdNodes AS n SET n += $metaData',{phase:$phase})", Collections.singletonMap("phase", phase));
private void testMetaData(String matchQuery) {
try (Transaction tx = db.beginTx()) {
KernelTransaction ktx = ((TransactionImpl)tx).kernelTransaction();
ktx.setMetaData(Collections.singletonMap("txMeta", "hello"));
tx.execute("CREATE (f:Bar)");
tx.commit();
}
TestUtil.testCall(db, "MATCH (f:Bar) RETURN f", (row) -> {
assertEquals("hello", ((Node) row.get("f")).getProperty("txMeta") );
TestUtil.testCall(db, matchQuery, (row) -> {
final Node node = (Node) row.get("n");
assertEquals("Bar", node.getProperty("label"));
assertEquals("hello", node.getProperty("txMeta"));
});
}

Expand Down
11 changes: 11 additions & 0 deletions docs/asciidoc/modules/ROOT/pages/background-operations/triggers.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,17 @@ You can use these helper functions to extract nodes or relationships by label/re
| apoc.trigger.propertiesByKey($assignedNodeProperties,'key') | function to filter propertyEntries by property-key, to be used within a trigger statement with $assignedNode/RelationshipProperties and $removedNode/RelationshipProperties. Returns [{old,[new],key,node,relationship}]
|===

[WARNING]
====
Unlike Neo4j up to 4, with Neo4j 5 it's not possible to modify an entity created in phase: "after".
Copy link
Contributor

@ncordon ncordon Mar 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The wording of this can be improved. I think here we mean we cannot modify the same node the trigger gets its information from? Cause otherwise I cannot explain this:

https://github.com/neo4j-contrib/neo4j-apoc-procedures/pull/2650/files#r835190507

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, maybe I should write this clearer. I mean entities "just" created (present in $createdNode or $createdRelationships)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy to approve provided we amend the wording (can be in another PR if we are in a rush to merge this one)

For example this query will throw an Exception with message `can't acquire ExclusiveLock...`:
```
CALL apoc.trigger.add('name','UNWIND $createdNodes AS n SET n.txId = $transactionId',{phase:'after'});
CREATE (f:Baz);
```

Instead, it's necessary to put another phase or perform only reading operations.
====

== Triggers Examples

Expand Down
4 changes: 2 additions & 2 deletions docs/asciidoc/modules/ROOT/partials/usage/draft/apoc.trigger.add.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ CALL apoc.trigger.add(
WITH prop.node as n
MATCH(n)-[]-(a)
SET a.surname = n.surname',
{phase:'after'}
{phase:'afterAsync'}
);
----

Expand All @@ -38,7 +38,7 @@ CALL apoc.trigger.add(
| "setAllConnectedNodes" | "UNWIND apoc.trigger.propertiesByKey($assignedNodeProperties,\"surname\") as prop
WITH prop.node as n
MATCH(n)-[]-(a)
SET a.surname = n.surname" | {phase: "after"} | {} | TRUE | FALSE
SET a.surname = n.surname" | {phase: "afterAsync"} | {} | TRUE | FALSE
|===


Expand Down
3 changes: 1 addition & 2 deletions full/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,6 @@ dependencies {
testImplementation group: 'com.amazonaws', name: 'aws-java-sdk-comprehend', version: '1.11.683'

testImplementation group: 'org.codehaus.jackson', name: 'jackson-mapper-asl', version: '1.9.7'
implementation group: 'com.fasterxml.jackson.core', name: 'jackson-databind', version: '2.10.5.1'
implementation group: 'commons-beanutils', name: 'commons-beanutils', version: '1.9.4'
implementation group: 'com.opencsv', name: 'opencsv', version: '4.6'
implementation group: 'commons-beanutils', name: 'commons-beanutils', version: '1.9.4'
Expand All @@ -138,7 +137,7 @@ dependencies {
testImplementation group: 'com.github.adejanovski', name: 'cassandra-jdbc-wrapper', version: '3.1.0'


testImplementation group: 'com.fasterxml.jackson.dataformat', name: 'jackson-dataformat-csv', version: '2.9.7'
testImplementation group: 'com.fasterxml.jackson.dataformat', name: 'jackson-dataformat-csv', version: '2.13.1'
testImplementation group: 'org.skyscreamer', name: 'jsonassert', version: '1.5.0'
testImplementation group: 'org.assertj', name: 'assertj-core', version: '3.13.2'

Expand Down