Conversation
…rt (#3724) Dependabot bumped groovy.version from 4.0.28 to 5.0.4, but TinkerPop 3.8.0 targets Groovy 4.x. Groovy 5 rejects the multiple wildcard static enum imports added by ImportGroovyCustomizer with "The name valueOf is already declared", preventing the Gremlin script engine from initializing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical startup failure in Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Code ReviewThis is a clean, well-motivated fix. The root cause analysis is correct: TinkerPop 3.8.x targets Groovy 4.x, and Groovy 5's stricter duplicate-symbol rules break ImportGroovyCustomizer's wildcard static enum imports. Reverting to 4.0.28 is the right call. Positives
Issue: Resource Leak in the TestGremlinGroovyScriptEngine implements AutoCloseable (via GremlinScriptEngine). The current test creates an engine but never closes it, which can leak thread pools and class loaders. Suggest using try-with-resources and changing "throws ScriptException" to "throws Exception" since AutoCloseable.close() declares a checked Exception. Minor NitThe assertion Overall this PR is in good shape. The resource-leak fix in the test is the only substantive concern before merging. |
📜 License Compliance Check✅ License check passed. See artifacts for full report. License Summary (first 50 lines) |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an incompatibility issue between Groovy 5 and TinkerPop 3.8.x by downgrading the Groovy version to 4.0.28. A new regression test, GremlinGroovyEngineTest, has been added to verify that the Gremlin Groovy script engine initializes correctly with core imports. The review suggests enhancing this test to more directly confirm the availability of Gremlin-specific imports, which were the root cause of the original problem.
| // A successful eval proves the engine started without "valueOf is already declared" | ||
| assertThat(engine.eval("1 + 1")).isEqualTo(2); |
There was a problem hiding this comment.
While eval("1 + 1") confirms the script engine is alive, it doesn't verify that the Gremlin-specific imports (which were the source of the original issue) are working correctly. A more robust test would be to evaluate an expression that uses one of the core Gremlin imports, such as an enum from T. This would make the regression test more specific and less likely to pass if there are other import-related issues.
// A successful eval proves the engine started without "valueOf is already declared"
// and that core Gremlin imports are available.
assertThat(engine.eval("T.id")).isEqualTo(org.apache.tinkerpop.gremlin.structure.T.id);
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3725 +/- ##
==========================================
- Coverage 64.43% 56.13% -8.31%
==========================================
Files 1579 1579
Lines 115224 115224
Branches 24378 24378
==========================================
- Hits 74247 64676 -9571
- Misses 30846 41436 +10590
+ Partials 10131 9112 -1019 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Summary
groovy.versioningremlin/pom.xmlfrom5.0.4back to4.0.2863b536a41) is incompatible with TinkerPop 3.8.0, which targets Groovy 4.x (<groovy.version>4.0.25</groovy.version>in its parent POM)import static SomeEnum.*) added by TinkerPop'sImportGroovyCustomizer, because all enums share avalueOf(String)method — causing"The name valueOf is already declared"GremlinGroovyEngineTestthat exercises the exact initialization pathCloses #3724
Test plan
GremlinGroovyEngineTest.groovyEngineInitializesWithCoreImportspassesGremlinGAVTest(8 tests) passesSQLFromGremlinTestpassesCypherQueryEngineTest,CypherTest,GremlinTest.gremlinFromDatabaseconfirmed unrelated (fail onmainbefore this change)🤖 Generated with Claude Code