Skip to content

refactor(server): allow TinkerPop exceptions in Gremlin resp#2987

Open
utafrali wants to merge 2 commits intoapache:masterfrom
utafrali:fix/issue-2986-bug-request-gremlin-exception-missingpro
Open

refactor(server): allow TinkerPop exceptions in Gremlin resp#2987
utafrali wants to merge 2 commits intoapache:masterfrom
utafrali:fix/issue-2986-bug-request-gremlin-exception-missingpro

Conversation

@utafrali
Copy link
Copy Markdown

Purpose of the PR

Main Changes

The Gremlin query API was filtering out TinkerPop exceptions like MissingPropertyException instead of passing them back to clients. Added org.apache.tinkerpop. to the exception allowlist so these errors bubble up properly. Also added a test to verify the behavior.

Verifying these changes

  • Trivial rework / code cleanup without any test coverage.
  • Already covered by existing tests.
  • Need tests and can be verified as follows:

Added unit test in GremlinQueryAPITest.java covering exception handling for TinkerPop packages.

Does this PR potentially affect the following parts?

  • Dependencies
  • Modify configurations
  • The public API
  • Other affects
  • Nope

Documentation Status

  • Doc - TODO
  • Doc - Done
  • Doc - No Need

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. api Changes of API gremlin TinkerPop gremlin tests Add or improve test cases labels Apr 10, 2026
"org.codehaus.",
"org.apache.hugegraph."
"org.apache.hugegraph.",
"org.apache.tinkerpop."
Copy link
Copy Markdown
Member

@imbajin imbajin Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

‼️ This package-wide allowlist is too broad. matchBadRequestException() would now classify classes such as org.apache.tinkerpop.gremlin.server.auth.AuthenticationException and org.apache.tinkerpop.gremlin.server.authz.AuthorizationException as 400, even though they carry 401/403 semantics.

That changes the API contract for auth failures and hides security/server-side errors behind a generic bad request. Please whitelist only the concrete exception class(es) reproduced by #2986 instead of the whole org.apache.tinkerpop. namespace.

@Test
public void testMatchBadRequestExceptionWithTinkerpop() throws Exception {
Assert.assertTrue(matchBadRequest(
"org.apache.tinkerpop.gremlin.structure.util.empty.EmptyProperty"));
Copy link
Copy Markdown
Member

@imbajin imbajin Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ EmptyProperty is not an exception type, so this test does not pin the reported bug. In TinkerPop 3.5.1, EmptyProperty.value() throws Property.Exceptions.propertyDoesNotExist() (IllegalStateException), which means this assertion can pass even if the real Exception-Class from Gremlin is still misclassified.

Please assert against the actual class observed in #2986, and add a negative case for auth/authorization exceptions so the allowlist boundary is covered.

@imbajin imbajin changed the title Allow TinkerPop exceptions in Gremlin query responses refactor(server): allow TinkerPop exceptions in Gremlin resp Apr 10, 2026
@utafrali
Copy link
Copy Markdown
Author

Done, pushed the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api Changes of API gremlin TinkerPop gremlin size:M This PR changes 30-99 lines, ignoring generated files. tests Add or improve test cases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Request Gremlin exception: MissingPropertyException

2 participants