fix(#7461): IMPORT DATABASE reported result=OK for the one failure it means to report as FAIL - #7493

No actionable comments were generated in the recent review. 🎉

Merge Risk: ⚪ Minimal · up to a2e69

The change preserves failed import results and regression coverage passed; no current merge-blocking risk is identified.

• Fix all pre-merge checks with AI

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

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

Review

I traced the fix against Importer.java, SourceDiscovery.java, and ImporterSettings.java to verify the PR's claims rather than take them at face value, and they hold up.

Correctness

• The root-cause fix is right: result=OK now only gets set right after setPropertiesFromMap(statistics) inside the success path, so it can no longer clobber a FAIL written in the InvocationTargetException handler. Matches the shape BackupDatabaseStatement already uses.
• Importer.load() really does throw new IllegalArgumentException(e) when probeOnly is set (line 98), and ImporterSettings.parseParameter really does throw the same exact type for a refused onRowError value (line 191) - confirming both producers named in the PR description, not just the one the linked issue calls out.
• openLocalStream (SourceDiscovery.java:216) does throw a bare-path FileNotFoundException, and IllegalArgumentException(Throwable)'s message is cause.toString(), so failureReason()'s choice to read the wrapper's own message (not the cause's) is correct and is exactly what the new test asserts (aFailedProbeReportsFailWithAReason).
• Hoisting e.getTargetException() into a null-guarded target local is a genuine (if narrow) latent-bug fix on top of the refactor: the old code called e.getCause().getClass() unguarded, which would NPE instead of falling through to the security-exception path if getCause() ever returned null. Low real-world likelihood since InvocationTargetException always sets its target, but the guard is free and correct.
• Keeping the exact class-name comparison instead of instanceof is well-justified and pinned by aNonNumericNumericSettingStillThrows, so a future "helpful" widening to instanceof will fail loudly instead of silently swallowing NumberFormatException (and other subclasses) into rows.

Tests

• Good coverage: both FAIL producers, the still-throws paths (genuine import failure, non-numeric setting), and a plain successful import for regression safety. Uses AssertJ assertThat consistent with the project's testing conventions.
• aSuccessfulProbeStillReportsOk and anUnusableSettingValueReportsFailWithAReason both assert on db.getSchema().existsType("Document") to prove the import didn't actually run, not just that the status code looks right - that's the kind of check that catches a fix which happens to return the right string for the wrong reason.
• Reasonable to skip the two server ITs locally given the port-2480 conflict, but worth confirming they're actually green in CI before merge since the PR didn't observe them directly.

Nits (non-blocking)

