Skip to content

[Feature][Connector-V2] Add Neo4j source connectivity dry-run - #12433

Open
goutamadwant wants to merge 1 commit into
apache:devfrom
goutamadwant:feature/neo4j-source-connect-dry-run
Open

goutamadwant wants to merge 1 commit into
apache:devfrom
goutamadwant:feature/neo4j-source-connect-dry-run

Conversation

@goutamadwant

Copy link
Copy Markdown
Collaborator

Purpose of this pull request

Relates to #10681. This is one connector slice, not completion of the umbrella issue.

Neo4j sources currently report connectivity as SKIPPED under --dry-run connect. This adds a source-only implementation of the existing SupportSourceDryRunValidation interface.

  • Share pure runtime configuration/catalog-table parsing for single-table and multi-table sources.
  • Create a temporary driver from the existing runtime connection/authentication configuration.
  • Call verifyConnectivityAsync() without creating a session or executing configured Cypher.
  • Bound verification to at most 15 seconds, retain smaller positive connection timeouts, and initiate driver cleanup on both success and failure with a five-second close wait.
  • Preserve interruption and omit raw URIs, driver text and secret-bearing causes from validation errors.
  • Update both connector documentation and the central supported-connectors table in English and Chinese.

No changes to DryRunConnectValidator, the shared SPI, or later dry-run layers.

Does this PR introduce any user-facing change?

Yes. Previously, --dry-run connect skipped Neo4j source connectivity. It now performs the scoped checks above and fails on connection/authentication errors instead of reporting the source as skipped. Configured output schemas are reused without creating a source reader or submitting a job.

Normal source execution, existing option names/defaults and sink behavior remain unchanged. No new dependencies or breaking changes are introduced.

Scope: This verifies driver connectivity and the authentication performed during its handshake, not database existence, database/query permissions, Cypher syntax or result-field/schema compatibility. An invalid query or nonexistent configured database can pass this connectivity-only check. DNS resolution remains governed by the JVM.

How was this patch tested?

  • The new factory-capability regression failed on the unchanged baseline.
  • 34 connector unit tests, covering schema/runtime parity and adjacent existing behavior, plus failure sanitization, interruption and resource cleanup.
  • Five real-service integration tests in the existing Neo4jIT, including the CLI connect command, authentication failure and multi-table schemas, unchanged graph data and the database/query-validation boundary.
  • Java 8 and Java 11 validation; focused commands below select only this connector's unit and dry-run integration tests.
  • Spotless and whitespace checks.
  • Full repository ./mvnw -q -DskipTests verify passed on Java 11. This compiles/packages the repository; it is not a claim that all repository tests ran.
./mvnw -B \
  -pl seatunnel-e2e/seatunnel-connector-v2-e2e/connector-neo4j-e2e -am \
  '-Dtest=*Neo4j*Test' -Dsurefire.failIfNoSpecifiedTests=false \
  -DskipIT=false '-Dit.test=Neo4jIT#testDryRun*' \
  -DfailIfNoTests=false verify

The Java 11 run builds the complete selected dependency reactor. Java 8 rebuilds the connector and relevant dependencies and uses the current test harness with the previously built unrelated engine starters. CLI coverage is in-process argument parsing and command execution against live containers, not execution of the packaged shell launcher. Live TLS and the full multi-engine job matrix were not tested.

Check list

  • No new JAR dependencies or license notices required.
  • English and Chinese connector and CLI capability documentation updated.
  • No incompatible change; incompatible-changes.md update is not required.
  • Existing connector integration tests extended. Plugin mapping, distribution, CI labels and plugin installation configuration are unchanged because no connector is added.

@DanielLeens DanielLeens left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What Problem Does This PR Solve?

  • User pain point: Before this PR, the --dry-run connect (Layer 1) preflight command already existed as a framework (SupportSourceDryRunValidation SPI, used by Kafka/JDBC/S3File/FakeSource), but the Neo4j source connector did not implement it. A user pointing a Neo4j job at a bad URI, wrong credentials, or an unreachable cluster would only find out at full job submission time, and --dry-run connect silently reported Neo4j sources as SKIPPED instead of actually checking anything.
  • Fix approach: Neo4jSourceFactory now implements SupportSourceDryRunValidation. inferSchemaForDryRun reuses the exact same config-parsing path the runtime source uses (via a new shared parseSourceConfig/SourceConfiguration helper) so the schema reported to downstream dry-run checks is guaranteed identical to what the real source produces. validateConnectionForDryRun builds a short-lived Driver through a new Neo4jSourceDryRunValidator and calls verifyConnectivityAsync() with a bounded timeout, then always closes it — without ever opening a session or executing the configured Cypher.
  • One-sentence summary: Adds a safe, bounded, opt-in connectivity/schema preflight check for the Neo4j source that mirrors runtime schema construction but never touches graph data.

