fix(#5208): engine ANTLR 4.13.2 + Gremlin ANTLR relocation (coexistence)#5216
Conversation
|
Tick the box to add this pull request to the merge queue (same as
|
There was a problem hiding this comment.
Code Review
This pull request upgrades the core ANTLR runtime to version 4.13.2 in the engine module while keeping the Gremlin module on ANTLR 4.9.1 by relocating it inside a shaded jar. It introduces a new gremlin-it integration test module to verify the coexistence of both ANTLR versions in a single JVM and runs the existing Gremlin test suite against the shaded artifact. Feedback on these changes includes wrapping ResultSet instances in try-with-resources blocks to prevent resource leaks in the coexistence test, adding a null check to avoid a potential NullPointerException when loading resources in VectorGremlinIT, and using the standard <type>test-jar</type> declaration instead of <classifier>tests</classifier> in the Maven configuration.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // Engine ANTLR SQL parser (org.antlr 4.13.2, v4 ATN). | ||
| final ResultSet sql = database.query("sql", "SELECT name, age FROM Person WHERE age = 25"); | ||
| assertThat(sql.hasNext()).isTrue(); | ||
| assertThat(sql.next().<String>getProperty("name")).isEqualTo("Jay"); | ||
|
|
||
| // Engine ANTLR Cypher parser (org.antlr 4.13.2, v4 ATN). | ||
| final ResultSet cypher = database.query("cypher", "MATCH (p:Person) RETURN p.name AS name"); | ||
| assertThat(cypher.hasNext()).isTrue(); | ||
|
|
||
| // TinkerPop Gremlin parser (relocated com.arcadedb.gremlin.shaded.org.antlr 4.9.1, v3 ATN). | ||
| final ResultSet gremlin = graph.gremlin("g.V().hasLabel('Person').count()").execute(); | ||
| assertThat((Long) gremlin.next().getProperty("result")).isEqualTo(1L); |
There was a problem hiding this comment.
The ResultSet instances returned by database.query and graph.gremlin(...).execute() implement AutoCloseable and should be closed to prevent resource leaks. Wrapping them in try-with-resources blocks ensures they are closed properly even if assertions fail.
// Engine ANTLR SQL parser (org.antlr 4.13.2, v4 ATN).
try (final ResultSet sql = database.query("sql", "SELECT name, age FROM Person WHERE age = 25")) {
assertThat(sql.hasNext()).isTrue();
assertThat(sql.next().<String>getProperty("name")).isEqualTo("Jay");
}
// Engine ANTLR Cypher parser (org.antlr 4.13.2, v4 ATN).
try (final ResultSet cypher = database.query("cypher", "MATCH (p:Person) RETURN p.name AS name")) {
assertThat(cypher.hasNext()).isTrue();
}
// TinkerPop Gremlin parser (relocated com.arcadedb.gremlin.shaded.org.antlr 4.9.1, v3 ATN).
try (final ResultSet gremlin = graph.gremlin("g.V().hasLabel('Person').count()").execute()) {
assertThat((Long) gremlin.next().getProperty("result")).isEqualTo(1L);
}| final URL inputFile = VectorGremlinIT.class.getClassLoader().getResource("importer-glove.txt"); | ||
| db.command("sql", "import database file://" + inputFile.getFile() + " "// |
There was a problem hiding this comment.
ClassLoader.getResource() can return null if the resource is not found on the classpath. Adding a null check before calling inputFile.getFile() prevents a potential NullPointerException and provides a clearer error message if the test environment is misconfigured.
final URL inputFile = VectorGremlinIT.class.getClassLoader().getResource("importer-glove.txt");
if (inputFile == null) {
throw new IllegalStateException("Required resource 'importer-glove.txt' not found on classpath");
}
db.command("sql", "import database file://" + inputFile.getFile() + " "//| <groupId>com.arcadedb</groupId> | ||
| <artifactId>arcadedb-gremlin</artifactId> | ||
| <version>${project.parent.version}</version> | ||
| <classifier>tests</classifier> |
There was a problem hiding this comment.
Using <type>test-jar</type> is the standard Maven way to depend on a test jar, rather than using <classifier>tests</classifier> with the default jar type. This ensures consistent classpath resolution across different IDEs and build environments, matching how arcadedb-server's test jar is declared on line 127.
| <classifier>tests</classifier> | |
| <type>test-jar</type> |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Review: engine ANTLR 4.13.2 + Gremlin ANTLR relocation (#5216)Thanks for the thorough write-up and the design/plan docs, this is a genuinely tricky classpath problem and the approach (engine on real 4.13.2, TinkerPop's precompiled v3 parser relocated inside the shaded jar, coexistence proven in a dedicated A few observations, mostly minor: Test robustness
Build / module coupling
Coexistence test visibility
Consumer isolation (deferred, but worth surfacing)
Nits
Overall this is a solid, carefully-reasoned fix with good test coverage of the coexistence scenario. The items above are hardening suggestions rather than blockers. Automated review by Claude. |
📜 License Compliance Check✅ License check passed. See artifacts for full report. License Summary (first 50 lines) |
…mlin-it build ordering Address PR #5216 review: null-check the classpath resource and resolve it via new File(url.toURI()) so paths with spaces/special chars decode correctly; note in the gremlin-it POM that it consumes gremlin's package-phase outputs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed in d2d698c. Details per item: Test robustness (VectorGremlinIT) - Fixed. Now null-checks the classpath resource with a clear assertion message and resolves it via gremlin-it depends on gremlin package-phase outputs - Documented. Added a note to the Relative maven-enforcer AntlrCoexistenceIT CI visibility - Confirmed it runs in CI. The Deferred consumer isolation - Filed as follow-up issue #5217 (mark gremlin's junit-vintage-engine version - Left as |
Review: engine ANTLR 4.13.2 + Gremlin ANTLR relocationReviewed the build/dependency changes (the two docs/superpowers specs are informational). Overall this is a well-reasoned, thoroughly documented fix for a genuinely nasty classpath problem, and the approach (engine on plain 4.13.2, TinkerPop precompiled 4.9.1 relocated inside the shaded jar, engine EXCLUDED from the shade artifactSet) is sound. The coexistence IT plus the migrated 2090-test suite give good confidence. A few things worth considering: Build / reactor ordering (main concern). Version drift: hardcoded TinkerPop version. VectorGremlinIT path handling. Good move resolving importer-glove.txt via the classpath and decoding with new File(url.toURI()) (handles %20 / special chars). One residual edge: the decoded absolute path is then re-embedded into a whitespace-delimited SQL string (import database file://... with ...). If the resolved path ever contains a space, the SQL import parser will truncate at the first space, so the space-safety is only partial. Test-only and unlikely under target/, so fine to leave, just flagging. Skips are the right kind. Using skipTests=true (not maven.test.skip) on gremlin surefire+failsafe correctly keeps the test sources compiling so the tests jar is still produced - good call, and matching the failsafe skip to the surefire one (so -Pintegration does not hang on the unshaded classpath) is exactly right. Minor.
Nice work isolating the two runtimes without forking TinkerPop. The artifactSet engine-exclusion (with the clear comment explaining why a bundled relocated engine parser would reintroduce the exact ATN error) is the subtle part and it is handled and documented well. |
📜 License Compliance Check✅ License check passed. See artifacts for full report. License Summary (first 50 lines) |
…fy-vs-test build Address PR #5216 review: promote gremlin.version to the parent so arcadedb-gremlin-it references the same TinkerPop version as arcadedb-gremlin instead of a hardcoded 3.8.1 that would silently drift on a bump. Document that a full reactor run needs mvn verify/ install (not bare mvn test) because gremlin-it consumes gremlin's package-phase artifacts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed in 6aaddf3. Build/reactor ordering - Documented. CI is safe: the unit-tests job runs Hardcoded TinkerPop version - Fixed. Promoted VectorGremlinIT residual space edge - Acknowledged, left as-is. You're right that the decoded path is re-embedded into a whitespace-delimited Skips / minor notes - The |
Review: engine ANTLR 4.13.2 + Gremlin ANTLR relocationOverall this is a careful, well-documented change. The PR description, the design/plan docs, the invariant-stating comments in the poms, and the migrated coexistence test make the intent and the tradeoffs easy to follow. ATTRIBUTIONS.md and CLAUDE.md are kept in sync, and the earlier review feedback (path-with-spaces hardening in Below are concerns, ranked. The first is the one I'd want verified before merge. 1. (High / please verify) Other modules still put unshaded gremlin + engine 4.13.2 on one classpathThe shading fix isolates the two ANTLR runtimes in the shipped distribution and in the new
Since the branch was validated with 2. (Medium)
|
📜 License Compliance Check✅ License check passed. See artifacts for full report. License Summary (first 50 lines) |
|
Good catch on #1 - it was a real regression. Addressed in 34d4675. 1. Unshaded gremlin + engine 4.13.2 clash (graphql / postgresw) - Confirmed and fixed. Running
2. 3. Coverage / CI signal - No build-failing coverage gate exists: the 4. gremlin-it re-declares gremlin's test deps - Added a note at the top of Nits - Added |
Review: engine ANTLR 4.13.2 + Gremlin ANTLR relocationOverall this is a well-scoped, carefully documented fix for a genuinely thorny classpath problem. The design spec and plan are excellent, the relocation approach is standard and sound (Trino/Spark precedent), and the coexistence proof ( A few points worth considering before merge: 1. CI can go green with zero Gremlin tests actually running (medium) 2. 3. Hand-mirrored dependency list in 4. Deferred Maven-consumer isolation is real for embedders (informational) 5. Minor DX regression Things I checked that look good
The riskiest functional change (engine ANTLR 4.9.1 to 4.13.2 API drift) is covered by the existing SQL/Cypher parser suites, which the PR reports green. Assuming CI confirms that plus the full 2090-test Gremlin run, this is in good shape. My only real ask before merge is item 1 (a |
📜 License Compliance Check✅ License check passed. See artifacts for full report. License Summary (first 50 lines) |
…lution Address PR #5216 review: add failIfNoTests=true to gremlin-it surefire so a mis-wired dependenciesToScan (unattached test-jar, classifier mismatch) fails loudly instead of passing with zero tests while the gremlin module hard-skips its own. Resolve VectorGremlinIT's fixture by streaming the classpath resource to a temp file, so it works whether getResource returns a file: or jar: URL. Point developers at gremlin-it for running individual gremlin tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed in 3e802ed. 1. CI green with zero Gremlin tests (the merge ask) - Fixed. Added 2. VectorGremlinIT jar-URL trap - Fixed. Replaced 3. Hand-mirrored dependency list - The 4. Deferred Maven-consumer isolation - Agreed it's the right follow-up to prioritize; tracked in #5217 (mark gremlin's 5. DX regression running a single gremlin test - Added a pointer in Thanks for the four rounds - the graphql/postgresw catch in particular was a genuine regression that |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
🟢 Coverage ∅ diff coverage · -7.77% coverage variation
Metric Results Coverage variation ✅ -7.77% coverage variation Diff coverage ✅ ∅ diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (3debfe4) 138241 103817 75.10% Head commit (42cdff0) 170162 (+31921) 114571 (+10754) 67.33% (-7.77%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#5216) 0 0 ∅ (not applicable) Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
Review: engine ANTLR 4.13.2 + Gremlin ANTLR relocation Reviewed the full diff against What I like
Points worth considering
Minor
Test coverage |
📜 License Compliance Check✅ License check passed. See artifacts for full report. License Summary (first 50 lines) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5216 +/- ##
============================================
- Coverage 66.10% 65.41% -0.70%
+ Complexity 917 916 -1
============================================
Files 1698 1698
Lines 138241 138261 +20
Branches 29659 29665 +6
============================================
- Hits 91391 90437 -954
- Misses 34464 35498 +1034
+ Partials 12386 12326 -60 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
… shading Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… tests against shaded jar Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…cation via gremlin-it Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Regenerates SQL/Cypher parsers to v4 ATN so the engine converges with the antlr-runtime 4.10+ pulled by Spring 6 / Hibernate 6, resolving the 'Could not deserialize ATN with version 3' clash for embedded engine users. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ed jar Keeps TinkerPop's precompiled 4.9.1 parser on its required v3 ATN runtime while letting the engine ship plain antlr 4.13.2, so both coexist on one classpath. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…coexistence New arcadedb-gremlin-it module runs AntlrCoexistenceIT against the shaded gremlin jar (relocated antlr 4.9.1) plus the real engine (antlr 4.13.2). One JVM runs a SQL parse, a Cypher parse, and a Gremlin traversal with no ATN deserialization clash. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Skips test execution in the gremlin module (test-jar still built) and executes the compiled tests in gremlin-it via dependenciesToScan, so engine 4.13.2 and gremlin's relocated 4.9.1 coexist during tests. Also mirrors gremlin's test fixture resources (graphml/graphson/orientdb-export test data, importer-glove.txt, gremlin-server config) into gremlin-it/src/test/resources: several importer/exporter/vector tests load these via getClassLoader().getResource(...) or a hardcoded 'src/test/resources/...' relative path, both of which only resolve correctly when the files are unpacked on disk under gremlin-it rather than packed inside the arcadedb-gremlin tests.jar dependency. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…esources Two follow-up fixes to the antlr 4.13.2 / gremlin antlr 4.9.1 coexistence work (93faa26): - gremlin/pom.xml only skipped surefire (*Test), leaving the parent's inherited failsafe config to run gremlin's *IT classes under -Pintegration (which CI uses) against the unshaded classpath, hanging indefinitely on the antlr version collision. Add a matching failsafe skip. - gremlin-it/src/test/resources held 9 byte-for-byte copies of gremlin's test fixtures. Replace them with a sibling testResources entry pointing at gremlin/src/test/resources, and remove the copies. VectorGremlinIT loaded its fixture via a hardcoded CWD-relative path instead of the classpath, which only worked by coincidence when the fixture was physically duplicated; switch it to classpath resolution (same pattern already used by the other importer ITs) so it works regardless of which module executes it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…haded 4.9.1 Engine now ships antlr4-runtime/antlr4-maven-plugin 4.13.2; the arcadedb-gremlin shaded jar bundles a relocated antlr4-runtime 4.9.1. ATTRIBUTIONS.md reflects both. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n spares its v4 parsers The gremlin-shade uber-jar pulls in a full copy of arcadedb-engine transitively (gremlin -> arcadedb-integration (compile) -> arcadedb-engine (compile)). The org.antlr -> com.arcadedb.gremlin.shaded.org.antlr relocation added to isolate TinkerPop's precompiled 4.9.1 (v3 ATN) parser from the engine's 4.13.2 (v4 ATN) runtime therefore also rewrote the bundled engine SQL/Cypher parser classes (SqlParser, Cypher25Parser, and friends) onto that relocated 4.9.1 runtime. Those parser classes are v4-ATN, generated by 4.13.2, so loading them against the 4.9.1 deserializer throws "Could not deserialize ATN with version 4 (expected 3)". The distribution ships both the plain arcadedb-engine jar (good) and the gremlin shaded jar (broken) in lib/, launched with -cp "lib/*". JVM wildcard classpath expansion order is unspecified, so on some hosts the broken shaded SqlParser loads first and all SQL/Cypher parsing breaks, reintroducing the exact bug this branch's antlr bump was meant to fix. Fix: exclude com.arcadedb:arcadedb-engine from the shade artifactSet so the org.antlr relocation only ever touches TinkerPop's own parser. Engine classes keep coming from a single correct copy: the plain arcadedb-engine jar in the distribution lib/, and an explicit test-scope dependency in arcadedb-gremlin-it. Scoped to engine only - integration and network are not shipped as separate distribution jars, gremlin's own com.arcadedb.integration.* importer classes need their bundled copy, and neither carries an antlr parser, so excluding them would break gremlin import without fixing anything. Also drops stray issue-number references from a few comments, per repo convention that comments state behavioral invariants without issue tags. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…mlin-it build ordering Address PR #5216 review: null-check the classpath resource and resolve it via new File(url.toURI()) so paths with spaces/special chars decode correctly; note in the gremlin-it POM that it consumes gremlin's package-phase outputs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…fy-vs-test build Address PR #5216 review: promote gremlin.version to the parent so arcadedb-gremlin-it references the same TinkerPop version as arcadedb-gremlin instead of a hardcoded 3.8.1 that would silently drift on a bump. Document that a full reactor run needs mvn verify/ install (not bare mvn test) because gremlin-it consumes gremlin's package-phase artifacts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…4.13.2 coexists The engine bump put antlr 4.13.2 on the graphql and postgresw test classpaths, where the plain arcadedb-gremlin jar's antlr 4.9.1 won mediation and broke the engine's v4 SQL/Cypher parsers (Could not initialize SQLLexer/Cypher25Lexer). Switch both modules to the shaded gremlin jar (relocated antlr, wildcard-excluded) so only engine's 4.13.2 is the real antlr. Also: strengthen AntlrCoexistenceIT's Cypher assertion (verify the returned name, not just hasNext); add maven.install.skip and a dependency-coupling note to gremlin-it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…lution Address PR #5216 review: add failIfNoTests=true to gremlin-it surefire so a mis-wired dependenciesToScan (unattached test-jar, classifier mismatch) fails loudly instead of passing with zero tests while the gremlin module hard-skips its own. Resolve VectorGremlinIT's fixture by streaming the classpath resource to a temp file, so it works whether getResource returns a file: or jar: URL. Point developers at gremlin-it for running individual gremlin tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
3e802ed to
8091e48
Compare
Review: engine ANTLR 4.13.2 + Gremlin ANTLR relocation (#5216)Thorough, well-reasoned change. The problem (v3-vs-v4 ATN clash when the engine is embedded in Spring 6 / Hibernate 6) is real, the two-runtime-coexistence approach is the standard fix, and the design/plan docs plus commit-by-commit narrative make the reasoning easy to follow. I verified the encapsulation claim: Correctness / build
Developer experience (documented, but worth emphasizing)
Deferred item worth reconsidering
Minor / nits
Test coverageGood: Overall a clean solution to a genuinely awkward dependency problem, with unusually good documentation. My only merge-blocker-adjacent suggestion is the automated shaded-jar-content assertion, given how easily that invariant regressed once already. |
📜 License Compliance Check✅ License check passed. See artifacts for full report. License Summary (first 50 lines) |
PR Review: engine ANTLR 4.13.2 + Gremlin ANTLR relocationThorough, well-documented piece of build engineering. The core strategy (bump the engine to a plain unshaded antlr 4.13.2, keep TinkerPop's precompiled v3 parser working by relocating A few things worth considering: 1.
|
📜 License Compliance Check✅ License check passed. See artifacts for full report. License Summary (first 50 lines) |
Sub-issue of #4647. Fixes #5208.
Problem
arcadedb-engineshipped ANTLR parsers generated with antlr 4.9.1 (ATN serialized in "version 3" format). Embedded in a Spring 6 / Hibernate 6 app, those frameworks pull antlr-runtime 4.10+ (reads only "version 4" ATN). Whichever runtime wins on the classpath breaks the other side:TinkerPop 3.8.1 (Gremlin) is hard-locked to antlr 4.9.1 because it ships a precompiled parser (v3 ATN) that cannot be regenerated without forking TinkerPop, so a single antlr version cannot serve both the engine and Gremlin.
Approach
org.antlr→com.arcadedb.gremlin.shaded.org.antlrinside the shaded jar - so TinkerPop's precompiled 4.9.1 (v3) parser keeps its runtime and never collides with the engine's 4.13.2. Onlyorg.antlris relocated; the TinkerPop grammar package is untouched.arcadedb-gremlin-itmodule runs the existing gremlin test suite (via the test-jar + surefire/failsafedependenciesToScan) plus a newAntlrCoexistenceITagainst the shaded jar, so the two antlr runtimes are exercised together in one JVM. The gremlin module itself skips test execution (still builds its test-jar) because engine-v4 + gremlin-v3 cannot share one unshaded classpath.Resulting classpath (shipped distribution)
org.antlr.v4.runtimecom.arcadedb.gremlin.shaded.org.antlr.v4.runtimeThe gremlin uber-jar excludes
arcadedb-enginefrom itsartifactSetso the relocation only ever rewrites TinkerPop's parser - not the engine's own v4 parsers (which the distribution provides as a plain jar). Without this, a bundled engine parser would be relocated onto the incompatible 4.9.1 runtime and could win under-cp lib/*, reintroducing the ATN error.Verification
org/antlrleak.AntlrCoexistenceIT: engine SQL + Cypher parse (4.13.2) and a Gremlin traversal (relocated 4.9.1) succeed in one JVM.gremlin-it: 2090 tests, 0 failures / 0 errors.SqlParser.classpresent in exactly onelib/jar (plain engine).mvn clean install -DskipTests: BUILD SUCCESS across all modules.Deferred (documented in the design spec)
Maven-coordinate consumer isolation (
<optional>on gremlin's antlr) for third-party embedders who depend on botharcadedb-gremlinandarcadedb-enginedirectly. Does not affect the shipped distribution or the reporter's engine-only embedding.Design spec and implementation plan:
docs/superpowers/specs/2026-07-10-gremlin-antlr-shading-engine-bump-design.md,docs/superpowers/plans/2026-07-10-gremlin-antlr-shading-engine-bump.md.🤖 Generated with Claude Code