• reason is a new property key with no precedent elsewhere in the tree (setProperty("reason" has exactly one hit, the new one). Not a problem for this PR, but if a future statement wants to report similar in-band failure detail, it'd be worth keeping the key name consistent with whatever this establishes.
• The in-code comment blocks are quite long (multi-paragraph, all-caps headers) - consistent with the rest of this file's existing style, so not something to change here, but worth keeping in mind that this file is trending toward comment-heavy for future edits.

Out of scope, correctly deferred

• The startup import: path (ArcadeDBServer.loadDefaultDatabases) still drains the result set without reading it, so this fix doesn't help there. Correctly filed separately as Startup 'import:' default-database command discards the statement's result row, so a failed import is silent #7484 rather than scope-crept into this PR.
• The commitEvery (throws) vs onRowError (returns FAIL row) asymmetry is pre-existing, documented, and intentionally not unified here - agreed that's a separate contract decision.

Overall this is a well-scoped, well-verified fix. No security or performance concerns - the SSRF/LFI guard's rethrow loop is unchanged in semantics, and no new dependencies were introduced.

CI: the branch's own tests pass; both red lanes are inherited

Posting the analysis so nobody has to redo it. The new test ran in CI and passed - run 34657385125's unit-test-reports artifact carries TEST-com.arcadedb.integration.importer.Issue7461ImportDatabaseFailResultTest.xml with tests="6" errors="0" skipped="0" failures="0".

unit-tests - three failures, pre-existing on the base branch. Filed as #7495.

Evidence rather than inference: the unrelated PR #7489 (RDF importer only, run 34657353357) fails on exactly the same three report files, and locally MultiColumnAggregationResultTest fails identically with ImportDatabaseStatement.java restored to its HEAD content. Neither branch touches time series or Raft. main never runs this workflow, so a regression merged there first appears on somebody else's PR.

integration-tests - one failure, a hardcoded-port collision. Filed as #7496.

Issue7304GrpcAdminLeaderRoutingIT errored in @BeforeEach for both methods, before any test code ran: Failed to bind to address 0.0.0.0/0.0.0.0:51142 ... Address already in use. That class hardcodes BASE_GRPC_PORT = 51141, so 51142 is its own node 1. The same lane fails on unrelated PRs with a different victim each run (PR #7491, run 34656829906: Issue6070GraphBatchLoadLeaderGuardIT, TimeSeriesGrpcForwardedInsertTypeIT, ServerProfilingIT).

I did not re-run either lane to chase green: the three unit-test failures are deterministic on the base, and the gRPC one would only be another roll of the same dice.

Review

Solid, well-scoped fix. result=OK was indeed unconditionally overwriting result=FAIL in ImportDatabaseStatement.executeSimple (engine/src/main/java/com/arcadedb/query/sql/parser/ImportDatabaseStatement.java), and moving the OK assignment into the success path immediately after setPropertiesFromMap is the right shape - it matches BackupDatabaseStatement and removes the dead branch entirely rather than patching around it.

Correctness

• Confirmed there's no key collision: ImporterContext.toMap() never emits result or reason, so folding statistics in before setting result/reason is safe.
• Confirmed via grep -rn 'getProperty("result")' that no production code branches on this property today, so making FAIL observable can't silently change existing caller behavior.
• Hoisting e.getTargetException() into a null-guarded target local is a genuine (if minor) improvement - e.getCause() was being dereferenced unchecked right next to a separate e.getTargetException() call on the same underlying value.
• Keeping the exact-class-name comparison ("IllegalArgumentException".equals(target.getClass().getSimpleName())) instead of instanceof is the right call given NumberFormatException reaches the same catch block from ImporterSettings.parseParameter - widening it would silently swallow those as rows instead of throwing. Good catch that this needed a dedicated regression test (aNonNumericNumericSettingStillThrows) rather than just a comment.
• failureReason() correctly prefers the wrapper exception's own message over the cause's - important because Importer.load() builds the probe failure as new IllegalArgumentException(cause), whose message already includes the cause's toString() (type + text), while the cause itself (e.g. FileNotFoundException) often carries only a bare path.

Test coverage
Good coverage of both producers of the branch (probeOnly failure and the setSettings/onRowError failure), plus regression tests pinning that NumberFormatException and non-probe ImportException failures still throw rather than becoming rows. The tests assert on root-cause type in addition to message text, which avoids false positives from an unrelated guard that happens to produce the same top-level exception type.

Minor/non-blocking notes

• reason is a new property with no precedent elsewhere in the result-row shape used by BackupDatabaseStatement/ExportDatabaseStatement. Not a problem for this PR, but worth keeping in mind if a future statement wants the same "FAIL + reason" shape - might be worth extracting a small convention/helper at that point rather than re-deriving it.
• The known residual asymmetry - onRowError = 'bogus' returns a FAIL row while commitEvery = 'abc' throws - is inherited behavior, correctly called out in the code comment, and each side is pinned by its own test, so this isn't something this PR needs to fix.
• The startup import: default-database path still discards the result set entirely (filed as Startup 'import:' default-database command discards the statement's result row, so a failed import is silent #7484), so this fix doesn't yet help that caller. That's fine as a documented out-of-scope gap rather than a silent one.

Security: The SSRF/LFI guard rethrow loop (for (Throwable cause = target; ...) if (cause instanceof SecurityException se) throw se;) is unchanged in semantics, just reading through the new local - still short-circuits ahead of the FAIL branch, so a security violation still surfaces as an exception (mapping to HTTP 403) rather than being swallowed into a row.

Performance: No concerns - this is exception-handling/result-shaping code on an already-slow-path (import), not a hot path.

Nothing blocking. Nice, tightly scoped fix with a strong regression suite.

Review

Went through the diff (ImportDatabaseStatement.java, the new Issue7461ImportDatabaseFailResultTest, and the tracking doc) plus traced the two producers (Importer.load() and ImporterSettings.parseParameter) against SourceDiscovery/ImporterSettings to confirm the claims in the PR body rather than take them at face value.

Correctness

• The core fix is right: moving result.setProperty("result", "OK") into the try block immediately after setPropertiesFromMap(statistics), and removing the unconditional assignment below the outer try, is exactly what makes the FAIL branch observable again. Confirmed the previous code truly discarded it (the unconditional result.setProperty("result", "OK") at the old bottom ran regardless of which catch fired).
• Verified independently (not just via the PR's grep) that ImporterSettings.parseParameter throws a bare IllegalArgumentException for onRowError and Integer.parseInt(value) (→ NumberFormatException, a subclass) for commitEvery — so the "second producer" claim and the "must stay exact class-name match, not instanceof" reasoning both hold up.
• Confirmed ImporterContext.toMap() emits no result/reason key, so folding statistics in before the outcome can't collide with the new reason property.
• failureReason() reading failure.getMessage() (not the cause's) is correct given Importer.load() builds the probe failure as new IllegalArgumentException(cause), whose message is already cause.toString().
• Hoisting e.getTargetException() into a null-checked target local fixes a real (if narrow) unchecked-getCause() bug and is behaviorally a no-op for the SecurityException rethrow loop.

Minor / non-blocking

• "IllegalArgumentException".equals(target.getClass().getSimpleName()) is pre-existing (not introduced here) but it's comparing a simple name string rather than target.getClass() == IllegalArgumentException.class. Since you're already touching this line, an exact-class comparison would express the same "no subclasses" intent slightly more robustly and a bit cheaper — not blocking, just noting it since the surrounding lines were rewritten anyway.
• The reason property is new and has no precedent elsewhere in the tree (already flagged as a non-blocking note in the tracking doc's own review cycles) — worth keeping in mind if a future statement wants the same in-band-failure shape, so the key name/convention doesn't drift.

Tests

Good coverage and good discipline: each test pins more than just the result string (root-cause exception type, schema untouched to prove a probe didn't actually import, reason text on both halves) which rules out the classic "green for the wrong reason" failure mode. The two regression guards (aGenuineImportFailureStillThrows, aNonNumericNumericSettingStillThrows) are exactly the tests that would catch a future attempt to widen the instanceof/class check. I was not able to execute mvn in this review sandbox to re-run the suite myself, so I'm relying on the PR's reported local run (6/6 passing) and the CI artifact reference for Issue7461ImportDatabaseFailResultTest; the logic traces correctly by inspection.

Security / performance

No concerns. The reason string surfaces to the same caller who supplied the URL/settings that caused the failure, so there's no new information disclosure beyond what that caller already controls. No hot-path or allocation changes — this is exception-handling logic only.

Docs

The docs/7461-*.md tracking file is consistent with the existing convention in this repo (docs/69xx-*.md, docs/72xx-*.md, etc.), so no concern there.

Nice, tightly-scoped fix with a clear invariant and tests that actually pin the behavior rather than just the happy path. Nothing blocking from my pass.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.01%. Comparing base (d3d9a48) to head (a2e694a).
⚠️ Report is 20 commits behind head on main.

@@           Coverage Diff           @@
##             main    #7493   +/-   ##
=======================================
  Coverage   74.00%   74.01%           
=======================================
  Files        1957     1957           
  Lines      178988   178988           
  Branches    37784    37784           
=======================================
+ Hits       132469   132476    +7     
+ Misses      31386    31385    -1     
+ Partials    15133    15127    -6     

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

Superseded by #7497, which implemented all five issues (#7341, #7342, #7345, #7347, #7461) independently and merged first, and by #7498, which carries the four follow-ups this branch only filed (#7487, #7488, #7490, #7494). Main is ahead of this branch, so merging it would regress that work.

Closing rather than resolving the conflicts. The regression tests on this branch are NOT superseded - they cover considerably more than what landed - and porting them onto the merged implementation's API is tracked separately.