bamaer commented on PR #8317:
URL: https://github.com/apache/hop/pull/8317#issuecomment-5633602237
Review against head `0a8c671`. The layering looks right (name map in
`engine`, scheme application through an extension point in the naming plugin),
and case-insensitive dedup with the `shared.xml` spelling winning matches what
the issue asks for. Findings below, most severe first.
### Correctness
**1. A name collision leaves rewritten references pointing at a different
connection** —
`engine/src/main/java/org/apache/hop/imp/ImportedConnectionRewriter.java:155`
and `:205`
`ConnectionNameMap` lets two distinct case-insensitive groups share one
target name (javadoc: *"they still share the target name"*). With a scheme such
as lower_underscore, `My-DB` and `My_DB` both map to `my_db`. Then:
- `renameConnections()` renames the first group, hits
`serializer.exists("my_db") && !oldName.equalsIgnoreCase(newName)` for the
second, logs *"that name already exists"* and `continue`s, so `My_DB` keeps its
original metadata name.
- `rewriteWrittenFiles()` runs on the same `nameMap` with no feedback from
the rename step, so every `My_DB` reference in the imported `.hpl`/`.hwf` files
is still rewritten to `my_db`.
Expected: a reference either follows its own connection or is left
untouched. Actual: transforms that used `My_DB` now resolve `my_db`, i.e.
`My-DB`'s host/database/credentials, and `My_DB.json` stays behind
unreferenced. The same path is taken for a rename that throws and is reverted
in the catch at `:172`. The summary box reports the collision count but not
that references were repointed. Either abort on collision, or leave the losing
group un-renamed in both the metadata and the files. `ConnectionNameMapTest`
covers collision recording at the map level; the rename-refused plus rewrite
combination is not covered.
**2. An explicit naming scheme that cannot be resolved is ignored rather
than reported as a failure** —
`plugins/misc/naming/src/main/java/org/apache/hop/naming/gui/NamingSchemeImportExtension.java:70`
With `hop-import --naming-scheme <unknown>` (or a stale last-used value in
the dialog combo), `NamingSchemeSelector.resolve` returns null for an unmatched
explicit name, the extension logs an error and returns, and the import
continues with case alignment only. Exit code is 0 and the summary still reads
*"Relational connection names were aligned to a single case-sensitive
spelling"*. Expected: the import fails, or the dialog/CLI reports that the
requested scheme was not applied.
### Dialog metadata lifecycle
**3. Selected metadata is written to the target before the cancellable
confirmation** —
`plugins/misc/import/src/main/java/org/apache/hop/imports/kettle/KettleImportDialog.java:597`
`persistDialogMetadataToTarget()` is called above the `SWT.ICON_WARNING |
SWT.OK | SWT.CANCEL` box. Pressing Cancel there sets `goForImport = false`, but
`<target>/metadata/pipeline-run-configuration/*.json` and the naming-scheme
JSON have already been written. The target folder being created before that box
(`setValidateOutputFolder`) is pre-existing behaviour; writing metadata objects
into it is new. Moving the call inside `if (goForImport)` covers it.
**4. `persistDialogMetadataToTarget()` replaces the dialog provider, so
later New/Edit writes to disk** — `KettleImportDialog.java:777`
The method ends with `applyMetadataProvider(json, metadataFolder)`, which
swaps the multi provider for a bare target `JsonMetadataProvider` and sets
`boundMetadataFolder` to that folder. `doImport()` does not dispose the dialog
(it only shows the summary), and `bindTargetMetadataProvider()` early-returns
while the folder is unchanged. After one Import attempt, `scratchMetadata` and
the HopGui provider are no longer in the chain, so New/Edit writes directly
into the target project rather than memory, and objects created earlier in
scratch but not selected are no longer listed. This conflicts with the
documented "New/Edit stay in memory until Import" behaviour.
**5. The bind guard cannot bind when the target folder does not resolve** —
`KettleImportDialog.java:696`
`dialogMetadataProvider` is non-null from the constructor (scratch only) and
`boundMetadataFolder` starts null, so `Objects.equals(metadataFolder,
boundMetadataFolder) && dialogMetadataProvider != null` returns immediately
whenever `metadataFolderFor(peekTargetFolder())` is null, for example on a
first run with no `LAST_USED_IMPORT_TARGET_PROJECT`. Expected: the run
configuration and naming scheme combos list the current project's objects, as
they did before this change via `hopGui.getMetadataProvider()`. Actual: all
three lines list nothing until a project is picked. A separate `bound` flag
would avoid the null-versus-null ambiguity.
### Other
**6. `HopGuiRunConfiguration` is no longer fired from the import dialog**
The replaced combos called `ExtensionPointHandler.callExtensionPoint(...,
HopExtensionPoint.HopGuiRunConfiguration.id, new Object[] {runConfigurations,
PipelineMeta.XML_TAG})` for both lists. `MetaSelectionLine.fillItems()` only
calls `listObjectNames()`. Every other run configuration dropdown still fires
that point (`PipelineExecutionConfigurationDialog:240`,
`WorkflowExecutionConfigurationDialog:271`, `MetaInjectDialog:992`,
`PipelineExecutorDialog:407`, `ActionPipelineDialog:302`,
`ActionWorkflowDialog:284`). Nothing in this repository implements the point,
so the effect is limited to external plugins contributing run configurations.
**7. Cancelling the progress monitor after the connection import yields an
inaccurate summary** —
`engine/src/main/java/org/apache/hop/imp/HopImportBase.java:145`
`saveConnectionsReport()` moved from `importConnections()` into
`afterConnectionRewrite()`, which sits below `if (monitor.isCanceled())
return;`. On cancel, `connectionsReportFileName` stays null while
`getImportReport()` still prints *"Relational connection names were aligned to
a single case-sensitive spelling"* and *"Check the following file for a list of
connections that might need extra attention: null"*.
**8. `shouldSkip()` recognises only the `${...}` variable syntax** —
`engine/src/main/java/org/apache/hop/imp/ConnectionNameMap.java:58`
`Variables.resolve` applies `substituteWindows` and `substituteHex` as well
(`core/src/main/java/org/apache/hop/core/util/StringUtil.java:262`), so a
connection field holding `%%DB_CONN%%` is treated as a literal name: with a
scheme it becomes `%%db_conn%%`, and without one two spellings such as
`%%DB_CONN%%` and `%%db_conn%%` collapse onto a single target. The same
`${`-only rule already exists in `NamingEngine.shouldSkip`
(`plugins/misc/naming/.../engine/NamingEngine.java:43`), so this is
pre-existing in the naming plugin rather than introduced here; since `engine`
cannot depend on that plugin, moving the predicate into `core` next to
`HopMetadataPropertyWalker` would keep the two from drifting.
--
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]
