This is an automated email from the ASF dual-hosted git repository.
mattcasters pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/hop.git
The following commit(s) were added to refs/heads/main by this push:
new 23a3df74f1 Issue #8294 : Fix the Select Values checks and make the
default lint rules usable (#8307)
23a3df74f1 is described below
commit 23a3df74f192edcf3f51698ac867a71656c3f317
Author: Bart Maertens <[email protected]>
AuthorDate: Thu Sep 10 11:34:10 2026 +0200
Issue #8294 : Fix the Select Values checks and make the default lint rules
usable (#8307)
Select Values applies its tabs in order, each to the row the one before it
produced, but check() compared all of them to the incoming row. A field
renamed on "Select & Alter" was reported missing by the Metadata and Remove
tabs, which only ever see the new name. Selecting the same field twice is
how a value is copied under a second name, so the duplicate check now counts
the names fields leave under. Two fields leaving under one name still is.
Hop's check() methods were written for the Verify button and work from the
design-time row stream. The linter repeated them unprompted at whatever
severity the transform picked, with no rule able to say otherwise. A new
`type: native` rule classifies them and the core pack reports them at
warning; naming-scheme remarks come through the same door.
The rest is the default rules meeting real projects:
- TRANS-002 and WORKFLOW-002 reported on the file, so the warning could not
name the transform it was about. They now report on the element. A file
holding a single transform is no longer an orphan, and neither is one
whose hops are disabled: that is what STRUCT-003 asks, and it ships off.
- SEC-002 and SEC-003 matched any field merely containing "token" or
"secret", at any type, putting three hardcoded-secret errors on every
Token Replacement transform.
---
.../modules/ROOT/pages/linting/lint-rules.adoc | 56 ++++
plugins/misc/lint/pom.xml | 12 +
.../java/org/apache/hop/lint/CustomLintRule.java | 50 ++++
.../org/apache/hop/lint/CustomRuleExecutor.java | 174 ++++++++-----
.../main/java/org/apache/hop/lint/HopLinter.java | 16 +-
.../org/apache/hop/lint/HopNativeCheckRunner.java | 18 +-
.../apache/hop/lint/LintCheckResultAdapter.java | 35 ++-
.../org/apache/hop/lint/NativeCheckClassifier.java | 220 ++++++++++++++++
.../apache/hop/lint/registry/EffectiveRuleSet.java | 33 +++
.../hop/lint/registry/ProjectLintYamlExporter.java | 28 ++
.../hop/lint/registry/YamlRulePackParser.java | 37 ++-
.../misc/lint/src/main/resources/hop-lint-core.yml | 46 +++-
.../lint/src/main/resources/hop-lint.yml.example | 9 +
.../apache/hop/lint/HardcodedSecretRuleTest.java | 116 +++++++++
.../hop/lint/LintSuppressionInEditorTest.java | 10 +-
.../apache/hop/lint/NativeCheckClassifierTest.java | 287 +++++++++++++++++++++
.../apache/hop/lint/OrphanedElementRuleTest.java | 114 ++++++++
.../hop/lint/registry/HopCoreRulePackTest.java | 44 ++++
.../apache/hop/lint/registry/RuleRegistryTest.java | 70 +++++
.../transforms/selectvalues/SelectValuesMeta.java | 63 +++--
.../selectvalues/SelectValuesMetaCheckTest.java | 158 ++++++++++++
21 files changed, 1492 insertions(+), 104 deletions(-)
diff --git a/docs/hop-user-manual/modules/ROOT/pages/linting/lint-rules.adoc
b/docs/hop-user-manual/modules/ROOT/pages/linting/lint-rules.adoc
index 20bedd4a62..701b6f0b2e 100644
--- a/docs/hop-user-manual/modules/ROOT/pages/linting/lint-rules.adoc
+++ b/docs/hop-user-manual/modules/ROOT/pages/linting/lint-rules.adoc
@@ -220,6 +220,51 @@ A rule aimed at `TRANSFORM` runs against every transform
in the pipeline.
A scoped rule naming a field the transform does not have is reported as a
configuration error rather than passing quietly, so that a typo in a field name
is visible instead of looking like a clean result.
+== Hop's own verify remarks
+
+Every transform and action has a `check()` method of its own, the one behind
the *Verify* button, and the linter reports what those find alongside its own
rules.
+They are not the same kind of claim.
+Verify is a question somebody asked, and reads the answers to in context; the
linter asks it on every save and every build, and a remark it repeats is a
remark it stands behind.
+`check()` also works from the row stream Hop infers at design time, which is
right for a plain pipeline and wrong wherever fields arrive at runtime —
through metadata injection, a mapping, or a transform filled in by a parameter.
+
+A `native` rule says how those remarks are reported. It checks nothing itself:
+
+[source,yaml]
+----
+rules:
+ HOP-CHECK:
+ type: native
+ enabled: true
+ severity: WARNING
+----
+
+`severity` is what the linter reports the remark as, whatever severity the
transform gave it, and `enabled: false` drops it.
+Two optional keys narrow a rule to less than every remark:
+
+* `appliesTo` — the plugin ids of the transforms or actions it covers, as for
any other rule.
+* `messageKey` — one single check, named as `<i18n package>:<key>` for the
message it prints, the same form Hop's plugin annotations use. The key is
resolved through the plugin's own message bundle, so the rule keeps matching in
every language rather than depending on the English wording. A key that no
longer resolves matches nothing rather than everything.
+
+The most specific rule wins — one naming the check beats one naming only the
plugin, which beats the blanket rule — so a pack can hold a general policy and
an exception to it, and the order of the file does not decide which applies.
+
+To have the linter treat Hop's remarks exactly as the transform meant them,
put the severity back in your project's `hop-lint.yml`:
+
+[source,yaml]
+----
+rules:
+ HOP-CHECK:
+ severity: ERROR
+----
+
+Findings from a native rule carry that rule's id, so they suppress by id like
any other:
+
+[source,yaml]
+----
+suppress:
+ - rule: HOP-CHECK
+ path: "templates/**"
+ reason: "Fields in these templates arrive through metadata injection"
+----
+
== The rules Hop ships
Hop's core rule pack is deliberately small. A rule is enabled by default only
when a violation is defensible as a defect in any project, whatever the house
style.
@@ -251,8 +296,19 @@ Hop's core rule pack is deliberately small. A rule is
enabled by default only wh
|`NAMING-004`
|WARNING
|A transform still carrying its auto-generated name
+
+|`HOP-CHECK`
+|WARNING
+|Every remark from Hop's own transform and action checks
|===
+`TRANS-002` and `WORKFLOW-002` report on the transform or action itself, so
the finding names it and the canvas can point at it.
+The only element in a file is not an orphan — there is nothing for it to be
disconnected from — and neither is one whose hops are all disabled: it has
hops, and whether a disabled hop is a problem is what `STRUCT-003` asks.
+
+Hop ships no narrowed `native` rule of its own.
+A check that is simply wrong is fixed in the transform rather than silenced
from a rule pack, which would leave it firing for everyone who presses *Verify*.
+`appliesTo` and `messageKey` are there for a project that disagrees with a
check the platform is right to ship.
+
A further set ships *disabled*, as worked examples of the format. Switch one
on by id in your project's `hop-lint.yml`.
[cols="1,3", options="header"]
diff --git a/plugins/misc/lint/pom.xml b/plugins/misc/lint/pom.xml
index 834478f55a..fb16b4f6b2 100644
--- a/plugins/misc/lint/pom.xml
+++ b/plugins/misc/lint/pom.xml
@@ -46,6 +46,18 @@
<groupId>org.yaml</groupId>
<artifactId>snakeyaml</artifactId>
</dependency>
+
+ <!--
+ The core pack names two Select Values checks by message key, so that
a rule about one
+ check keeps working in every locale. Only a test that resolves those
keys against the
+ real bundle can catch them being renamed; without it a rule would
quietly stop matching.
+ -->
+ <dependency>
+ <groupId>org.apache.hop</groupId>
+ <artifactId>hop-transform-selectvalues</artifactId>
+ <version>${project.version}</version>
+ <scope>test</scope>
+ </dependency>
</dependencies>
</project>
diff --git
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/CustomLintRule.java
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/CustomLintRule.java
index 271ce6e26a..abba36736e 100644
--- a/plugins/misc/lint/src/main/java/org/apache/hop/lint/CustomLintRule.java
+++ b/plugins/misc/lint/src/main/java/org/apache/hop/lint/CustomLintRule.java
@@ -62,6 +62,27 @@ public class CustomLintRule {
/** How the clauses combine. Meaningless, and ignored, for a rule with a
single clause. */
private RuleCombinator combinator = RuleCombinator.ALL_OF;
+ /**
+ * What kind of rule this is: {@link #TYPE_CUSTOM}, which the linter
evaluates itself, or {@link
+ * #TYPE_NATIVE}, which classifies a remark Hop's own {@code check()}
produced.
+ */
+ private String type = TYPE_CUSTOM;
+
+ /**
+ * For a native rule, the single check it speaks about, named by the message
it prints as {@code
+ * <i18n package>:<key>}. Empty means every native remark.
+ */
+ private String messageKey;
+
+ /** A rule the linter evaluates against a pipeline, workflow or metadata
object. */
+ public static final String TYPE_CUSTOM = "custom";
+
+ /**
+ * A rule that says how to report a remark from Hop's own transform and
action {@code check()}
+ * methods, rather than one the linter evaluates itself.
+ */
+ public static final String TYPE_NATIVE = "native";
+
public CustomLintRule() {
this.id = UUID.randomUUID().toString();
this.enabled = true;
@@ -247,6 +268,33 @@ public class CustomLintRule {
*
* @return the clauses, in the order they were written
*/
+ public String getType() {
+ return type;
+ }
+
+ public void setType(String type) {
+ this.type = type;
+ }
+
+ public String getMessageKey() {
+ return messageKey;
+ }
+
+ public void setMessageKey(String messageKey) {
+ this.messageKey = messageKey;
+ }
+
+ /**
+ * Whether this rule classifies Hop's own verify remarks instead of being
evaluated by the linter.
+ *
+ * <p>The two kinds share this class, and the rule registry, so that a
native rule is merged,
+ * overridden by a project's {@code hop-lint.yml} and listed in the rule
manager on exactly the
+ * same terms as any other rule.
+ */
+ public boolean isNativeVerify() {
+ return TYPE_NATIVE.equalsIgnoreCase(type);
+ }
+
public List<RuleClause> getClauses() {
List<RuleClause> clauses = new ArrayList<>();
clauses.add(new RuleClause(targetField, condition, conditionValue));
@@ -270,6 +318,8 @@ public class CustomLintRule {
copy.additionalParameters = new HashMap<>(this.additionalParameters);
copy.appliesTo = new ArrayList<>(this.appliesTo);
copy.combinator = this.combinator;
+ copy.type = this.type;
+ copy.messageKey = this.messageKey;
copy.additionalClauses = new ArrayList<>();
for (RuleClause clause : this.additionalClauses) {
copy.additionalClauses.add(clause.copy());
diff --git
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/CustomRuleExecutor.java
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/CustomRuleExecutor.java
index 55c5cd5c79..e16ef121c0 100644
---
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/CustomRuleExecutor.java
+++
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/CustomRuleExecutor.java
@@ -62,6 +62,17 @@ public class CustomRuleExecutor {
private static final ThreadLocal<LintProjectIndex> PROJECT_INDEX =
ThreadLocal.withInitial(LintProjectIndex::empty);
+ /**
+ * The pipeline or workflow the transform or action being evaluated belongs
to.
+ *
+ * <p>Rules are handed one transform at a time and a {@link TransformMeta}
does not know its
+ * pipeline, so a question about how a transform is connected has nowhere to
look. Holding the
+ * file being linted here is what lets a finding name the transform rather
than the file: the
+ * alternative is a rule on the pipeline reporting "something here is
orphaned", which is not a
+ * thing anyone can click on.
+ */
+ private static final ThreadLocal<Object> SUBJECT = new ThreadLocal<>();
+
/**
* Make a project index available to the rules evaluated on this thread.
*
@@ -75,6 +86,19 @@ public class CustomRuleExecutor {
}
}
+ /**
+ * Make the file being linted available to the rules evaluated on this
thread.
+ *
+ * @param subject the pipeline or workflow, or null to clear it
+ */
+ public static void setSubject(Object subject) {
+ if (subject == null) {
+ SUBJECT.remove();
+ } else {
+ SUBJECT.set(subject);
+ }
+ }
+
/** Execute a custom rule against a Hop object */
public static List<LintResult> executeRule(
CustomLintRule rule, Object hopObject, String fileName) {
@@ -472,6 +496,10 @@ public class CustomRuleExecutor {
return
"Dummy".equalsIgnoreCase(transformMeta.getTransformPluginId());
case "hasDefaultName":
return hasDefaultGeneratedName(transformMeta.getName());
+ case "isOrphaned":
+ return SUBJECT.get() instanceof PipelineMeta pipeline
+ ? isOrphaned(transformMeta, pipeline.getPipelineHops(),
pipeline.getTransforms())
+ : null;
case "isBlockingTransform":
return
isBlockingTransformPlugin(transformMeta.getTransformPluginId(), rule);
default:
@@ -501,6 +529,10 @@ public class CustomRuleExecutor {
return actionMeta.getAction().getPluginId();
case "hasDefaultName":
return hasDefaultGeneratedName(actionMeta.getName());
+ case "isOrphaned":
+ return SUBJECT.get() instanceof WorkflowMeta workflow
+ ? isOrphaned(actionMeta, workflow.getWorkflowHops(),
workflow.getActions())
+ : null;
default:
// Try to get from the action implementation
Object action = actionMeta.getAction();
@@ -573,92 +605,72 @@ public class CustomRuleExecutor {
}
/** Check if a pipeline has orphaned transforms (transforms with no incoming
or outgoing hops) */
- private static boolean hasOrphanedTransforms(PipelineMeta pipeline) {
- if (pipeline == null
- || pipeline.getTransforms() == null
- || pipeline.getTransforms().isEmpty()) {
+ /**
+ * Whether this element is connected to nothing in the file it lives in.
+ *
+ * <p>Two things it deliberately does not count as orphaned, both of which
put a warning on
+ * projects that had nothing wrong with them:
+ *
+ * <ul>
+ * <li>the only element in the file. A one-transform pipeline is a normal
thing to write, which
+ * is why the core pack does not ship a rule on transform counts
either. With nothing to be
+ * disconnected from, it cannot be disconnected.
+ * <li>an element whose hops are all disabled. It has hops; they are
switched off, which is a
+ * different observation and one the core pack ships switched off
(STRUCT-003), because a
+ * disabled hop is work in progress to most teams. Reporting it here
as "never executes"
+ * made that opinion the default through the back door.
+ * </ul>
+ */
+ private static <T> boolean isOrphaned(T element, List<? extends Object>
hops, List<T> elements) {
+ if (element == null || elements == null || elements.size() < 2) {
return false;
}
-
- List<PipelineHopMeta> hops = pipeline.getPipelineHops();
if (hops == null || hops.isEmpty()) {
- // If no hops exist, all transforms with more than 0 transforms are
orphaned
- return pipeline.getTransforms().size() > 0;
+ return true;
}
-
- // Build sets of transforms that have incoming and outgoing connections
- java.util.Set<TransformMeta> transformsWithIncoming = new
java.util.HashSet<>();
- java.util.Set<TransformMeta> transformsWithOutgoing = new
java.util.HashSet<>();
-
- for (PipelineHopMeta hop : hops) {
- if (hop.isEnabled()) {
- TransformMeta fromTransform = hop.getFromTransform();
- TransformMeta toTransform = hop.getToTransform();
-
- if (fromTransform != null) {
- transformsWithOutgoing.add(fromTransform);
- }
- if (toTransform != null) {
- transformsWithIncoming.add(toTransform);
- }
+ for (Object hop : hops) {
+ if (connects(hop, element)) {
+ return false;
}
}
+ return true;
+ }
- // A transform is orphaned if it has no incoming AND no outgoing hops
- for (TransformMeta transform : pipeline.getTransforms()) {
- boolean hasIncoming = transformsWithIncoming.contains(transform);
- boolean hasOutgoing = transformsWithOutgoing.contains(transform);
-
- if (!hasIncoming && !hasOutgoing) {
- return true; // Found at least one orphaned transform
- }
+ /** Whether the hop has this element at either end, enabled or not. */
+ private static boolean connects(Object hop, Object element) {
+ if (hop instanceof PipelineHopMeta pipelineHop) {
+ return element.equals(pipelineHop.getFromTransform())
+ || element.equals(pipelineHop.getToTransform());
+ }
+ if (hop instanceof WorkflowHopMeta workflowHop) {
+ return element.equals(workflowHop.getFromAction())
+ || element.equals(workflowHop.getToAction());
}
-
return false;
}
- /**
- * Check if a workflow has orphaned actions (actions with no incoming or
outgoing workflow hops)
- */
- private static boolean hasOrphanedActions(WorkflowMeta workflow) {
- if (workflow == null || workflow.getActions() == null ||
workflow.getActions().isEmpty()) {
+ private static boolean hasOrphanedTransforms(PipelineMeta pipeline) {
+ if (pipeline == null || pipeline.getTransforms() == null) {
return false;
}
-
- List<WorkflowHopMeta> hops = workflow.getWorkflowHops();
- if (hops == null || hops.isEmpty()) {
- // If no hops exist, all actions with more than 0 actions are orphaned
- return workflow.getActions().size() > 0;
- }
-
- // Build sets of actions that have incoming and outgoing connections
- java.util.Set<ActionMeta> actionsWithIncoming = new java.util.HashSet<>();
- java.util.Set<ActionMeta> actionsWithOutgoing = new java.util.HashSet<>();
-
- for (WorkflowHopMeta hop : hops) {
- if (hop.isEnabled()) {
- ActionMeta fromAction = hop.getFromAction();
- ActionMeta toAction = hop.getToAction();
-
- if (fromAction != null) {
- actionsWithOutgoing.add(fromAction);
- }
- if (toAction != null) {
- actionsWithIncoming.add(toAction);
- }
+ for (TransformMeta transform : pipeline.getTransforms()) {
+ if (isOrphaned(transform, pipeline.getPipelineHops(),
pipeline.getTransforms())) {
+ return true;
}
}
+ return false;
+ }
- // An action is orphaned if it has no incoming AND no outgoing hops
+ /** Whether a workflow has actions connected to nothing. */
+ private static boolean hasOrphanedActions(WorkflowMeta workflow) {
+ if (workflow == null || workflow.getActions() == null) {
+ return false;
+ }
for (ActionMeta action : workflow.getActions()) {
- boolean hasIncoming = actionsWithIncoming.contains(action);
- boolean hasOutgoing = actionsWithOutgoing.contains(action);
-
- if (!hasIncoming && !hasOutgoing) {
- return true; // Found at least one orphaned action
+ if (isOrphaned(action, workflow.getWorkflowHops(),
workflow.getActions())) {
+ return true;
}
}
-
return false;
}
@@ -864,9 +876,8 @@ public class CustomRuleExecutor {
// Check all password-related fields
Class<?> clazz = transformOrAction.getClass();
for (Field field : getAllFields(clazz)) {
- String fieldName = field.getName().toLowerCase();
for (String pattern : fieldPatterns) {
- if (fieldName.contains(pattern.toLowerCase())) {
+ if (namesASecret(field, pattern)) {
try {
field.setAccessible(true);
Object value = field.get(transformOrAction);
@@ -898,6 +909,30 @@ public class CustomRuleExecutor {
return results;
}
+ /**
+ * Whether this field holds the secret the pattern names, rather than merely
mentioning it.
+ *
+ * <p>Two narrowings, both of which cost nothing in coverage and remove
findings that were simply
+ * wrong. A substring match reported every Token Replacement transform in
the project three times
+ * over — {@code tokenStartString} defaults to {@code "${"}, {@code
tokenEndString} to {@code "}"}
+ * — and every Get Data From XML transform once, for the boolean {@code
useToken}. Neither is a
+ * credential, and a rule that cries wolf on a stock transform is one people
switch off.
+ *
+ * <ul>
+ * <li>the name has to <em>end</em> with the pattern, so {@code
sessionToken} and {@code
+ * trustStorePassword} match while {@code tokenStartString}, {@code
oauth2TokenUrl} and
+ * {@code credentialsFile} do not: a secret is what the field is, not
what it is about;
+ * <li>the field has to hold a string, because a flag, a count or a list
of columns is never a
+ * credential however it is named.
+ * </ul>
+ */
+ private static boolean namesASecret(Field field, String pattern) {
+ if (Utils.isEmpty(pattern) || !String.class.equals(field.getType())) {
+ return false;
+ }
+ return
field.getName().toLowerCase().endsWith(pattern.trim().toLowerCase());
+ }
+
/** Get password field patterns from rule parameters or return defaults */
private static List<String> getPasswordFieldPatterns(CustomLintRule rule) {
List<String> defaultPatterns =
@@ -911,6 +946,7 @@ public class CustomRuleExecutor {
"credentials",
"apiKey",
"apikey",
+ "secretAccessKey",
"token",
"accessToken",
"authToken");
diff --git a/plugins/misc/lint/src/main/java/org/apache/hop/lint/HopLinter.java
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/HopLinter.java
index 180d8f7581..3c85a2bc7b 100644
--- a/plugins/misc/lint/src/main/java/org/apache/hop/lint/HopLinter.java
+++ b/plugins/misc/lint/src/main/java/org/apache/hop/lint/HopLinter.java
@@ -584,8 +584,14 @@ public class HopLinter {
throws HopException {
List<LintResult> results = new ArrayList<>(policyResults);
if (shouldIncludeNativeChecks() && hopObject != null) {
+ ensureEffectiveRuleSet();
results.addAll(
- HopNativeCheckRunner.runNativeChecks(hopObject, fileName, variables,
metadataProvider));
+ HopNativeCheckRunner.runNativeChecks(
+ hopObject,
+ fileName,
+ variables,
+ metadataProvider,
+ new
NativeCheckClassifier(effectiveRuleSet.getNativeVerifyRules())));
}
// Suppressions are applied last, so they cover Hop's native remarks as
well as policy
// findings — a team accepting something should not have to care which
produced it.
@@ -703,8 +709,12 @@ public class HopLinter {
private List<LintResult> runPolicyRulesInternal(Object hopObject, String
fileName) {
List<LintResult> results = new ArrayList<>();
ensureEffectiveRuleSet();
+ // A rule is handed one transform at a time, and a transform does not know
its pipeline. This
+ // is how a rule about the way an element is connected can be answered,
and so report the
+ // element rather than the file.
+ CustomRuleExecutor.setSubject(hopObject);
try {
- for (CustomLintRule customRule : effectiveRuleSet.getEnabledRules()) {
+ for (CustomLintRule customRule :
effectiveRuleSet.getEnabledPolicyRules()) {
List<LintResult> customResults =
CustomRuleExecutor.executeRule(customRule, hopObject, fileName);
results.addAll(customResults);
@@ -746,6 +756,8 @@ public class HopLinter {
}
} catch (Exception e) {
log.logError("Error executing custom rules on file " + fileName + ": " +
e.getMessage(), e);
+ } finally {
+ CustomRuleExecutor.setSubject(null);
}
return results;
}
diff --git
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/HopNativeCheckRunner.java
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/HopNativeCheckRunner.java
index c8f92e213f..78d804a03e 100644
---
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/HopNativeCheckRunner.java
+++
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/HopNativeCheckRunner.java
@@ -39,6 +39,22 @@ public final class HopNativeCheckRunner {
IVariables variables,
IHopMetadataProvider metadataProvider)
throws HopException {
+ return runNativeChecks(hopObject, fileName, variables, metadataProvider,
null);
+ }
+
+ /**
+ * Run Hop's own checks and report what the project's native rules say to
report.
+ *
+ * @param classifier the native rules in force, or null to report every
remark as the transform
+ * wrote it
+ */
+ public static List<LintResult> runNativeChecks(
+ Object hopObject,
+ String fileName,
+ IVariables variables,
+ IHopMetadataProvider metadataProvider,
+ NativeCheckClassifier classifier)
+ throws HopException {
List<ICheckResult> remarks = new ArrayList<>();
IProgressMonitor monitor = new NullProgressMonitor();
@@ -59,7 +75,7 @@ public final class HopNativeCheckRunner {
}
});
- return LintCheckResultAdapter.fromCheckResults(remarks, fileName);
+ return LintCheckResultAdapter.fromCheckResults(remarks, fileName,
classifier);
}
private static final class NullProgressMonitor implements IProgressMonitor {
diff --git
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintCheckResultAdapter.java
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintCheckResultAdapter.java
index 24742982e1..c9633d9f88 100644
---
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintCheckResultAdapter.java
+++
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintCheckResultAdapter.java
@@ -63,12 +63,24 @@ public final class LintCheckResultAdapter {
}
public static List<LintResult> fromCheckResults(List<ICheckResult> remarks,
String fileName) {
+ return fromCheckResults(remarks, fileName, null);
+ }
+
+ /**
+ * Convert Hop's own verify remarks, letting the project's native rules
decide how each is
+ * reported.
+ *
+ * @param classifier the native rules in force, or null to keep every remark
as the transform
+ * wrote it
+ */
+ public static List<LintResult> fromCheckResults(
+ List<ICheckResult> remarks, String fileName, NativeCheckClassifier
classifier) {
List<LintResult> results = new ArrayList<>();
if (remarks == null) {
return results;
}
for (ICheckResult remark : remarks) {
- LintResult lintResult = fromCheckResult(remark, fileName);
+ LintResult lintResult = fromCheckResult(remark, fileName, classifier);
if (lintResult != null) {
results.add(lintResult);
}
@@ -77,11 +89,31 @@ public final class LintCheckResultAdapter {
}
public static LintResult fromCheckResult(ICheckResult remark, String
fileName) {
+ return fromCheckResult(remark, fileName, null);
+ }
+
+ public static LintResult fromCheckResult(
+ ICheckResult remark, String fileName, NativeCheckClassifier classifier) {
if (remark == null || remark.getType() == ICheckResult.TYPE_RESULT_OK) {
return null;
}
+ String severity = LintSeverity.fromCheckResultType(remark.getType());
String ruleId = remark.getErrorCode();
+
+ if (classifier != null && !classifier.isEmpty()) {
+ NativeCheckClassifier.Classification classification =
classifier.classify(remark);
+ if (classification == null) {
+ // A rule that names this check and is switched off: the project has
said the check is
+ // not one it wants to hear about, so there is no finding at all.
+ return null;
+ }
+ severity = classification.severity();
+ if (!Utils.isEmpty(classification.ruleId())) {
+ ruleId = classification.ruleId();
+ }
+ }
+
if (Utils.isEmpty(ruleId)) {
ruleId = "HOP-CHECK";
}
@@ -89,7 +121,6 @@ public final class LintCheckResultAdapter {
LintSourceRef sourceRef = sourceFromCheckResult(remark.getSourceInfo());
String ruleName =
remark.getSourceInfo() != null ? remark.getSourceInfo().getName() :
"Hop verify";
- String severity = LintSeverity.fromCheckResultType(remark.getType());
return new LintResult(
ruleId,
diff --git
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/NativeCheckClassifier.java
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/NativeCheckClassifier.java
new file mode 100644
index 0000000000..dee87b80ab
--- /dev/null
+++
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/NativeCheckClassifier.java
@@ -0,0 +1,220 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hop.lint;
+
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.hop.core.ICheckResult;
+import org.apache.hop.core.ICheckResultSource;
+import org.apache.hop.core.util.Utils;
+import org.apache.hop.i18n.BaseMessages;
+import org.apache.hop.pipeline.transform.TransformMeta;
+import org.apache.hop.workflow.action.ActionMeta;
+
+/**
+ * Decides how the linter reports a remark from Hop's own {@code check()}
methods.
+ *
+ * <p>Those remarks were written for the Verify button, where a person asked
the question and reads
+ * every answer in context. The linter asks it unprompted, on every save and
on every build, and a
+ * remark it repeats is a remark it stands behind. The two are not the same
claim, so the severity a
+ * transform chose is not automatically the severity the linter reports:
{@code check()} works from
+ * a row stream inferred statically at design time, which is right for a plain
pipeline and wrong
+ * whenever metadata injection, a mapping or a runtime-populated stream is
involved.
+ *
+ * <p>So native remarks pass through the rules like everything else. The core
pack caps them at
+ * warning and drops the two checks known to be incorrect; a project raises,
lowers or silences them
+ * by id in its own {@code hop-lint.yml}.
+ *
+ * @see <a href="https://github.com/apache/hop/issues/8294";>#8294</a>
+ */
+public final class NativeCheckClassifier {
+
+ /** What a matching rule says should happen to a remark. */
+ public record Classification(String severity, String ruleId) {}
+
+ private final List<CustomLintRule> rules;
+
+ public NativeCheckClassifier(List<CustomLintRule> rules) {
+ List<CustomLintRule> nativeRules = new ArrayList<>();
+ if (rules != null) {
+ for (CustomLintRule rule : rules) {
+ if (rule != null && rule.isNativeVerify()) {
+ nativeRules.add(rule);
+ }
+ }
+ }
+ this.rules = nativeRules;
+ }
+
+ /** Whether any rule speaks about native remarks at all. */
+ public boolean isEmpty() {
+ return rules.isEmpty();
+ }
+
+ /**
+ * How to report one remark, or null when the rules say to drop it.
+ *
+ * <p>With no rule matching, the remark keeps the severity the transform
gave it. That is what a
+ * pack which says nothing about native checks should mean, and it is what
the linter did before
+ * the core pack had an opinion.
+ */
+ public Classification classify(ICheckResult remark) {
+ if (remark == null) {
+ return null;
+ }
+ CustomLintRule match = bestMatch(remark);
+ if (match == null) {
+ return new
Classification(LintSeverity.fromCheckResultType(remark.getType()), null);
+ }
+ if (!match.isEnabled()) {
+ return null;
+ }
+ return new Classification(match.getSeverity(), match.generateRuleId());
+ }
+
+ /**
+ * The most specific rule that covers this remark.
+ *
+ * <p>Specificity is what lets the pack hold both a blanket "native remarks
are warnings" and a
+ * "this one check is wrong, drop it" without the order of the YAML deciding
which wins. A rule
+ * naming the check beats one naming only the plugin, which beats the
blanket rule.
+ */
+ private CustomLintRule bestMatch(ICheckResult remark) {
+ CustomLintRule best = null;
+ int bestScore = -1;
+ for (CustomLintRule rule : rules) {
+ int score = score(rule, remark);
+ if (score > bestScore) {
+ best = rule;
+ bestScore = score;
+ }
+ }
+ return bestScore < 0 ? null : best;
+ }
+
+ /** How specifically the rule matches, or -1 when it does not apply. */
+ private static int score(CustomLintRule rule, ICheckResult remark) {
+ int score = 0;
+ if (!rule.getAppliesTo().isEmpty()) {
+ String pluginId = pluginIdOf(remark.getSourceInfo());
+ if (Utils.isEmpty(pluginId) || !containsIgnoreCase(rule.getAppliesTo(),
pluginId)) {
+ return -1;
+ }
+ score += 1;
+ }
+ if (!Utils.isEmpty(rule.getMessageKey())) {
+ if (!printsMessage(remark.getText(), rule.getMessageKey(),
bundleClassOf(remark))) {
+ return -1;
+ }
+ score += 2;
+ }
+ return score;
+ }
+
+ /**
+ * Whether the remark is the one that message key prints.
+ *
+ * <p>Matching the resolved message rather than a pattern is what keeps this
working outside
+ * English: the key is resolved in the running locale, so the rule names the
check itself instead
+ * of naming the English words a check happens to use. A key that resolves
to nothing — the plugin
+ * is not installed, or the key was renamed — matches nothing rather than
everything, so a stale
+ * rule loses its narrowing instead of silencing every remark.
+ *
+ * @param text the remark as the transform built it, usually a heading
followed by detail lines
+ * @param messageKey {@code <i18n package>:<key>}, the same form Hop's own
plugin annotations use
+ * @param bundleClass the class whose class loader holds the bundle, or null
+ */
+ static boolean printsMessage(String text, String messageKey, Class<?>
bundleClass) {
+ if (Utils.isEmpty(text)) {
+ return false;
+ }
+ int separator = messageKey.lastIndexOf(':');
+ if (separator <= 0 || separator == messageKey.length() - 1) {
+ return false;
+ }
+ String packageName = messageKey.substring(0, separator).trim();
+ String key = messageKey.substring(separator + 1).trim();
+ String message = resolve(packageName, key, bundleClass);
+ if (Utils.isEmpty(message)) {
+ return false;
+ }
+ return text.contains(message.trim());
+ }
+
+ /**
+ * The message, or null when it cannot be resolved.
+ *
+ * <p>A transform in its own plugin folder has its own class loader, and the
bundle is only on
+ * that one, so the lookup goes through the class the remark came from first
and falls back to the
+ * core loader — the order the plugin registry uses to resolve the same
{@code package:key} form
+ * in a plugin's annotations.
+ */
+ private static String resolve(String packageName, String key, Class<?>
bundleClass) {
+ try {
+ if (bundleClass != null) {
+ String message = BaseMessages.getString(packageName, key, bundleClass);
+ if (!unresolved(message)) {
+ return message;
+ }
+ }
+ String message = BaseMessages.getString(packageName, key);
+ return unresolved(message) ? null : message;
+ } catch (Exception e) {
+ return null;
+ }
+ }
+
+ /**
+ * BaseMessages answers {@code !key!} for a key it cannot resolve. A rule
whose key has been
+ * renamed, or whose plugin is not installed, then matches nothing rather
than everything: it
+ * loses its narrowing instead of silencing every remark.
+ */
+ private static boolean unresolved(String message) {
+ return Utils.isEmpty(message) || (message.startsWith("!") &&
message.endsWith("!"));
+ }
+
+ /** The class whose loader can see the bundle the remark's message came
from. */
+ private static Class<?> bundleClassOf(ICheckResult remark) {
+ ICheckResultSource source = remark.getSourceInfo();
+ if (source instanceof TransformMeta transformMeta &&
transformMeta.getTransform() != null) {
+ return transformMeta.getTransform().getClass();
+ }
+ if (source instanceof ActionMeta actionMeta && actionMeta.getAction() !=
null) {
+ return actionMeta.getAction().getClass();
+ }
+ return null;
+ }
+
+ private static String pluginIdOf(ICheckResultSource source) {
+ if (source instanceof TransformMeta transformMeta) {
+ return transformMeta.getTransformPluginId();
+ }
+ if (source instanceof ActionMeta actionMeta) {
+ return actionMeta.getAction() != null ?
actionMeta.getAction().getPluginId() : null;
+ }
+ return null;
+ }
+
+ private static boolean containsIgnoreCase(List<String> values, String
candidate) {
+ for (String value : values) {
+ if (value != null && value.trim().equalsIgnoreCase(candidate)) {
+ return true;
+ }
+ }
+ return false;
+ }
+}
diff --git
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/EffectiveRuleSet.java
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/EffectiveRuleSet.java
index b6dae14759..8f17447167 100644
---
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/EffectiveRuleSet.java
+++
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/EffectiveRuleSet.java
@@ -65,4 +65,37 @@ public final class EffectiveRuleSet {
}
return enabled;
}
+
+ /**
+ * The enabled rules the linter evaluates itself.
+ *
+ * <p>Native rules live in the same list, and are merged and overridden on
the same terms, but
+ * they have no target or condition to evaluate: they say how a remark Hop's
own {@code check()}
+ * already produced should be reported.
+ */
+ public List<CustomLintRule> getEnabledPolicyRules() {
+ List<CustomLintRule> enabled = new ArrayList<>();
+ for (CustomLintRule rule : rules) {
+ if (rule.isEnabled() && !rule.isNativeVerify()) {
+ enabled.add(rule);
+ }
+ }
+ return enabled;
+ }
+
+ /**
+ * The rules covering Hop's own verify remarks, disabled ones included.
+ *
+ * <p>A disabled native rule is not a rule that does nothing: it is the
project saying that the
+ * check it names should not be reported, so the classifier has to see it.
+ */
+ public List<CustomLintRule> getNativeVerifyRules() {
+ List<CustomLintRule> nativeRules = new ArrayList<>();
+ for (CustomLintRule rule : rules) {
+ if (rule.isNativeVerify()) {
+ nativeRules.add(rule);
+ }
+ }
+ return nativeRules;
+ }
}
diff --git
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/ProjectLintYamlExporter.java
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/ProjectLintYamlExporter.java
index 9370176289..3bbd481f93 100644
---
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/ProjectLintYamlExporter.java
+++
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/ProjectLintYamlExporter.java
@@ -91,6 +91,8 @@ public final class ProjectLintYamlExporter {
|| !Objects.equals(desired.getTargetField(),
packDefault.getTargetField())
|| !Objects.equals(desired.getCondition(), packDefault.getCondition())
|| !Objects.equals(desired.getAppliesTo(), packDefault.getAppliesTo())
+ || !Objects.equals(desired.getType(), packDefault.getType())
+ || !Objects.equals(desired.getMessageKey(),
packDefault.getMessageKey())
|| !Objects.equals(desired.getName(), packDefault.getName())
|| !Objects.equals(desired.getDescription(),
packDefault.getDescription());
}
@@ -130,6 +132,9 @@ public final class ProjectLintYamlExporter {
}
private static Map<String, Object> toFullCustomRuleMap(CustomLintRule rule) {
+ if (rule.isNativeVerify()) {
+ return toFullNativeRuleMap(rule);
+ }
Map<String, Object> ruleConfig = new LinkedHashMap<>();
ruleConfig.put("enabled", rule.isEnabled());
ruleConfig.put("severity", rule.getSeverity());
@@ -163,4 +168,27 @@ public final class ProjectLintYamlExporter {
ruleConfig.put("parameters", new
HashMap<>(rule.getAdditionalParameters()));
return ruleConfig;
}
+
+ /**
+ * A native rule written out in full.
+ *
+ * <p>It has no target, field or condition — it says how a remark Hop's own
{@code check()}
+ * produced is reported — so the custom form would write a null target and
read back as a rule
+ * that checks nothing.
+ */
+ private static Map<String, Object> toFullNativeRuleMap(CustomLintRule rule) {
+ Map<String, Object> ruleConfig = new LinkedHashMap<>();
+ ruleConfig.put("type", CustomLintRule.TYPE_NATIVE);
+ ruleConfig.put("enabled", rule.isEnabled());
+ ruleConfig.put("severity", rule.getSeverity());
+ if (!rule.getAppliesTo().isEmpty()) {
+ ruleConfig.put("appliesTo", new ArrayList<>(rule.getAppliesTo()));
+ }
+ if (rule.getMessageKey() != null && !rule.getMessageKey().isEmpty()) {
+ ruleConfig.put("messageKey", rule.getMessageKey());
+ }
+ ruleConfig.put("name", rule.getName());
+ ruleConfig.put("description", rule.getDescription());
+ return ruleConfig;
+ }
}
diff --git
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/YamlRulePackParser.java
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/YamlRulePackParser.java
index 111ab52f8c..9e00b9e16b 100644
---
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/YamlRulePackParser.java
+++
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/YamlRulePackParser.java
@@ -258,7 +258,10 @@ public final class YamlRulePackParser {
String ruleId = entry.getKey();
@SuppressWarnings("unchecked")
Map<String, Object> ruleData = (Map<String, Object>) entry.getValue();
- if (isCustomRuleDefinition(ruleData)) {
+ if (isNativeRuleDefinition(ruleData)) {
+ projectRules.add(
+ parseNativeRule(ruleId, ruleData, RulePackIds.PROJECT,
RulePackOwner.PROJECT));
+ } else if (isCustomRuleDefinition(ruleData)) {
CustomLintRule rule =
parseCustomRule(ruleId, ruleData, RulePackIds.PROJECT,
RulePackOwner.PROJECT);
projectRules.add(rule);
@@ -375,6 +378,10 @@ public final class YamlRulePackParser {
String ruleId = entry.getKey();
@SuppressWarnings("unchecked")
Map<String, Object> ruleData = (Map<String, Object>) entry.getValue();
+ if (isNativeRuleDefinition(ruleData)) {
+ rules.add(parseNativeRule(ruleId, ruleData, metadata.packId(),
metadata.owner()));
+ continue;
+ }
if (!isCustomRuleDefinition(ruleData)) {
continue;
}
@@ -395,6 +402,34 @@ public final class YamlRulePackParser {
return ruleData.containsKey("target") && ruleData.containsKey("condition");
}
+ public static boolean isNativeRuleDefinition(Map<String, Object> ruleData) {
+ return ruleData != null &&
CustomLintRule.TYPE_NATIVE.equals(ruleData.get("type"));
+ }
+
+ /**
+ * Read a rule that says how the linter should report one of Hop's own
verify remarks.
+ *
+ * <p>A native rule has no target or condition: Hop's {@code check()} has
already decided what to
+ * look at, and the rule only decides whether the answer is reported and at
what severity. It
+ * narrows with {@code appliesTo}, on the plugin id of the transform or
action the remark is
+ * about, and with {@code messageKey}, on the message the check prints.
+ */
+ public static CustomLintRule parseNativeRule(
+ String ruleId, Map<String, Object> ruleData, String packId,
RulePackOwner owner) {
+ CustomLintRule rule = new CustomLintRule();
+ rule.setId(ruleId);
+ rule.setPackId(packId);
+ rule.setPackOwner(owner);
+ rule.setType(CustomLintRule.TYPE_NATIVE);
+ rule.setName(stringValue(ruleData.get("name"), ruleId));
+ rule.setDescription(stringValue(ruleData.get("description"), ""));
+ rule.setEnabled(booleanValue(ruleData.get("enabled"), true));
+ rule.setSeverity(stringValue(ruleData.get("severity"), "WARNING"));
+ rule.setAppliesTo(stringListValue(ruleData.get("appliesTo")));
+ rule.setMessageKey(stringValue(ruleData.get("messageKey"), ""));
+ return rule;
+ }
+
public static CustomLintRule parseCustomRule(
String ruleId, Map<String, Object> ruleData, String packId,
RulePackOwner owner) {
CustomLintRule customRule = new CustomLintRule();
diff --git a/plugins/misc/lint/src/main/resources/hop-lint-core.yml
b/plugins/misc/lint/src/main/resources/hop-lint-core.yml
index bb3537cb6d..c44898e300 100644
--- a/plugins/misc/lint/src/main/resources/hop-lint-core.yml
+++ b/plugins/misc/lint/src/main/resources/hop-lint-core.yml
@@ -75,6 +75,7 @@ rules:
- "credentials"
- "apiKey"
- "apikey"
+ - "secretAccessKey"
- "token"
- "accessToken"
- "authToken"
@@ -99,29 +100,34 @@ rules:
- "credentials"
- "apiKey"
- "apikey"
+ - "secretAccessKey"
- "token"
- "accessToken"
- "authToken"
+ # Reported on the transform, not the pipeline, so the finding is something
you can click on.
+ # A pipeline holding one transform and nothing else is not orphaned — there
is nothing for it
+ # to be disconnected from — and neither is a transform whose hops are all
disabled: it has
+ # hops, and whether a disabled hop is a problem is what STRUCT-003 is for.
TRANS-002:
type: custom
enabled: true
severity: WARNING
- target: PIPELINE
- targetField: hasOrphanedTransforms
+ target: TRANSFORM
+ targetField: isOrphaned
condition: MUST_BE_FALSE
name: "Orphaned Transform"
- description: "A transform with no incoming or outgoing hop never executes"
+ description: "This transform has no incoming or outgoing hop, so it never
executes"
WORKFLOW-002:
type: custom
enabled: true
severity: WARNING
- target: WORKFLOW
- targetField: hasOrphanedActions
+ target: ACTION
+ targetField: isOrphaned
condition: MUST_BE_FALSE
name: "Orphaned Action"
- description: "An action with no incoming or outgoing hop never executes"
+ description: "This action has no incoming or outgoing hop, so it never
executes"
NAMING-004:
type: custom
@@ -133,6 +139,34 @@ rules:
name: "Default Transform Name"
description: "Auto-generated names such as 'Transform 1' make logs and
error messages hard to trace back to the pipeline"
+ # ------------------------------------------------------------------
+ # Hop's own verify remarks.
+ #
+ # A `native` rule does not check anything: Hop's transforms and actions have
+ # `check()` methods of their own, and these rules say how the linter reports
+ # what they found. `appliesTo` narrows to a plugin id, `messageKey` to a
+ # single check, named by the message it prints as `<i18n package>:<key>` so
+ # that the rule still matches outside English. The most specific rule wins,
+ # so a blanket rule and an exception for one check can both be in force.
+ # ------------------------------------------------------------------
+
+ HOP-CHECK:
+ type: native
+ enabled: true
+ severity: WARNING
+ name: "Hop Verify Remark"
+ description: >
+ Every remark Hop's own transform and action checks produce. They work
from the row stream
+ inferred at design time, which is right for a plain pipeline and wrong
wherever fields
+ arrive at runtime, so the linter reports them as warnings rather than
letting them fail a
+ build. Set severity to ERROR to treat them as the transform meant them,
or disable this
+ rule to leave Hop's verify remarks to the Verify button.
+
+ # No narrowed rule ships enabled or disabled here. A check that is simply
wrong is fixed in the
+ # transform, which is what #8294 did for the two in Select Values; silencing
it from a rule pack
+ # would have left it firing for everyone who presses Verify. `appliesTo` and
`messageKey` are
+ # for a project that disagrees with a check the platform is right to ship.
+
# ------------------------------------------------------------------
# Worked examples, shipped disabled.
#
diff --git a/plugins/misc/lint/src/main/resources/hop-lint.yml.example
b/plugins/misc/lint/src/main/resources/hop-lint.yml.example
index 4ff9467f10..52c40e0ab0 100644
--- a/plugins/misc/lint/src/main/resources/hop-lint.yml.example
+++ b/plugins/misc/lint/src/main/resources/hop-lint.yml.example
@@ -37,3 +37,12 @@ rules:
enabled: true
severity: ERROR
parameters: {}
+
+ # Hop's own transform and action checks — everything reported as [HOP-CHECK].
+ # The core pack reports them as warnings, because check() works from the row
+ # stream Hop infers at design time and is wrong wherever fields arrive at
+ # runtime. Put the severity back to ERROR to treat them as the transform
+ # meant them, or set enabled: false to leave them to the Verify button.
+ HOP-CHECK:
+ enabled: true
+ severity: WARNING
diff --git
a/plugins/misc/lint/src/test/java/org/apache/hop/lint/HardcodedSecretRuleTest.java
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/HardcodedSecretRuleTest.java
new file mode 100644
index 0000000000..04635dd895
--- /dev/null
+++
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/HardcodedSecretRuleTest.java
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hop.lint;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.hop.pipeline.transform.BaseTransformMeta;
+import org.apache.hop.pipeline.transform.ITransformMeta;
+import org.apache.hop.pipeline.transform.TransformMeta;
+import org.junit.jupiter.api.Test;
+
+/**
+ * What SEC-002 counts as a hardcoded secret.
+ *
+ * <p>The rule matched any field whose name merely contained "token", "secret"
or "password", at
+ * whatever type, so it reported Token Replacement's {@code tokenStartString}
— which defaults to
+ * {@code "${"} — and Get Data From XML's boolean {@code useToken} as leaked
credentials, as errors,
+ * on a project that had nothing wrong with it.
+ *
+ * @see <a href="https://github.com/apache/hop/issues/8294";>#8294</a>
+ */
+public class HardcodedSecretRuleTest {
+
+ /** Stands in for the stock transforms whose field names mention a secret
without holding one. */
+ public static class FakeTransformMeta extends BaseTransformMeta {
+ private String tokenStartString = "${";
+ private String tokenEndString = "}";
+ private String oauth2TokenUrl = "https://example.org/oauth2/token";;
+ private String credentialsFile = "/etc/hop/service-account.json";
+ private boolean useToken = true;
+ private List<String> tokenReplacementFields = List.of("a", "b");
+
+ private String password = "letmein";
+ private String awsSessionToken = "AQoDYXdzEJr...";
+ private String proxyPassword = "${PROXY_PASSWORD}";
+ }
+
+ @Test
+ public void reportsOnlyTheFieldsThatActuallyHoldASecret() {
+ List<String> reported = fieldsReportedFor(new FakeTransformMeta());
+
+ assertTrue(reported.contains("password"), "a plain password is the point
of the rule");
+ assertTrue(reported.contains("awsSessionToken"), "a session token is a
credential");
+ assertEquals(2, reported.size(), "unexpected findings: " + reported);
+ }
+
+ @Test
+ public void aSecretTakenFromAVariableIsNotAFinding() {
+ assertTrue(
+ !fieldsReportedFor(new FakeTransformMeta()).contains("proxyPassword"),
+ "${PROXY_PASSWORD} is exactly what the rule asks people to do");
+ }
+
+ /** The field names that put an error on every Token Replacement and Get
Data From XML step. */
+ @Test
+ public void aFieldNameThatMerelyMentionsASecretIsNotOne() {
+ List<String> reported = fieldsReportedFor(new FakeTransformMeta());
+
+ for (String notASecret :
+ List.of(
+ "tokenStartString",
+ "tokenEndString",
+ "oauth2TokenUrl",
+ "credentialsFile",
+ "useToken",
+ "tokenReplacementFields")) {
+ assertTrue(!reported.contains(notASecret), notASecret + " is not a
credential");
+ }
+ }
+
+ private static List<String> fieldsReportedFor(ITransformMeta meta) {
+ CustomLintRule rule = new CustomLintRule();
+ rule.setId("SEC-002");
+ rule.setName("Hardcoded Password or Secret in Transform");
+ rule.setSeverity("ERROR");
+ rule.setEnabled(true);
+ rule.setTarget(RuleTarget.TRANSFORM);
+ rule.setTargetField("password");
+ rule.setCondition(RuleCondition.NO_HARDCODED);
+
+ TransformMeta transformMeta = new TransformMeta("Fake", "a transform",
meta);
+ List<String> fields = new ArrayList<>();
+ for (LintResult result :
+ CustomRuleExecutor.executeRule(rule, transformMeta,
"/tmp/secrets.hpl")) {
+ fields.add(fieldNameIn(result.getMessage()));
+ }
+ return fields;
+ }
+
+ /** The message names the field it read, between the single quotes after "in
field". */
+ private static String fieldNameIn(String message) {
+ int start = message.indexOf("in field '");
+ if (start < 0) {
+ return message;
+ }
+ start += "in field '".length();
+ return message.substring(start, message.indexOf('\'', start));
+ }
+}
diff --git
a/plugins/misc/lint/src/test/java/org/apache/hop/lint/LintSuppressionInEditorTest.java
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/LintSuppressionInEditorTest.java
index 1dea460569..b7e5629d0d 100644
---
a/plugins/misc/lint/src/test/java/org/apache/hop/lint/LintSuppressionInEditorTest.java
+++
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/LintSuppressionInEditorTest.java
@@ -103,8 +103,16 @@ public class LintSuppressionInEditorTest {
assertEquals(
0,
- results.stream().filter(r -> "Fonte
Sql".equals(sourceName(r))).count(),
+ results.stream()
+ .filter(r -> "Fonte Sql".equals(sourceName(r)) &&
"HOP-CHECK".equals(r.getRuleId()))
+ .count(),
"the accepted finding should be gone from the editor: " + results);
+ // The suppression named one rule, so the linter's own rules still have
their say about that
+ // transform. Silencing everything on it would be a different entry,
without a rule id.
+ assertTrue(
+ results.stream()
+ .anyMatch(r -> "Fonte Sql".equals(sourceName(r)) &&
"TRANS-002".equals(r.getRuleId())),
+ "a suppression naming one rule must not silence the others: " +
results);
assertTrue(
results.stream().anyMatch(r -> "Salva S3".equals(sourceName(r))),
"a suppression naming one transform must not silence the other: " +
results);
diff --git
a/plugins/misc/lint/src/test/java/org/apache/hop/lint/NativeCheckClassifierTest.java
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/NativeCheckClassifierTest.java
new file mode 100644
index 0000000000..4ef31e8c39
--- /dev/null
+++
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/NativeCheckClassifierTest.java
@@ -0,0 +1,287 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hop.lint;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.hop.core.CheckResult;
+import org.apache.hop.core.Const;
+import org.apache.hop.core.ICheckResult;
+import org.apache.hop.core.row.IRowMeta;
+import org.apache.hop.core.row.RowMeta;
+import org.apache.hop.core.row.value.ValueMetaNumber;
+import org.apache.hop.i18n.BaseMessages;
+import org.apache.hop.lint.registry.HopCoreRulePack;
+import org.apache.hop.pipeline.transform.TransformMeta;
+import org.apache.hop.pipeline.transforms.selectvalues.SelectField;
+import org.apache.hop.pipeline.transforms.selectvalues.SelectMetadataChange;
+import org.apache.hop.pipeline.transforms.selectvalues.SelectValuesMeta;
+import org.junit.jupiter.api.Test;
+
+/**
+ * How the linter reports Hop's own verify remarks.
+ *
+ * @see <a href="https://github.com/apache/hop/issues/8294";>#8294</a>
+ */
+public class NativeCheckClassifierTest {
+
+ private static final String SELECT_VALUES_PACKAGE =
+ "org.apache.hop.pipeline.transforms.selectvalues";
+
+ @Test
+ public void remarksAreUntouchedWhenNoRuleSpeaksAboutThem() {
+ NativeCheckClassifier classifier = new NativeCheckClassifier(List.of());
+
+ assertTrue(classifier.isEmpty());
+ assertEquals(
+ "ERROR", classifier.classify(remark(ICheckResult.TYPE_RESULT_ERROR,
"boom")).severity());
+ }
+
+ /**
+ * The finding in the issue: a perfectly good pipeline greeted the user with
five red errors, all
+ * of them Hop's own design-time advice rather than anything the linter
could stand behind.
+ */
+ @Test
+ public void theBlanketRuleCapsEveryRemarkAtItsSeverity() {
+ NativeCheckClassifier classifier =
+ new NativeCheckClassifier(List.of(nativeRule("HOP-CHECK", "WARNING",
true)));
+
+ NativeCheckClassifier.Classification classification =
+ classifier.classify(remark(ICheckResult.TYPE_RESULT_ERROR, "fields not
found"));
+
+ assertNotNull(classification);
+ assertEquals("WARNING", classification.severity());
+ assertEquals("HOP-CHECK", classification.ruleId(), "the finding stays
suppressible by id");
+ }
+
+ @Test
+ public void aProjectCanPutTheRemarksBackToTheSeverityTheTransformMeant() {
+ NativeCheckClassifier classifier =
+ new NativeCheckClassifier(List.of(nativeRule("HOP-CHECK", "ERROR",
true)));
+
+ assertEquals(
+ "ERROR",
+ classifier.classify(remark(ICheckResult.TYPE_RESULT_ERROR, "fields not
found")).severity());
+ }
+
+ @Test
+ public void disablingTheBlanketRuleDropsEveryRemark() {
+ NativeCheckClassifier classifier =
+ new NativeCheckClassifier(List.of(nativeRule("HOP-CHECK", "WARNING",
false)));
+
+ assertNull(classifier.classify(remark(ICheckResult.TYPE_RESULT_ERROR,
"fields not found")));
+ }
+
+ @Test
+ public void aRuleNamingAPluginLeavesOtherPluginsAlone() {
+ CustomLintRule scoped = nativeRule("HOP-CHECK-SV", "INFO", true);
+ scoped.setAppliesTo(List.of("SelectValues"));
+ NativeCheckClassifier classifier =
+ new NativeCheckClassifier(List.of(nativeRule("HOP-CHECK", "WARNING",
true), scoped));
+
+ assertEquals(
+ "INFO",
+ classifier
+ .classify(remark(ICheckResult.TYPE_RESULT_ERROR, "anything",
"SelectValues"))
+ .severity());
+ assertEquals(
+ "WARNING",
+ classifier
+ .classify(remark(ICheckResult.TYPE_RESULT_ERROR, "anything",
"TableInput"))
+ .severity());
+ }
+
+ /**
+ * The narrow rule has to win wherever it sits in the YAML: a pack holds
both "native remarks are
+ * warnings" and "this one check is wrong", and which one applies cannot
depend on file order.
+ */
+ @Test
+ public void theRuleNamingTheCheckWinsOverTheBlanketOne() {
+ List<CustomLintRule> rules =
+ List.of(
+ selectValuesRule(
+ "HOP-CHECK-SELECTVALUES-METADATA",
+ "SelectValuesMeta.CheckResult.MetadataFieldsNotFound",
+ false),
+ nativeRule("HOP-CHECK", "WARNING", true));
+ NativeCheckClassifier classifier = new NativeCheckClassifier(rules);
+
+ assertNull(
+ classifier.classify(metadataFieldsNotFoundRemark()),
+ "the check the project switched off produces no finding at all");
+ assertEquals(
+ "WARNING",
+ classifier
+ .classify(remark(ICheckResult.TYPE_RESULT_ERROR, "something else",
"SelectValues"))
+ .severity(),
+ "the blanket rule still covers the transform's other checks");
+ }
+
+ /** Select Values' remaining checks are real, and stay visible as warnings.
*/
+ @Test
+ public void theCorePackKeepsSelectValuesOtherChecksAsWarnings() {
+ NativeCheckClassifier classifier = new NativeCheckClassifier(new
HopCoreRulePack().loadRules());
+
+ ICheckResult noInput =
+ remark(
+ ICheckResult.TYPE_RESULT_ERROR,
+ BaseMessages.getString(
+ SELECT_VALUES_PACKAGE,
"SelectValuesMeta.CheckResult.NoInputReceivedError"),
+ "SelectValues");
+
+ NativeCheckClassifier.Classification classification =
classifier.classify(noInput);
+ assertNotNull(classification);
+ assertEquals("WARNING", classification.severity());
+ }
+
+ @Test
+ public void aMessageKeyThatCannotBeResolvedNarrowsToNothing() {
+ assertFalse(
+ NativeCheckClassifier.printsMessage(
+ "Meta-data fields that were not found in input stream:",
+ SELECT_VALUES_PACKAGE +
":SelectValuesMeta.CheckResult.RenamedAtSomePoint",
+ null),
+ "an unresolvable key must match nothing rather than everything");
+ assertFalse(NativeCheckClassifier.printsMessage("anything",
"no-separator-here", null));
+ assertFalse(NativeCheckClassifier.printsMessage("", SELECT_VALUES_PACKAGE
+ ":a.key", null));
+ }
+
+ /**
+ * The message key is resolved through the bundle rather than matched as a
pattern, which is what
+ * lets a rule name a check without naming the English words it happens to
use.
+ */
+ @Test
+ public void aMessageKeyIsResolvedAgainstThePluginsOwnBundle() {
+ for (String key :
+ List.of(
+ "SelectValuesMeta.CheckResult.MetadataFieldsNotFound",
+ "SelectValuesMeta.CheckResult.DuplicateFieldsSpecified")) {
+ String message = BaseMessages.getString(SELECT_VALUES_PACKAGE, key);
+ assertFalse(
+ message.startsWith("!") && message.endsWith("!"),
+ "Select Values no longer prints " + key);
+ assertTrue(
+ NativeCheckClassifier.printsMessage(
+ message + Const.CR + Const.CR + "\t\tvalueToSqrt",
+ SELECT_VALUES_PACKAGE + ":" + key,
+ SelectValuesMeta.class));
+ }
+ }
+
+ /**
+ * The transform from the issue, checked by Select Values itself.
+ *
+ * <p>Reproduced rather than hand-written: the point of naming a check by
its message key is that
+ * the rule matches what the transform actually prints, and only running
{@code check()} proves
+ * that. "valueToSqrt" renames "value" on the Select & Alter tab and
then sets the metadata on
+ * the new name, which is ordinary and correct, and names "value" twice,
which is how a value is
+ * copied under a second name.
+ */
+ @Test
+ public void thePipelineFromTheIssueProducesNoErrors() {
+ List<ICheckResult> remarks = new ArrayList<>();
+ TransformMeta transformMeta = new TransformMeta("SelectValues",
"valueToSqrt", null);
+
+ IRowMeta previousRow = new RowMeta();
+ previousRow.addValueMeta(new ValueMetaNumber("value"));
+
+ selectValuesFromTheIssue()
+ .check(
+ remarks,
+ null,
+ transformMeta,
+ previousRow,
+ new String[] {"input"},
+ null,
+ null,
+ null,
+ null);
+
+ List<LintResult> reported =
+ LintCheckResultAdapter.fromCheckResults(
+ remarks,
+ "/tmp/sqrt-mapping.hpl",
+ new NativeCheckClassifier(new HopCoreRulePack().loadRules()));
+
+ assertTrue(
+ reported.isEmpty(),
+ "a pipeline with nothing wrong with it must not be greeted with
anything: " + reported);
+ }
+
+ /** Select Values as the issue's screenshot has it, before any of it reaches
the linter. */
+ private static SelectValuesMeta selectValuesFromTheIssue() {
+ SelectValuesMeta meta = new SelectValuesMeta();
+
+ SelectField copied = new SelectField();
+ copied.setName("value");
+ copied.setRename("valueToSqrt");
+ SelectField kept = new SelectField();
+ kept.setName("value");
+ meta.getSelectOption().setSelectFields(new ArrayList<>(List.of(copied,
kept)));
+
+ // The Metadata tab names the field as it is after the rename, which is
the only name it has
+ // by then. SelectValuesMeta.check() looks for it in the incoming row,
where it is not.
+ SelectMetadataChange metadata = new SelectMetadataChange();
+ metadata.setName("valueToSqrt");
+ meta.getSelectOption().setMeta(new ArrayList<>(List.of(metadata)));
+
+ return meta;
+ }
+
+ private static ICheckResult metadataFieldsNotFoundRemark() {
+ // Built the way SelectValuesMeta.check() builds it: the heading, then the
field names.
+ return remark(
+ ICheckResult.TYPE_RESULT_ERROR,
+ BaseMessages.getString(
+ SELECT_VALUES_PACKAGE,
"SelectValuesMeta.CheckResult.MetadataFieldsNotFound")
+ + Const.CR
+ + Const.CR
+ + "\t\tvalueToSqrt"
+ + Const.CR,
+ "SelectValues");
+ }
+
+ private static ICheckResult remark(int type, String text) {
+ return remark(type, text, "SelectValues");
+ }
+
+ private static ICheckResult remark(int type, String text, String pluginId) {
+ return new CheckResult(type, text, new TransformMeta(pluginId,
"valueToSqrt", null));
+ }
+
+ private static CustomLintRule nativeRule(String id, String severity, boolean
enabled) {
+ CustomLintRule rule = new CustomLintRule();
+ rule.setId(id);
+ rule.setType(CustomLintRule.TYPE_NATIVE);
+ rule.setSeverity(severity);
+ rule.setEnabled(enabled);
+ return rule;
+ }
+
+ private static CustomLintRule selectValuesRule(String id, String key,
boolean enabled) {
+ CustomLintRule rule = nativeRule(id, "WARNING", enabled);
+ rule.setAppliesTo(List.of("SelectValues"));
+ rule.setMessageKey(SELECT_VALUES_PACKAGE + ":" + key);
+ return rule;
+ }
+}
diff --git
a/plugins/misc/lint/src/test/java/org/apache/hop/lint/OrphanedElementRuleTest.java
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/OrphanedElementRuleTest.java
new file mode 100644
index 0000000000..678de50490
--- /dev/null
+++
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/OrphanedElementRuleTest.java
@@ -0,0 +1,114 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hop.lint;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.hop.pipeline.PipelineHopMeta;
+import org.apache.hop.pipeline.PipelineMeta;
+import org.apache.hop.pipeline.transform.TransformMeta;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+
+/**
+ * What TRANS-002 counts as an orphan.
+ *
+ * <p>It used to be answered for the pipeline as a whole, which meant the
warning could not name the
+ * transform it was about, and it counted two things as orphaned that are not:
a pipeline holding a
+ * single transform, and a transform whose hops happen to be disabled.
+ *
+ * @see <a href="https://github.com/apache/hop/issues/8294";>#8294</a>
+ */
+public class OrphanedElementRuleTest {
+
+ @AfterEach
+ public void clearSubject() {
+ CustomRuleExecutor.setSubject(null);
+ }
+
+ @Test
+ public void aTransformConnectedToNothingIsReportedByName() {
+ PipelineMeta pipeline = pipelineOf("read", "write", "left over");
+ connect(pipeline, "read", "write");
+
+ List<LintResult> findings = lint(pipeline);
+
+ assertEquals(1, findings.size(), "only the disconnected transform: " +
findings);
+ assertEquals("left over", findings.get(0).getSource().getName());
+ }
+
+ /** A one-transform pipeline has nothing to be disconnected from. */
+ @Test
+ public void theOnlyTransformInAPipelineIsNotAnOrphan() {
+ assertTrue(lint(pipelineOf("do the thing")).isEmpty());
+ }
+
+ /**
+ * A disabled hop is still a hop. Whether one is a problem is what
STRUCT-003 asks, and that ships
+ * switched off because most teams treat a disabled hop as work in progress.
+ */
+ @Test
+ public void aTransformWhoseHopsAreDisabledIsNotAnOrphan() {
+ PipelineMeta pipeline = pipelineOf("read", "work in progress");
+ connect(pipeline, "read", "work in progress").setEnabled(false);
+
+ assertTrue(lint(pipeline).isEmpty(), "a disabled hop is not the same as no
hop");
+ }
+
+ /** A pipeline of several transforms and no hops at all is still every one
of them. */
+ @Test
+ public void transformsInAPipelineWithNoHopsAreAllOrphans() {
+ assertEquals(3, lint(pipelineOf("a", "b", "c")).size());
+ }
+
+ private static List<LintResult> lint(PipelineMeta pipeline) {
+ CustomLintRule rule = new CustomLintRule();
+ rule.setId("TRANS-002");
+ rule.setName("Orphaned Transform");
+ rule.setSeverity("WARNING");
+ rule.setEnabled(true);
+ rule.setTarget(RuleTarget.TRANSFORM);
+ rule.setTargetField("isOrphaned");
+ rule.setCondition(RuleCondition.MUST_BE_FALSE);
+
+ CustomRuleExecutor.setSubject(pipeline);
+ List<LintResult> findings = new ArrayList<>();
+ for (TransformMeta transformMeta : pipeline.getTransforms()) {
+ findings.addAll(CustomRuleExecutor.executeRule(rule, transformMeta,
"/tmp/orphans.hpl"));
+ }
+ return findings;
+ }
+
+ private static PipelineMeta pipelineOf(String... transformNames) {
+ PipelineMeta pipeline = new PipelineMeta();
+ pipeline.setName("orphans");
+ for (String name : transformNames) {
+ pipeline.addTransform(new TransformMeta("Dummy", name, null));
+ }
+ return pipeline;
+ }
+
+ private static PipelineHopMeta connect(PipelineMeta pipeline, String from,
String to) {
+ PipelineHopMeta hop =
+ new PipelineHopMeta(pipeline.findTransform(from),
pipeline.findTransform(to));
+ pipeline.addPipelineHop(hop);
+ return hop;
+ }
+}
diff --git
a/plugins/misc/lint/src/test/java/org/apache/hop/lint/registry/HopCoreRulePackTest.java
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/registry/HopCoreRulePackTest.java
index 9b7a761df5..00cc602d31 100644
---
a/plugins/misc/lint/src/test/java/org/apache/hop/lint/registry/HopCoreRulePackTest.java
+++
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/registry/HopCoreRulePackTest.java
@@ -41,6 +41,49 @@ public class HopCoreRulePackTest {
"every rule in the core pack must belong to the core pack");
}
+ /**
+ * The pack says how Hop's own verify remarks are reported. Without a
blanket rule the linter
+ * repeats whatever severity a transform's {@code check()} chose, which is
how a clean pipeline
+ * came up with five errors in issue #8294.
+ */
+ @Test
+ public void shipsANativeRuleCoveringHopsOwnVerifyRemarks() {
+ List<CustomLintRule> rules = new HopCoreRulePack().loadRules();
+
+ CustomLintRule blanket =
+ rules.stream()
+ .filter(rule -> "HOP-CHECK".equals(rule.generateRuleId()))
+ .findFirst()
+ .orElseThrow();
+
+ assertTrue(blanket.isNativeVerify());
+ assertTrue(blanket.isEnabled());
+ assertEquals("WARNING", blanket.getSeverity());
+ assertTrue(blanket.getAppliesTo().isEmpty(), "the blanket rule covers
every remark");
+
+ // A check that is simply wrong is fixed in the transform, not silenced
from here: a rule
+ // narrowed to one check would leave it firing for everyone who presses
Verify.
+ assertTrue(
+ rules.stream()
+ .filter(CustomLintRule::isNativeVerify)
+ .allMatch(rule -> rule.getAppliesTo().isEmpty() &&
rule.getMessageKey().isEmpty()),
+ "the core pack does not ship a native rule narrowed to a single
check");
+ }
+
+ /** Native rules are not evaluated against anything, so they must not reach
the executor. */
+ @Test
+ public void nativeRulesAreNotPolicyRules() {
+ List<CustomLintRule> rules = new HopCoreRulePack().loadRules();
+
+ assertTrue(
+ rules.stream().filter(CustomLintRule::isNativeVerify).noneMatch(rule
-> rule.isComposed()));
+ assertTrue(
+ rules.stream()
+ .filter(rule -> !rule.isNativeVerify())
+ .allMatch(rule -> rule.getTarget() != null),
+ "every rule the linter evaluates has something to evaluate it
against");
+ }
+
/**
* The pack ships a composed rule as a worked example of the format, so a
parse regression in the
* allOf/anyOf handling shows up here rather than in someone's project.
@@ -70,6 +113,7 @@ public class HopCoreRulePackTest {
new HopCoreRulePack()
.loadRules().stream()
.filter(CustomLintRule::isEnabled)
+ .filter(rule -> !rule.isNativeVerify())
.map(CustomLintRule::generateRuleId)
.sorted()
.toList();
diff --git
a/plugins/misc/lint/src/test/java/org/apache/hop/lint/registry/RuleRegistryTest.java
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/registry/RuleRegistryTest.java
index 9ab1bbd836..2475f5dd5a 100644
---
a/plugins/misc/lint/src/test/java/org/apache/hop/lint/registry/RuleRegistryTest.java
+++
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/registry/RuleRegistryTest.java
@@ -128,6 +128,76 @@ public class RuleRegistryTest {
assertFalse(yaml.contains("type: custom"));
}
+ /**
+ * Toggling HOP-CHECK in the rule manager is how a project decides it wants
Hop's own verify
+ * remarks back at the severity the transforms meant, or gone. It has to
survive the round trip
+ * through the project's hop-lint.yml — a native rule written out with the
custom rule's shape
+ * would carry a null target and read back as a rule that checks nothing.
+ */
+ @Test
+ public void aNativeRuleSurvivesTheRoundTripThroughTheProjectYaml() throws
Exception {
+ CustomLintRule hopCheck =
+ RuleRegistry.getInstance().resolve(null).getRules().stream()
+ .filter(rule -> "HOP-CHECK".equals(rule.generateRuleId()))
+ .findFirst()
+ .orElseThrow()
+ .copy();
+ hopCheck.setSeverity("ERROR");
+
+ String yaml = ProjectLintYamlExporter.export(java.util.List.of(hopCheck));
+ assertTrue(yaml.contains("HOP-CHECK"));
+ assertTrue(yaml.contains("severity: ERROR"));
+ assertFalse(yaml.contains("target:"), "a native rule has nothing to
evaluate against");
+
+ File projectYaml = File.createTempFile("hop-lint", ".yml");
+ try {
+ Files.writeString(projectYaml.toPath(), yaml);
+ CustomLintRule readBack =
+ RuleRegistry.getInstance().resolve(projectYaml).getRules().stream()
+ .filter(rule -> "HOP-CHECK".equals(rule.generateRuleId()))
+ .findFirst()
+ .orElseThrow();
+
+ assertTrue(readBack.isNativeVerify());
+ assertEquals("ERROR", readBack.getSeverity());
+ } finally {
+ projectYaml.delete();
+ }
+ }
+
+ /** A project can name a check of its own, and the narrowing has to come
back with the rule. */
+ @Test
+ public void aProjectCanDefineItsOwnNativeRule() throws Exception {
+ File projectYaml = File.createTempFile("hop-lint", ".yml");
+ try {
+ Files.writeString(
+ projectYaml.toPath(),
+ """
+ rules:
+ HOP-CHECK-TABLEINPUT-SQL:
+ type: native
+ enabled: false
+ appliesTo:
+ - TableInput
+ messageKey:
"org.apache.hop.pipeline.transforms.tableinput:TableInputMeta.CheckResult.NoInput"
+ name: "Table Input SQL remark"
+ """);
+
+ CustomLintRule rule =
+ RuleRegistry.getInstance().resolve(projectYaml).getRules().stream()
+ .filter(r ->
"HOP-CHECK-TABLEINPUT-SQL".equals(r.generateRuleId()))
+ .findFirst()
+ .orElseThrow();
+
+ assertTrue(rule.isNativeVerify());
+ assertFalse(rule.isEnabled());
+ assertEquals(java.util.List.of("TableInput"), rule.getAppliesTo());
+
assertTrue(rule.getMessageKey().endsWith(":TableInputMeta.CheckResult.NoInput"));
+ } finally {
+ projectYaml.delete();
+ }
+ }
+
@Test
public void projectYamlCanDefineAComposedRule() throws Exception {
File projectYaml = File.createTempFile("hop-lint", ".yml");
diff --git
a/plugins/transforms/selectvalues/src/main/java/org/apache/hop/pipeline/transforms/selectvalues/SelectValuesMeta.java
b/plugins/transforms/selectvalues/src/main/java/org/apache/hop/pipeline/transforms/selectvalues/SelectValuesMeta.java
index 899fcc973a..8fd6978b21 100644
---
a/plugins/transforms/selectvalues/src/main/java/org/apache/hop/pipeline/transforms/selectvalues/SelectValuesMeta.java
+++
b/plugins/transforms/selectvalues/src/main/java/org/apache/hop/pipeline/transforms/selectvalues/SelectValuesMeta.java
@@ -121,6 +121,23 @@ public class SelectValuesMeta extends
BaseTransformMeta<SelectValues, SelectValu
transformMeta);
remarks.add(cr);
+ // The three tabs run in order, each on the row the one before it
produced. Checking all of
+ // them against the incoming row reported a field renamed on "Select &
Alter" as missing on
+ // the tabs that only ever see the new name.
+ IRowMeta afterSelect = prev;
+ IRowMeta afterDelete = prev;
+ try {
+ afterSelect = prev.clone();
+ getSelectFields(afterSelect, transformMeta.getName());
+ afterDelete = afterSelect.clone();
+ getDeleteFields(afterDelete);
+ } catch (HopTransformException e) {
+ // The rows cannot be worked out, so the later tabs are checked
against the incoming row
+ // rather than not at all. No worse than having no answer.
+ afterSelect = prev;
+ afterDelete = prev;
+ }
+
/*
* Take care of the normal SELECT fields...
*/
@@ -155,6 +172,9 @@ public class SelectValuesMeta extends
BaseTransformMeta<SelectValues, SelectValu
}
if (!getSelectOption().getSelectFields().isEmpty()) {
+ errorMessage = "";
+ errorFound = false;
+
// Starting from prev...
for (int i = 0; i < prev.size(); i++) {
IValueMeta pv = prev.getValueMeta(i);
@@ -193,7 +213,7 @@ public class SelectValuesMeta extends
BaseTransformMeta<SelectValues, SelectValu
// Starting from selected fields in ...
for (int i = 0; i < getSelectOption().getDeleteName().size(); i++) {
- int idx =
prev.indexOfValue(getSelectOption().getDeleteName().get(i).getName());
+ int idx =
afterSelect.indexOfValue(getSelectOption().getDeleteName().get(i).getName());
if (idx < 0) {
errorMessage += "\t\t" + getSelectOption().getDeleteName().get(i) +
Const.CR;
errorFound = true;
@@ -227,7 +247,7 @@ public class SelectValuesMeta extends
BaseTransformMeta<SelectValues, SelectValu
// Starting from selected fields in ...
for (int i = 0; i < getSelectOption().getMeta().size(); i++) {
var currentName = getSelectOption().getMeta().get(i).getName();
- int idx = prev.indexOfValue(currentName);
+ int idx = afterDelete.indexOfValue(currentName);
if (idx < 0) {
errorMessage += "\t\t" + currentName + Const.CR;
errorFound = true;
@@ -277,41 +297,35 @@ public class SelectValuesMeta extends
BaseTransformMeta<SelectValues, SelectValu
remarks.add(cr);
}
- // Check for doubles in the selected fields...
- var selectFieldsSize = getSelectOption().getSelectFields().size();
- int[] cnt = new int[selectFieldsSize];
+ // Check for doubles in the fields this transform produces.
+ //
+ // Naming the same incoming field twice is not a mistake: it is how a
value is copied under a
+ // second name, which is what the Rename column is for. What downstream
transforms cannot deal
+ // with is two fields arriving under the same name, so that is what is
counted here.
+ var selectFields = getSelectOption().getSelectFields();
boolean errorFound = false;
String errorMessage = "";
- for (int i = 0; i < selectFieldsSize; i++) {
- cnt[i] = 0;
- for (int j = 0; j < selectFieldsSize; j++) {
- if (getSelectOption()
- .getSelectFields()
- .get(i)
- .getName()
- .equals(getSelectOption().getSelectFields().get(j).getName())) {
- cnt[i]++;
+ for (int i = 0; i < selectFields.size(); i++) {
+ String outputName = outputNameOf(selectFields.get(i));
+ int occurrences = 0;
+ for (SelectField other : selectFields) {
+ if (outputName.equals(outputNameOf(other))) {
+ occurrences++;
}
}
- if (cnt[i] > 1) {
+ if (occurrences > 1) {
if (!errorFound) { // first time...
errorMessage =
BaseMessages.getString(PKG,
"SelectValuesMeta.CheckResult.DuplicateFieldsSpecified")
+ Const.CR;
- } else {
- errorFound = true;
}
errorMessage +=
BaseMessages.getString(
PKG,
"SelectValuesMeta.CheckResult.OccurentRow",
- i
- + " : "
- + getSelectOption().getSelectFields().get(i).getName()
- + " ("
- + cnt[i])
+ i + " : " + outputName + " (" + occurrences)
+ Const.CR;
errorFound = true;
}
@@ -322,6 +336,11 @@ public class SelectValuesMeta extends
BaseTransformMeta<SelectValues, SelectValu
}
}
+ /** The name the field leaves this transform under: the rename when there is
one, else its own. */
+ private static String outputNameOf(SelectField field) {
+ return Utils.isEmpty(field.getRename()) ? Const.NVL(field.getName(), "") :
field.getRename();
+ }
+
@Override
public boolean supportsErrorHandling() {
return true;
diff --git
a/plugins/transforms/selectvalues/src/test/java/org/apache/hop/pipeline/transforms/selectvalues/SelectValuesMetaCheckTest.java
b/plugins/transforms/selectvalues/src/test/java/org/apache/hop/pipeline/transforms/selectvalues/SelectValuesMetaCheckTest.java
new file mode 100644
index 0000000000..ae704e052f
--- /dev/null
+++
b/plugins/transforms/selectvalues/src/test/java/org/apache/hop/pipeline/transforms/selectvalues/SelectValuesMetaCheckTest.java
@@ -0,0 +1,158 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hop.pipeline.transforms.selectvalues;
+
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.hop.core.ICheckResult;
+import org.apache.hop.core.row.IRowMeta;
+import org.apache.hop.core.row.RowMeta;
+import org.apache.hop.core.row.value.ValueMetaNumber;
+import org.apache.hop.core.row.value.ValueMetaString;
+import org.apache.hop.pipeline.transform.TransformMeta;
+import org.junit.jupiter.api.Test;
+
+/**
+ * What Verify says about a Select Values transform.
+ *
+ * <p>The three tabs run in order, each on the row the one before it produced,
and the checks used
+ * to compare all of them against the incoming row. A field renamed on "Select
& Alter" was
+ * therefore reported as missing on the tabs that only ever see its new name.
+ *
+ * @see <a href="https://github.com/apache/hop/issues/8294";>#8294</a>
+ */
+public class SelectValuesMetaCheckTest {
+
+ /** The transform from the issue: rename a field, then set the metadata on
the new name. */
+ @Test
+ public void aFieldRenamedOnTheSelectTabIsFoundByTheMetadataTab() {
+ SelectValuesMeta meta = new SelectValuesMeta();
+ meta.getSelectOption()
+ .setSelectFields(new ArrayList<>(List.of(select("value",
"valueToSqrt"))));
+ meta.getSelectOption().setMeta(new
ArrayList<>(List.of(metadataChange("valueToSqrt"))));
+
+ assertNoProblem(check(meta, rowOf("value")));
+ }
+
+ /** The Remove tab runs after the rename too, so it sees the new name as
well. */
+ @Test
+ public void aFieldRenamedOnTheSelectTabIsFoundByTheRemoveTab() {
+ SelectValuesMeta meta = new SelectValuesMeta();
+ meta.getSelectOption()
+ .setSelectFields(new ArrayList<>(List.of(select("value",
"valueToSqrt"), select("id"))));
+ DeleteField delete = new DeleteField();
+ delete.setName("valueToSqrt");
+ meta.getSelectOption().setDeleteName(new ArrayList<>(List.of(delete)));
+
+ assertNoProblem(check(meta, rowOf("value", "id")));
+ }
+
+ /** Naming the same field twice is how a value is copied under a second
name. */
+ @Test
+ public void selectingTheSameFieldTwiceUnderDifferentNamesIsNotAProblem() {
+ SelectValuesMeta meta = new SelectValuesMeta();
+ meta.getSelectOption()
+ .setSelectFields(new ArrayList<>(List.of(select("value",
"valueToSqrt"), select("value"))));
+
+ assertNoProblem(check(meta, rowOf("value")));
+ }
+
+ /** What downstream transforms genuinely cannot deal with: two fields under
one name. */
+ @Test
+ public void twoFieldsLeavingUnderTheSameNameIsStillReported() {
+ SelectValuesMeta meta = new SelectValuesMeta();
+ meta.getSelectOption()
+ .setSelectFields(
+ new ArrayList<>(List.of(select("value", "amount"), select("total",
"amount"))));
+
+ assertTrue(
+ problems(check(meta, rowOf("value", "total"))).stream()
+ .anyMatch(text -> text.contains("amount")),
+ "two fields renamed to 'amount' is a real problem");
+ }
+
+ /** A field that really is absent is still reported, on every tab. */
+ @Test
+ public void afieldThatIsNowhereIsStillReported() {
+ SelectValuesMeta meta = new SelectValuesMeta();
+ meta.getSelectOption().setSelectFields(new
ArrayList<>(List.of(select("value"))));
+ meta.getSelectOption().setMeta(new
ArrayList<>(List.of(metadataChange("noSuchField"))));
+
+ assertTrue(
+ problems(check(meta, rowOf("value"))).stream()
+ .anyMatch(text -> text.contains("noSuchField")),
+ "a metadata change on a field that does not exist is a real problem");
+ }
+
+ private static List<ICheckResult> check(SelectValuesMeta meta, IRowMeta
previousRow) {
+ List<ICheckResult> remarks = new ArrayList<>();
+ meta.check(
+ remarks,
+ null,
+ new TransformMeta("SelectValues", "valueToSqrt", meta),
+ previousRow,
+ new String[] {"input"},
+ null,
+ null,
+ null,
+ null);
+ return remarks;
+ }
+
+ private static List<String> problems(List<ICheckResult> remarks) {
+ List<String> texts = new ArrayList<>();
+ for (ICheckResult remark : remarks) {
+ if (remark.getType() != ICheckResult.TYPE_RESULT_OK) {
+ texts.add(remark.getText());
+ }
+ }
+ return texts;
+ }
+
+ private static void assertNoProblem(List<ICheckResult> remarks) {
+ List<String> problems = problems(remarks);
+ assertTrue(problems.isEmpty(), "nothing is wrong with this transform: " +
problems);
+ }
+
+ private static IRowMeta rowOf(String... names) {
+ IRowMeta row = new RowMeta();
+ for (String name : names) {
+ row.addValueMeta(
+ "value".equals(name) ? new ValueMetaNumber(name) : new
ValueMetaString(name));
+ }
+ return row;
+ }
+
+ private static SelectField select(String name) {
+ return select(name, null);
+ }
+
+ private static SelectField select(String name, String rename) {
+ SelectField field = new SelectField();
+ field.setName(name);
+ field.setRename(rename);
+ return field;
+ }
+
+ private static SelectMetadataChange metadataChange(String name) {
+ SelectMetadataChange change = new SelectMetadataChange();
+ change.setName(name);
+ return change;
+ }
+}
