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

Gh-3303: No error reported when Gremlin endpoint fails #3304

Merged
merged 5 commits into from
Sep 30, 2024
Merged

Gh-3303: No error reported when Gremlin endpoint fails #3304

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a493e2c
fix error reporting
tb06904 Sep 26, 2024
d71d8e8
add view for otel
tb06904 Sep 26, 2024
9cac801
testing
tb06904 Sep 26, 2024
6714841
null check
tb06904 Sep 26, 2024
d26074d
Merge branch 'develop' into gh-3303-no-error-reported-gremlin-endpoint
wb36499 Sep 30, 2024
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 ...store/src/main/java/uk/gov/gchq/gaffer/store/operation/handler/OperationChainHandler.java
View file
Open in desktop
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 @@
// 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());

Check warning on line 57 in core/store/src/main/java/uk/gov/gchq/gaffer/store/operation/handler/OperationChainHandler.java

View check run for this annotation

Codecov / codecov/patch

core/store/src/main/java/uk/gov/gchq/gaffer/store/operation/handler/OperationChainHandler.java#L57 

Added line #L57 was not covered by tests
}

// 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
Open in desktop
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
Open in desktop
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);
}


}
Loading