mattcasters commented on issue #8280: URL: https://github.com/apache/hop/issues/8280#issuecomment-5575248839
Code review of `plugins/tech/neo4j` against [discussion #8204](https://github.com/apache/hop/discussions/8204) (Hop 2.19.0 + Neo4j v5: warning that execution data/state could not be registered; history/logs/metrics work; lineage, error-lineage and data tabs do not; drill-down into a child pipeline/workflow shows no red marks). Hop 2.19 already ships Neo4j Java Driver 6.2.0 (#7739), which talks to Neo4j 5.x. The symptoms line up with specific failure points below. The forum post is thin because **the real exception is discarded**. ## What the report is describing The warning is not from the Neo4j plugin. It is this catch in the local pipeline engine: ```java // LocalPipelineEngine timer } catch (Exception e) { log.logBasic( "Warning: unable to register execution info (data and state) at location " + executionInfoLocation.getName() + "(non-fatal)"); } ``` That block does three things in one try: `registerData()`, pipeline `updateExecutionState()`, then every transform `updateExecutionState()`. If **sample-row registration** throws, **state updates for that tick are skipped too**, and the cause is never logged. | What works | Why it can still work | |---|---| | Execution list, logs, metrics | `registerExecution()` and the **final** state write in `stopTransformExecutionInfoTimer()` happen outside that timer catch | | Data tab | Driven by `registerData()` → `ExecutionData` / `ExecutionDataSetRow` nodes. If that transaction rolls back, the tab is empty | | Lineage / error-lineage tabs | Neo4j-only GUI tabs; they run path Cypher against `EXECUTES` | | Nested canvas not marked | Workflow red icons come from **action ExecutionData**. Child pipeline canvas is painted **without** execution state | ## Findings ### 1. Sample rows abort the whole data+state tick `NeoExecutionInfoLocation.registerData()` writes sampled rows as node properties (`field0`, `field1`, …). Conversion failures are logged for the first 10 fields, but any remaining value Neo4j will not store still fails the transaction. `mapTypes()` only special-cases `BigDecimal`, `Timestamp`, `Map`, and `JsonNode`. Native types Neo4j rejects as properties (`List` of mixed values, `Object[]`, `InetAddress`, serializable blobs, some JSON structures) roll back **all** data for that pipeline. `Map.of(parentId, ownerId)` also throws if either id is null. A missing `setMeta` or `rowMeta` NPEs the same way. This is the same family as the old #1850 (`String` stored in an Integer field). Conversion is now caught, but **unsupported Neo4j types are not**. ### 2. Data tab and workflow “red action” both depend on that same write Workflow execution viewer paints failed actions from `ExecutionData` (`KEY_RESULT` rows), not from `Execution.failed`. Parent workflow action result rows are simple (boolean/string) → often stored → action shows red. Child pipeline sampled transform rows are typed and much more likely to fail → data tab empty, and drill-down has nothing to paint. Pipeline execution viewer never feeds runtime state into the painter (`null, // No state yet` in `drawPipelineImage()`), so **nested pipeline canvases cannot show transform error icons** even when metrics exist. Related: `PipelineExecutionViewer.drillDown()` loads the **parent** state twice. Workflow drill-down correctly uses `child.getId()`. ### 3. Lineage / error tabs: Cypher is fragile on Neo4j 5 and untested The Execution Information lineage/error tabs are **not** the same query that was fixed for Neo4j 5 in `HopNeo4jPerspective` (#7662 / `ErrorPathCypherIT`). - Cartesian `MATCH (top:Execution)` × `shortestPath` over an undirected unbounded `[:EXECUTES*]` is expensive and can time out on a busy logging graph (especially if `NEO4J_LOGGING_CONNECTION` and the execution location share one database — both use `:Execution`). - Error path has **no leaf predicate**. The perspective query was changed to `AND NOT (err)-[:EXECUTES]->()` because Neo4j 5 removed `size((err)-[:EXECUTES]->())`. These tabs never got that fix. - `WHERE child.failed` is a truthiness check. Prefer `child.failed = true`. - Tab refresh has **no try/catch**. A ClientException leaves the tree empty. - There is **no IT** for these two statements (only for `HopNeo4jPerspective.getErrorPathCypher()`). ### 4. `unBuffer()` is a no-op while a process-wide cache can lie `getExecutionState()` returns `NeoLocationCache` first. `updateNeo4jExecutionState()` stores into that cache in a `finally`, **even when the Neo4j write failed**. After a failed tick the GUI can show metrics that were never persisted. `manageCacheSize()` is also wrong: when size ≥ 1050 it deletes **every** entry (comment says “remove the last 50”). ## Other Neo4j v5 / hardening notes - `NeoConnection.getDriver()`: after `routingDriver()` was removed, only `uris.get(0)` is passed. Driver 6 has no multi-URI overload; extra servers are silently ignored. Prefer `neo4j://` for routing. - `Neo4jIndex.generateDropIndexCypher()`: nameless drop still emits `DROP INDEX FOR :Label(prop)`, which Neo4j 5 rejects. - `Neo4jConstraint` NODE_KEY is Enterprise-only. On Community this fails, and the action **swallows** the error (`return false` inside `executeWrite`, `execute()` still succeeds). - Importer version combo is `"4.x"` / `"5.x"` and defaults to **4.x**. Neo4j 2025.x / 2026.x is CalVer; `isNeo4j5` is only `startsWith("5.")`. - `GraphPropertyDataType.Number` import type is `"doubler"` (typo for `"double"`). - Metrics are `CREATE` + `CREATE` relationship on every timer tick, duplicating `ExecutionMetric` nodes. Use `MERGE`. - `assert` is used as input validation (`execution.getName() != null`). `-da` disables it. - Index action copied to clipboard omits `idx_execution_start_date`, which the DDL button includes. - `findParentId` / `findLastExecution` only inspect 100 ids. - `NeoExecutionInfoLocation.close()` can NPE if `initialize()` failed. - `LoggingCore.writeHierarchies()` catches exceptions inside the transaction callback and does not rethrow, so `executeWrite` **commits** a partial graph. - Old logging (`NEO4J_LOGGING_CONNECTION`) still uses `:Execution` with `type` / `errors` / `copy`. Execution info uses `executionType` / `failed` / `copyNr`. Mixing both on one database pollutes lineage. ## Tests What exists: metadata XML tests, `ErrorPathCypherIT` for the **old** perspective query, `HopNeo4jPerspectiveTest` for Cypher shape. Missing, and would have caught this report: 1. `NeoExecutionInfoLocation` IT: register execution + state + sample rows (including a deliberately bad type) against Testcontainers `neo4j:5.26`, then `getExecutionData()`. 2. IT for `getPathToRootCypher()` / `getPathToFailedCypher()` on the **execution-info** property names. 3. Unit tests for `mapTypes()` / row property coercion and cache eviction. ## Planned fixes (this issue) 1. Log the swallowed exception in `LocalPipelineEngine` and split `registerData` vs `updateExecutionState` so a data failure cannot skip state. 2. Make `registerData` Neo4j-safe: coerce/skip unsupported property types; null-safe maps; do not abort the whole data set on one field. 3. Fix lineage/error Cypher for Neo4j 5 (directed paths, leaf predicate, no cartesian `MATCH (top:Execution)`), with tests. 4. Cache: honor `unBuffer()`, do not cache a state whose write failed, evict 50 oldest not the whole map. 5. MERGE execution metrics; null-safe close; require index names on drop; fail the constraint/index action on Cypher errors; typo + importer CalVer. 6. GUI: pass child execution state on pipeline drill-down; paint pipeline error icons from stored metrics. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
