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

Jump to bottom

Make spatial query procedures read only #403

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Make spatial query procedures read only #403

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 5 additions & 0 deletions src/main/java/org/neo4j/gis/spatial/EditableLayer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,4 +59,9 @@ public interface EditableLayer extends Layer {
void setCoordinateReferenceSystem(Transaction tx, CoordinateReferenceSystem coordinateReferenceSystem);

void removeFromIndex(Transaction tx, String geomNodeId);

/**
* Do any cleanup or final calculation required by the layer implementation.
*/
void close(Transaction tx);
}
5 changes: 5 additions & 0 deletions src/main/java/org/neo4j/gis/spatial/EditableLayerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,4 +81,9 @@ protected Node addGeomNode(Transaction tx, Geometry geom, String[] fieldsName, O
public String getSignature() {
return "Editable" + super.getSignature();
}

@Override
public void close(Transaction tx) {
getIndex().close(tx);
}
}
4 changes: 4 additions & 0 deletions src/main/java/org/neo4j/gis/spatial/ShapefileImporter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,10 @@ record = shpReader.nextRecord();
}
}
}
try (Transaction tx = database.beginTx()) {
layer.close(tx);
tx.commit();
}

long stopTime = System.currentTimeMillis();
log("info | elapsed time in seconds: " + (1.0 * (stopTime - startTime) / 1000));
Expand Down
3 changes: 3 additions & 0 deletions src/main/java/org/neo4j/gis/spatial/index/SpatialIndexReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,4 +47,7 @@ public interface SpatialIndexReader {
void addMonitor(TreeMonitor monitor);

void configure(Map<String, Object> config);

default void close(Transaction tx) {
}
}
1 change: 1 addition & 0 deletions src/main/java/org/neo4j/gis/spatial/osm/OSMImporter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,7 @@ public long reIndex(GraphDatabaseService database, int commitInterval, boolean i
}
} // TODO ask charset to user?
}
layer.close(tx);
tx.commit();
} finally {
endProgressMonitor();
Expand Down
42 changes: 30 additions & 12 deletions src/main/java/org/neo4j/gis/spatial/procedures/SpatialProcedures.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
package org.neo4j.gis.spatial.procedures;

import static org.neo4j.gis.spatial.SpatialDatabaseService.RTREE_INDEX_NAME;
import static org.neo4j.procedure.Mode.READ;
import static org.neo4j.procedure.Mode.WRITE;

import java.io.File;
Expand Down Expand Up @@ -182,7 +183,7 @@ public Stream<NameResult> upgradeSpatial() {
return builder.build();
}

@Procedure(value = "spatial.layers", mode = WRITE)
@Procedure(value = "spatial.layers", mode = READ)
@Description("Returns name, and details for all layers")
public Stream<NameResult> getAllLayers() {
SpatialDatabaseService sdb = spatial();
Expand Down Expand Up @@ -521,21 +522,27 @@ public void removeLayer(@Name("name") String name) {
@Description("Adds the given node to the layer, returns the geometry-node")
public Stream<NodeResult> addNodeToLayer(@Name("layerName") String name, @Name("node") Node node) {
EditableLayer layer = getEditableLayerOrThrow(tx, spatial(), name);
return streamNode(layer.add(tx, node).getGeomNode());
Node geomNode = layer.add(tx, node).getGeomNode();
layer.close(tx);
Copy link
Contributor

Choose a reason for hiding this comment

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

The use of the word close and in particular close(tx) almost makes it feel like the transaction is being closed. It also makes me look for a matching open(tx) statement. I wonder if it would not be better to use a word like finalize(tx)? Although that also has connotations.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My idea behind the close was to implement java.io.Closeable so one can use try-with-resource. The problem here is that the index itself is not aware of the transaction, so the tx must be passed to each method as param- also to the close method. And so, with the additional parameter, it can no longer implement java.io.Closeable.

What do you think about having a Transaction-Aware Facade that can be Closable?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks sounds nice. Worth a try, as long as the transaction does not leak anywhere.

return streamNode(geomNode);
}

@Procedure(value = "spatial.addNodes", mode = WRITE)
@Description("Adds the given nodes list to the layer, returns the count")
public Stream<CountResult> addNodesToLayer(@Name("layerName") String name, @Name("nodes") List<Node> nodes) {
EditableLayer layer = getEditableLayerOrThrow(tx, spatial(), name);
return Stream.of(new CountResult(layer.addAll(tx, nodes)));
int count = layer.addAll(tx, nodes);
layer.close(tx);
return Stream.of(new CountResult(count));
}

@Procedure(value = "spatial.addNode.byId", mode = WRITE)
@Description("Adds the given node to the layer, returns the geometry-node")
public Stream<NodeResult> addNodeIdToLayer(@Name("layerName") String name, @Name("nodeId") String nodeId) {
EditableLayer layer = getEditableLayerOrThrow(tx, spatial(), name);
return streamNode(layer.add(tx, tx.getNodeByElementId(nodeId)).getGeomNode());
Node geomNode = layer.add(tx, tx.getNodeByElementId(nodeId)).getGeomNode();
layer.close(tx);
return streamNode(geomNode);
}

@Procedure(value = "spatial.addNodes.byId", mode = WRITE)
Expand All @@ -544,14 +551,17 @@ public Stream<CountResult> addNodeIdsToLayer(@Name("layerName") String name,
@Name("nodeIds") List<String> nodeIds) {
EditableLayer layer = getEditableLayerOrThrow(tx, spatial(), name);
List<Node> nodes = nodeIds.stream().map(id -> tx.getNodeByElementId(id)).collect(Collectors.toList());
return Stream.of(new CountResult(layer.addAll(tx, nodes)));
int count = layer.addAll(tx, nodes);
layer.close(tx);
return Stream.of(new CountResult(count));
}

@Procedure(value = "spatial.removeNode", mode = WRITE)
@Description("Removes the given node from the layer, returns the geometry-node")
public Stream<NodeIdResult> removeNodeFromLayer(@Name("layerName") String name, @Name("node") Node node) {
EditableLayer layer = getEditableLayerOrThrow(tx, spatial(), name);
layer.removeFromIndex(tx, node.getElementId());
layer.close(tx);
return streamNode(node.getElementId());
}

Expand All @@ -565,6 +575,7 @@ public Stream<CountResult> removeNodesFromLayer(@Name("layerName") String name,
layer.removeFromIndex(tx, node.getElementId());
}
int after = layer.getIndex().count(tx);
layer.close(tx);
return Stream.of(new CountResult(before - after));
}

Expand All @@ -573,6 +584,7 @@ public Stream<CountResult> removeNodesFromLayer(@Name("layerName") String name,
public Stream<NodeIdResult> removeNodeFromLayer(@Name("layerName") String name, @Name("nodeId") String nodeId) {
EditableLayer layer = getEditableLayerOrThrow(tx, spatial(), name);
layer.removeFromIndex(tx, nodeId);
layer.close(tx);
return streamNode(nodeId);
}

Expand All @@ -587,6 +599,7 @@ public Stream<CountResult> removeNodeIdsFromLayer(@Name("layerName") String name
layer.removeFromIndex(tx, nodeId);
}
int after = layer.getIndex().count(tx);
layer.close(tx);
return Stream.of(new CountResult(before - after));
}

Expand All @@ -596,7 +609,9 @@ public Stream<NodeResult> addGeometryWKTToLayer(@Name("layerName") String name,
@Name("geometry") String geometryWKT) {
EditableLayer layer = getEditableLayerOrThrow(tx, spatial(), name);
WKTReader reader = new WKTReader(layer.getGeometryFactory());
return streamNode(addGeometryWkt(layer, reader, geometryWKT));
Node node = addGeometryWkt(layer, reader, geometryWKT);
layer.close(tx);
return streamNode(node);
}

@Procedure(value = "spatial.addWKTs", mode = WRITE)
Expand All @@ -606,7 +621,8 @@ public Stream<NodeResult> addGeometryWKTsToLayer(@Name("layerName") String name,
EditableLayer layer = getEditableLayerOrThrow(tx, spatial(), name);
WKTReader reader = new WKTReader(layer.getGeometryFactory());
return geometryWKTs.stream().map(geometryWKT -> addGeometryWkt(layer, reader, geometryWKT))
.map(NodeResult::new);
.map(NodeResult::new)
.onClose(() -> layer.close(tx));
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice trick. Perhaps we could add a lambda for this to the streamNode() method?

}

private Node addGeometryWkt(EditableLayer layer, WKTReader reader, String geometryWKT) {
Expand All @@ -624,7 +640,9 @@ public Stream<CountResult> importShapefile(
@Name("layerName") String name,
@Name("uri") String uri) throws IOException {
EditableLayerImpl layer = getEditableLayerOrThrow(tx, spatial(), name);
return Stream.of(new CountResult(importShapefileToLayer(uri, layer, 1000).size()));
List<Node> nodes = importShapefileToLayer(uri, layer, 1000);
layer.close(tx);
return Stream.of(new CountResult(nodes.size()));
}

@Procedure(value = "spatial.importShapefile", mode = WRITE)
Expand Down Expand Up @@ -744,7 +762,7 @@ public void run() {
}
}

@Procedure(value = "spatial.bbox", mode = WRITE)
@Procedure(value = "spatial.bbox", mode = READ)
@Description("Finds all geometry nodes in the given layer within the lower left and upper right coordinates of a box")
public Stream<NodeResult> findGeometriesInBBox(
@Name("layerName") String name,
Expand All @@ -758,7 +776,7 @@ public Stream<NodeResult> findGeometriesInBBox(
.stream().map(GeoPipeFlow::getGeomNode).map(NodeResult::new);
}

@Procedure(value = "spatial.closest", mode = WRITE)
@Procedure(value = "spatial.closest", mode = READ)
@Description("Finds all geometry nodes in the layer within the distance to the given coordinate")
public Stream<NodeResult> findClosestGeometries(
@Name("layerName") String name,
Expand All @@ -772,7 +790,7 @@ public Stream<NodeResult> findClosestGeometries(
return edgeResults.stream().map(e -> e.getValue().getGeomNode()).map(NodeResult::new);
}

@Procedure(value = "spatial.withinDistance", mode = WRITE)
@Procedure(value = "spatial.withinDistance", mode = READ)
@Description("Returns all geometry nodes and their ordered distance in the layer within the distance to the given coordinate")
public Stream<NodeDistanceResult> findGeometriesWithinDistance(
@Name("layerName") String name,
Expand Down Expand Up @@ -809,7 +827,7 @@ public Stream<GeometryResult> asExternalGeometry(
return Stream.of(geometry).map(geom -> new GeometryResult(toNeo4jGeometry(null, geom)));
}

@Procedure(value = "spatial.intersects", mode = WRITE)
@Procedure(value = "spatial.intersects", mode = READ)
@Description("Returns all geometry nodes that intersect the given geometry (shape, polygon) in the layer")
public Stream<NodeResult> findGeometriesIntersecting(
@Name("layerName") String name,
Expand Down
5 changes: 5 additions & 0 deletions src/main/java/org/neo4j/gis/spatial/rtree/RTreeIndex.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1544,4 +1544,9 @@ public int compare(NodeWithEnvelope o1, NodeWithEnvelope o2) {
return Double.compare(o1.envelope.getArea(), o2.envelope.getArea());
}
}

@Override
public void close(Transaction tx) {
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned above, I wonder if there is a better choice of word than close?

saveCount(tx);
}
}
1 change: 1 addition & 0 deletions src/test/java/org/neo4j/gis/spatial/LayersTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,7 @@ public void testIndexAccessAfterBulkInsertion() {
coordinateNodes.add(node);
}
layer.addAll(tx, coordinateNodes);
layer.close(tx);
tx.commit();
}

Expand Down