Cole-Greer commented on code in PR #3328:
URL: https://github.com/apache/tinkerpop/pull/3328#discussion_r2967807755
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/GremlinServer.java:
##########
@@ -293,6 +293,11 @@ public synchronized CompletableFuture<Void> stop() {
logger.warn("Timeout waiting for boss/worker thread pools to
shutdown - continuing with shutdown process.");
}
+ if (serverGremlinExecutor != null) {
Review Comment:
Nit: can this be folded into the if-statement below
##########
gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerHttpTransactionIntegrateTest.java:
##########
@@ -0,0 +1,753 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.server;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.buffer.Unpooled;
+import io.netty.buffer.UnpooledByteBufAllocator;
+import org.apache.http.Consts;
+import org.apache.http.HttpHeaders;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpPost;
+import org.apache.http.entity.ByteArrayEntity;
+import org.apache.http.entity.StringEntity;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.util.EntityUtils;
+import org.apache.tinkerpop.gremlin.server.channel.HttpChannelizer;
+import org.apache.tinkerpop.gremlin.server.handler.TransactionManager;
+import org.apache.tinkerpop.gremlin.structure.io.graphson.GraphSONTokens;
+import org.apache.tinkerpop.gremlin.util.Tokens;
+import org.apache.tinkerpop.gremlin.util.message.RequestMessage;
+import org.apache.tinkerpop.gremlin.util.message.ResponseMessage;
+import org.apache.tinkerpop.gremlin.util.ser.GraphBinaryMessageSerializerV4;
+import org.apache.tinkerpop.gremlin.util.ser.GraphSONMessageSerializerV4;
+import org.apache.tinkerpop.gremlin.util.ser.Serializers;
+import org.apache.tinkerpop.shaded.jackson.databind.JsonNode;
+import org.apache.tinkerpop.shaded.jackson.databind.ObjectMapper;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.util.List;
+import java.util.Map;
+import java.util.UUID;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+
+import static org.apache.tinkerpop.gremlin.util.ser.SerTokens.TOKEN_DATA;
+import static org.apache.tinkerpop.gremlin.util.ser.SerTokens.TOKEN_RESULT;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+/**
+ * Server-side integration tests for HTTP transaction protocol.
+ *
+ * These tests bypass the driver entirely and use a raw Apache HTTP client to
hit the server's HTTP endpoint directly.
+ * This validates that the server returns the correct status codes and error
messages independent of any client-side
+ * guards.
+ */
+public class GremlinServerHttpTransactionIntegrateTest extends
AbstractGremlinServerIntegrationTest {
+ private final String GTX = "gtx";
+ private final ObjectMapper mapper = new ObjectMapper();
+ private CloseableHttpClient client;
+
+ @Before
+ public void createHttpClient() {
+ client = HttpClients.createDefault();
+ }
+
+ @After
+ public void closeHttpClient() throws Exception {
+ client.close();
+ }
+
+ @Override
+ public Settings overrideSettings(final Settings settings) {
+ settings.channelizer = HttpChannelizer.class.getName();
+ final String nameOfTest = name.getMethodName();
+ switch (nameOfTest) {
+ case "shouldRejectRequestWhenMaxConcurrentTransactionsExceeded":
+ settings.maxConcurrentTransactions = 1;
+ break;
+ case "shouldTimeoutFreeSlotUnderMaxConcurrentTransactions":
+ settings.maxConcurrentTransactions = 1;
+ settings.transactionTimeout = 1000;
+ break;
+ case "shouldTimeoutIdleTransactionWithNoOperations":
+ settings.transactionTimeout = 1;
+ break;
+ case "shouldTimeoutAndRejectLateCommit":
+ case "shouldTrackTransactionCountAccurately":
+ settings.transactionTimeout = 1000;
+ break;
+ case "shouldRollbackAbandonedTransaction":
+ settings.transactionTimeout = 300;
+ break;
+ }
+ return settings;
+ }
+
+ /**
+ * Sends a JSON POST request and returns the response. Caller must close
the response.
+ */
+ private CloseableHttpResponse postJson(final CloseableHttpClient client,
final String json) throws Exception {
+ final HttpPost post = new
HttpPost(TestClientFactory.createURLString());
+ post.addHeader("Content-Type", "application/json");
+ post.setEntity(new StringEntity(json, Consts.UTF_8));
+ return client.execute(post);
+ }
+
+ /**
+ * Sends a begin transaction request for the given graph alias and returns
the server-generated transaction ID.
+ */
+ private String beginTx(final CloseableHttpClient client, final String
graphAlias) throws Exception {
+ try (final CloseableHttpResponse response = postJson(client,
+ "{\"gremlin\":\"g.tx().begin()\",\"g\":\"" + graphAlias +
"\"}")) {
+ assertEquals(200, response.getStatusLine().getStatusCode());
+ final String txIdHeader =
response.getFirstHeader(Tokens.Headers.TRANSACTION_ID).getValue();
+ final String json = EntityUtils.toString(response.getEntity());
+ final JsonNode node = mapper.readTree(json);
+ final String txIdBody = node.get(TOKEN_RESULT).
+ get(TOKEN_DATA).
+ get(GraphSONTokens.VALUEPROP).get(0).
+ get(GraphSONTokens.VALUEPROP).get(1).
+ asText();
+ assertNotNull(txIdHeader);
+ assertEquals(txIdHeader, txIdBody);
+ return txIdHeader;
+ }
+ }
+
+ /**
+ * Sends a traversal within an existing transaction.
+ */
+ private CloseableHttpResponse submitInTx(final CloseableHttpClient client,
+ final String txId,
+ final String gremlin,
+ final String graphAlias)
+ throws Exception {
+ return postJson(client,
+ "{\"gremlin\":\"" + gremlin + "\",\"g\":\"" + graphAlias +
"\",\"transactionId\":\"" + txId + "\"}");
+ }
+
+ /**
+ * Sends a commit for an existing transaction.
+ */
+ private CloseableHttpResponse commitTx(final CloseableHttpClient client,
+ final String txId, final String
graphAlias) throws Exception {
+ return postJson(client,
+ "{\"gremlin\":\"g.tx().commit()\",\"g\":\"" + graphAlias +
"\",\"transactionId\":\"" + txId + "\"}");
+ }
+
+ /**
+ * Sends a rollback for an existing transaction.
+ */
+ private CloseableHttpResponse rollbackTx(final CloseableHttpClient client,
+ final String txId, final String
graphAlias) throws Exception {
+ return postJson(client,
+ "{\"gremlin\":\"g.tx().rollback()\",\"g\":\"" + graphAlias +
"\",\"transactionId\":\"" + txId + "\"}");
+ }
+
+ /**
+ * Sends a non-transactional traversal (no transactionId).
+ */
+ private CloseableHttpResponse submitNonTx(final CloseableHttpClient client,
+ final String gremlin, final
String graphAlias) throws Exception {
+ return postJson(client,
+ "{\"gremlin\":\"" + gremlin + "\",\"g\":\"" + graphAlias +
"\"}");
+ }
+
+ /**
+ * Extracts the integer count from a typical count() response.
+ */
+ private int extractCount(final CloseableHttpResponse response) throws
Exception {
+ final String json = EntityUtils.toString(response.getEntity());
+ final JsonNode node = mapper.readTree(json);
+ return node.get("result").get(TOKEN_DATA)
+ .get(GraphSONTokens.VALUEPROP).get(0)
+ .get(GraphSONTokens.VALUEPROP).intValue();
+ }
+
+ /**
+ * Extracts the status message from the response body's status field.
+ */
+ private String extractStatusMessage(final CloseableHttpResponse response)
throws Exception {
+ final String json = EntityUtils.toString(response.getEntity());
+ final JsonNode node = mapper.readTree(json);
+ return node.get("status").get("message").asText();
+ }
+
+ @Test
+ public void shouldNotBeginTransactionWithUserProvidedId() throws Exception
{
+ final String txId = beginTx(client, GTX);
+
+ try (final CloseableHttpResponse r = postJson(client,
+ "{\"gremlin\":\"g.tx().begin()\",\"g\":\"" + GTX +
"\",\"transactionId\":\"" + txId + "\"}")) {
+ // Depending on whether the transaction is still open on the
server when the second request arrives, there
+ // may be two different errors that the server throws.
+ assertTrue(r.getStatusLine().getStatusCode() == 404 ||
r.getStatusLine().getStatusCode() == 400);
+ final String msg = extractStatusMessage(r);
+ assertTrue(msg.contains("Begin transaction request cannot have a
user-supplied transactionId") ||
+ msg.contains("Transaction not found"));
Review Comment:
Out of curiosity, what are the conditions which lead to the "Transaction not
found" response? The "Begin transaction request cannot have a user-supplied
transactionId" message is much clearer.
--
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]
