Skip to content

Commit

Permalink
Gh-3303: No error reported when Gremlin endpoint fails (#3304)
Browse files Browse the repository at this point in the history 
* fix error reporting

* add view for otel

* testing

* null check

---------

Co-authored-by: wb36499 <[email protected]>
  • Loading branch information
@tb06904 @wb36499
tb06904 and wb36499 committed Sep 30, 2024
1 parent 5960ac1 commit d3d494e
Show file tree
Hide file tree
Showing 3 changed files with 88 additions and 21 deletions.
5 changes: 5 additions & 0 deletions ...store/src/main/java/uk/gov/gchq/gaffer/store/operation/handler/OperationChainHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import uk.gov.gchq.gaffer.operation.Operation;
import uk.gov.gchq.gaffer.operation.OperationChain;
import uk.gov.gchq.gaffer.operation.OperationException;
import uk.gov.gchq.gaffer.operation.graph.OperationView;
import uk.gov.gchq.gaffer.store.Context;
import uk.gov.gchq.gaffer.store.Store;
import uk.gov.gchq.gaffer.store.operation.OperationChainValidator;
Expand Down Expand Up @@ -52,6 +53,10 @@ public OUT doOperation(final OperationChain<OUT> operationChain, final Context c
// OpenTelemetry hooks
Span span = OtelUtil.startSpan(this.getClass().getName(), op.getClass().getName());
span.setAttribute("jobId", context.getJobId());
if (op instanceof OperationView && ((OperationView) op).getView() != null) {
span.setAttribute("view", ((OperationView) op).getView().toString());
}

// Sets the span to current so parent child spans are auto linked
try (Scope scope = span.makeCurrent()) {
updateOperationInput(op, result);
Expand Down
62 changes: 43 additions & 19 deletions rest-api/spring-rest/src/main/java/uk/gov/gchq/gaffer/rest/controller/GremlinController.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,12 @@
import org.opencypher.gremlin.server.jsr223.CypherPlugin;
import org.opencypher.gremlin.translation.CypherAst;
import org.opencypher.gremlin.translation.translator.Translator;
import org.opencypher.v9_0.util.SyntaxException;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.http.HttpHeaders;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.PostMapping;
import org.springframework.web.bind.annotation.RequestBody;
Expand All @@ -58,7 +60,6 @@
import uk.gov.gchq.koryphe.tuple.n.Tuple2;

import java.io.IOException;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
import java.util.HashMap;
import java.util.LinkedList;
Expand Down Expand Up @@ -145,16 +146,25 @@ public String explain(@RequestHeader final HttpHeaders httpHeaders, @RequestBody
@io.swagger.v3.oas.annotations.Operation(
summary = "Run a Gremlin Query",
description = "Runs a Gremlin Groovy script and outputs the result as GraphSONv3 JSON")
public ResponseEntity<StreamingResponseBody> execute(@RequestHeader final HttpHeaders httpHeaders,
public ResponseEntity<StreamingResponseBody> execute(
@RequestHeader final HttpHeaders httpHeaders,
@RequestBody final String gremlinQuery) throws IOException {
// Set up
preExecuteSetUp(httpHeaders);

// Write to output stream for response
StreamingResponseBody responseBody = outputStream -> GRAPHSON_V3_WRITER.writeObject(
outputStream,
runGremlinQuery(gremlinQuery).get0());
HttpStatus status = HttpStatus.OK;
StreamingResponseBody responseBody;
try {
Object result = runGremlinQuery(gremlinQuery).get0();
// Write to output stream for response
responseBody = os -> GRAPHSON_V3_WRITER.writeObject(os, result);
} catch (final GafferRuntimeException e) {
status = HttpStatus.INTERNAL_SERVER_ERROR;
responseBody = os -> os.write(
new JSONObject().put("simpleMessage", e.getMessage()).toString().getBytes(StandardCharsets.UTF_8));
}

return ResponseEntity.ok()
return ResponseEntity.status(status)
.contentType(APPLICATION_NDJSON)
.body(responseBody);
}
Expand Down Expand Up @@ -198,20 +208,32 @@ public String cypherExplain(@RequestHeader final HttpHeaders httpHeaders, @Reque
summary = "Run a Cypher Query",
description = "Translates a Cypher query to Gremlin and executes it returning a GraphSONv3 JSON result." +
"Note will always append a '.toList()' to the translation")
public ResponseEntity<StreamingResponseBody> cypherExecute(@RequestHeader final HttpHeaders httpHeaders,
public ResponseEntity<StreamingResponseBody> cypherExecute(
@RequestHeader final HttpHeaders httpHeaders,
@RequestBody final String cypherQuery) throws IOException {
// Set up
preExecuteSetUp(httpHeaders);
final CypherAst ast = CypherAst.parse(cypherQuery);
// Translate the cypher to gremlin, always add a .toList() otherwise Gremlin
// wont execute it as its lazy
final String translation = ast.buildTranslation(Translator.builder().gremlinGroovy().enableCypherExtensions().build()) + ".toList()";

// Write to output stream for response
StreamingResponseBody responseBody = outputStream -> GRAPHSON_V3_WRITER.writeObject(
outputStream,
runGremlinQuery(translation).get0());

return ResponseEntity.ok()
HttpStatus status = HttpStatus.OK;
StreamingResponseBody responseBody;
try {
final CypherAst ast = CypherAst.parse(cypherQuery);
// Translate the cypher to gremlin, always add a .toList() otherwise Gremlin
// wont execute it as its lazy
final String translation = ast.buildTranslation(
Translator.builder().gremlinGroovy().enableCypherExtensions().build()) + ".toList()";
// Run Query
Object result = runGremlinQuery(translation).get0();
// Write to output stream for response
responseBody = os -> GRAPHSON_V3_WRITER.writeObject(os, result);
} catch (final GafferRuntimeException | SyntaxException e) {
status = HttpStatus.INTERNAL_SERVER_ERROR;
responseBody = os -> os.write(
new JSONObject().put("simpleMessage", e.getMessage()).toString().getBytes(StandardCharsets.UTF_8));
}

return ResponseEntity.status(status)
.contentType(APPLICATION_NDJSON)
.body(responseBody);
}
Expand Down Expand Up @@ -286,7 +308,7 @@ private Tuple2<Object, JSONObject> runGremlinQuery(final String gremlinQuery) {

// OpenTelemetry hooks
Span span = OtelUtil.startSpan(
this.getClass().getName(), "Gremlin Request: " + UUID.nameUUIDFromBytes(gremlinQuery.getBytes(Charset.defaultCharset())));
this.getClass().getName(), "Gremlin Request: " + UUID.nameUUIDFromBytes(gremlinQuery.getBytes(StandardCharsets.UTF_8)));
span.setAttribute("gaffer.gremlin.query", gremlinQuery);

// tuple to hold the result and explain
Expand All @@ -296,7 +318,7 @@ private Tuple2<Object, JSONObject> runGremlinQuery(final String gremlinQuery) {
try (Scope scope = span.makeCurrent();
GremlinExecutor gremlinExecutor = getGremlinExecutor()) {
// Execute the query
Object result = gremlinExecutor.eval(gremlinQuery).join();
Object result = gremlinExecutor.eval(gremlinQuery).get();

// Store the result and explain for returning
pair.put0(result);
Expand All @@ -319,6 +341,8 @@ private Tuple2<Object, JSONObject> runGremlinQuery(final String gremlinQuery) {
span.setStatus(StatusCode.ERROR, e.getMessage());
span.recordException(e);
throw new GafferRuntimeException(GENERAL_ERROR_MSG + e.getMessage(), e);
} finally {
span.end();
}

return pair;
Expand Down
42 changes: 40 additions & 2 deletions ...i/spring-rest/src/test/java/uk/gov/gchq/gaffer/rest/controller/GremlinControllerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ void shouldReturnExplainOfValidGremlinQuery() throws Exception {
}

@Test
void shouldRejectMalformedGremlinQuery() throws Exception {
void shouldRejectMalformedGremlinQueryFromExplain() throws Exception {
// Given
String gremlinString = "g.V().stepDoesNotExist().toList()";

Expand All @@ -163,6 +163,25 @@ void shouldRejectMalformedGremlinQuery() throws Exception {
assertThat(result.getResponse().getStatus()).isEqualTo(500);
}

@Test
void shouldRejectMalformedGremlinQueryFromExecute() throws Exception {
// Given
String gremlinString = "g.V().stepDoesNotExist().toList()";

// When
MvcResult result = mockMvc
.perform(MockMvcRequestBuilders
.post(GREMLIN_EXECUTE_ENDPOINT)
.content(gremlinString)
.contentType(TEXT_PLAIN_VALUE)
.accept(APPLICATION_NDJSON))
.andReturn();

// Then
// Expect a server error response
assertThat(result.getResponse().getStatus()).isEqualTo(500);
}

@Test
void shouldExecuteValidCypherQuery() throws Exception {
String cypherString = "MATCH (p:person) WHERE ID(p) = '" + MARKO.getId() + "' RETURN p";
Expand Down Expand Up @@ -285,7 +304,7 @@ void shouldReturnExplainOfCypherQueryWithExtensions() throws Exception {
}

@Test
void shouldRejectMalformedCypherQuery() throws Exception {
void shouldRejectMalformedCypherQueryFromExplain() throws Exception {
// Given
String cypherString = "MATCH (p:person) WHERE RETURN p";

Expand All @@ -302,5 +321,24 @@ void shouldRejectMalformedCypherQuery() throws Exception {
assertThat(result.getResponse().getStatus()).isEqualTo(500);
}

@Test
void shouldRejectMalformedCypherQueryFromExecute() throws Exception {
// Given
String cypherString = "MATCH (p:person) WHERE RETURN p";

// When
MvcResult result = mockMvc
.perform(MockMvcRequestBuilders
.post(CYPHER_EXECUTE_ENDPOINT)
.content(cypherString)
.contentType(TEXT_PLAIN_VALUE)
.accept(APPLICATION_NDJSON))
.andReturn();

// Then
// Expect a server error response
assertThat(result.getResponse().getStatus()).isEqualTo(500);
}


}

0 comments on commit d3d494e

Please sign in to comment.