mattcasters commented on code in PR #8825:
URL: https://github.com/apache/hop/pull/8825#discussion_r4231976231
##########
plugins/misc/lint/src/main/java/org/apache/hop/lint/CustomRuleExecutor.java:
##########
@@ -433,9 +441,18 @@ private static Object extractFieldValue(Object hopObject,
String fieldName, Cust
return dbMeta.getUsername();
case "password":
return dbMeta.getPassword();
+ case "servername":
+ return dbMeta.getServername();
+ case "dataTablespace":
+ return dbMeta.getDataTablespace();
+ case "indexTablespace":
+ return dbMeta.getIndexTablespace();
+ case "attributes":
+ // The extra options are attributes too, stored as
EXTRA_OPTION_<type>.<option>.
+ return dbMeta.getAttributes();
default:
- log.logDetailed("Unknown database field: " + fieldName);
- return null;
+ // Any other connection property, such as manualUrl, by getter or
field.
+ return extractFieldFromObject(dbMeta, fieldName);
Review Comment:
**[bug]** The new default calls `extractFieldFromObject`, so a rule on
`sshTunnelPassphrase` invokes `DatabaseMeta.getSshTunnelPassphrase()`. That
property is `@HopMetadataProperty(password = true)` on the nested database
interface, but `storedAsPassword` only looks for a field of that name on
`DatabaseMeta` itself, and `holdsASecret` only matches names that end with
`password`, `secret`, `token`, and the other existing suffixes. `passphrase`
matches none of them, so `generateErrorMessage` appends the stored secret as
`(current value: ...)`. `sshTunnelPassword` stays hidden only because of the
`password` suffix. Attribute maps are redacted; this string is not. A
`NO_HARDCODED` rule, which is the rule someone would write for this field, is
exactly the case that prints it. That is the CI-log leak DB-001 was closed to
stop, on a getter this fallback newly exposes.
**Suggestion:** When the object is a `DatabaseMeta`, also resolve `password
= true` on `getIDatabase()`. Treat a name ending in `passphrase` as a secret.
Add a test that a finding for `sshTunnelPassphrase` does not contain the value.
##########
plugins/misc/lint/src/main/java/org/apache/hop/lint/CustomRuleExecutor.java:
##########
@@ -586,6 +619,26 @@ private static Object extractFieldFromAction(ActionMeta
actionMeta, String field
return actionMeta.getDescription();
case "pluginId":
return actionMeta.getAction().getPluginId();
+ case "errorHandling":
+ // An action handles its errors with a hop followed on failure.
+ return SUBJECT.get() instanceof WorkflowMeta workflow
+ ? workflow.getWorkflowHops().stream()
+ .anyMatch(
+ hop ->
+ hop.isEnabled()
+ && actionMeta.equals(hop.getFromAction())
+ && !hop.isUnconditional()
+ && !hop.isEvaluation())
+ : null;
+ case "targetActions":
+ return SUBJECT.get() instanceof WorkflowMeta workflow
+ ? workflow.getWorkflowHops().stream()
+ .filter(hop -> hop.isEnabled() &&
actionMeta.equals(hop.getFromAction()))
+ .map(hop -> hop.getToAction().getName())
Review Comment:
**[bug]** `hop.getToAction().getName()` throws when the hop's target was not
resolved. `WorkflowHopMeta` still keeps that hop, with `to == null`. The same
try also calls `actionMeta.equals(hop.getFromAction())` for `errorHandling` and
for this filter. `ActionMeta.equals` returns false for a null argument, but it
dereferences `action.getName()` on both sides when the argument is non-null, so
a from-action whose inner `IAction` is null throws too. The catch returns null
for the entire `targetActions` list and for `errorHandling`. A
`NOT_EMPTY_COLLECTION` or `MUST_BE_TRUE` rule then passes, including for the
hops that were fine.
**Suggestion:** Keep a hop only when both ends are non-null, and compare
those ends without going through `ActionMeta.equals` unless the inner action is
present. Build the list from the hops that resolved instead of failing the
whole field.
##########
plugins/misc/lint/src/main/java/org/apache/hop/lint/CustomRuleExecutor.java:
##########
@@ -548,6 +571,16 @@ private static Object extractFieldFromTransform(
case "copies":
return transformMeta.getCopies(
org.apache.hop.core.variables.Variables.getADefaultVariableSpace());
+ case "distributes":
+ return transformMeta.isDistributes();
+ case "errorHandling":
+ return transformMeta.getTransform() != null &&
transformMeta.isDoingErrorHandling();
+ case "targetTransforms":
+ return SUBJECT.get() instanceof PipelineMeta pipeline
+ ? pipeline.findNextTransforms(transformMeta).stream()
+ .map(TransformMeta::getName)
Review Comment:
**[bug]** `PipelineMeta.findNextTransforms` puts a hop's `to` transform in
the list even when that reference is null. A saved hop whose target name no
longer matches a transform is loaded that way (`PipelineHopMeta` keeps the
hop). `TransformMeta::getName` then throws. A null `from` throws inside
`findNextTransforms` itself (`getFromTransform().equals(...)`). Either
exception is caught below, logged at detailed level, and turned into null for
the whole field. Null is not `FIELD_NOT_FOUND`: `NOT_EMPTY_COLLECTION` and
`MAX_COLLECTION_SIZE` return false, so the rule passes and every other target
on that transform is dropped. One broken hop blanks `targetTransforms` for
every transform, because `findNextTransforms` walks every hop before it returns.
**Suggestion:** Do not call `findNextTransforms` unchecked. Walk enabled
hops, skip those with a null from or to, and map only the names that resolved.
An extraction failure should not become a null that the presence checks treat
as success.
##########
plugins/misc/lint/src/main/java/org/apache/hop/lint/CustomRuleExecutor.java:
##########
@@ -433,9 +441,18 @@ private static Object extractFieldValue(Object hopObject,
String fieldName, Cust
return dbMeta.getUsername();
case "password":
return dbMeta.getPassword();
+ case "servername":
+ return dbMeta.getServername();
+ case "dataTablespace":
+ return dbMeta.getDataTablespace();
+ case "indexTablespace":
+ return dbMeta.getIndexTablespace();
+ case "attributes":
+ // The extra options are attributes too, stored as
EXTRA_OPTION_<type>.<option>.
+ return dbMeta.getAttributes();
default:
- log.logDetailed("Unknown database field: " + fieldName);
- return null;
+ // Any other connection property, such as manualUrl, by getter or
field.
Review Comment:
**[suggestion]** `extractFieldFromObject` tries `getX` and a method whose
name is the field, not `isX`. Connection flags are `isSshTunnelEnabled`,
`isQuoteAllFields`, `isStreamingResults`, and so on, so they come back as
`FIELD_NOT_FOUND`. This change reports every unresolved connection field as an
error, so a rule on `sshTunnelEnabled` says the field does not exist on every
connection, even though the getter is there. `manualUrl` works only because it
is `getManualUrl`.
**Suggestion:** Before returning `FIELD_NOT_FOUND`, also try `is` plus the
capitalized name. Otherwise the new "any other connection property" path and
the "missing connection field is always an error" path disagree for booleans.
--
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]
