[VP7iRcuF] Introduce helper methods from neo4j #187

Lojjs · 2022-09-16T13:01:51Z

In order to decrease the dependencies of internal Neo4j APIs, it was decided to copy over some of the helper classes into APOC.

Proposed Changes (Mandatory)

A brief list of proposed changes in order to fix the issue:

Copy over all relevant parts of the org.neo4j.internal.helper.collection package from Neo4j
Remove benchmarking code which is old and unused

Notes for the reviewer

Some of the helper files which have been copied over from Neo4j also had accompanying test classes. Do we want to move over relevant parts of these as well?
This will break APOC extended, as several places there call public core methods which have now changed signature from e.g. org.neo4j.internal.helpers.collection.Pair to apoc.util.collection.Pair. Should we make a Trello card for fixing this?

vga91

A really good job :), I just added some small (mostly subjective) things I think could be changed

vga91 · 2022-09-26T15:18:56Z

common/src/main/java/apoc/util/collection/Iterables.java

+    }
+
+    public static <FROM, TO> Iterable<TO> map(Function<? super FROM, ? extends TO> function, Iterable<FROM> from) {
+        return new MapIterable<>(from, function);


Since all Iterables.map(..) is always used in together with Iterables.asList(..), maybe it might be worth using the stream API, writing e.g. Iterables.stream(node.getRelationshipTypes()).map(RelationshipType::name).collect (Collectors.toList()); instead of Iterables.asList(Iterables.map(RelationshipType::name, node.getRelationshipTypes())); [line 576 of apoc.nodes.Nodes.java].

This way, we can remove both this method and the MapIterable class.

vga91 · 2022-09-26T15:24:13Z

common/src/main/java/apoc/util/collection/Pair.java

+ * @param <T1> the type of the {@link #first() first value} of the pair.
+ * @param <T2> the type of the {@link #other() other value} of the pair.
+ */
+public abstract class Pair<T1, T2> {


Maybe we could use org.apache.commons.lang3.tuple.Pair instead of this?

Hmm I guess we could, as we already depend on org.apache.commons.lang3

vga91 · 2022-09-26T15:29:21Z

common/src/main/java/org/neo4j/cypher/export/CypherResultSubGraph.java

@@ -34,7 +34,7 @@ public void add( Node node )
    void addNode( long id, Node data )
    {
        nodes.put( id, data );
-        labels.addAll( Iterables.asCollection( data.getLabels() ) );
+        labels.addAll( Iterables.addAll( new ArrayList<>(), data.getLabels() ) );


Suggested change

labels.addAll( Iterables.addAll( new ArrayList<>(), data.getLabels() ) );

Iterables.asList(data.getLabels())

vga91 · 2022-09-26T16:15:37Z

common/src/main/java/apoc/convert/ConvertUtils.java

@@ -1,7 +1,6 @@
 package apoc.convert;

-import org.neo4j.internal.helpers.collection.Iterators;
-
+import apoc.util.collection.Iterators;


Not directly related, but we could change rows 20 and 21 with these?

else if (list instanceof Iterable) return Iterables.asList((Iterable)list); else if (list instanceof Iterator) return Iterators.asList((Iterator)list);

Nice catch!

vga91 · 2022-09-26T16:19:14Z

common/src/main/java/apoc/util/collection/Iterators.java

+     * @param iterator the iterator to expose as an {@link Iterable}.
+     * @return the supplied iterator posing as an {@link Iterable}.
+     */
+    public static <T> Iterable<T> loop(final Iterator<T> iterator) {


I think we could also use Iterator.forEachRemaining instead of this?

vga91 · 2022-09-26T16:31:34Z

Some of the helper files which have been copied over from Neo4j also had accompanying test classes. Do we want to move over relevant parts of these as well?

This will break APOC extended, as several places there call public core methods which have now changed signature from e.g. org.neo4j.internal.helpers.collection.Pair to apoc.util.collection.Pair. Should we make a Trello card for fixing this?

For me yes, both, I think it is worth adding some tests and the APOC extended trello card

To reduce dependencies of internal APIs

To reduce dependencies of internal Neo4j APIs

This benchmarking code which used internal Neo4j APIs is unused and related to auto-indexing which does not exist in Neo4j anymore.

vga91

I'd just change one more thing (optional). Except this one LGTM :)

vga91 · 2022-10-04T09:07:59Z

common/src/test/java/apoc/util/collection/AbstractResourceIterableTest.java

+
+        // Then
+        assertThat( Iterators.asList(iterator)).containsExactlyElementsOf(items);
+        assertThat(iteratorClosed.isTrue()).isTrue();


This seems a bit repetitive, at least for me. Maybe I would write:

Suggested change

assertThat(iteratorClosed.isTrue()).isTrue();

assertTrue(iteratorClosed.booleanValue());

or

Suggested change

assertThat(iteratorClosed.isTrue()).isTrue();

assertTrue(iteratorClosed.getValue());

Same thing for other similar ones (lines 55, 86, 114-117, 167, 194-196, 232-234, 264-266, 295, 296, 318).

I ended up doing the first suggestion and applied it for the other test files as well

Lojjs added the team-cypher-surface label Sep 16, 2022

nadja-muller self-assigned this Sep 19, 2022

nadja-muller assigned vga91 and unassigned nadja-muller Sep 26, 2022

vga91 reviewed Sep 26, 2022

View reviewed changes

Lojjs force-pushed the dev-introduce-helper-methods-from-neo4j branch 3 times, most recently from ed358d8 to d2b7c42 Compare September 30, 2022 10:06

Lojjs changed the title ~~Introduce helper methods from neo4j~~ [VP7iRcuF] Introduce helper methods from neo4j Oct 3, 2022

Lojjs added 8 commits October 4, 2022 08:47

Copy over Pair helper from Neo4j

ce784a6

To reduce dependencies of internal APIs

Copy over relevant parts of Iterators and Iterables

ff193a4

To reduce dependencies of internal Neo4j APIs

Use apoc MapUtil instead of Neo4j one

fbb6999

Copy over rest of Neo4j helpers

5088d3d

Remove unused benchmarking code

290c827

This benchmarking code which used internal Neo4j APIs is unused and related to auto-indexing which does not exist in Neo4j anymore.

Fix smaller review comments

2523252

Use Pair from apache instead of Neo4j

029b29b

Add tests for helper methods from Neo4j

61ef528

Lojjs force-pushed the dev-introduce-helper-methods-from-neo4j branch from d2b7c42 to 61ef528 Compare October 4, 2022 06:57

vga91 approved these changes Oct 4, 2022

View reviewed changes

Simplify test assertions

322240d

Lojjs merged commit d22a145 into dev Oct 5, 2022

Lojjs deleted the dev-introduce-helper-methods-from-neo4j branch October 5, 2022 11:37

vga91 mentioned this pull request Oct 11, 2022

Fixes neo4j/apoc#126: apoc.periodic.submit fails with schema operations (extended) neo4j-contrib/neo4j-apoc-procedures#3211

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[VP7iRcuF] Introduce helper methods from neo4j #187

[VP7iRcuF] Introduce helper methods from neo4j #187

Lojjs commented Sep 16, 2022 •

edited

Loading

vga91 left a comment

vga91 Sep 26, 2022

vga91 Sep 26, 2022

Lojjs Sep 28, 2022

vga91 Sep 26, 2022

vga91 Sep 26, 2022

Lojjs Sep 28, 2022

vga91 Sep 26, 2022

vga91 commented Sep 26, 2022 •

edited

Loading

vga91 left a comment •

edited

Loading

vga91 Oct 4, 2022 •

edited

Loading

Lojjs Oct 5, 2022

	labels.addAll( Iterables.addAll( new ArrayList<>(), data.getLabels() ) );
	Iterables.asList(data.getLabels())

	assertThat(iteratorClosed.isTrue()).isTrue();
	assertTrue(iteratorClosed.booleanValue());

	assertThat(iteratorClosed.isTrue()).isTrue();
	assertTrue(iteratorClosed.getValue());

[VP7iRcuF] Introduce helper methods from neo4j #187

[VP7iRcuF] Introduce helper methods from neo4j #187

Conversation

Lojjs commented Sep 16, 2022 • edited Loading

Proposed Changes (Mandatory)

Notes for the reviewer

vga91 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vga91 commented Sep 26, 2022 • edited Loading

vga91 left a comment • edited Loading

Choose a reason for hiding this comment

vga91 Oct 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lojjs commented Sep 16, 2022 •

edited

Loading

vga91 commented Sep 26, 2022 •

edited

Loading

vga91 left a comment •

edited

Loading

vga91 Oct 4, 2022 •

edited

Loading