TINKERPOP-1044: Gremlin server REST endpoint - Add Exception Class and Message in Response

[vrkrishn]

When using Gremlin-Server Client over REST endpoint and the gremlin query results in a groovy script Exception/Error, will output the Throwable class name as well as the Throwable Message in the message body of the HTTP Response

[rjbriody]

Nice work. Looks good for the REST endpoint. I'm wondering - what about other endpoints such as websocket - shouldn't they be consistent? Also, I would expect the same information to be available when using a gremlin driver.

[vrkrishn]

I added this functionality to the REST endpoint because I am using the HttpChannelizer, I can add the functionality to the other channelizers manually but I think you are getting at that there should be some common response between all channelizer methods that is defined in only one location

[rjbriody]

there should be some common response between all channelizer methods that is defined in only one location

Could be. I'm not familiar enough with the channelizers to know for sure. I'm mostly just selfishly interested in getting the exception type when using the java driver, so this inconsistency jumped out at me.

[spmallette]

First of all, thanks for taking the time to do this. TINKERPOP-1044 was less about REST and more about the Gremlin Server protocol but as it turns out the issue is the same in REST as it is over there.

there should be some common response between all channelizer methods that is defined in only one location

There's no such place to do that. There isn't any shared exception processing logic between REST and the Gremlin Server protocol. I think you would add that logic here:

tinkerpop/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/AbstractEvalOpProcessor.java

Lines 260 to 266 in 850e159

	} catch (Exception ex) {
	logger.warn(String.format("Exception processing a script on request [%s].", msg), ex);
	final String err = ex.getMessage();
	ctx.writeAndFlush(ResponseMessage.build(msg).code(ResponseStatusCode.SERVER_ERROR)
	.statusMessage(null == err || err.isEmpty() ? ex.getClass().getSimpleName() : err).create());
	if (managedTransactionsForRequest) attemptRollback(msg, context.getGraphManager(), settings.strictTransactionManagement);
	}

as i look at it now the error messaging logic is a bit weird there to begin with (i.e. if we don't have an "error message" we just use the class simple name?? - no idea why that is like that). Note that there are a number of integration tests that may fail if with these changes you are making as they rely on the asserting error messages in some cases. If you haven't done so already please be sure to run those tests:

mvn clean install && mvn verify -pl gremlin-server -DskipIntegrationTests=false

Also, please add an entry to the CHANGELOG that describes your fix:

https://github.com/apache/tinkerpop/blob/850e159b8dc883ac3995c8d775b8f09b7a64b440/CHANGELOG.asciidoc#tinkerpop-323-release-date-not-officially-released-yet

[vrkrishn]

I'm having trouble running the tests on my Windows Box (the tests fail on different modules so I don't think that this change caused the failures). I also see that the CI build passed which should indicate that the unit tests that are not running probably fail to run due to Windows-specific shenanigans.

I'll update this once I can successfully run tests on my box.

UPDATE
I looked in the Gremlin Server Integration test and I noticed the Integration tests should500OnGETwithGremlinEvalFailure(). This test sends a 1/0 to the gremlin server and which should respond with a DivisionByZero Exception

I notice here that the only check that the test is running is for the 500 status code (which I am still sending along with the updated message). Furthermore, I grep'd through the integration tests to see if I could find the default error message and confirmed that none of the gremlin-server integration tests contain that constant.

I know that this is not robust but its all I can really do until I figure out how to properly run tests on my box

[spmallette]

Sorry for the trouble on running the tests. It's not terribly friendly to Windows as we don't have any core devs who use that OS. Hopefully you can figure out how to get the tests working. Maybe they won't run well with maven, but might run from an IDE like Intellij?

[vrkrishn]

Luckily I was able to spin up a Linux VM and run the integration tests

Output of mvn verify
Tests run: 31, Failures: 0, Errors: 0, Skipped: 1, Time elapsed: 162.807 sec - in org.apache.tinkerpop.gremlin.server.GremlinServerIntegrateTest

Results :
Tests run: 163, Failures: 0, Errors: 0, Skipped: 13

Also, I am currently using titan 1 which depends on gremlin-server 3.1 i think, what would be the best way to adapt this change to my current distro of titan?

[spmallette]

Glad that's working. Did you still intend to make the fix for the other channelizers in AbstractEvalOpProcessor?

As for titan, i guess you would build this branch of tinkerpop with the work ongoing here: thinkaurelius/titan#1312

[vrkrishn]

Do you mean modify the appropriate handlers for NIO and WebSocket or is there a different module to look at?

[spmallette]

We intend to freeze the code base for release of 3.2.3 (and 3.1.5) in a couple of days. If you'd like to see this merged in time for 3.2.3, we'd need to have your changes pretty soon for review. No pressure, of course....I just wanted to inform you of our release schedule in case you really needed this change in an official packaged release.

[vrkrishn]

Would it be possible to merge this change then and add the other functionality as a different ticket?

[spmallette]

Ok - I'll look to just merge this as-is then will add in the change to the other channelizers.

[spmallette]

This was a pretty simple change - merge this via CTR