1. Code Change Review

1.1 Core Logic Analysis

Neo4jSourceFactory.java (seatunnel-connectors-v2/connector-neo4j/.../source/Neo4jSourceFactory.java)

Before, createNeo4jSource(ReadonlyConfig) directly built a Neo4jSource from either a single-table or tables_configs config. After, the parsing logic was extracted verbatim into a private parseSourceConfig(ReadonlyConfig) that returns a new SourceConfiguration(catalogTables, connectionInfo, tableConfigs) holder, and createNeo4jSource just picks the right Neo4jSource constructor from it:

private Neo4jSource createNeo4jSource(ReadonlyConfig config) {
    SourceConfiguration parsed = parseSourceConfig(config);
    return parsed.tableConfigs == null
            ? new Neo4jSource(parsed.catalogTables.get(0), parsed.connectionInfo)
            : new Neo4jSource(parsed.catalogTables, parsed.connectionInfo, parsed.tableConfigs);
}

This is a pure extract-method refactor; the single-table branch reconstructs the exact call new Neo4jSource(CatalogTableUtil.buildWithConfig(config), new Neo4jSourceQueryInfo(config.toConfig())) that existed before, and the multi-table branch is untouched. Neo4jFactoryTest#dryRunSchemaMatchesRuntimeForSingleAndMultipleTables explicitly asserts the dry-run-inferred schema equals the runtime-created source's schema for both branches, which is good evidence the refactor is behavior-preserving.

The new SPI methods:

public List<CatalogTable> inferSchemaForDryRun(TableSourceFactoryContext context) throws IOException {
    return parseDryRunSourceConfig(context).catalogTables;
}

private SourceConfiguration parseDryRunSourceConfig(TableSourceFactoryContext context) throws IOException {
    try {
        ConfigValidator.of(context.getOptions()).validate(optionRule());
        return parseSourceConfig(context.getOptions());
    } catch (RuntimeException e) {
        throw new IOException("Neo4j connect dry-run source configuration or schema is invalid");
    }
}

public void validateConnectionForDryRun(TableSourceFactoryContext context, List<CatalogTable> catalogTables) throws Exception {
    Neo4jSourceDryRunValidator.validate(parseDryRunSourceConfig(context).connectionInfo.getDriverBuilder());
}

Each call to parseDryRunSourceConfig builds a brand-new Neo4jSourceQueryInfo, which in turn builds a brand-new DriverBuilder (see Neo4jQueryInfo.prepareDriver). That matters because validateConnectionForDryRun mutates the DriverBuilder it receives (see below) — since it's a fresh instance every time and is never the same object used by createNeo4jSource, the mutation cannot leak into the real runtime source's driver settings. This was worth tracing carefully since DriverBuilder is a plain mutable @Getter @Setter bean with no defensive copy, and it worked out correctly.

Neo4jSourceDryRunValidator.java (new file)

