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 802351a16f Fixes #8814 : Lint: let rules carry tags, and write them to
SARIF and… (#8824)
802351a16f is described below
commit 802351a16f272bdbdf6b840fdcf592c30d215087
Author: Bart Maertens <[email protected]>
AuthorDate: Sat Oct 10 11:08:46 2026 +0200
Fixes #8814 : Lint: let rules carry tags, and write them to SARIF and…
(#8824)
* Fixes #8814 : Lint: let rules carry tags, and write them to SARIF and JSON
- Rules take an optional tags: map (string or list values) and helpUri:.
The linter never evaluates tags; every finding carries them.
- SARIF: properties.tags as key:value strings, properties.hopTags as a map,
fullDescription and helpUri on the rule.
- JSON: a tags object on each finding.
- A project's hop-lint.yml can set tags on a pack rule, replacing per key;
a key with no values removes the pack's tag.
- Unknown keys on a rule or an override are logged as warnings, with the
closest known key suggested.
- Hop GUI: Help URL field and Tags table in the rule editor, Tags column
in the rule manager. Editing a pack rule's tags writes an override.
- Docs: tags and help links in lint-rules, lint-rule-packs and hop-lint.
* Issue #8814 : Clamp rule editor to the screen, name hop-lint.yml in
warnings
---
.../modules/ROOT/pages/hop-tools/hop-lint.adoc | 1 +
.../ROOT/pages/linting/lint-rule-packs.adoc | 3 +-
.../modules/ROOT/pages/linting/lint-rules.adoc | 46 +++
.../java/org/apache/hop/lint/CustomLintRule.java | 32 +++
.../org/apache/hop/lint/CustomRuleExecutor.java | 15 +-
.../apache/hop/lint/LintCheckResultAdapter.java | 22 +-
.../java/org/apache/hop/lint/LintReportWriter.java | 72 +++--
.../main/java/org/apache/hop/lint/LintResult.java | 35 +++
.../java/org/apache/hop/lint/LintRuleDetails.java | 92 ++++++
.../org/apache/hop/lint/NativeCheckClassifier.java | 19 +-
.../org/apache/hop/lint/RuleBuilderDialog.java | 132 ++++++++-
.../org/apache/hop/lint/RuleManagerDialog.java | 5 +
.../org/apache/hop/lint/registry/LintWarnings.java | 47 +++
.../hop/lint/registry/ProjectLintYamlExporter.java | 53 ++++
.../hop/lint/registry/ProjectYamlOverlay.java | 44 ++-
.../org/apache/hop/lint/registry/RuleRegistry.java | 17 +-
.../hop/lint/registry/YamlRulePackParser.java | 202 ++++++++++++-
.../hop/lint/messages/messages_en_US.properties | 7 +
.../apache/hop/lint/CustomRuleExecutorTest.java | 23 ++
.../org/apache/hop/lint/LintReportWriterTest.java | 125 ++++++++
.../apache/hop/lint/NativeCheckClassifierTest.java | 50 ++++
.../hop/lint/RuleBuilderDialogNativeRuleTest.java | 34 +++
.../lint/registry/YamlRulePackParserTagsTest.java | 316 +++++++++++++++++++++
23 files changed, 1329 insertions(+), 63 deletions(-)
diff --git a/docs/hop-user-manual/modules/ROOT/pages/hop-tools/hop-lint.adoc
b/docs/hop-user-manual/modules/ROOT/pages/hop-tools/hop-lint.adoc
index 02bd08af91..60f8a277ba 100644
--- a/docs/hop-user-manual/modules/ROOT/pages/hop-tools/hop-lint.adoc
+++ b/docs/hop-user-manual/modules/ROOT/pages/hop-tools/hop-lint.adoc
@@ -134,6 +134,7 @@ hop lint --max-warnings 20 .
`sarif` is the format GitHub code scanning, Azure DevOps and most review
tooling read, so findings show up as annotations on the pull request.
`json` is there for everything else.
+Both carry the tags a rule pack gives its rules; see
xref:linting/lint-rules.adoc#tags[Tags and help links].
Stdout carries only the report, in every format, so it can be piped or
redirected.
Hop's log lines and notes such as the baseline count go to stderr.
diff --git
a/docs/hop-user-manual/modules/ROOT/pages/linting/lint-rule-packs.adoc
b/docs/hop-user-manual/modules/ROOT/pages/linting/lint-rule-packs.adoc
index 3eaf7d69d8..7096b430eb 100644
--- a/docs/hop-user-manual/modules/ROOT/pages/linting/lint-rule-packs.adoc
+++ b/docs/hop-user-manual/modules/ROOT/pages/linting/lint-rule-packs.adoc
@@ -54,6 +54,7 @@ rules:
----
The rules themselves are written exactly as described in
xref:linting/lint-rules.adoc[Lint rules].
+Give rules `tags` and a `helpUri` to carry your own category, owner or policy
reference into the SARIF and JSON reports, as described in
xref:linting/lint-rules.adoc#tags[Tags and help links].
[cols="1,3", options="header"]
|===
@@ -120,5 +121,5 @@ Rule ids are matched ignoring case.
An id that no installed pack defines, such as a typo, is reported as a warning
in the Hop log and by `hop lint`, with the closest id as a suggestion, and the
override is ignored.
The rule manager in Hop Gui writes these overrides for you.
-Editing a pack rule there never changes the pack: a change to its severity,
threshold or enabled state is written to `hop-lint.yml` as an override, and a
change to what the rule actually looks at is written there as a complete rule
which replaces the pack rule of that id.
+Editing a pack rule there never changes the pack: a change to its severity,
threshold, enabled state, tags or help URL is written to `hop-lint.yml` as an
override, and a change to what the rule actually looks at is written there as a
complete rule which replaces the pack rule of that id.
Pack rules cannot be deleted, only switched off.
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 ca65acdb39..946cd07826 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
@@ -292,6 +292,52 @@ suppress:
reason: "Fields in these templates arrive through metadata injection"
----
+[[tags]]
+== Tags and help links
+
+A rule can carry `tags`: labels a team or vendor attaches to it, such as a
category, the team that owns it or the policy it enforces.
+Each value is a string or a list of strings.
+
+[source,yaml]
+----
+rules:
+ ACME-002:
+ type: custom
+ # ...the rule as before...
+ helpUri: https://wiki.example.com/lint/ACME-002
+ tags:
+ category: secrets
+ owner: platform-team
+ policy: ["SEC-POL-4", "SEC-POL-7"]
+----
+
+The linter never evaluates tags, so changing them cannot change what a rule
finds.
+They are written to the reports, on every finding the rule produces:
+
+* `sarif`: on the rule, `properties.tags` holds one `key:value` string per
value (`category:secrets`, `policy:SEC-POL-4`, `policy:SEC-POL-7`), the form
GitHub code scanning and Azure DevOps read, and `properties.hopTags` holds the
same tags as a map. The rule's `description` is written as `fullDescription`,
and `helpUri` as `helpUri`.
+* `json`: each finding has a `tags` object. Every value is written as a list,
also a single one.
+
+Tags work the same on a native rule.
+
+A project sets tags on any rule, a pack's included, in its own `hop-lint.yml`.
+Each key replaces the pack's values for that key and leaves the pack's other
tags as they are; a key with no values removes the pack's tag:
+
+[source,yaml]
+----
+rules:
+ DB-001:
+ tags:
+ owner: data-team # replaces the pack's owner, if it has one
+ policy: [] # removes the pack's policy tag
+ helpUri: https://wiki.example.com/lint/DB-001
+----
+
+In Hop Gui, the rule editor has a *Help URL* field and a *Tags* table with one
row per key and value; repeat a key to give it several values.
+Saving a pack rule there writes only the changed keys to `hop-lint.yml`, as
above.
+The rule manager lists each rule's tags in its *Tags* column.
+
+A key the linter does not know, such as `tag:` for `tags:`, is logged as a
warning and ignored.
+
== 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.
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 abba36736e..69b337de63 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
@@ -18,6 +18,7 @@ package org.apache.hop.lint;
import java.util.ArrayList;
import java.util.HashMap;
+import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.UUID;
@@ -74,6 +75,18 @@ public class CustomLintRule {
*/
private String messageKey;
+ /**
+ * Free-form labels a pack attaches to the rule: a category, an owning team,
a policy reference.
+ *
+ * <p>The linter never evaluates them. They are copied onto every finding
the rule produces and
+ * written to the SARIF and JSON reports, so a pack can change its tags
without any risk of
+ * changing its findings. Each key holds one or more values, in the order
the pack wrote them.
+ */
+ private Map<String, List<String>> tags = new LinkedHashMap<>();
+
+ /** Where a reader can learn more about the rule, written to SARIF as {@code
helpUri}. */
+ private String helpUri;
+
/** A rule the linter evaluates against a pipeline, workflow or metadata
object. */
public static final String TYPE_CUSTOM = "custom";
@@ -284,6 +297,22 @@ public class CustomLintRule {
this.messageKey = messageKey;
}
+ public Map<String, List<String>> getTags() {
+ return tags;
+ }
+
+ public void setTags(Map<String, List<String>> tags) {
+ this.tags = tags != null ? tags : new LinkedHashMap<>();
+ }
+
+ public String getHelpUri() {
+ return helpUri;
+ }
+
+ public void setHelpUri(String helpUri) {
+ this.helpUri = helpUri;
+ }
+
/**
* Whether this rule classifies Hop's own verify remarks instead of being
evaluated by the linter.
*
@@ -320,6 +349,9 @@ public class CustomLintRule {
copy.combinator = this.combinator;
copy.type = this.type;
copy.messageKey = this.messageKey;
+ copy.helpUri = this.helpUri;
+ copy.tags = new LinkedHashMap<>();
+ this.tags.forEach((key, values) -> copy.tags.put(key, new
ArrayList<>(values)));
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 94386a913f..6bef6b582c 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
@@ -263,13 +263,14 @@ public class CustomRuleExecutor {
private static LintResult createResult(
CustomLintRule rule, String message, String fileName, Object hopObject,
String severity) {
return new LintResult(
- rule.generateRuleId(),
- rule.getName(),
- severity,
- message,
- fileName,
- sourceFrom(hopObject),
- LintResult.Origin.LINT);
+ rule.generateRuleId(),
+ rule.getName(),
+ severity,
+ message,
+ fileName,
+ sourceFrom(hopObject),
+ LintResult.Origin.LINT)
+ .withRuleDetails(LintRuleDetails.of(rule));
}
/**
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 4b7c59487b..fb5d8908a6 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
@@ -102,6 +102,7 @@ public final class LintCheckResultAdapter {
String severity = LintSeverity.fromCheckResultType(remark.getType());
String ruleId = remark.getErrorCode();
List<String> aliasRuleIds = new ArrayList<>();
+ LintRuleDetails ruleDetails = LintRuleDetails.NONE;
if (classifier != null && !classifier.isEmpty()) {
NativeCheckClassifier.Classification classification =
classifier.classify(remark);
@@ -111,6 +112,7 @@ public final class LintCheckResultAdapter {
return null;
}
severity = classification.severity();
+ ruleDetails = classification.ruleDetails();
String classifyingRule = classification.ruleId();
if (!Utils.isEmpty(classifyingRule)) {
if (Utils.isEmpty(ruleId)) {
@@ -125,6 +127,9 @@ public final class LintCheckResultAdapter {
// The blanket rule names every remark, and taking its id would
collapse them all into
// one. It is kept alongside, so what a project wrote against it
still applies.
aliasRuleIds.add(classifyingRule);
+ // The rule's tags apply to the remark; its description and help
link describe the
+ // rule, not this code.
+ ruleDetails = ruleDetails.tagsOnly();
}
}
// A narrowed rule took the id above, so the rule covering every remark
would stop naming
@@ -144,14 +149,15 @@ public final class LintCheckResultAdapter {
remark.getSourceInfo() != null ? remark.getSourceInfo().getName() :
"Hop verify";
return new LintResult(
- ruleId,
- ruleName,
- severity,
- remark.getText(),
- fileName,
- sourceRef,
- LintResult.Origin.HOP_NATIVE,
- aliasRuleIds);
+ ruleId,
+ ruleName,
+ severity,
+ remark.getText(),
+ fileName,
+ sourceRef,
+ LintResult.Origin.HOP_NATIVE,
+ aliasRuleIds)
+ .withRuleDetails(ruleDetails);
}
private static String formatCheckText(LintResult lintResult) {
diff --git
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintReportWriter.java
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintReportWriter.java
index e2b660a33d..cfbf4bcc08 100644
--- a/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintReportWriter.java
+++ b/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintReportWriter.java
@@ -122,6 +122,7 @@ public final class LintReportWriter {
source.put("kind", result.getSource().getKind().name());
source.put("name", result.getSource().getName());
}
+ putTags(finding.putObject("tags"), result.getRuleDetails());
}
return mapper.writerWithDefaultPrettyPrinter().writeValueAsString(root) +
"\n";
}
@@ -144,25 +145,48 @@ public final class LintReportWriter {
driver.put("version", toolVersion);
}
- // SARIF wants each rule declared once, then referenced by index from
every result.
- Map<String, Integer> ruleIndex = new LinkedHashMap<>();
- ArrayNode rules = driver.putArray("rules");
+ // SARIF wants each rule declared once, then referenced by index from
every result. The rule is
+ // described by the first finding of that id which carries the rule's
details, so a finding
+ // without them, such as a remark checked without the native rules, cannot
hide the tags.
+ Map<String, LintResult> describing = new LinkedHashMap<>();
for (LintResult result : results) {
- ruleIndex.computeIfAbsent(
- result.getRuleId(),
- id -> {
- ObjectNode rule = rules.addObject();
- rule.put("id", id);
- // The id is the only stable per-rule name available: Hop's own
verify remarks all share
- // one rule id while carrying the transform name as their result
name, so using that
- // here would label the rule after whichever transform happened to
be reported first.
- rule.put("name", id);
- rule.putObject("shortDescription")
- .put("text", result.getRuleName() != null ?
result.getRuleName() : id);
- rule.putObject("defaultConfiguration").put("level",
sarifLevel(result.getSeverity()));
- return rules.size() - 1;
- });
+ LintResult current = describing.get(result.getRuleId());
+ if (current == null
+ || (LintRuleDetails.NONE.equals(current.getRuleDetails())
+ && !LintRuleDetails.NONE.equals(result.getRuleDetails()))) {
+ describing.put(result.getRuleId(), result);
+ }
}
+ Map<String, Integer> ruleIndex = new LinkedHashMap<>();
+ ArrayNode rules = driver.putArray("rules");
+ describing.forEach(
+ (id, result) -> {
+ ruleIndex.put(id, rules.size());
+ ObjectNode rule = rules.addObject();
+ rule.put("id", id);
+ // The id is the only stable per-rule name available: Hop's own
verify remarks all share
+ // one rule id while carrying the transform name as their result
name, so using that
+ // here would label the rule after whichever transform happened to
be reported first.
+ rule.put("name", id);
+ rule.putObject("shortDescription")
+ .put("text", result.getRuleName() != null ? result.getRuleName()
: id);
+ LintRuleDetails details = result.getRuleDetails();
+ if (!details.description().isBlank()) {
+ rule.putObject("fullDescription").put("text",
details.description());
+ }
+ if (details.helpUri() != null && !details.helpUri().isBlank()) {
+ rule.put("helpUri", details.helpUri());
+ }
+ rule.putObject("defaultConfiguration").put("level",
sarifLevel(result.getSeverity()));
+ if (details.hasTags()) {
+ ObjectNode properties = rule.putObject("properties");
+ // GitHub code scanning and Azure DevOps read tags as a flat list
of strings, so a
+ // key with several values becomes several key:value entries.
+ ArrayNode flat = properties.putArray("tags");
+ LintRuleDetails.flatTags(details.tags()).forEach(flat::add);
+ putTags(properties.putObject("hopTags"), details);
+ }
+ });
ArrayNode sarifResults = run.putArray("results");
for (LintResult result : results) {
@@ -197,6 +221,20 @@ public final class LintReportWriter {
return result.getSource().getName() + ": " + result.getMessage();
}
+ /**
+ * Write a rule's tags as an object of arrays. Every value is an array, also
a single one, so a
+ * consumer reads one shape whatever the pack wrote.
+ */
+ private static void putTags(ObjectNode target, LintRuleDetails details) {
+ details
+ .tags()
+ .forEach(
+ (key, values) -> {
+ ArrayNode array = target.putArray(key);
+ values.forEach(array::add);
+ });
+ }
+
private static String sarifLevel(String severity) {
if ("ERROR".equalsIgnoreCase(severity)) {
return "error";
diff --git
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintResult.java
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintResult.java
index bc74abd849..6b1bc11372 100644
--- a/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintResult.java
+++ b/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintResult.java
@@ -36,6 +36,7 @@ public class LintResult {
private final LintSourceRef source;
private final Origin origin;
private final List<String> aliasRuleIds;
+ private final LintRuleDetails ruleDetails;
public LintResult(
String ruleId, String ruleName, String severity, String message, String
fileName) {
@@ -79,6 +80,28 @@ public class LintResult {
LintSourceRef source,
Origin origin,
List<String> aliasRuleIds) {
+ this(
+ ruleId,
+ ruleName,
+ severity,
+ message,
+ fileName,
+ source,
+ origin,
+ aliasRuleIds,
+ LintRuleDetails.NONE);
+ }
+
+ private LintResult(
+ String ruleId,
+ String ruleName,
+ String severity,
+ String message,
+ String fileName,
+ LintSourceRef source,
+ Origin origin,
+ List<String> aliasRuleIds,
+ LintRuleDetails ruleDetails) {
this.ruleId = ruleId;
this.ruleName = ruleName;
this.severity = severity;
@@ -87,6 +110,13 @@ public class LintResult {
this.source = source;
this.origin = origin != null ? origin : Origin.LINT;
this.aliasRuleIds = aliasRuleIds == null ? Collections.emptyList() :
List.copyOf(aliasRuleIds);
+ this.ruleDetails = ruleDetails != null ? ruleDetails :
LintRuleDetails.NONE;
+ }
+
+ /** This finding, carrying the description, help link and tags of the rule
that produced it. */
+ public LintResult withRuleDetails(LintRuleDetails details) {
+ return new LintResult(
+ ruleId, ruleName, severity, message, fileName, source, origin,
aliasRuleIds, details);
}
public String getRuleId() {
@@ -117,6 +147,11 @@ public class LintResult {
return origin;
}
+ /** The description, help link and tags of the rule that produced this
finding. */
+ public LintRuleDetails getRuleDetails() {
+ return ruleDetails;
+ }
+
/**
* The other id this finding answers to, or null.
*
diff --git
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintRuleDetails.java
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintRuleDetails.java
new file mode 100644
index 0000000000..c7b05f9c67
--- /dev/null
+++ b/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintRuleDetails.java
@@ -0,0 +1,92 @@
+/*
+ * 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.Collections;
+import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * What a finding carries about the rule that produced it, for the reports to
write out.
+ *
+ * <p>None of it changes what the rule checks: it is the rule's description,
where to read more
+ * about it, and the tags its pack gave it.
+ *
+ * @param description the rule's description, empty when it has none
+ * @param helpUri where to read more about the rule, or null
+ * @param tags the rule's tags, each key holding one or more values
+ */
+public record LintRuleDetails(String description, String helpUri, Map<String,
List<String>> tags) {
+
+ public static final LintRuleDetails NONE = new LintRuleDetails("", null,
Map.of());
+
+ public LintRuleDetails {
+ description = description != null ? description : "";
+ tags = copyOf(tags);
+ }
+
+ /** The details of a rule, or {@link #NONE} when there is no rule. */
+ public static LintRuleDetails of(CustomLintRule rule) {
+ if (rule == null) {
+ return NONE;
+ }
+ return new LintRuleDetails(rule.getDescription(), rule.getHelpUri(),
rule.getTags());
+ }
+
+ /**
+ * Only the tags, for a finding reported under an id other than the rule's
own.
+ *
+ * <p>A remark from Hop's own checks with an error code of its own is
reported under that code.
+ * The tags of the native rule that classified it apply to it, but that
rule's description and
+ * help link are about the rule, not about the code.
+ */
+ public LintRuleDetails tagsOnly() {
+ return hasTags() ? new LintRuleDetails("", null, tags) : NONE;
+ }
+
+ public boolean hasTags() {
+ return !tags.isEmpty();
+ }
+
+ /**
+ * The tags as one {@code key:value} string per value, the form SARIF
consumers read and the rule
+ * manager shows.
+ */
+ public static List<String> flatTags(Map<String, List<String>> tags) {
+ List<String> flat = new ArrayList<>();
+ if (tags != null) {
+ tags.forEach((key, values) -> values.forEach(value -> flat.add(key + ":"
+ value)));
+ }
+ return flat;
+ }
+
+ private static Map<String, List<String>> copyOf(Map<String, List<String>>
tags) {
+ if (tags == null || tags.isEmpty()) {
+ return Map.of();
+ }
+ // Insertion order is kept so the reports list tags as the pack wrote
them. A value given twice
+ // is kept once: SARIF requires the tags of a rule to be unique.
+ Map<String, List<String>> copy = new LinkedHashMap<>();
+ tags.forEach(
+ (key, values) ->
+ copy.put(key, values != null ? List.copyOf(new
LinkedHashSet<>(values)) : List.of()));
+ return Collections.unmodifiableMap(copy);
+ }
+}
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
index a3571965e3..2a2c8ad737 100644
---
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
@@ -49,9 +49,14 @@ public final class NativeCheckClassifier {
* What a matching rule says should happen to a remark.
*
* @param narrowed whether the rule names a plugin or a check, rather than
every remark
+ * @param ruleDetails the description, help link and tags of the matching
rule
*/
public record Classification(
- String severity, String ruleId, boolean narrowed, String blanketRuleId)
{}
+ String severity,
+ String ruleId,
+ boolean narrowed,
+ String blanketRuleId,
+ LintRuleDetails ruleDetails) {}
/** Where MessageFormat left a value out: {@code {0}}, {@code {1}}, and so
on. */
private static final Pattern PLACEHOLDER = Pattern.compile("\\{\\d+\\}");
@@ -89,7 +94,11 @@ public final class NativeCheckClassifier {
CustomLintRule match = bestMatch(remark);
if (match == null) {
return new Classification(
- LintSeverity.fromCheckResultType(remark.getType()), null, false,
null);
+ LintSeverity.fromCheckResultType(remark.getType()),
+ null,
+ false,
+ null,
+ LintRuleDetails.NONE);
}
if (!match.isEnabled()) {
return null;
@@ -103,7 +112,11 @@ public final class NativeCheckClassifier {
// naming this finding, and a project's existing suppression of it would
quietly lapse the
// moment that project named the check. It is carried along as another
name.
return new Classification(
- severity, match.generateRuleId(), narrowed, narrowed ? blanketRuleId()
: null);
+ severity,
+ match.generateRuleId(),
+ narrowed,
+ narrowed ? blanketRuleId() : null,
+ LintRuleDetails.of(match));
}
/**
diff --git
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/RuleBuilderDialog.java
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/RuleBuilderDialog.java
index cb864aa625..2ba1d817c9 100644
--- a/plugins/misc/lint/src/main/java/org/apache/hop/lint/RuleBuilderDialog.java
+++ b/plugins/misc/lint/src/main/java/org/apache/hop/lint/RuleBuilderDialog.java
@@ -17,12 +17,19 @@
package org.apache.hop.lint;
import java.util.ArrayList;
+import java.util.LinkedHashMap;
import java.util.List;
+import java.util.Map;
import org.apache.hop.core.util.Utils;
+import org.apache.hop.core.variables.Variables;
import org.apache.hop.i18n.BaseMessages;
+import org.apache.hop.ui.core.PropsUi;
+import org.apache.hop.ui.core.widget.ColumnInfo;
+import org.apache.hop.ui.core.widget.TableView;
import org.eclipse.swt.SWT;
import org.eclipse.swt.events.SelectionAdapter;
import org.eclipse.swt.events.SelectionEvent;
+import org.eclipse.swt.graphics.Rectangle;
import org.eclipse.swt.layout.FormAttachment;
import org.eclipse.swt.layout.FormData;
import org.eclipse.swt.layout.FormLayout;
@@ -63,6 +70,8 @@ public class RuleBuilderDialog extends Dialog {
private Table clauseTable;
private Button addClauseButton;
private Button removeClauseButton;
+ private Text helpUriText;
+ private TableView tagsTable;
/**
* The clauses being edited. Always at least one; a rule which checks one
thing has exactly one.
@@ -88,12 +97,21 @@ public class RuleBuilderDialog extends Dialog {
populateControls();
shell.pack();
- // Roomy enough for the clause table and its buttons on top of the fields
that were here
- // before; the dialog is resizable if a rule grows more clauses than that.
- shell.setSize(700, 660);
- shell.setLocation(
- parent.getLocation().x + (parent.getSize().x - 700) / 2,
- parent.getLocation().y + (parent.getSize().y - 660) / 2);
+ // Roomy enough for the clause table, the fields and the tags table, but
never taller than the
+ // screen: on a laptop display an 820 pixel dialog put OK and Cancel out
of reach. Below the
+ // minimum the fixed rows would run into the button bar, so the dialog
does not shrink past it.
+ // The whole dialog stays inside the work area, wherever the Hop window
sits: clamping only the
+ // top edge still put the buttons off-screen below a window in the lower
half of the screen.
+ Rectangle screen = parent.getMonitor().getClientArea();
+ int width = Math.min(700, screen.width);
+ int height = Math.min(820, screen.height);
+ int x = parent.getLocation().x + (parent.getSize().x - width) / 2;
+ int y = parent.getLocation().y + (parent.getSize().y - height) / 2;
+ x = Math.max(screen.x, Math.min(x, screen.x + screen.width - width));
+ y = Math.max(screen.y, Math.min(y, screen.y + screen.height - height));
+ shell.setMinimumSize(Math.min(560, width), Math.min(700, height));
+ shell.setSize(width, height);
+ shell.setLocation(x, y);
shell.open();
Display display = parent.getDisplay();
@@ -347,6 +365,22 @@ public class RuleBuilderDialog extends Dialog {
enabledData.top = new FormAttachment(severityCombo, margin);
enabledCheck.setLayoutData(enabledData);
+ // Help URL, written to SARIF as helpUri
+ Label helpUriLabel = new Label(shell, SWT.RIGHT);
+ helpUriLabel.setText(BaseMessages.getString(PKG,
"RuleBuilderDialog.Label.HelpUri"));
+ FormData helpUriLabelData = new FormData();
+ helpUriLabelData.left = new FormAttachment(0, margin);
+ helpUriLabelData.right = new FormAttachment(0, labelWidth);
+ helpUriLabelData.top = new FormAttachment(enabledCheck, margin);
+ helpUriLabel.setLayoutData(helpUriLabelData);
+
+ helpUriText = new Text(shell, SWT.BORDER);
+ FormData helpUriData = new FormData();
+ helpUriData.left = new FormAttachment(helpUriLabel, margin);
+ helpUriData.right = new FormAttachment(100, -margin);
+ helpUriData.top = new FormAttachment(enabledCheck, margin);
+ helpUriText.setLayoutData(helpUriData);
+
// Buttons
Button okButton = new Button(shell, SWT.PUSH);
okButton.setText(BaseMessages.getString(PKG,
"RuleBuilderDialog.Button.Ok"));
@@ -378,6 +412,45 @@ public class RuleBuilderDialog extends Dialog {
cancelData.width = 80;
cancelButton.setLayoutData(cancelData);
+ // Tags: one row per key and value, so a value never needs escaping. The
table takes whatever
+ // height is left between the help URL and the buttons, so resizing the
dialog grows it rather
+ // than pushing it under the buttons.
+ Label tagsLabel = new Label(shell, SWT.RIGHT);
+ tagsLabel.setText(BaseMessages.getString(PKG,
"RuleBuilderDialog.Label.Tags"));
+ tagsLabel.setToolTipText(BaseMessages.getString(PKG,
"RuleBuilderDialog.Tags.Tooltip"));
+ FormData tagsLabelData = new FormData();
+ tagsLabelData.left = new FormAttachment(0, margin);
+ tagsLabelData.right = new FormAttachment(0, labelWidth);
+ tagsLabelData.top = new FormAttachment(helpUriText, margin);
+ tagsLabel.setLayoutData(tagsLabelData);
+
+ ColumnInfo[] tagColumns = {
+ new ColumnInfo(
+ BaseMessages.getString(PKG, "RuleBuilderDialog.Column.TagKey"),
+ ColumnInfo.COLUMN_TYPE_TEXT,
+ false),
+ new ColumnInfo(
+ BaseMessages.getString(PKG, "RuleBuilderDialog.Column.TagValue"),
+ ColumnInfo.COLUMN_TYPE_TEXT,
+ false)
+ };
+ tagsTable =
+ new TableView(
+ Variables.getADefaultVariableSpace(),
+ shell,
+ SWT.BORDER | SWT.FULL_SELECTION | SWT.MULTI,
+ tagColumns,
+ 1,
+ null,
+ PropsUi.getInstance());
+ tagsTable.setToolTipText(BaseMessages.getString(PKG,
"RuleBuilderDialog.Tags.Tooltip"));
+ FormData tagsData = new FormData();
+ tagsData.left = new FormAttachment(tagsLabel, margin);
+ tagsData.right = new FormAttachment(100, -margin);
+ tagsData.top = new FormAttachment(helpUriText, margin);
+ tagsData.bottom = new FormAttachment(okButton, -margin);
+ tagsTable.setLayoutData(tagsData);
+
shell.setDefaultButton(okButton);
}
@@ -401,6 +474,8 @@ public class RuleBuilderDialog extends Dialog {
}
}
enabledCheck.setSelection(rule.isEnabled());
+ helpUriText.setText(rule.getHelpUri() != null ? rule.getHelpUri() : "");
+ populateTags();
loadClausesFromRule();
// A native rule, such as HOP-CHECK, says how Hop's own verify remarks are
reported. It has no
@@ -491,6 +566,13 @@ public class RuleBuilderDialog extends Dialog {
showError("Rule name is required");
return false;
}
+ // A row with only a key or only a value is not a tag. Saving would drop
it without a word.
+ for (TableItem item : tagsTable.getNonEmptyItems()) {
+ if (item.getText(1).trim().isEmpty() ||
item.getText(2).trim().isEmpty()) {
+ showError(BaseMessages.getString(PKG,
"RuleBuilderDialog.Tags.Incomplete"));
+ return false;
+ }
+ }
if (rule.isNativeVerify()) {
return true;
}
@@ -522,6 +604,9 @@ public class RuleBuilderDialog extends Dialog {
rule.setDescription(descriptionText.getText());
rule.setSeverity(severityCombo.getText());
rule.setEnabled(enabledCheck.getSelection());
+ rule.setHelpUri(
+ Utils.isEmpty(helpUriText.getText().trim()) ? null :
helpUriText.getText().trim());
+ rule.setTags(readTags());
if (rule.isNativeVerify()) {
return;
}
@@ -545,6 +630,41 @@ public class RuleBuilderDialog extends Dialog {
rule.setAdditionalClauses(rest);
}
+ private void populateTags() {
+ tagsTable.clearAll(false);
+ rule.getTags()
+ .forEach(
+ (key, values) -> {
+ for (String value : values) {
+ TableItem item = new TableItem(tagsTable.table, SWT.NONE);
+ item.setText(1, key);
+ item.setText(2, value);
+ }
+ });
+ tagsTable.removeEmptyRows();
+ tagsTable.setRowNums();
+ tagsTable.optWidth(true);
+ }
+
+ /**
+ * The tags as entered, grouped by key in the order the keys first appear.
{@link #validate()} has
+ * refused rows with only a key or only a value; a row entered twice is kept
once.
+ */
+ private Map<String, List<String>> readTags() {
+ Map<String, List<String>> tags = new LinkedHashMap<>();
+ for (TableItem item : tagsTable.getNonEmptyItems()) {
+ String key = item.getText(1).trim();
+ String value = item.getText(2).trim();
+ if (!key.isEmpty() && !value.isEmpty()) {
+ List<String> values = tags.computeIfAbsent(key, k -> new
ArrayList<>());
+ if (!values.contains(value)) {
+ values.add(value);
+ }
+ }
+ }
+ return tags;
+ }
+
/** Load the rule's clauses into the table, selecting the first. */
private void loadClausesFromRule() {
editingClauses.clear();
diff --git
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/RuleManagerDialog.java
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/RuleManagerDialog.java
index a9f673b649..fff060060e 100644
--- a/plugins/misc/lint/src/main/java/org/apache/hop/lint/RuleManagerDialog.java
+++ b/plugins/misc/lint/src/main/java/org/apache/hop/lint/RuleManagerDialog.java
@@ -171,6 +171,10 @@ public class RuleManagerDialog extends Dialog {
enabledColumn.setText(BaseMessages.getString(PKG,
"RuleManagerDialog.Column.Enabled"));
enabledColumn.setWidth(80);
+ TableColumn tagsColumn = new TableColumn(rulesTable, SWT.LEFT);
+ tagsColumn.setText(BaseMessages.getString(PKG,
"RuleManagerDialog.Column.Tags"));
+ tagsColumn.setWidth(200);
+
FormData tableData = new FormData();
tableData.left = new FormAttachment(0, margin);
tableData.right = new FormAttachment(100, -(BUTTON_PANEL_WIDTH + 2 *
margin));
@@ -304,6 +308,7 @@ public class RuleManagerDialog extends Dialog {
}
item.setText(7, rule.getSeverity() != null ? rule.getSeverity() :
"WARNING");
item.setText(8, rule.isEnabled() ? "✓" : "✗");
+ item.setText(9, String.join(", ",
LintRuleDetails.flatTags(rule.getTags())));
item.setData(rule);
}
}
diff --git
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/LintWarnings.java
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/LintWarnings.java
new file mode 100644
index 0000000000..2a92a85ae5
--- /dev/null
+++
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/LintWarnings.java
@@ -0,0 +1,47 @@
+/*
+ * 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.registry;
+
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import org.apache.hop.core.logging.LogChannel;
+
+/**
+ * Logs a configuration warning prominently once, and quietly after that.
+ *
+ * <p>Rule packs and the project's hop-lint.yml are read again on every lint
run, and the editor
+ * lints on a timer, so logging each time repeated the same line for as long
as the file kept the
+ * mistake.
+ */
+final class LintWarnings {
+
+ private static final Set<String> logged = ConcurrentHashMap.newKeySet();
+
+ private LintWarnings() {}
+
+ /**
+ * @param key what makes this warning the same as one already logged
+ * @param warning the line to log
+ */
+ static void logOnce(String key, String warning) {
+ if (logged.add(key)) {
+ LogChannel.GENERAL.logMinimal(warning);
+ } else {
+ LogChannel.GENERAL.logDetailed(warning);
+ }
+ }
+}
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 443adcb106..d5f3b34417 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
@@ -99,6 +99,8 @@ public final class ProjectLintYamlExporter {
|| !Objects.equals(desired.getSeverity(), packDefault.getSeverity())
|| !Objects.equals(desired.getAdditionalParameters(),
packDefault.getAdditionalParameters())
|| !Objects.equals(desired.getConditionValue(),
packDefault.getConditionValue())
+ || !Objects.equals(desired.getHelpUri(), packDefault.getHelpUri())
+ || !Objects.equals(desired.getTags(), packDefault.getTags())
|| structurallyDiffersFromPackDefault(desired, packDefault);
}
@@ -155,9 +157,43 @@ public final class ProjectLintYamlExporter {
&& !desired.getAdditionalParameters().isEmpty()) {
ruleConfig.put("parameters", new
HashMap<>(desired.getAdditionalParameters()));
}
+ if (!Objects.equals(desired.getHelpUri(), packDefault.getHelpUri())) {
+ // An empty value is how a project removes the pack's link.
+ ruleConfig.put("helpUri", desired.getHelpUri() != null ?
desired.getHelpUri() : "");
+ }
+ Map<String, Object> tags = tagOverrides(desired.getTags(),
packDefault.getTags());
+ if (!tags.isEmpty()) {
+ ruleConfig.put("tags", tags);
+ }
return ruleConfig;
}
+ /**
+ * Only the tag keys the project changed. A key the project removed is
written with no values, so
+ * the pack's tag is dropped, while the pack's other tags, and any it adds
later, still apply.
+ */
+ static Map<String, Object> tagOverrides(
+ Map<String, List<String>> desired, Map<String, List<String>>
packDefault) {
+ Map<String, Object> overrides = new LinkedHashMap<>();
+ desired.forEach(
+ (key, values) -> {
+ if (!values.equals(packDefault.get(key))) {
+ overrides.put(key, tagYamlValue(values));
+ }
+ });
+ for (String key : packDefault.keySet()) {
+ if (!desired.containsKey(key)) {
+ overrides.put(key, new ArrayList<>());
+ }
+ }
+ return overrides;
+ }
+
+ /** One value is written as a plain string, several as a list, the way a
person writes them. */
+ private static Object tagYamlValue(List<String> values) {
+ return values.size() == 1 ? values.get(0) : new ArrayList<>(values);
+ }
+
private static Map<String, Object> toFullCustomRuleMap(CustomLintRule rule) {
if (rule.isNativeVerify()) {
return toFullNativeRuleMap(rule);
@@ -196,9 +232,25 @@ public final class ProjectLintYamlExporter {
// Omitted when empty, like appliesTo: "parameters: {}" on every rule
was noise in the diff.
ruleConfig.put("parameters", new
HashMap<>(rule.getAdditionalParameters()));
}
+ putDocumentation(ruleConfig, rule);
return ruleConfig;
}
+ /**
+ * Write the rule's help link and tags, so a rule saved from the rule
manager keeps them. Both are
+ * omitted when absent, like appliesTo.
+ */
+ private static void putDocumentation(Map<String, Object> ruleConfig,
CustomLintRule rule) {
+ if (rule.getHelpUri() != null && !rule.getHelpUri().isEmpty()) {
+ ruleConfig.put("helpUri", rule.getHelpUri());
+ }
+ if (!rule.getTags().isEmpty()) {
+ Map<String, Object> tags = new LinkedHashMap<>();
+ rule.getTags().forEach((key, values) -> tags.put(key,
tagYamlValue(values)));
+ ruleConfig.put("tags", tags);
+ }
+ }
+
/**
* A native rule written out in full.
*
@@ -219,6 +271,7 @@ public final class ProjectLintYamlExporter {
}
ruleConfig.put("name", rule.getName());
ruleConfig.put("description", rule.getDescription());
+ putDocumentation(ruleConfig, rule);
return ruleConfig;
}
}
diff --git
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/ProjectYamlOverlay.java
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/ProjectYamlOverlay.java
index f7db88aa9a..cbb787c497 100644
---
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/ProjectYamlOverlay.java
+++
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/ProjectYamlOverlay.java
@@ -16,6 +16,7 @@
*/
package org.apache.hop.lint.registry;
+import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Map;
@@ -66,16 +67,34 @@ public final class ProjectYamlOverlay {
private final String severity;
private final String conditionValue;
private final Map<String, Object> parameters;
+ private final String helpUri;
+
+ /**
+ * Tags the project sets on the pack's rule, by key. A key replaces the
pack's values for that
+ * key and leaves its other tags alone; a key with no values removes the
pack's tag.
+ */
+ private final Map<String, List<String>> tags;
private ProjectRuleOverlay(
- Boolean enabled, String severity, String conditionValue, Map<String,
Object> parameters) {
+ Boolean enabled,
+ String severity,
+ String conditionValue,
+ Map<String, Object> parameters,
+ String helpUri,
+ Map<String, List<String>> tags) {
this.enabled = enabled;
this.severity = severity;
this.conditionValue = conditionValue;
this.parameters = parameters;
+ this.helpUri = helpUri;
+ this.tags = tags != null ? tags : Collections.emptyMap();
}
- public static ProjectRuleOverlay fromMap(Map<String, Object> ruleData) {
+ /**
+ * @param location where the override is written, for the warnings
+ */
+ public static ProjectRuleOverlay fromMap(
+ String ruleId, Map<String, Object> ruleData, String location) {
Boolean enabled = ruleData.containsKey("enabled") ? (Boolean)
ruleData.get("enabled") : null;
String severity =
ruleData.containsKey("severity") ?
String.valueOf(ruleData.get("severity")) : null;
@@ -87,7 +106,15 @@ public final class ProjectYamlOverlay {
: null;
@SuppressWarnings("unchecked")
Map<String, Object> parameters = (Map<String, Object>)
ruleData.get("parameters");
- return new ProjectRuleOverlay(enabled, severity, conditionValue,
parameters);
+ // A key with no value, like an empty one, removes the pack's link.
+ String helpUri = null;
+ if (ruleData.containsKey("helpUri")) {
+ Object value = ruleData.get("helpUri");
+ helpUri = value != null ? value.toString().trim() : "";
+ }
+ Map<String, List<String>> tags =
+ YamlRulePackParser.tagsValue(ruleData.get("tags"), ruleId, location,
true);
+ return new ProjectRuleOverlay(enabled, severity, conditionValue,
parameters, helpUri, tags);
}
public void applyTo(org.apache.hop.lint.CustomLintRule rule) {
@@ -103,6 +130,17 @@ public final class ProjectYamlOverlay {
if (parameters != null && !parameters.isEmpty()) {
rule.getAdditionalParameters().putAll(parameters);
}
+ if (helpUri != null) {
+ rule.setHelpUri(helpUri.isEmpty() ? null : helpUri);
+ }
+ tags.forEach(
+ (key, values) -> {
+ if (values.isEmpty()) {
+ rule.getTags().remove(key);
+ } else {
+ rule.getTags().put(key, new ArrayList<>(values));
+ }
+ });
}
public RuleConfig toRuleConfig() {
diff --git
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/RuleRegistry.java
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/RuleRegistry.java
index 30b1452506..fd807c1b6f 100644
---
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/RuleRegistry.java
+++
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/RuleRegistry.java
@@ -22,8 +22,6 @@ import java.util.Collection;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
-import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
import org.apache.commons.lang3.exception.ExceptionUtils;
import org.apache.hop.core.logging.LogChannel;
import org.apache.hop.core.util.Utils;
@@ -61,13 +59,6 @@ public class RuleRegistry {
*/
private volatile List<String> packErrors = List.of();
- /**
- * The unknown rule id warnings already logged, per hop-lint.yml. Resolution
runs once per file
- * and on every background check, so logging each time repeated the same
line for as long as the
- * project kept the id.
- */
- private final Set<String> loggedWarnings = ConcurrentHashMap.newKeySet();
-
private RuleRegistry() {
this(new RulePackDiscovery());
}
@@ -213,11 +204,7 @@ public class RuleRegistry {
// no sign anything had gone wrong.
String warning = unknownRuleWarning(entry.getKey(),
merged.keySet(), projectYaml);
warnings.add(warning);
- if (loggedWarnings.add(projectYaml.getAbsolutePath() + '\n' +
warning)) {
- LogChannel.GENERAL.logMinimal(warning);
- } else {
- LogChannel.GENERAL.logDetailed(warning);
- }
+ LintWarnings.logOnce(projectYaml.getAbsolutePath() + '\n' +
warning, warning);
}
}
LogChannel.GENERAL.logDetailed(
@@ -271,7 +258,7 @@ public class RuleRegistry {
}
/** The known id within two edits of the one given, or null. */
- private static String closestId(String ruleId, Collection<String> knownIds) {
+ static String closestId(String ruleId, Collection<String> knownIds) {
String best = null;
int bestDistance = 3;
for (String known : knownIds) {
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 d809b28e1d..979a60e5c7 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
@@ -23,8 +23,11 @@ import java.nio.file.Files;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
+import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
+import java.util.Set;
import org.apache.hop.core.logging.LogChannel;
import org.apache.hop.core.util.Utils;
import org.apache.hop.lint.CustomLintRule;
@@ -40,6 +43,42 @@ import org.yaml.snakeyaml.constructor.SafeConstructor;
/** Parses YAML rule pack and project overlay files. */
public final class YamlRulePackParser {
+ /** The keys a rule the linter evaluates may carry. */
+ static final Set<String> CUSTOM_RULE_KEYS =
+ Set.of(
+ "type",
+ "name",
+ "description",
+ "enabled",
+ "severity",
+ "target",
+ "targetField",
+ "condition",
+ "conditionValue",
+ "appliesTo",
+ RuleCombinator.ALL_OF.getYamlKey(),
+ RuleCombinator.ANY_OF.getYamlKey(),
+ "parameters",
+ "tags",
+ "helpUri");
+
+ /** The keys a rule classifying Hop's own verify remarks may carry. */
+ static final Set<String> NATIVE_RULE_KEYS =
+ Set.of(
+ "type",
+ "name",
+ "description",
+ "enabled",
+ "severity",
+ "appliesTo",
+ "messageKey",
+ "tags",
+ "helpUri");
+
+ /** The keys a project's hop-lint.yml may use to tune a rule from a pack. */
+ static final Set<String> OVERRIDE_KEYS =
+ Set.of("enabled", "severity", "conditionValue", "parameters", "tags",
"helpUri");
+
private YamlRulePackParser() {}
/**
@@ -253,6 +292,8 @@ public final class YamlRulePackParser {
List<CustomLintRule> projectRules = new ArrayList<>();
Map<String, ProjectYamlOverlay.ProjectRuleOverlay> overlays = new
HashMap<>();
+ // Every project has a hop-lint.yml, so the warnings name the file by its
path.
+ String location = inFile(projectYaml);
for (Map.Entry<String, Object> entry : rulesSection.entrySet()) {
String ruleId = entry.getKey();
@@ -260,13 +301,16 @@ public final class YamlRulePackParser {
Map<String, Object> ruleData = (Map<String, Object>) entry.getValue();
if (isNativeRuleDefinition(ruleData)) {
projectRules.add(
- parseNativeRule(ruleId, ruleData, RulePackIds.PROJECT,
RulePackOwner.PROJECT));
+ parseNativeRule(
+ ruleId, ruleData, RulePackIds.PROJECT, RulePackOwner.PROJECT,
location));
} else if (isCustomRuleDefinition(ruleData)) {
CustomLintRule rule =
- parseCustomRule(ruleId, ruleData, RulePackIds.PROJECT,
RulePackOwner.PROJECT);
+ parseCustomRule(ruleId, ruleData, RulePackIds.PROJECT,
RulePackOwner.PROJECT, location);
projectRules.add(rule);
} else {
- overlays.put(ruleId,
ProjectYamlOverlay.ProjectRuleOverlay.fromMap(ruleData));
+ warnUnknownKeys(ruleId, location, ruleData, OVERRIDE_KEYS);
+ overlays.put(
+ ruleId, ProjectYamlOverlay.ProjectRuleOverlay.fromMap(ruleId,
ruleData, location));
}
}
return new ProjectYamlOverlay(projectRules, overlays, policy);
@@ -429,6 +473,18 @@ public final class YamlRulePackParser {
*/
public static CustomLintRule parseNativeRule(
String ruleId, Map<String, Object> ruleData, String packId,
RulePackOwner owner) {
+ return parseNativeRule(ruleId, ruleData, packId, owner, inPack(packId));
+ }
+
+ /**
+ * @param location where the rule is written, for the warnings: {@link
#inPack} or {@link #inFile}
+ */
+ private static CustomLintRule parseNativeRule(
+ String ruleId,
+ Map<String, Object> ruleData,
+ String packId,
+ RulePackOwner owner,
+ String location) {
CustomLintRule rule = new CustomLintRule();
rule.setId(ruleId);
rule.setPackId(packId);
@@ -440,11 +496,25 @@ public final class YamlRulePackParser {
rule.setSeverity(stringValue(ruleData.get("severity"), "WARNING"));
rule.setAppliesTo(stringListValue(ruleData.get("appliesTo")));
rule.setMessageKey(stringValue(ruleData.get("messageKey"), ""));
+ applyDocumentation(rule, ruleData, location);
+ warnUnknownKeys(ruleId, location, ruleData, NATIVE_RULE_KEYS);
return rule;
}
public static CustomLintRule parseCustomRule(
String ruleId, Map<String, Object> ruleData, String packId,
RulePackOwner owner) {
+ return parseCustomRule(ruleId, ruleData, packId, owner, inPack(packId));
+ }
+
+ /**
+ * @param location where the rule is written, for the warnings: {@link
#inPack} or {@link #inFile}
+ */
+ private static CustomLintRule parseCustomRule(
+ String ruleId,
+ Map<String, Object> ruleData,
+ String packId,
+ RulePackOwner owner,
+ String location) {
CustomLintRule customRule = new CustomLintRule();
customRule.setId(ruleId);
customRule.setPackId(packId);
@@ -481,9 +551,135 @@ public final class YamlRulePackParser {
if (parameters != null && !parameters.isEmpty()) {
customRule.setAdditionalParameters(new HashMap<>(parameters));
}
+ applyDocumentation(customRule, ruleData, location);
+ warnUnknownKeys(ruleId, location, ruleData, CUSTOM_RULE_KEYS);
return customRule;
}
+ /** Read the keys that describe a rule without changing what it checks: tags
and helpUri. */
+ private static void applyDocumentation(
+ CustomLintRule rule, Map<String, Object> ruleData, String location) {
+ String helpUri = stringValue(ruleData.get("helpUri"), null);
+ if (!Utils.isEmpty(helpUri)) {
+ rule.setHelpUri(helpUri.trim());
+ }
+ rule.setTags(tagsValue(ruleData.get("tags"), rule.getId(), location));
+ }
+
+ /**
+ * Read a {@code tags:} block. Each value is a string or a list of strings.
+ *
+ * <p>Anything else is logged and left out rather than failing the pack:
tags never change what a
+ * rule finds, so a malformed one is no reason to stop linting.
+ */
+ static Map<String, List<String>> tagsValue(Object value, String ruleId,
String location) {
+ return tagsValue(value, ruleId, location, false);
+ }
+
+ /**
+ * @param keepEmpty whether a key with no values is kept: in a project
override it removes the
+ * pack's tag of that key, on a rule it means nothing
+ */
+ static Map<String, List<String>> tagsValue(
+ Object value, String ruleId, String location, boolean keepEmpty) {
+ Map<String, List<String>> tags = new LinkedHashMap<>();
+ if (value == null) {
+ return tags;
+ }
+ if (!(value instanceof Map<?, ?> map)) {
+ warn(
+ "Warning: tags of rule '"
+ + ruleId
+ + "' "
+ + location
+ + " are not a mapping and are ignored.");
+ return tags;
+ }
+ for (Map.Entry<?, ?> entry : map.entrySet()) {
+ String key = entry.getKey() != null ? entry.getKey().toString().trim() :
"";
+ Object tagValue = entry.getValue();
+ if (key.isEmpty() || tagValue instanceof Map) {
+ warn(
+ "Warning: tag '"
+ + key
+ + "' of rule '"
+ + ruleId
+ + "' "
+ + location
+ + " is ignored: a tag is a string or a list of strings.");
+ continue;
+ }
+ // A value given twice is kept once: SARIF requires the tags of a rule
to be unique.
+ List<String> values = new ArrayList<>(new
LinkedHashSet<>(stringListValue(tagValue)));
+ if (keepEmpty || !values.isEmpty()) {
+ tags.put(key, values);
+ }
+ }
+ return tags;
+ }
+
+ /**
+ * Log every key on a rule that the linter does not read.
+ *
+ * <p>An unknown key is dropped, and a typo such as {@code tag:} for {@code
tags:} or {@code
+ * severty:} would otherwise change the rule with nothing to show for it.
+ */
+ private static void warnUnknownKeys(
+ String ruleId, String location, Map<String, Object> ruleData,
Set<String> knownKeys) {
+ unknownKeyWarnings(ruleId, location, ruleData,
knownKeys).forEach(YamlRulePackParser::warn);
+ }
+
+ /**
+ * One warning for each key on the rule that is not one of the known keys.
+ *
+ * @param location where the rule is written: {@link #inPack} or {@link
#inFile}
+ */
+ static List<String> unknownKeyWarnings(
+ String ruleId, String location, Map<String, Object> ruleData,
Set<String> knownKeys) {
+ List<String> warnings = new ArrayList<>();
+ if (ruleData == null) {
+ return warnings;
+ }
+ for (Object key : ruleData.keySet()) {
+ String name = String.valueOf(key);
+ if (knownKeys.contains(name)) {
+ continue;
+ }
+ StringBuilder warning =
+ new StringBuilder("Warning: rule '")
+ .append(ruleId)
+ .append("' ")
+ .append(location)
+ .append(" has an unknown key '")
+ .append(name)
+ .append("', which is ignored.");
+ String closest = RuleRegistry.closestId(name, knownKeys);
+ if (closest != null) {
+ warning.append(" Did you mean ").append(closest).append("?");
+ }
+ warnings.add(warning.toString());
+ }
+ return warnings;
+ }
+
+ /** Where a rule from a pack is written, for a warning about it. */
+ static String inPack(String packId) {
+ return RulePackIds.PROJECT.equals(packId) ? "in hop-lint.yml" : "in pack
'" + packId + "'";
+ }
+
+ /**
+ * Where a rule in a project's hop-lint.yml is written. Every project has a
file of that name, so
+ * the path says which one, and two projects with the same mistake are each
told about it.
+ */
+ static String inFile(File projectYaml) {
+ return "in " + projectYaml.getAbsolutePath();
+ }
+
+ /** The location is part of the text, so the same mistake in another file is
logged again. */
+ private static void warn(String warning) {
+ LintWarnings.logOnce(warning, warning);
+ }
+
private static String stringValue(Object value, String defaultValue) {
return value != null ? value.toString() : defaultValue;
}
diff --git
a/plugins/misc/lint/src/main/resources/org/apache/hop/lint/messages/messages_en_US.properties
b/plugins/misc/lint/src/main/resources/org/apache/hop/lint/messages/messages_en_US.properties
index db564b37d7..050fbcc81a 100644
---
a/plugins/misc/lint/src/main/resources/org/apache/hop/lint/messages/messages_en_US.properties
+++
b/plugins/misc/lint/src/main/resources/org/apache/hop/lint/messages/messages_en_US.properties
@@ -66,6 +66,12 @@ RuleBuilderDialog.Label.Condition=Condition:
RuleBuilderDialog.Label.Value=Value:
RuleBuilderDialog.Label.Severity=Severity:
RuleBuilderDialog.Checkbox.Enabled=Rule Enabled
+RuleBuilderDialog.Label.HelpUri=Help URL:
+RuleBuilderDialog.Label.Tags=Tags:
+RuleBuilderDialog.Tags.Tooltip=Labels written to the SARIF and JSON reports.
They do not change what the rule checks. Repeat a key to give it several values.
+RuleBuilderDialog.Column.TagKey=Key
+RuleBuilderDialog.Column.TagValue=Value
+RuleBuilderDialog.Tags.Incomplete=Each tag needs both a key and a value.
RuleBuilderDialog.Button.Ok=OK
RuleBuilderDialog.Button.Cancel=Cancel
RuleBuilderDialog.Dialog.ValidationError.Title=Validation Error
@@ -83,6 +89,7 @@ RuleManagerDialog.Column.Condition=Condition
RuleManagerDialog.Column.Value=Value
RuleManagerDialog.Column.Severity=Severity
RuleManagerDialog.Column.Enabled=Enabled
+RuleManagerDialog.Column.Tags=Tags
RuleManagerDialog.Button.AddProjectRule=Add Project Rule
RuleManagerDialog.Button.EditRule=Edit Rule
RuleManagerDialog.Button.DeleteRule=Delete Rule
diff --git
a/plugins/misc/lint/src/test/java/org/apache/hop/lint/CustomRuleExecutorTest.java
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/CustomRuleExecutorTest.java
index dd1d61dfd9..f34759a8ce 100644
---
a/plugins/misc/lint/src/test/java/org/apache/hop/lint/CustomRuleExecutorTest.java
+++
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/CustomRuleExecutorTest.java
@@ -21,7 +21,9 @@ import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import java.util.Arrays;
+import java.util.LinkedHashMap;
import java.util.List;
+import java.util.Map;
import org.apache.hop.pipeline.PipelineHopMeta;
import org.apache.hop.pipeline.PipelineMeta;
import org.apache.hop.pipeline.transform.TransformMeta;
@@ -62,6 +64,27 @@ public class CustomRuleExecutorTest {
assertTrue(results.get(0).getSource().getName().contains("A -> B"));
}
+ /** Tags are inert: they change nothing about the finding except that it
carries them. */
+ @Test
+ public void findingsCarryTheRuleTagsAndHelpUri() {
+ TransformMeta from = new TransformMeta();
+ from.setName("A");
+ TransformMeta to = new TransformMeta();
+ to.setName("B");
+ PipelineHopMeta hop = new PipelineHopMeta(from, to, false);
+
+ CustomLintRule rule = baseRule(RuleTarget.HOP, "enabled",
RuleCondition.MUST_BE_TRUE);
+ rule.setTags(new LinkedHashMap<>(Map.of("owner",
List.of("platform-team"))));
+ rule.setHelpUri("https://example.com/rules/hop";);
+
+ LintRuleDetails details =
+ CustomRuleExecutor.executeRule(rule, hop,
"/tmp/test.hpl").get(0).getRuleDetails();
+
+ assertEquals(List.of("platform-team"), details.tags().get("owner"));
+ assertEquals("https://example.com/rules/hop";, details.helpUri());
+ assertEquals("Test rule description", details.description());
+ }
+
@Test
public void enabledPipelineHopPasses() {
TransformMeta from = new TransformMeta();
diff --git
a/plugins/misc/lint/src/test/java/org/apache/hop/lint/LintReportWriterTest.java
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/LintReportWriterTest.java
index 34264ddbdf..d9b9beae87 100644
---
a/plugins/misc/lint/src/test/java/org/apache/hop/lint/LintReportWriterTest.java
+++
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/LintReportWriterTest.java
@@ -25,7 +25,9 @@ import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
import java.nio.file.Path;
import java.nio.file.Paths;
+import java.util.LinkedHashMap;
import java.util.List;
+import java.util.Map;
import org.junit.jupiter.api.Test;
/** The machine-readable reports are a contract with CI, so their shape is
pinned here. */
@@ -141,6 +143,129 @@ public class LintReportWriterTest {
assertEquals(0, json.get("summary").get("total").asInt());
}
+ private static List<LintResult> taggedResults() {
+ Map<String, List<String>> tags = new LinkedHashMap<>();
+ tags.put("category", List.of("secrets"));
+ tags.put("policy", List.of("SEC-POL-4", "SEC-POL-7"));
+ LintRuleDetails details =
+ new LintRuleDetails(
+ "Passwords come from a variable",
"https://example.com/rules/SEC-002";, tags);
+ return List.of(
+ new LintResult(
+ "SEC-002",
+ "Hardcoded Secret",
+ "ERROR",
+ "Use a variable",
+ PROJECT.resolve("pipelines/load.hpl").toString(),
+ LintSourceRef.transform("REST"),
+ LintResult.Origin.LINT)
+ .withRuleDetails(details),
+ sampleResults().get(1));
+ }
+
+ /** GitHub code scanning and Azure DevOps read a flat list of key:value
strings. */
+ @Test
+ public void sarifWritesTagsDescriptionAndHelpUriOnTheRule() throws Exception
{
+ String sarif =
+ LintReportWriter.render(LintReportFormat.SARIF, taggedResults(),
"1.0.0", PROJECT);
+ JsonNode rules =
+ new ObjectMapper()
+ .readTree(sarif)
+ .get("runs")
+ .get(0)
+ .get("tool")
+ .get("driver")
+ .get("rules");
+
+ JsonNode tagged = rules.get(0);
+ assertEquals(
+ "Passwords come from a variable",
tagged.get("fullDescription").get("text").asText());
+ assertEquals("https://example.com/rules/SEC-002";,
tagged.get("helpUri").asText());
+
+ JsonNode flat = tagged.get("properties").get("tags");
+ assertEquals(3, flat.size());
+ assertEquals("category:secrets", flat.get(0).asText());
+ assertEquals("policy:SEC-POL-4", flat.get(1).asText());
+ assertEquals("policy:SEC-POL-7", flat.get(2).asText());
+
+ JsonNode hopTags = tagged.get("properties").get("hopTags");
+ assertEquals("secrets", hopTags.get("category").get(0).asText());
+ assertEquals(2, hopTags.get("policy").size());
+
+ // A rule without tags, description or help link gets none of those keys.
+ JsonNode plain = rules.get(1);
+ assertFalse(plain.has("properties"));
+ assertFalse(plain.has("fullDescription"));
+ assertFalse(plain.has("helpUri"));
+ }
+
+ private static JsonNode sarifRules(List<LintResult> results) throws
Exception {
+ return new ObjectMapper()
+ .readTree(LintReportWriter.render(LintReportFormat.SARIF, results,
"1.0.0", PROJECT))
+ .get("runs")
+ .get(0)
+ .get("tool")
+ .get("driver")
+ .get("rules");
+ }
+
+ /**
+ * The rule is described by a finding that carries its details, even when
one without them came
+ * first, so the order of the results cannot hide the tags.
+ */
+ @Test
+ public void sarifDescribesTheRuleFromAFindingThatCarriesItsDetails() throws
Exception {
+ LintResult tagged = taggedResults().get(0);
+ LintResult bare =
+ new LintResult(
+ "SEC-002",
+ "Hardcoded Secret",
+ "ERROR",
+ "Use a variable",
+ PROJECT.resolve("pipelines/other.hpl").toString());
+
+ JsonNode rules = sarifRules(List.of(bare, tagged));
+
+ assertEquals(1, rules.size());
+ assertEquals("category:secrets",
rules.get(0).get("properties").get("tags").get(0).asText());
+ JsonNode results =
+ new ObjectMapper()
+ .readTree(
+ LintReportWriter.render(
+ LintReportFormat.SARIF, List.of(bare, tagged), "1.0.0",
PROJECT))
+ .get("runs")
+ .get(0)
+ .get("results");
+ assertEquals(0, results.get(0).get("ruleIndex").asInt());
+ assertEquals(0, results.get(1).get("ruleIndex").asInt());
+ }
+
+ /** The SARIF schema requires a rule's tags to be unique. */
+ @Test
+ public void sarifTagsAreUnique() throws Exception {
+ LintResult finding =
+ sampleResults()
+ .get(0)
+ .withRuleDetails(
+ new LintRuleDetails("", null, Map.of("policy",
List.of("SEC-POL-4", "SEC-POL-4"))));
+
+ JsonNode flat =
sarifRules(List.of(finding)).get(0).get("properties").get("tags");
+
+ assertEquals(1, flat.size());
+ assertEquals("policy:SEC-POL-4", flat.get(0).asText());
+ }
+
+ @Test
+ public void jsonWritesTheRuleTagsOnEachFinding() throws Exception {
+ String json = LintReportWriter.render(LintReportFormat.JSON,
taggedResults(), "1.0.0", PROJECT);
+ JsonNode findings = new ObjectMapper().readTree(json).get("findings");
+
+ JsonNode tags = findings.get(0).get("tags");
+ assertEquals("secrets", tags.get("category").get(0).asText());
+ assertEquals("SEC-POL-7", tags.get("policy").get(1).asText());
+ assertEquals(0, findings.get(1).get("tags").size(), "an untagged rule
writes an empty object");
+ }
+
@Test
public void formatParsingIsCaseInsensitiveAndRejectsUnknownValues() {
assertEquals(LintReportFormat.SARIF, LintReportFormat.parse("SARIF"));
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
index cb98909311..ce37e149aa 100644
---
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
@@ -23,7 +23,9 @@ import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import java.util.ArrayList;
+import java.util.LinkedHashMap;
import java.util.List;
+import java.util.Map;
import org.apache.hop.core.CheckResult;
import org.apache.hop.core.Const;
import org.apache.hop.core.ICheckResult;
@@ -359,6 +361,54 @@ public class NativeCheckClassifierTest {
"SelectValues");
}
+ /** The tags of the rule that classified a remark travel with the finding it
becomes. */
+ @Test
+ public void aNativeFindingCarriesTheTagsOfTheRuleThatClassifiedIt() {
+ CustomLintRule rule = nativeRule("HOP-CHECK", "WARNING", true);
+ rule.setTags(new LinkedHashMap<>(Map.of("category", List.of("verify"))));
+ NativeCheckClassifier classifier = new
NativeCheckClassifier(List.of(rule));
+
+ LintResult finding =
+ LintCheckResultAdapter.fromCheckResult(
+ remark(ICheckResult.TYPE_RESULT_ERROR, "boom"), "/tmp/a.hpl",
classifier);
+
+ assertEquals(List.of("verify"),
finding.getRuleDetails().tags().get("category"));
+ assertFalse(
+ LintCheckResultAdapter.fromCheckResult(
+ remark(ICheckResult.TYPE_RESULT_ERROR, "boom"), "/tmp/a.hpl")
+ .getRuleDetails()
+ .hasTags(),
+ "a remark no rule classified has no tags");
+ }
+
+ /**
+ * A check reported under its own error code carries the tags of the rule
that classified it, but
+ * not that rule's description or help link: those describe the rule, not
the code.
+ */
+ @Test
+ public void anErrorCodeFindingCarriesTheTagsButNotTheDescription() {
+ CustomLintRule rule = nativeRule("HOP-CHECK", "WARNING", true);
+ rule.setDescription("Every remark Hop's own checks produce");
+ rule.setHelpUri("https://example.com/HOP-CHECK";);
+ rule.setTags(new LinkedHashMap<>(Map.of("category", List.of("verify"))));
+ NativeCheckClassifier classifier = new
NativeCheckClassifier(List.of(rule));
+
+ LintResult finding =
+ LintCheckResultAdapter.fromCheckResult(
+ new CheckResult(
+ ICheckResult.TYPE_RESULT_ERROR,
+ "CONNECTION_DOES_NOT_EXIST",
+ "no such connection",
+ new TransformMeta("TableInput", "read", null)),
+ "/tmp/a.hpl",
+ classifier);
+
+ assertEquals("CONNECTION_DOES_NOT_EXIST", finding.getRuleId());
+ assertEquals(List.of("verify"),
finding.getRuleDetails().tags().get("category"));
+ assertEquals("", finding.getRuleDetails().description());
+ assertNull(finding.getRuleDetails().helpUri());
+ }
+
private static ICheckResult remark(int type, String text) {
return remark(type, text, "SelectValues");
}
diff --git
a/plugins/misc/lint/src/test/java/org/apache/hop/lint/RuleBuilderDialogNativeRuleTest.java
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/RuleBuilderDialogNativeRuleTest.java
index d609256ea6..f4ae23edcb 100644
---
a/plugins/misc/lint/src/test/java/org/apache/hop/lint/RuleBuilderDialogNativeRuleTest.java
+++
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/RuleBuilderDialogNativeRuleTest.java
@@ -22,6 +22,9 @@ 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.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
import java.util.concurrent.atomic.AtomicReference;
import org.apache.hop.lint.registry.RuleRegistry;
import org.apache.hop.ui.testing.SwtBotTestBase;
@@ -66,4 +69,35 @@ class RuleBuilderDialogNativeRuleTest extends SwtBotTestBase
{
assertTrue(saved.get().isNativeVerify(), "still a native rule");
assertNull(saved.get().getTarget(), "and still without a target");
}
+
+ /**
+ * The tags table is filled from the rule and read back on OK, so a rule
saved without touching
+ * its tags keeps every one of them, a key with several values included.
+ */
+ @Test
+ void tagsAndHelpUrlSurviveTheEditor() {
+ CustomLintRule hopCheck =
+ RuleRegistry.getInstance().resolve(null).getRules().stream()
+ .filter(rule -> "HOP-CHECK".equals(rule.generateRuleId()))
+ .findFirst()
+ .orElseThrow()
+ .copy();
+ Map<String, List<String>> tags = new LinkedHashMap<>();
+ tags.put("category", List.of("verify"));
+ tags.put("policy", List.of("SEC-POL-4", "SEC-POL-7"));
+ hopCheck.setTags(new LinkedHashMap<>(tags));
+ AtomicReference<CustomLintRule> saved = new AtomicReference<>();
+
+ withDialog(
+ parent -> saved.set(new RuleBuilderDialog(parent, hopCheck).open()),
+ bot -> {
+ SWTBot dialog = bot.shell("Edit Lint Rule").bot();
+ dialog.textWithLabel("Help
URL:").setText("https://wiki.example.com/HOP-CHECK";);
+ dialog.button("OK").click();
+ });
+
+ assertNotNull(saved.get(), "the dialog refused to save");
+ assertEquals(tags, saved.get().getTags());
+ assertEquals("https://wiki.example.com/HOP-CHECK";,
saved.get().getHelpUri());
+ }
}
diff --git
a/plugins/misc/lint/src/test/java/org/apache/hop/lint/registry/YamlRulePackParserTagsTest.java
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/registry/YamlRulePackParserTagsTest.java
new file mode 100644
index 0000000000..917c6322ac
--- /dev/null
+++
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/registry/YamlRulePackParserTagsTest.java
@@ -0,0 +1,316 @@
+/*
+ * 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.registry;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.io.ByteArrayInputStream;
+import java.io.File;
+import java.nio.charset.StandardCharsets;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.hop.lint.CustomLintRule;
+import org.junit.jupiter.api.Test;
+import org.yaml.snakeyaml.Yaml;
+
+/** Tags and help links on a rule, and the warning for keys the linter does
not read. */
+public class YamlRulePackParserTagsTest {
+
+ private static final String PACK =
+ """
+ pack:
+ id: acme
+ rules:
+ ACME-001:
+ type: custom
+ target: DATABASE_CONNECTION
+ targetField: password
+ condition: NO_HARDCODED
+ helpUri: https://example.com/rules/ACME-001
+ tags:
+ category: secrets
+ owner: platform-team
+ policy: ["SEC-POL-4", "SEC-POL-7"]
+ level: 3
+ ACME-002:
+ type: native
+ tags:
+ category: verify
+ ACME-003:
+ type: custom
+ target: PIPELINE
+ targetField: name
+ condition: NOT_EMPTY
+ """;
+
+ private static Map<String, CustomLintRule> load(String yaml) throws
Exception {
+ Map<String, CustomLintRule> rules = new LinkedHashMap<>();
+ for (CustomLintRule rule :
+ YamlRulePackParser.loadFromStream(
+ new ByteArrayInputStream(yaml.getBytes(StandardCharsets.UTF_8)),
+ "acme",
+ RulePackOwner.VENDOR)) {
+ rules.put(rule.getId(), rule);
+ }
+ return rules;
+ }
+
+ @Test
+ public void tagsAreReadAsListsInTheOrderTheyWereWritten() throws Exception {
+ CustomLintRule rule = load(PACK).get("ACME-001");
+
+ assertEquals(
+ List.of("category", "owner", "policy", "level"),
List.copyOf(rule.getTags().keySet()));
+ assertEquals(List.of("secrets"), rule.getTags().get("category"));
+ assertEquals(List.of("SEC-POL-4", "SEC-POL-7"),
rule.getTags().get("policy"));
+ assertEquals(List.of("3"), rule.getTags().get("level"));
+ assertEquals("https://example.com/rules/ACME-001";, rule.getHelpUri());
+ }
+
+ @Test
+ public void nativeRulesCarryTagsToo() throws Exception {
+ assertEquals(Map.of("category", List.of("verify")),
load(PACK).get("ACME-002").getTags());
+ }
+
+ @Test
+ public void aRuleWithoutTagsHasNone() throws Exception {
+ CustomLintRule rule = load(PACK).get("ACME-003");
+
+ assertTrue(rule.getTags().isEmpty());
+ assertNull(rule.getHelpUri());
+ }
+
+ @Test
+ public void copyKeepsTagsAndHelpUriWithoutSharingThem() throws Exception {
+ CustomLintRule rule = load(PACK).get("ACME-001");
+ CustomLintRule copy = rule.copy();
+
+ assertEquals(rule.getTags(), copy.getTags());
+ assertEquals(rule.getHelpUri(), copy.getHelpUri());
+ copy.getTags().get("policy").add("SEC-POL-9");
+ assertEquals(2, rule.getTags().get("policy").size());
+ }
+
+ /** A rule saved from the rule manager keeps its tags and help link. */
+ @Test
+ public void tagsSurviveARoundTripThroughTheProjectFile() throws Exception {
+ CustomLintRule rule = load(PACK).get("ACME-001");
+ rule.setPackOwner(RulePackOwner.PROJECT);
+
+ Map<String, Object> entry = ProjectLintYamlExporter.entryFor(rule);
+ assertEquals("secrets", ((Map<?, ?>) entry.get("tags")).get("category"));
+
+ CustomLintRule reread =
+ YamlRulePackParser.parseCustomRule("ACME-001", entry, "project",
RulePackOwner.PROJECT);
+ assertEquals(rule.getTags(), reread.getTags());
+ assertEquals(rule.getHelpUri(), reread.getHelpUri());
+ }
+
+ /**
+ * A project sets a tag per key: its owner replaces the pack's owner, the
pack's other tags stay,
+ * and a key with no values removes the pack's tag.
+ */
+ @Test
+ public void aProjectOverrideReplacesTagsPerKey() throws Exception {
+ CustomLintRule rule = load(PACK).get("ACME-001");
+ Map<String, Object> override = new LinkedHashMap<>();
+ Map<String, Object> tags = new LinkedHashMap<>();
+ tags.put("owner", "data-team");
+ tags.put("level", List.of());
+ tags.put("team", List.of("crm", "sales"));
+ override.put("tags", tags);
+ override.put("helpUri", "https://wiki.example.com/ACME-001";);
+
+ ProjectYamlOverlay.ProjectRuleOverlay.fromMap("ACME-001", override, "in
hop-lint.yml")
+ .applyTo(rule);
+
+ assertEquals(List.of("data-team"), rule.getTags().get("owner"));
+ assertEquals(List.of("secrets"), rule.getTags().get("category"));
+ assertEquals(List.of("SEC-POL-4", "SEC-POL-7"),
rule.getTags().get("policy"));
+ assertEquals(List.of("crm", "sales"), rule.getTags().get("team"));
+ assertTrue(!rule.getTags().containsKey("level"), "an empty key removes the
pack's tag");
+ assertEquals("https://wiki.example.com/ACME-001";, rule.getHelpUri());
+ }
+
+ /** A bare helpUri: removes the pack's link, rather than becoming the text
"null". */
+ @Test
+ public void aBareHelpUriOverrideRemovesTheLink() throws Exception {
+ CustomLintRule rule = load(PACK).get("ACME-001");
+ Map<String, Object> override = new LinkedHashMap<>();
+ override.put("helpUri", null);
+
+ ProjectYamlOverlay.ProjectRuleOverlay.fromMap("ACME-001", override, "in
hop-lint.yml")
+ .applyTo(rule);
+
+ assertNull(rule.getHelpUri());
+ }
+
+ @Test
+ public void aValueGivenTwiceIsKeptOnce() {
+ assertEquals(
+ Map.of("policy", List.of("SEC-POL-4", "SEC-POL-7")),
+ YamlRulePackParser.tagsValue(
+ Map.of("policy", List.of("SEC-POL-4", "SEC-POL-7", "SEC-POL-4")),
+ "ACME-001",
+ YamlRulePackParser.inPack("acme")));
+ }
+
+ /**
+ * The rule manager writes only the keys the project changed, so the pack's
other tags follow it.
+ */
+ @Test
+ public void theExporterWritesOnlyTheChangedTagKeys() {
+ Map<String, List<String>> pack = new LinkedHashMap<>();
+ pack.put("category", List.of("secrets"));
+ pack.put("owner", List.of("platform-team"));
+ pack.put("level", List.of("3"));
+ Map<String, List<String>> desired = new LinkedHashMap<>();
+ desired.put("category", List.of("secrets"));
+ desired.put("owner", List.of("data-team"));
+ desired.put("team", List.of("crm", "sales"));
+
+ Map<String, Object> expected = new LinkedHashMap<>();
+ expected.put("owner", "data-team");
+ expected.put("team", List.of("crm", "sales"));
+ expected.put("level", List.of());
+ assertEquals(expected, ProjectLintYamlExporter.tagOverrides(desired,
pack));
+ }
+
+ /** Tagging a core rule in the rule manager is an override, not a copy of
the whole rule. */
+ @Test
+ public void taggingAPackRuleIsWrittenAsAnOverride() {
+ CustomLintRule rule =
+ RuleRegistry.getInstance().resolve(null).getRules().stream()
+ .filter(r -> "DB-001".equals(r.generateRuleId()))
+ .findFirst()
+ .orElseThrow()
+ .copy();
+ rule.getTags().put("owner", List.of("data-team"));
+
+ assertEquals(
+ Map.of("tags", Map.of("owner", "data-team")),
ProjectLintYamlExporter.entryFor(rule));
+ }
+
+ /** A nested mapping is not a tag. It is left out rather than failing the
whole pack. */
+ @Test
+ public void malformedTagsAreLeftOut() {
+ Map<String, Object> tags = new LinkedHashMap<>();
+ tags.put("owner", "platform-team");
+ tags.put("nested", Map.of("a", "b"));
+
+ assertEquals(
+ Map.of("owner", List.of("platform-team")),
+ YamlRulePackParser.tagsValue(tags, "ACME-001",
YamlRulePackParser.inPack("acme")));
+ assertTrue(
+ YamlRulePackParser.tagsValue("secrets", "ACME-001",
YamlRulePackParser.inPack("acme"))
+ .isEmpty());
+ }
+
+ /** A typo such as tag: for tags: would otherwise drop every tag without
anyone noticing. */
+ @Test
+ public void anUnknownKeyIsReportedWithTheClosestKnownOne() {
+ Map<String, Object> ruleData = new LinkedHashMap<>();
+ ruleData.put("type", "custom");
+ ruleData.put("tag", Map.of("owner", "platform-team"));
+
+ List<String> warnings =
+ YamlRulePackParser.unknownKeyWarnings(
+ "ACME-001",
+ YamlRulePackParser.inPack("acme"),
+ ruleData,
+ YamlRulePackParser.CUSTOM_RULE_KEYS);
+
+ assertEquals(
+ List.of(
+ "Warning: rule 'ACME-001' in pack 'acme' has an unknown key 'tag',
which is ignored."
+ + " Did you mean tags?"),
+ warnings);
+ }
+
+ @Test
+ public void knownKeysProduceNoWarning() {
+ Map<String, Object> ruleData = new LinkedHashMap<>();
+ for (String key : YamlRulePackParser.CUSTOM_RULE_KEYS) {
+ ruleData.put(key, "x");
+ }
+
+ assertTrue(
+ YamlRulePackParser.unknownKeyWarnings(
+ "ACME-001",
+ YamlRulePackParser.inPack("acme"),
+ ruleData,
+ YamlRulePackParser.CUSTOM_RULE_KEYS)
+ .isEmpty());
+ }
+
+ /**
+ * A project override that misspells a key used to leave the pack's rule
unchanged silently. Every
+ * project has a hop-lint.yml, so the warning names the file by its path:
the same typo in a
+ * second project is a second warning, not a repeat of the first.
+ */
+ @Test
+ public void anUnknownOverrideKeyNamesTheProjectFile() {
+ Map<String, Object> ruleData = Map.of("severty", "ERROR");
+ File crm = new File("/projects/crm/hop-lint.yml");
+ File sales = new File("/projects/sales/hop-lint.yml");
+
+ List<String> warnings =
+ YamlRulePackParser.unknownKeyWarnings(
+ "DB-001", YamlRulePackParser.inFile(crm), ruleData,
YamlRulePackParser.OVERRIDE_KEYS);
+
+ assertEquals(
+ List.of(
+ "Warning: rule 'DB-001' in "
+ + crm.getAbsolutePath()
+ + " has an unknown key 'severty', which is ignored. Did you
mean severity?"),
+ warnings);
+ assertNotEquals(
+ warnings,
+ YamlRulePackParser.unknownKeyWarnings(
+ "DB-001",
+ YamlRulePackParser.inFile(sales),
+ ruleData,
+ YamlRulePackParser.OVERRIDE_KEYS));
+ }
+
+ /** Every key the core pack uses has to be a known key, or Hop warns about
its own rules. */
+ @Test
+ public void theCorePackUsesOnlyKnownKeys() throws Exception {
+ try (var in =
+
YamlRulePackParser.class.getClassLoader().getResourceAsStream("hop-lint-core.yml"))
{
+ @SuppressWarnings("unchecked")
+ Map<String, Object> rules =
+ (Map<String, Object>) ((Map<String, Object>) new
Yaml().load(in)).get("rules");
+ for (Map.Entry<String, Object> entry : rules.entrySet()) {
+ @SuppressWarnings("unchecked")
+ Map<String, Object> ruleData = (Map<String, Object>) entry.getValue();
+ var known =
+ YamlRulePackParser.isNativeRuleDefinition(ruleData)
+ ? YamlRulePackParser.NATIVE_RULE_KEYS
+ : YamlRulePackParser.CUSTOM_RULE_KEYS;
+ assertEquals(
+ List.of(),
+ YamlRulePackParser.unknownKeyWarnings(
+ entry.getKey(), YamlRulePackParser.inPack("hop-core"),
ruleData, known));
+ }
+ }
+ }
+}
