Up to standards ✅

_{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.}

Code Review

The core fix is correct and addresses a real SQL injection vulnerability. Here are my findings:

Security Fix - Correct

The parameterized query approach properly eliminates the injection vector. The colon-prefixed key format (":p0") in the params map is consistent with the existing convention in the codebase (e.g., QueryTest.java uses params.put(":name", "Jay")), and NamedParameter.getValue() handles this format in its first check (params.containsKey(":" + key)).

Minor: bindFromInputParams cannot resolve colon-prefixed keys

NamedParameter.bindFromInputParams() checks params.containsKey(paramName) (without colon), then falls back to the positional index. With :p0 as the map key, the fallback params.get("0") returns null. This only affects EXPLAIN/query plan display - runtime execution is unaffected - but it is an inconsistency worth knowing about.

The existing tests in QueryTest.java use colon-prefixed keys too, so either the EXPLAIN path is already broken there or it is not exercised. No action required for this PR, but worth tracking.

Minor: Remaining injection surface not addressed in this PR

RemoteVertex.java still concatenates user-supplied strings into queries in three places:

• countEdges() (line 57-59): edgeTypes[i] is inserted with single-quote delimiters
• isConnectedTo(toVertex, direction, edgeType) (line 142): edgeType is inserted with single quotes
• fetch() (line 256): types[i] is inserted with single quotes

These deal with edge-type names (schema identifiers) rather than property values, so the exploitability is lower. However, a follow-up issue to apply the same parameterization (or at least backtick-quoting for identifiers) would be a good hardening step.

Minor: Test design - indirect exception path

The tests rely on Optional.empty().get() throwing NoSuchElementException to capture the mock invocation after command() is called. This works, but it ties test correctness to the internal post-command code path. A cleaner alternative is to configure the mock to return a properly stubbed edge so the test can also verify the returned MutableEdge.

Minor: Test gaps in map-form test

newEdgeWithMapPropertiesUsesParameterBinding only asserts containsValue("it's a test") without verifying the parameter key (e.g., containsEntry(":p0", "it's a test")). The other four tests do verify specific keys. Adding that assertion here would make the map-form coverage symmetric.

Nit: Documentation file

docs/4327-remotevertex-newedge-sql-injection.md is a PR-specific analysis document. Once merged, it is orphaned from the issue tracker and contributes noise to the docs directory. The same information belongs in the commit message or the issue itself. Consider removing this file from the PR.

Nit: Raw-type suppressions

Several test methods use ArgumentCaptor<Map> (raw type) with @SuppressWarnings("unchecked"). The annotation should be @SuppressWarnings({"unchecked", "rawtypes"}) since both warnings are generated, or the code could use ArgumentCaptor.forClass(Map.class) with an explicit cast at the assertion site.

Summary

The security fix is sound. The implementation is clean and minimal. The main action items are: (1) remove the documentation file, (2) optionally strengthen the map-form test assertion, and (3) open a follow-up issue for the remaining edge-type injection surfaces.

Code Review

Summary

This PR correctly fixes a real SQL injection vulnerability in RemoteVertex.newEdge() (#4327). The approach - replacing string interpolation with named SQL parameters - is the right solution.

Positive

• Correct fix: The :p0 key convention is confirmed correct by NamedParameter.getValue() which checks params.get(":" + key) first (line 58 of NamedParameter.java), so colon-prefixed keys work as expected.
• Both call forms covered: The pair-list form and the map form both go through the same parameterised path.
• Tests verify the contract: The 5 new tests confirm that apostrophes never appear in the SQL string and that values are passed as parameters.

Issues

1. Docs file should not be committed (blocking)

docs/4327-remotevertex-newedge-sql-injection.md is a PR tracking document containing internal review-cycle notes, working-tree status, and Gemini comment rebuttals. This is not project documentation - it belongs in the PR description (where it largely already exists), not committed to the repository. Please remove this file before merging.

2. Same injection pattern remains in other methods (non-blocking, worth tracking)

The fix covers newEdge(), but several other methods in the same class concatenate caller-supplied strings into SQL with single-quote wrapping - the same root cause as the original bug:

• countEdges() (line ~56-59): edgeTypes values appended as 'value'
• isConnectedTo(toVertex, direction, edgeType) (line ~142): edgeType appended as 'value'
• fetch() (line ~253-257): types array elements appended as 'value'

These are lower-severity since type/edge-type names are usually developer-controlled, but the pattern is the same. Consider filing a follow-up issue.

3. No integration test against a real server (non-blocking)

The test suite mocks RemoteDatabase and relies on assertThatThrownBy (the mock returns Optional.empty() so get() throws) to capture the command() call before the exception. This verifies the parameter binding contract but does not exercise the query against an actual ArcadeDB server. A follow-up integration test would add confidence.

Minor / Style

• params is initialised as new HashMap<>() unconditionally even when there are no properties. Moving the initialisation inside the if block would be cleaner.
• @SuppressWarnings("unchecked") appears on every test method due to ArgumentCaptor<Map> being a raw type - acceptable, but avoidable with a typed cast pattern.
• The assertThatThrownBy pattern is a setup mechanism (the mock returns an empty Optional). A brief inline comment explaining why the exception is expected would help future readers understand the intent.

Verdict

The security fix is correct and well-targeted. The committed docs file is the only item that should be addressed before merge.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.11%. Comparing base (c4cf514) to head (b58ba41).

@@             Coverage Diff              @@
##               main    #4340      +/-   ##
============================================
- Coverage     64.21%   64.11%   -0.10%     
+ Complexity      448      422      -26     
============================================
  Files          1645     1645              
  Lines        127540   127539       -1     
  Branches      27350    27348       -2     
============================================
- Hits          81899    81777     -122     
- Misses        34102    34244     +142     
+ Partials      11539    11518      -21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.