mattcasters commented on code in PR #8584:
URL: https://github.com/apache/hop/pull/8584#discussion_r4106369645
##########
plugins/transforms/dynamicsqlrow/src/main/java/org/apache/hop/pipeline/transforms/dynamicsqlrow/DynamicSqlRowMeta.java:
##########
@@ -333,6 +344,34 @@ public void analyseImpact(
}
}
+ /**
+ * Looks up the connection by name every time it's needed. The resolved
connection isn't kept on
+ * the meta: a freshly loaded pipeline would otherwise report no template
fields until the dialog
+ * was opened.
+ */
+ private DatabaseMeta loadDatabaseMeta(IVariables variables,
IHopMetadataProvider metadataProvider)
+ throws HopTransformException {
+ if (Utils.isEmpty(connection)) {
+ return null;
+ }
+ String realConnection = variables.resolve(connection);
+ DatabaseMeta databaseMeta;
+ try {
+ databaseMeta =
metadataProvider.getSerializer(DatabaseMeta.class).load(realConnection);
+ } catch (HopException e) {
+ throw new HopTransformException(
+ BaseMessages.getString(
+ PKG, "DynamicSQLRowMeta.Exception.ConnectionNotFound",
realConnection),
+ e);
+ }
+ if (databaseMeta == null) {
Review Comment:
**[suggestion]** A missing connection is now a hard `HopTransformException`
from every `getFields` caller. That matches `TableInputMeta` for a concrete
unknown name, and `getFieldsWithUnknownConnectionThrows` locks it in, but an
unset variable is left as `${NAME}` by `StringUtil.substitute` and then hits
this same throw. `DynamicSqlRowDialog.getSqlReservedWords()` already returns no
keywords when the resolved name still starts with `${`. Field discovery and
pipeline verify call `getFields` first, so a connection variable that is only
set at runtime now fails the whole field chain instead of omitting the template
fields.
**Suggestion:** Return null when the resolved name is empty or still
contains `${`, and keep the throw for a resolved name that is not in the
metadata store. `check()` can keep reporting the error.
##########
plugins/transforms/dynamicsqlrow/src/main/java/org/apache/hop/pipeline/transforms/dynamicsqlrow/DynamicSqlRowMeta.java:
##########
@@ -303,6 +313,7 @@ public void analyseImpact(
IHopMetadataProvider metadataProvider)
throws HopTransformException {
+ DatabaseMeta databaseMeta = loadDatabaseMeta(variables, metadataProvider);
Review Comment:
**[suggestion]** `loadDatabaseMeta` returns null when `connection` is empty,
but `analyseImpact` still calls `databaseMeta.getDatabaseName()` for every
field in `out`. `out` is `prev.clone()`, so any upstream fields turn this into
an NPE and `PipelineMeta.analyseImpact` fails for the whole pipeline.
`getFields` and `check` both handle that null. The crash was already possible
when the old field was unset, but this is the only new caller that ignores the
null return.
**Suggestion:** If `databaseMeta` is null, return before `getFields` and the
impact loop. An empty connection has nothing to report.
##########
plugins/transforms/dynamicsqlrow/src/main/java/org/apache/hop/pipeline/transforms/dynamicsqlrow/DynamicSqlRowMeta.java:
##########
@@ -141,6 +138,7 @@ public void getFields(
IHopMetadataProvider metadataProvider)
throws HopTransformException {
+ DatabaseMeta databaseMeta = loadDatabaseMeta(variables, metadataProvider);
if (databaseMeta == null) {
Review Comment:
**[bug]** `getFields()` returns only when the connection is missing. With a
connection name set and the template SQL still empty (`setDefault()` stores
`""`), execution falls through to `Database.getQueryFields`. The unconnected
call returns null, then `db.connect()` runs and the second call prepares an
empty statement. That becomes `HopTransformException`
(`DynamicSQLRowMeta.Exception.ErrorObtainingFields`).
`PipelineMeta.getTransformFields` does not catch it, and `checkTransforms`
treats the failure as fatal (`stopChecking = true`), so Show Output Fields, Get
Fields, and Verify fail for every downstream transform. It is easy to hit: in a
project with one connection the dialog auto-selects it and OK stores the name
without requiring SQL. `check()` already skips an empty template (line 269),
and `processRow()` rejects empty SQL only at execution time. The new tests
never pass an empty SQL with a connection, and they stub `getQueryFields` so
this path stays green.
**Suggestion:** Resolve the template first and return before connecting when
it is empty, including a variable that resolves to empty: `if (databaseMeta ==
null || Utils.isEmpty(realSql)) return;`. Add a test that a set connection plus
empty SQL leaves the input row unchanged and does not construct a `Database`.
--
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]
