Uploaded image for project: 'TinkerPop'
  1. TinkerPop
  2. TINKERPOP-1044

ResponseMessage should contain server-side exception name.

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Implemented
    • Affects Version/s: 3.1.0-incubating
    • Fix Version/s: 3.2.5
    • Component/s: server
    • Labels:
      None

      Description

      When an exception occurs during execution by gremlin server, the ResponseMessage currently contains the Exception message, but it would also be helpful to include the Exception class name.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user vrkrishn opened a pull request:

          https://github.com/apache/tinkerpop/pull/440

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

          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

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/vrkrishn/tinkerpop TINKERPOP-1044

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/tinkerpop/pull/440.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #440


          commit de9bbda5e44d5f4a8a7c024894027d1dccd6e2c5
          Author: Vivek Krishnan <vikri@microsoft.com>
          Date: 2016-09-28T06:21:42Z

          TINKERPOP-1044: 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


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user vrkrishn opened a pull request: https://github.com/apache/tinkerpop/pull/440 TINKERPOP-1044 : Gremlin server REST endpoint - Add Exception Class and Message in Response 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 You can merge this pull request into a Git repository by running: $ git pull https://github.com/vrkrishn/tinkerpop TINKERPOP-1044 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tinkerpop/pull/440.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #440 commit de9bbda5e44d5f4a8a7c024894027d1dccd6e2c5 Author: Vivek Krishnan <vikri@microsoft.com> Date: 2016-09-28T06:21:42Z TINKERPOP-1044 : 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
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user rjbriody commented on the issue:

          https://github.com/apache/tinkerpop/pull/440

          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.

          Show
          githubbot ASF GitHub Bot added a comment - Github user rjbriody commented on the issue: https://github.com/apache/tinkerpop/pull/440 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user vrkrishn commented on the issue:

          https://github.com/apache/tinkerpop/pull/440

          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

          Show
          githubbot ASF GitHub Bot added a comment - Github user vrkrishn commented on the issue: https://github.com/apache/tinkerpop/pull/440 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
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user rjbriody commented on the issue:

          https://github.com/apache/tinkerpop/pull/440

          > 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.

          Show
          githubbot ASF GitHub Bot added a comment - Github user rjbriody commented on the issue: https://github.com/apache/tinkerpop/pull/440 > 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/440

          First of all, thanks for taking the time to do this. TINKERPOP-1044(https://issues.apache.org/jira/browse/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:

          https://github.com/apache/tinkerpop/blob/850e159b8dc883ac3995c8d775b8f09b7a64b440/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/AbstractEvalOpProcessor.java#L260-L266

          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:

          ```text
          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

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/440 First of all, thanks for taking the time to do this. TINKERPOP-1044 ( https://issues.apache.org/jira/browse/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: https://github.com/apache/tinkerpop/blob/850e159b8dc883ac3995c8d775b8f09b7a64b440/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/AbstractEvalOpProcessor.java#L260-L266 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: ```text 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
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user vrkrishn commented on the issue:

          https://github.com/apache/tinkerpop/pull/440

          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

          Show
          githubbot ASF GitHub Bot added a comment - Github user vrkrishn commented on the issue: https://github.com/apache/tinkerpop/pull/440 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
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/440

          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?

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/440 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?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user vrkrishn commented on the issue:

          https://github.com/apache/tinkerpop/pull/440

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

          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?

          Show
          githubbot ASF GitHub Bot added a comment - Github user vrkrishn commented on the issue: https://github.com/apache/tinkerpop/pull/440 Luckily I was able to spin up a Linux VM and run the integration tests 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?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/440

          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: https://github.com/thinkaurelius/titan/pull/1312

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/440 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: https://github.com/thinkaurelius/titan/pull/1312
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user vrkrishn commented on the issue:

          https://github.com/apache/tinkerpop/pull/440

          Yeah I can take care of the AbstractEvalOpProcessor

          Show
          githubbot ASF GitHub Bot added a comment - Github user vrkrishn commented on the issue: https://github.com/apache/tinkerpop/pull/440 Yeah I can take care of the AbstractEvalOpProcessor
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/440

          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.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/440 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user vrkrishn commented on the issue:

          https://github.com/apache/tinkerpop/pull/440

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

          Show
          githubbot ASF GitHub Bot added a comment - Github user vrkrishn commented on the issue: https://github.com/apache/tinkerpop/pull/440 Would it be possible to push this change then and add the other functionality as a different ticket?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/440

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

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/440 Ok - I'll look to just merge this as-is then will add in the change to the other channelizers.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/tinkerpop/pull/440

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/tinkerpop/pull/440
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/440

          This was a pretty simple change - merge this via CTR

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/440 This was a pretty simple change - merge this via CTR
          Hide
          spmallette stephen mallette added a comment -

          This one remains open at the pull request related to this change only added a change for REST. There is a larger body of work related to the gremlin server protocol and the error responses.

          Show
          spmallette stephen mallette added a comment - This one remains open at the pull request related to this change only added a change for REST. There is a larger body of work related to the gremlin server protocol and the error responses.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user spmallette opened a pull request:

          https://github.com/apache/tinkerpop/pull/598

          TINKERPOP-1044 Include more error information in remote exceptions

          https://issues.apache.org/jira/browse/TINKERPOP-1044

          Adds both the remote stacktrace and exception hierarchy to `ResponseException` for the driver and to the JSON payload in the HTTP protocol. Deprecates the "Exception-Class" in the HTTP protocol.

          All tests pass with `docker/build.sh -t -n -i`

          VOTE +1

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/apache/tinkerpop TINKERPOP-1044

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/tinkerpop/pull/598.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #598


          commit b29bab0acada54aa525184935d96b41f9cf0854b
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2017-04-06T21:55:39Z

          TINKERPOP-1044 Additional error data to Gremlin Server responses

          Applies to all protocols: http, nio, and websocket. Added stack trace and exception hierarchy. Deprecated the "Exception-Class" in the http protocol.

          commit c33069cecd227dff664a02330a8462efce32e121
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2017-04-06T22:10:08Z

          TINKERPOP-1044 Added documentation for additional error info in ResponseMessage


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user spmallette opened a pull request: https://github.com/apache/tinkerpop/pull/598 TINKERPOP-1044 Include more error information in remote exceptions https://issues.apache.org/jira/browse/TINKERPOP-1044 Adds both the remote stacktrace and exception hierarchy to `ResponseException` for the driver and to the JSON payload in the HTTP protocol. Deprecates the "Exception-Class" in the HTTP protocol. All tests pass with `docker/build.sh -t -n -i` VOTE +1 You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/tinkerpop TINKERPOP-1044 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tinkerpop/pull/598.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #598 commit b29bab0acada54aa525184935d96b41f9cf0854b Author: Stephen Mallette <spmva@genoprime.com> Date: 2017-04-06T21:55:39Z TINKERPOP-1044 Additional error data to Gremlin Server responses Applies to all protocols: http, nio, and websocket. Added stack trace and exception hierarchy. Deprecated the "Exception-Class" in the http protocol. commit c33069cecd227dff664a02330a8462efce32e121 Author: Stephen Mallette <spmva@genoprime.com> Date: 2017-04-06T22:10:08Z TINKERPOP-1044 Added documentation for additional error info in ResponseMessage
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user spmallette opened a pull request:

          https://github.com/apache/tinkerpop/pull/599

          TinkerPop-1044 master

          https://issues.apache.org/jira/browse/TINKERPOP-1044

          Companion pull request for #598 since there were some conflicts that needed resolution that were somewhat non-trivial.

          All tests pass with `docker/build.sh -t -n -i`

          VOTE +1

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/apache/tinkerpop TINKERPOP-1044-master

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/tinkerpop/pull/599.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #599


          commit b29bab0acada54aa525184935d96b41f9cf0854b
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2017-04-06T21:55:39Z

          TINKERPOP-1044 Additional error data to Gremlin Server responses

          Applies to all protocols: http, nio, and websocket. Added stack trace and exception hierarchy. Deprecated the "Exception-Class" in the http protocol.

          commit c33069cecd227dff664a02330a8462efce32e121
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2017-04-06T22:10:08Z

          TINKERPOP-1044 Added documentation for additional error info in ResponseMessage

          commit 92eceb4e620409be2c9ee810cc4132b7ae2dafc7
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2017-04-07T12:22:31Z

          Merge branch 'TINKERPOP-1044' into TINKERPOP-1044-master

          Conflicts:
          gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/AbstractEvalOpProcessor.java
          gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerHttpIntegrateTest.java


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user spmallette opened a pull request: https://github.com/apache/tinkerpop/pull/599 TinkerPop-1044 master https://issues.apache.org/jira/browse/TINKERPOP-1044 Companion pull request for #598 since there were some conflicts that needed resolution that were somewhat non-trivial. All tests pass with `docker/build.sh -t -n -i` VOTE +1 You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/tinkerpop TINKERPOP-1044 -master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tinkerpop/pull/599.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #599 commit b29bab0acada54aa525184935d96b41f9cf0854b Author: Stephen Mallette <spmva@genoprime.com> Date: 2017-04-06T21:55:39Z TINKERPOP-1044 Additional error data to Gremlin Server responses Applies to all protocols: http, nio, and websocket. Added stack trace and exception hierarchy. Deprecated the "Exception-Class" in the http protocol. commit c33069cecd227dff664a02330a8462efce32e121 Author: Stephen Mallette <spmva@genoprime.com> Date: 2017-04-06T22:10:08Z TINKERPOP-1044 Added documentation for additional error info in ResponseMessage commit 92eceb4e620409be2c9ee810cc4132b7ae2dafc7 Author: Stephen Mallette <spmva@genoprime.com> Date: 2017-04-07T12:22:31Z Merge branch ' TINKERPOP-1044 ' into TINKERPOP-1044 -master Conflicts: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/AbstractEvalOpProcessor.java gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerHttpIntegrateTest.java
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the issue:

          https://github.com/apache/tinkerpop/pull/598

          VOTE +1.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/598 VOTE +1.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the issue:

          https://github.com/apache/tinkerpop/pull/599

          VOTE +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/599 VOTE +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkuppitz commented on the issue:

          https://github.com/apache/tinkerpop/pull/598

          VOTE: +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the issue: https://github.com/apache/tinkerpop/pull/598 VOTE: +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkuppitz commented on the issue:

          https://github.com/apache/tinkerpop/pull/599

          VOTE: +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the issue: https://github.com/apache/tinkerpop/pull/599 VOTE: +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/tinkerpop/pull/599

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/tinkerpop/pull/599
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/tinkerpop/pull/598

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/tinkerpop/pull/598

            People

            • Assignee:
              spmallette stephen mallette
              Reporter:
              rjbriody Bob Briody
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development