GumpacG commented on code in PR #3423:
URL: https://github.com/apache/tinkerpop/pull/3423#discussion_r3235633959
##########
docs/src/dev/provider/index.asciidoc:
##########
@@ -1260,8 +1260,25 @@ Graph transactions are typically `ThreadLocal`-bound.
The server maintains a sin
to ensure all operations execute on the same thread. This is an important
implementation detail for graph system
providers whose `Transaction` implementation relies on thread-local state.
-NOTE: Non-transactional requests (those without a `transactionId`) are not
affected by any of the transaction-specific
-behavior described above. They continue to operate exactly as before.
+==== Auto-Commit for Non-Transactional Requests
+
+Non-transactional requests (those without a `transactionId`) are not affected
by any of the transaction-specific
+behavior described above. However, if the underlying graph supports
transactions, the server must automatically manage
+transactions for these requests:
+
+* Before processing: roll back any stale open transaction to prevent state
leakage between requests.
+* On success: commit the transaction after the traversal has been fully
iterated and serialized.
+* On error: roll back the transaction so that partial mutations are not
persisted.
+
+This auto-commit behavior ensures that users who do not use explicit
transactions still get durable writes on success
+and clean rollback on failure. Graph system providers implementing their own
server or HTTP endpoint must replicate
Review Comment:
Since we are using "must", is there a consequence if they don't (Exception)?
Can we document that?
##########
gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerHttpIntegrateTest.java:
##########
@@ -489,36 +491,97 @@ public void
should200OnPOSTWithGremlinJsonEndcodedBodyForJavaTime() throws Excep
}
}
- /*@Test disabled for now as current implementation doesn't support
implicit transactions.
+ @Test
public void should200OnPOSTTransactionalGraph() throws Exception {
final CloseableHttpClient httpclient = HttpClients.createDefault();
+
+ // add a vertex without an explicit transaction - should be
auto-committed
final HttpPost httppost = new
HttpPost(TestClientFactory.createURLString());
httppost.addHeader("Content-Type", "application/json");
- httppost.setEntity(new
StringEntity("{\"gremlin\":\"graph.addVertex('name','stephen');g.V().count()\"}",
Consts.UTF_8));
+ httppost.setEntity(new StringEntity(
+
"{\"gremlin\":\"g.addV('person').property('name','stephen')\",\"g\":\"g\"}",
Consts.UTF_8));
try (final CloseableHttpResponse response =
httpclient.execute(httppost)) {
assertEquals(200, response.getStatusLine().getStatusCode());
- assertEquals("application/json",
response.getEntity().getContentType().getValue());
- final String json = EntityUtils.toString(response.getEntity());
- final JsonNode node = mapper.readTree(json);
- assertEquals(1,
node.get("result").get(TOKEN_DATA).get(GraphSONTokens.VALUEPROP).get(0).get(GraphSONTokens.VALUEPROP).intValue());
+ EntityUtils.consume(response.getEntity());
}
- final HttpGet httpget = new
HttpGet(TestClientFactory.createURLString("?gremlin=g.V().count()"));
- httpget.addHeader("Accept", "application/json");
+ // verify the vertex is visible on subsequent requests from
potentially different threads
+ final HttpPost countPost = new
HttpPost(TestClientFactory.createURLString());
+ countPost.addHeader("Content-Type", "application/json");
+ countPost.setEntity(new
StringEntity("{\"gremlin\":\"g.V().count()\",\"g\":\"g\"}", Consts.UTF_8));
- // execute this a bunch of times so that there's a good chance a
different thread on the server processes
- // the request
for (int ix = 0; ix < 100; ix++) {
- try (final CloseableHttpResponse response =
httpclient.execute(httpget)) {
+ try (final CloseableHttpResponse response =
httpclient.execute(countPost)) {
assertEquals(200, response.getStatusLine().getStatusCode());
- assertEquals("application/json",
response.getEntity().getContentType().getValue());
final String json = EntityUtils.toString(response.getEntity());
final JsonNode node = mapper.readTree(json);
- assertEquals(1,
node.get("result").get(TOKEN_DATA).get(GraphSONTokens.VALUEPROP).get(0).get(GraphSONTokens.VALUEPROP).intValue());
+ assertEquals(1, node.get("result").get(TOKEN_DATA)
+ .get(GraphSONTokens.VALUEPROP).get(0)
+ .get(GraphSONTokens.VALUEPROP).intValue());
}
}
- } */
+ }
+
+ @Test
+ public void shouldRollbackOnFailedMutatingTraversal() throws Exception {
+ final CloseableHttpClient httpclient = HttpClients.createDefault();
+
+ // submit a traversal that adds a vertex then fails - should be rolled
back
+ final HttpPost httppost = new
HttpPost(TestClientFactory.createURLString());
+ httppost.addHeader("Content-Type", "application/json");
+ httppost.setEntity(new
StringEntity("{\"gremlin\":\"g.addV('person').fail()\",\"g\":\"g\"}",
Consts.UTF_8));
+
+ try (final CloseableHttpResponse response =
httpclient.execute(httppost)) {
+ // the fail() error appears in the response body, not the HTTP
status
+ }
+
+ // verify the vertex was not persisted
+ final HttpPost countPost = new
HttpPost(TestClientFactory.createURLString());
+ countPost.addHeader("Content-Type", "application/json");
+ countPost.setEntity(new StringEntity(
+
"{\"gremlin\":\"g.V().hasLabel('person').count()\",\"g\":\"g\"}",
Consts.UTF_8));
+
+ try (final CloseableHttpResponse response =
httpclient.execute(countPost)) {
+ assertEquals(200, response.getStatusLine().getStatusCode());
+ final String json = EntityUtils.toString(response.getEntity());
+ final JsonNode node = mapper.readTree(json);
+ assertEquals(0, node.get("result").get(TOKEN_DATA)
+ .get(GraphSONTokens.VALUEPROP).get(0)
+ .get(GraphSONTokens.VALUEPROP).intValue());
+ }
+ }
+
+ @Test
+ public void shouldCommitMutatingTraversalWithEmptyResult() throws
Exception {
+ final CloseableHttpClient httpclient = HttpClients.createDefault();
+
+ // g.addV() followed by iterate-style consumption returns no results
but should still commit
+ final HttpPost httppost = new
HttpPost(TestClientFactory.createURLString());
+ httppost.addHeader("Content-Type", "application/json");
+ httppost.setEntity(new StringEntity(
+
"{\"gremlin\":\"g.addV('person').property('name','p').iterate()\",\"g\":\"g\"}",
Consts.UTF_8));
+
+ try (final CloseableHttpResponse response =
httpclient.execute(httppost)) {
+ assertEquals(200, response.getStatusLine().getStatusCode());
+ EntityUtils.consume(response.getEntity());
+ }
+
+ // verify the vertex was committed despite the empty result set
+ final HttpPost countPost2 = new
HttpPost(TestClientFactory.createURLString());
+ countPost2.addHeader("Content-Type", "application/json");
+ countPost2.setEntity(new StringEntity(
+
"{\"gremlin\":\"g.V().hasLabel('person').count()\",\"g\":\"g\"}",
Consts.UTF_8));
+
+ try (final CloseableHttpResponse response =
httpclient.execute(countPost2)) {
+ assertEquals(200, response.getStatusLine().getStatusCode());
+ final String json = EntityUtils.toString(response.getEntity());
+ final JsonNode node = mapper.readTree(json);
+ assertEquals(1, node.get("result").get(TOKEN_DATA)
+ .get(GraphSONTokens.VALUEPROP).get(0)
+ .get(GraphSONTokens.VALUEPROP).intValue());
+ }
+ }
Review Comment:
Is there also a way to test stale open transactions?
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpGremlinEndpointHandler.java:
##########
Review Comment:
Could any of these lines throw an error and should be in the rollback try
catch in this case?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