static void validate(DriverBuilder builder) throws Exception {
    if (Thread.currentThread().isInterrupted()) {
        throw new InterruptedException("Neo4j connect dry-run interrupted");
    }
    ...
    long timeout = configuredTimeout == null || configuredTimeout == 0
            ? MAX_TIMEOUT_SECONDS
            : Math.min(configuredTimeout, MAX_TIMEOUT_SECONDS);
    builder.setMaxConnectionTimeoutSeconds(timeout);
    builder.setMaxTransactionRetryTimeSeconds(0L);
    driver = builder.build();
    driver.verifyConnectivityAsync().toCompletableFuture().get(timeout, TimeUnit.SECONDS);
    ...

Runtime path traced: verifyConnectivityAsync() performs the driver's Bolt handshake and auth verification without opening a Session or running any Cypher, matching the "never executes configured Cypher" contract in the class Javadoc and the SPI contract in SupportSourceDryRunValidation. This claim is backed by a real E2E assertion (Neo4jIT#testDryRunDoesNotExecuteConfiguredCypher) that plants a sentinel node, runs the dry-run with a destructive DELETE query configured, and asserts the sentinel is untouched — a genuinely strong proof, not just a unit-test mock assertion.

Resource/interrupt handling in the finally block is careful: it clears/re-applies the interrupt flag around the bounded closeAsync().get(CLOSE_TIMEOUT_SECONDS, ...) call so a close-after-interrupt doesn't immediately fail, then restores Thread.currentThread().interrupt() before returning. This mirrors the interrupt-handling idiom already used by the sibling KafkaSourceDryRunValidator, so it's consistent with established patterns in this codebase rather than a one-off.

Runtime path (text flow, since this touches a remote-call/async-timeout pattern):

DryRunConnectValidator.validateSource
  -> Neo4jSourceFactory.inferSchemaForDryRun (parses config, builds catalog tables, brand-new DriverBuilder #1)
  -> Neo4jSourceFactory.validateConnectionForDryRun (parses config AGAIN, brand-new DriverBuilder #2)
       -> Neo4jSourceDryRunValidator.validate(DriverBuilder #2)
            -> builder.build() -> org.neo4j.driver.Driver (pool size 1, connect timeout capped at 15s)
            -> driver.verifyConnectivityAsync().get(<=15s)   // handshake + auth only, no session
            -> finally: driver.closeAsync().get(<=5s)        // always attempted, even on failure/timeout
  -> on success: schema trusted, downstream transform/sink schema checks proceed
  -> on failure: DryRunConnectValidator.wrap() sanitizes e.getMessage() and reports ConfigCheckException

Key Findings:

  1. The refactor of createNeo4jSource into parseSourceConfig/SourceConfiguration is behavior-preserving and is proven so by both a new unit test comparing dry-run vs. runtime schema and the pre-existing multi-table test.
  2. validateConnectionForDryRun always operates on a freshly parsed, independent DriverBuilder/Driver, so mutating it for preflight purposes (forcing maxConnectionTimeoutSeconds<=15, maxTransactionRetryTimeSeconds=0) cannot affect the real source's connection settings at job runtime.
  3. verifyConnectivityAsync() is the correct, minimal-footprint driver call for this purpose — it does not open a Session, matching both the SPI contract and the connector's own Javadoc claim, and this is proven with a real Testcontainers Neo4j instance, not just mocks.
  4. Timeout and interrupt handling (bounded connect wait, bounded close wait even after failure, careful interrupt-flag save/restore) mirrors the already-merged KafkaSourceDryRunValidator pattern, so it's consistent with the codebase's established idiom for this kind of async dry-run validator.
  5. inferSchemaForDryRun and validateConnectionForDryRun each independently re-parse and re-validate the full config (ConfigValidator.of(...).validate(optionRule()) runs twice, and Neo4jSourceQueryInfo/DriverBuilder are constructed twice) when the framework calls both hooks back-to-back for the same source. This is intentional per the Javadoc ("the connection hook can be called directly, without the preceding schema hook"), but it does mean double config validation work per dry-run source — negligible for a one-shot CLI check, not a runtime hot path.

1.2 Compatibility Impact

Fully compatible. This PR only adds a new optional interface (SupportSourceDryRunValidation) implementation that is exclusively invoked by the opt-in --dry-run connect command path (DryRunConnectValidator.validateSource, gated on factory instanceof SupportSourceDryRunValidation). It does not change:

  • Any existing config option, default value, or optionRule() contract (untouched in this diff).
  • The runtime createSource/createNeo4jSource code path's observable behavior (verified above).
  • Any serialization/state/checkpoint format — the Neo4j source has no split/checkpoint state involved here.
  • Sink behavior — no sink files are touched; docs correctly state "Sink connectivity remains unsupported."

Existing jobs, saved configs, and checkpoints are unaffected because normal job submission never calls into the new code paths.

1.3 Performance / Side-Effect Analysis

  • The preflight driver uses withMaxConnectionPoolSize(1) (pre-existing in DriverBuilder.build()) and is closed unconditionally in finally, so no connection/thread leak across repeated dry-run invocations.
  • Worst-case wall-clock cost per Neo4j source in a dry-run job is bounded: ~15s connect wait + ~5s close wait = ~20s, which is reasonable for a one-shot CLI preflight and matches the order of magnitude of the sibling Kafka (30s) and S3 (5s) validators.
  • No new background threads, timers, or persistent state are introduced; everything is scoped to the single synchronous validate() call.

1.4 Error Handling and Logging

Issue 1 — Underlying failure detail is fully discarded, keeping the user's dry-run error message identical for every failure cause.

  • Location: seatunnel-connectors-v2/connector-neo4j/src/main/java/org/apache/seatunnel/connectors/seatunnel/neo4j/source/Neo4jSourceDryRunValidator.java, the catch (Exception e) block that produces failure = new IOException("Neo4j connect dry-run connectivity check failed; check URI, authentication and TLS settings").
  • Problem: The real Neo4j driver exception (e) — e.g. AuthenticationException, ServiceUnavailableException, TLS handshake failure, DNS resolution failure — is neither wrapped as a cause nor included in the message. A wrong password, an unreachable host, and a TLS misconfiguration all produce byte-for-byte the same error text.
  • Raised by another reviewer: No.
  • Risk: Reduced diagnosability for users troubleshooting a failed preflight; they cannot distinguish "auth failed" from "host unreachable" from the tool's output alone and must fall back to full job logs or external debugging.
  • Context that matters for severity: this looks deliberate, not accidental. Neo4jFactoryTest#dryRunInvalidUriDoesNotEchoCredentials, Neo4jSourceDryRunValidatorTest#closesAfterAuthenticationFailureAndDoesNotExposeDriverText/#cleanupFailureCannotTurnIntoSuccessOrExposeSecrets, and the E2E testDryRunRejectsWrongPasswordWithoutEchoingIt all specifically assert the raw driver exception text (which could plausibly echo back a neo4j://user:secret@host-style URI or other connection text) never reaches the user, and the docs explicitly call this out: "Validation errors omit raw URIs, driver error text and secret-bearing exception causes." I checked whether DryRunConnectFailureMessageSanitizer (used one layer up in DryRunConnectValidator.wrap()) could have done this scrubbing generically instead — it only masks jdbc: URLs and key=value-style credential patterns, so it would not reliably catch every shape of Neo4j driver error text. Given that gap, discarding the driver's raw message is a defensible, tested safety tradeoff rather than an oversight.
  • Best improvement: Non-blocking suggestion — consider classifying into a small, fixed set of safe categories (e.g. "authentication failed", "host unreachable", "timed out") based on the driver's exception type rather than one single generic string, to recover some diagnostic value without risking a raw-text leak. Not required for this PR.
  • Severity: Low (documented, intentionally tested tradeoff; does not affect the pass/fail correctness of the check itself).

2. Code Quality Assessment

2.1 Coding Standards

  • Every new public/protected method (inferSchemaForDryRun, validateConnectionForDryRun, Neo4jSourceDryRunValidator.validate) has a Javadoc or inline comment explaining intent, matching the project's "add comments for important methods (lifecycle hooks, complex logic)" rule.
  • SourceConfiguration is a straightforward private data holder (three final fields, one constructor) — exempt as boilerplate.
  • Minor gap: the private parseDryRunSourceConfig helper has no comment explaining why it swallows RuntimeException into a generic IOException (same intentional secret-avoidance rationale as Issue 1 above). A one-line comment there would help a future maintainer avoid "fixing" it into a leak. Not blocking — Low.
  • New license headers are present and correct on both new files (Neo4jSourceDryRunValidator.java, Neo4jSourceDryRunValidatorTest.java).
  • No wildcard imports, no System.out.println, no shaded-dependency violations spotted in the diff.

2.2 Test Coverage and Test Stability

Coverage is strong and well-targeted:

  • Unit tests (Neo4jSourceDryRunValidatorTest) cover: success path with the exact expected timeout/retry overrides, non-echoing of secrets on connectivity failure, timeout with a stalled handshake, negative-timeout rejection before any driver is created, pre-interruption short-circuit, mid-verification interruption with closeAsync still invoked, and close-failure-cannot-mask-as-success.
  • Neo4jFactoryTest additions cover: SPI implementation marker, dry-run schema parity with runtime schema for both single-table and tables_configs modes (using deliberately destructive/invalid Cypher to prove it's never executed), and invalid-URI-does-not-echo-credentials.
  • E2E (Neo4jIT) additions are the strongest evidence here: a real Testcontainers Neo4j is used to prove (a) a destructive configured query is never executed during dry-run, (b) multi-table dry-run doesn't execute any configured query, (c) a wrong password is rejected without appearing in the exception text, (d) the dry-run explicitly does not falsely validate a nonexistent database (an important honesty check, since it would be easy to over-claim coverage here), and (e) the actual --dry-run connect CLI command (SeaTunnelConfValidateCommand/ClientCommandArgs) runs end-to-end successfully against a real container, not just the SPI methods in isolation.
  • Diff review of Neo4jFactoryTest.java confirms all changes are additive — no existing assertion was weakened, removed, or had its tolerance loosened.
  • Stability rating: Stable. No new sleeps, fixed wall-clock waits, or timing-sensitive polling were introduced; bounded async waits use real completion futures against a Testcontainers instance, consistent with the rest of this IT's existing style (awaitJobFinish, etc., unchanged).

2.3 Documentation Updates

docs/en and docs/zh for both connectors/source/Neo4j.md and engines/zeta/user-command.md were updated together and are consistent with each other and with the implementation:

  • New "Connectivity dry-run" sections (English doc and its Chinese-language counterpart) accurately state what is and is not validated (driver connectivity + handshake auth, not database existence, permissions, Cypher syntax, or schema-vs-data compatibility) — matches the E2E test's explicit "does not claim database validation" check.
  • Timeout/close-wait bounds (15s / 5s) are documented and match the constants in the validator.
  • The user-command.md connector support matrix row for Neo4j is added in both languages with a working anchor link to the new doc section.
  • Secret-handling behavior is explicitly documented, matching Issue 1's finding that this is intentional rather than an oversight.

3. Architectural Soundness

3.1 Elegance of the Solution

Precise fix / long-term solution. This is not a workaround: it implements the exact SPI (SupportSourceDryRunValidation) that the framework already defines and that three other connectors (Kafka, JDBC, S3File) already implement, following the same file-per-validator, bounded-timeout, interrupt-safe pattern as KafkaSourceDryRunValidator. The schema-parity refactor (parseSourceConfig/SourceConfiguration) is a clean way to guarantee the dry-run schema can never drift from the runtime schema, since both now go through one code path.

3.2 Maintainability

Good. The new validator is a small, self-contained, stateless utility class with clear single responsibility. The extract-method refactor in Neo4jSourceFactory slightly increases indirection (one more layer: parseDryRunSourceConfig -> parseSourceConfig -> SourceConfiguration) but meaningfully reduces duplication risk between the runtime and dry-run schema-construction paths, which is the right tradeoff.

3.3 Extensibility

The pattern established here (separate XxxSourceDryRunValidator class, SourceConfiguration-style shared parsing) is consistent with, and reusable by, other connectors that later want to add dry-run support — no changes to shared/core code were needed to add this, which keeps the blast radius connector-local as intended by the SPI's design (per its own Javadoc: "existing connectors are not affected").

3.4 Historical-Version Compatibility

No impact. No config option, default, protocol, or serialized state was changed; the new code path is only reachable through the already-existing, already-opt-in --dry-run connect command, and only for jobs that include a Neo4j source.

4. Issue Summary

Number Issue Location Severity
1 Underlying driver exception is fully discarded in favor of one generic, undifferentiated failure message (intentional/tested secret-avoidance tradeoff; documented) Neo4jSourceDryRunValidator.java, catch (Exception e) block Low

(Out-of-scope note, not counted as a formal issue: while tracing DriverBuilder/Neo4jQueryInfo, I noticed the pre-existing, untouched Neo4jQueryInfo.prepareDriver kerberos branch calls driverBuilder.setBearerToken(kerberosTicket) instead of setKerberosTicket(kerberosTicket). This predates this PR, is not part of this diff, and is unrelated to the dry-run feature, so it is not a blocker here — flagging only for a possible separate follow-up.)

5. Merge Recommendation

Conclusion: Ready to merge

  1. Blockers - none found in the source-code diff. CI ("Build") is still queued on both the Apache-side pointer check and the contributor's fork run for this exact head SHA (adaef48...) — not failed, just not yet completed — so the only remaining gate before an actual merge is a green CI run.
  2. Recommended fixes - non-blocking, purely optional polish:
    • Consider a short comment on parseDryRunSourceConfig explaining why RuntimeException is intentionally collapsed into a generic IOException (secret-avoidance, same rationale as Issue 1).
    • Optional: classify dry-run connectivity failures into a small set of safe categories (auth/unreachable/timeout) instead of one fixed string, to recover some diagnostic value — only if the maintainers feel the current generic message is too coarse in practice.

Overall assessment: this is an unusually well-engineered contribution for a first formal Daniel review pass — the author traced the exact runtime schema-construction path to guarantee dry-run/runtime parity, proactively guarded against credential/URI leakage in error paths (with both unit and real-container E2E tests proving it), matched the established interrupt-safe/bounded-timeout idiom from the Kafka validator, and was careful not to overclaim what the check validates (explicitly testing and documenting that database existence and query validity are not checked). No alternative architectural approach is warranted here; this follows the intended extension point for the feature exactly as designed. Nice work.

This branch has not been deployed

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants