-
Notifications
You must be signed in to change notification settings - Fork 192
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.
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
Make spatial query procedures read only #403
base: master
Are you sure you want to change the base?
Make spatial query procedures read only #403
Conversation
d730ddd
to
083dafe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice fix. I do wonder if we should think of a better word than close
for the method that flushes the count to the index?
@@ -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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
@@ -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)); |
There was a problem hiding this comment.
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?
@@ -1528,4 +1528,9 @@ public int compare(NodeWithEnvelope o1, NodeWithEnvelope o2) { | |||
return Double.compare(o1.envelope.getArea(), o2.envelope.getArea()); | |||
} | |||
} | |||
|
|||
@Override | |||
public void close(Transaction tx) { |
There was a problem hiding this comment.
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
?
083dafe
to
0be5527
Compare
6844765
to
bee28a4
Compare
This PR changes the following procedure to be read-only:
spatial.layers
spatial.bbox
spatial.closest
spatial.withinDistance
spatial.intersects