This is an automated email from the ASF dual-hosted git repository.
hansva 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 306aefa5c3 Lint: check the staged files in the pre-commit hook,
resolve relative… (#9076)
306aefa5c3 is described below
commit 306aefa5c3dae17db4b1f6fc851fac9fbb4cd2ed
Author: Bart Maertens <[email protected]>
AuthorDate: Thu Oct 22 14:45:38 2026 +0200
Lint: check the staged files in the pre-commit hook, resolve relative…
(#9076)
* Lint: check the staged files in the pre-commit hook, resolve relative
paths from the working folder, support --fail-on INFO, validate rule values,
define the SYSTEM rules in the core pack, fixes #9071
* Lint: flag only rule conditions that can never fire, lint symlinked and
filtered files on disk in the pre-commit hook, fixes #9071
---
.../modules/ROOT/pages/hop-tools/hop-lint.adoc | 7 +-
.../modules/ROOT/pages/linting/lint-rules.adoc | 20 ++
.../ROOT/pages/workflow/actions/runlinter.adoc | 2 +-
.../java/org/apache/hop/lint/CustomLintRule.java | 19 ++
.../org/apache/hop/lint/ExplorerLintGuiPlugin.java | 20 +-
.../main/java/org/apache/hop/lint/HopLinter.java | 148 ++++++++++--
.../org/apache/hop/lint/HopMetadataFileLoader.java | 44 +++-
.../main/java/org/apache/hop/lint/LintCommand.java | 92 ++++++--
.../java/org/apache/hop/lint/LintSeverity.java | 24 +-
.../org/apache/hop/lint/PreCommitLintService.java | 92 ++++++--
.../org/apache/hop/lint/RuleBuilderDialog.java | 29 ++-
.../main/java/org/apache/hop/lint/RuleClause.java | 104 +++++++++
.../org/apache/hop/lint/RuleManagerDialog.java | 9 +
.../java/org/apache/hop/lint/RuleTargetFields.java | 79 +++++++
.../apache/hop/lint/registry/EffectiveRuleSet.java | 12 +-
.../hop/lint/registry/ProjectLintYamlExporter.java | 4 +-
.../hop/lint/registry/RulePackDiscovery.java | 4 +-
.../org/apache/hop/lint/registry/RuleRegistry.java | 32 +++
.../hop/lint/registry/YamlRulePackParser.java | 115 +++++++++-
.../misc/lint/src/main/resources/hop-lint-core.yml | 30 +++
.../hop/lint/messages/messages_en_US.properties | 7 +
.../apache/hop/lint/HopMetadataFileLoaderTest.java | 32 +++
.../apache/hop/lint/LintCommandOptionsTest.java | 249 +++++++++++++++++++++
.../java/org/apache/hop/lint/LintCommandTest.java | 135 ++++++++++-
.../apache/hop/lint/LintSeverityThresholdTest.java | 27 +++
.../apache/hop/lint/RuleBuilderValidationTest.java | 123 ++++++++++
.../org/apache/hop/lint/RuleEditorChoicesTest.java | 2 +-
.../hop/lint/registry/HopCoreRulePackTest.java | 4 +-
.../apache/hop/lint/registry/RuleRegistryTest.java | 127 ++++++++++-
29 files changed, 1475 insertions(+), 117 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 60f8a277ba..6ae394d785 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
@@ -41,6 +41,8 @@ hop lint --list-rules # show the effective
rule set
hop lint --help # full option list
----
+Relative paths, for the target and for options such as `--output`, `--config`
and `--baseline`, are relative to the folder the command is run from.
+
== Project and environment
`hop lint` takes the project and environment options of `hop run`.
@@ -68,7 +70,8 @@ A project chosen with `-j` or `-e` applies whatever is
linted; for a target outs
|Write the report to a file instead of stdout
|`--fail-on <severity>`
-|Exit with 1 when a finding reaches this severity. Default `ERROR`.
+|Exit with 1 when a finding reaches this severity: `ERROR` (the default),
`WARNING`, `INFO` or `NONE`.
+Any other value is refused.
|`--max-warnings <n>`
|Exit with 1 when more than `n` warnings are reported
@@ -170,6 +173,8 @@ hop lint --install-hook
This writes a `pre-commit` hook into the current repository's `.git/hooks`.
The hook lints the staged Hop files and blocks the commit when they fail.
+It lints the staged version of each file, which is what the commit holds, not
the one on disk, and it includes files renamed with `git mv`.
+Set `HOP_LINT_FAIL_ON` to change the threshold the hook fails on, for example
`HOP_LINT_FAIL_ON=WARNING`.
It is a copy, so run `hop lint --install-hook` again after upgrading Hop to
pick up changes to it.
Hop Gui can do the same for commits made from within Hop Gui. See
*Configuration -> Plugins -> Linter -> Block Git Commits on Lint Failures*,
which is *off by default*: installing and enabling the linter does not on its
own change how committing behaves.
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 ea016d7c01..be6068f081 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
@@ -119,6 +119,11 @@ Scope such a rule with `appliesTo`, naming the metadata
key — the folder name
|is a boolean with the required value
|===
+`conditionValue` is used as written: variables are not resolved in it.
+A value the condition cannot use, such as `abc` or `3,0` for `MAX_VALUE`, or
`[a-` for `MATCHES_PATTERN`, would make the rule report nothing at all.
+The rule editor refuses it, and a rule loaded from `hop-lint.yml` or a rule
pack gets a warning in the log that names the rule and the problem.
+The same goes for a condition that does not fit one of the fields the linter
provides, such as `NOT_NULL` on `transformCount`.
+
== Fields
`targetField` is the name of a property on the target. Besides the properties
Hop exposes, the linter computes a few of its own:
@@ -399,8 +404,23 @@ Hop's core rule pack is deliberately small. A rule is
enabled by default only wh
|`HOP-CHECK`
|WARNING
|Every remark from Hop's own transform and action checks
+
+|`SYSTEM-001`
+|ERROR
+|A pipeline, workflow or metadata file that could not be loaded, so nothing
was checked against it
+
+|`SYSTEM-002`
+|ERROR
+|A lint run that stopped before it finished
+
+|`SYSTEM-003`
+|ERROR
+|A metadata file, such as a connection, that could not be read
|===
+The `SYSTEM` rules are of `type: system`: like a native rule they check
nothing, and say how the linter reports a file it could not read or a run it
could not finish.
+A project can change their severity, or switch one off, in its `hop-lint.yml`
like any other rule.
+
`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.
diff --git
a/docs/hop-user-manual/modules/ROOT/pages/workflow/actions/runlinter.adoc
b/docs/hop-user-manual/modules/ROOT/pages/workflow/actions/runlinter.adoc
index 530dcad5eb..54a3dea297 100644
--- a/docs/hop-user-manual/modules/ROOT/pages/workflow/actions/runlinter.adoc
+++ b/docs/hop-user-manual/modules/ROOT/pages/workflow/actions/runlinter.adoc
@@ -61,7 +61,7 @@ It does not change whether the action fails, nor the counts
in the variables. De
|Fail on
|`--fail-on`
-|Fail when a finding reaches this severity: `Error` (the default), `Warning`
or `None`.
+|Fail when a finding reaches this severity: `Error` (the default), `Warning`,
`Info` or `None`.
|Max warnings
|`--max-warnings`
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 3c367ac14b..8bcaefc797 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
@@ -107,6 +107,12 @@ public class CustomLintRule {
*/
public static final String TYPE_NATIVE = "native";
+ /**
+ * A finding the linter raises about the run itself, such as a file it could
not read. The rule
+ * only says whether that is reported and at what severity.
+ */
+ public static final String TYPE_SYSTEM = "system";
+
public CustomLintRule() {
this.id = UUID.randomUUID().toString();
this.enabled = true;
@@ -335,6 +341,19 @@ public class CustomLintRule {
return TYPE_NATIVE.equalsIgnoreCase(type);
}
+ /** Whether this rule covers a finding the linter raises about the run, such
as SYSTEM-001. */
+ public boolean isSystem() {
+ return TYPE_SYSTEM.equalsIgnoreCase(type);
+ }
+
+ /**
+ * Whether this rule has no target or condition, and only says how a finding
produced elsewhere is
+ * reported: a native or a system rule.
+ */
+ public boolean isReportOnly() {
+ return isNativeVerify() || isSystem();
+ }
+
public List<RuleClause> getClauses() {
List<RuleClause> clauses = new ArrayList<>();
clauses.add(new RuleClause(targetField, condition, conditionValue));
diff --git
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/ExplorerLintGuiPlugin.java
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/ExplorerLintGuiPlugin.java
index 22e778f4af..7215ac7184 100644
---
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/ExplorerLintGuiPlugin.java
+++
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/ExplorerLintGuiPlugin.java
@@ -588,17 +588,19 @@ public class ExplorerLintGuiPlugin {
processedFilesCount++;
} catch (Exception e) {
log.logError("Error processing file: " +
file.getAbsolutePath(), e);
- LintResult errorResult =
- new LintResult(
- "SYSTEM-001",
- "File Processing Error",
- "ERROR",
- "Failed to process file: " + e.getMessage(),
- LintPathUtils.normalizePath(file.getAbsolutePath()));
- results.add(errorResult);
+ List<LintResult> errorResults =
+ linter
+ .systemFinding(
+ HopLinter.SYSTEM_FILE_PROCESSING,
+ "Failed to process file: " + e.getMessage(),
+
LintPathUtils.normalizePath(file.getAbsolutePath()),
+ null)
+ .map(List::of)
+ .orElse(List.of());
+ results.addAll(errorResults);
service.publish(
LintPathUtils.normalizePath(file.getAbsolutePath()),
- List.of(errorResult),
+ errorResults,
passGeneration);
processedFilesCount++;
}
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 1b91cf0e36..f17d2b2f86 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
@@ -18,6 +18,7 @@ package org.apache.hop.lint;
import java.io.File;
import java.io.IOException;
+import java.io.InputStream;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
@@ -25,6 +26,7 @@ import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
import java.util.Set;
import java.util.stream.Stream;
import org.apache.hop.core.ICheckResult;
@@ -35,6 +37,7 @@ import org.apache.hop.core.logging.LogChannel;
import org.apache.hop.core.util.Utils;
import org.apache.hop.core.variables.IVariables;
import org.apache.hop.core.variables.Variables;
+import org.apache.hop.core.xml.XmlHandler;
import org.apache.hop.lint.registry.EffectiveRuleSet;
import org.apache.hop.lint.registry.LintConfigurationException;
import org.apache.hop.lint.registry.RuleRegistry;
@@ -45,6 +48,8 @@ import org.apache.hop.pipeline.transform.TransformMeta;
import org.apache.hop.workflow.WorkflowHopMeta;
import org.apache.hop.workflow.WorkflowMeta;
import org.apache.hop.workflow.action.ActionMeta;
+import org.w3c.dom.Document;
+import org.w3c.dom.Node;
/**
* Main engine class that orchestrates the entire linting process. This class
loads configuration,
@@ -54,6 +59,25 @@ public class HopLinter {
private static final ILogChannel log = LogChannel.GENERAL;
+ /** A file that could not be read or linted. */
+ public static final String SYSTEM_FILE_PROCESSING = "SYSTEM-001";
+
+ /** A lint run that could not be completed. */
+ public static final String SYSTEM_CONFIGURATION = "SYSTEM-002";
+
+ /** A metadata file that could not be read. */
+ public static final String SYSTEM_UNREADABLE_METADATA = "SYSTEM-003";
+
+ /**
+ * The names the system findings carry when no rule pack defines them, as
when the linter is
+ * switched off. The core pack defines all three.
+ */
+ private static final Map<String, String> SYSTEM_RULE_NAMES =
+ Map.of(
+ SYSTEM_FILE_PROCESSING, "File Processing Error",
+ SYSTEM_CONFIGURATION, "Configuration Error",
+ SYSTEM_UNREADABLE_METADATA, "Unreadable Metadata");
+
private LinterConfig config;
private EffectiveRuleSet effectiveRuleSet;
@@ -314,13 +338,12 @@ public class HopLinter {
"Error processing file: " + file.getAbsolutePath() + " - " +
e.getMessage(), e);
// Add an error result for the file that couldn't be processed
- allResults.add(
- new LintResult(
- "SYSTEM-001",
- "File Processing Error",
- "ERROR",
+ systemFinding(
+ SYSTEM_FILE_PROCESSING,
"Failed to process file: " + e.getMessage(),
- file.getName()));
+ file.getName(),
+ null)
+ .ifPresent(allResults::add);
}
}
@@ -342,13 +365,9 @@ public class HopLinter {
} catch (Exception e) {
log.logError("Error during linting process: " + e.getMessage(), e);
- allResults.add(
- new LintResult(
- "SYSTEM-002",
- "Configuration Error",
- "ERROR",
- "Failed to complete linting: " + e.getMessage(),
- "system"));
+ systemFinding(
+ SYSTEM_CONFIGURATION, "Failed to complete linting: " +
e.getMessage(), "system", null)
+ .ifPresent(allResults::add);
} finally {
if (ownIndex) {
CustomRuleExecutor.setProjectIndex(null);
@@ -513,27 +532,38 @@ public class HopLinter {
/** Process a single Hop file and run all enabled rules against it */
public List<LintResult> processFile(
File file, IHopMetadataProvider metadataProvider, IVariables variables)
throws HopException {
+ return processFile(file, null, metadataProvider, variables);
+ }
+
+ /**
+ * Process a file, reading it from another file: the pre-commit hook lints
the staged version of a
+ * file, which can differ from the one on disk, but reports it as the file
in the repository.
+ *
+ * @param content the file to read, or null to read {@code file}
+ */
+ public List<LintResult> processFile(
+ File file, File content, IHopMetadataProvider metadataProvider,
IVariables variables)
+ throws HopException {
String fileName = LintPathUtils.normalizePath(file.getAbsolutePath());
log.logDetailed("Processing file: " + file.getName());
ensureConfigLoaded();
if (HopMetadataFileLoader.isMetadataJsonFile(fileName)) {
- HopMetadataFileLoader.MetadataLoad load =
HopMetadataFileLoader.read(file, metadataProvider);
+ HopMetadataFileLoader.MetadataLoad load =
+ HopMetadataFileLoader.read(file, content, metadataProvider);
if (load.isFailure()) {
// Report it. A metadata file that cannot be read passes every rule,
so staying quiet
// here would let a corrupt connection be reported as a clean one.
- return List.of(
- new LintResult(
- "SYSTEM-003",
- "Unreadable Metadata",
- "ERROR",
+ return systemFinding(
+ SYSTEM_UNREADABLE_METADATA,
"This metadata file could not be read, so no rule could be
checked against"
+ " it: "
+ load.error(),
fileName,
-
LintSourceRef.metadata(HopMetadataFileLoader.metadataNameOf(file)),
- LintResult.Origin.LINT));
+
LintSourceRef.metadata(HopMetadataFileLoader.metadataNameOf(file)))
+ .map(List::of)
+ .orElse(List.of());
}
if (load.object() == null) {
return List.of();
@@ -542,7 +572,7 @@ public class HopLinter {
runPolicyRules(load.object(), fileName), null, fileName, variables,
metadataProvider);
}
- Object hopObject = loadHopObject(file, metadataProvider, variables,
fileName);
+ Object hopObject = loadHopObject(file, content, metadataProvider,
variables, fileName);
if (hopObject == null) {
return List.of();
}
@@ -673,9 +703,16 @@ public class HopLinter {
}
private Object loadHopObject(
- File file, IHopMetadataProvider metadataProvider, IVariables variables,
String fileName)
+ File file,
+ File content,
+ IHopMetadataProvider metadataProvider,
+ IVariables variables,
+ String fileName)
throws HopException {
try {
+ if (content != null) {
+ return loadHopObjectFrom(content, file, metadataProvider, variables,
fileName);
+ }
if (fileName.toLowerCase().endsWith(".hpl")) {
return new PipelineMeta(file.getAbsolutePath(), metadataProvider,
variables);
}
@@ -690,6 +727,45 @@ public class HopLinter {
}
}
+ /**
+ * Load a pipeline or workflow from {@code content} as if it were {@code
file}, so that the
+ * filename, and the internal variables derived from it, are those of the
file in the repository.
+ */
+ private static Object loadHopObjectFrom(
+ File content,
+ File file,
+ IHopMetadataProvider metadataProvider,
+ IVariables variables,
+ String fileName)
+ throws HopException {
+ boolean pipeline = fileName.toLowerCase().endsWith(".hpl");
+ if (!pipeline && !fileName.toLowerCase().endsWith(".hwf")) {
+ log.logError("Unknown file type: " + fileName);
+ return null;
+ }
+ Document document;
+ try (InputStream inputStream = Files.newInputStream(content.toPath())) {
+ document = XmlHandler.loadXmlFile(inputStream, null, false, false);
+ } catch (IOException e) {
+ throw new HopException("Could not read " + content, e);
+ }
+ String tag = pipeline ? PipelineMeta.XML_TAG : WorkflowMeta.XML_TAG;
+ Node node = XmlHandler.getSubNode(document, tag);
+ if (node == null) {
+ throw new HopException("No <" + tag + "> element in " + fileName);
+ }
+ if (pipeline) {
+ PipelineMeta pipelineMeta = new PipelineMeta();
+ pipelineMeta.setMetadataProvider(metadataProvider);
+ pipelineMeta.loadXml(node, file.getAbsolutePath(), metadataProvider,
variables);
+ return pipelineMeta;
+ }
+ WorkflowMeta workflowMeta = new WorkflowMeta();
+ workflowMeta.setMetadataProvider(metadataProvider);
+ workflowMeta.loadXml(node, file.getAbsolutePath(), metadataProvider,
variables);
+ return workflowMeta;
+ }
+
private List<LintResult> finalizeResults(
List<LintResult> policyResults,
Object hopObject,
@@ -887,6 +963,32 @@ public class HopLinter {
return results;
}
+ /**
+ * A finding about the run itself, such as a file that could not be read,
with the name and
+ * severity the rule packs and the project give it.
+ *
+ * <p>These were hardcoded at ERROR, so a project could neither see them in
the rule list nor
+ * change their severity.
+ *
+ * @param ruleId one of the {@code SYSTEM_} ids
+ * @return the finding, or empty when the project switched the rule off
+ */
+ public Optional<LintResult> systemFinding(
+ String ruleId, String message, String fileName, LintSourceRef source) {
+ String name = SYSTEM_RULE_NAMES.getOrDefault(ruleId, ruleId);
+ String severity = "ERROR";
+ CustomLintRule rule = getEffectiveRuleSet().findRule(ruleId);
+ if (rule != null) {
+ if (!rule.isEnabled()) {
+ return Optional.empty();
+ }
+ name = Utils.isEmpty(rule.getName()) ? name : rule.getName();
+ severity = Utils.isEmpty(rule.getSeverity()) ? severity :
rule.getSeverity();
+ }
+ return Optional.of(
+ new LintResult(ruleId, name, severity, message, fileName, source,
LintResult.Origin.LINT));
+ }
+
public EffectiveRuleSet getEffectiveRuleSet() {
ensureEffectiveRuleSet();
return effectiveRuleSet;
diff --git
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/HopMetadataFileLoader.java
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/HopMetadataFileLoader.java
index 3b06c9510a..ecbe1f23d5 100644
---
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/HopMetadataFileLoader.java
+++
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/HopMetadataFileLoader.java
@@ -16,13 +16,18 @@
*/
package org.apache.hop.lint;
+import com.fasterxml.jackson.core.JsonFactory;
+import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.databind.JsonNode;
import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
import java.lang.reflect.Method;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.util.Iterator;
import org.apache.hop.core.database.DatabaseMeta;
+import org.apache.hop.core.exception.HopException;
import org.apache.hop.core.json.HopJson;
import org.apache.hop.core.logging.ILogChannel;
import org.apache.hop.core.logging.LogChannel;
@@ -30,6 +35,7 @@ import org.apache.hop.core.util.Utils;
import org.apache.hop.metadata.api.IHopMetadata;
import org.apache.hop.metadata.api.IHopMetadataProvider;
import org.apache.hop.metadata.api.IHopMetadataSerializer;
+import org.apache.hop.metadata.serializer.json.JsonMetadataParser;
/**
* Loads Hop metadata objects from project metadata JSON files.
@@ -137,7 +143,17 @@ public final class HopMetadataFileLoader {
* which is the one outcome a linter must never produce.
*/
public static MetadataLoad read(File file, IHopMetadataProvider
metadataProvider) {
- if (file == null || !file.isFile()) {
+ return read(file, null, metadataProvider);
+ }
+
+ /**
+ * Read a metadata file from another file, as the pre-commit hook does with
the staged version.
+ *
+ * @param file the file in the project, which gives the type and the name
+ * @param content the file to read, or null to read {@code file}
+ */
+ public static MetadataLoad read(File file, File content,
IHopMetadataProvider metadataProvider) {
+ if (file == null || !(content != null ? content : file).isFile()) {
return MetadataLoad.skipped();
}
@@ -163,9 +179,14 @@ public final class HopMetadataFileLoader {
return MetadataLoad.skipped();
}
try {
- IHopMetadataSerializer<IHopMetadata> serializer =
- metadataProvider.getSerializer(managedClass);
- IHopMetadata loaded = serializer.load(name);
+ IHopMetadata loaded;
+ if (content != null) {
+ loaded = parse(content, managedClass, metadataProvider);
+ } else {
+ IHopMetadataSerializer<IHopMetadata> serializer =
+ metadataProvider.getSerializer(managedClass);
+ loaded = serializer.load(name);
+ }
if (loaded != null) {
return MetadataLoad.of(named(loaded, name));
}
@@ -178,7 +199,7 @@ public final class HopMetadataFileLoader {
// Best effort without a provider. Only relational connections can be
reconstructed by hand;
// for anything else the plugin class is what gives the document meaning.
if ("rdbms".equalsIgnoreCase(key)) {
- DatabaseMeta databaseMeta = readDatabaseMetaDirectly(file);
+ DatabaseMeta databaseMeta = readDatabaseMetaDirectly(content != null ?
content : file);
return databaseMeta != null
? MetadataLoad.of(named(databaseMeta, name))
: MetadataLoad.failed("could not be parsed as a database
connection");
@@ -189,6 +210,19 @@ public final class HopMetadataFileLoader {
return MetadataLoad.skipped();
}
+ /** Parse a metadata document the way the JSON serializer does, from any
file. */
+ private static IHopMetadata parse(
+ File content, Class<IHopMetadata> managedClass, IHopMetadataProvider
metadataProvider)
+ throws IOException, HopException {
+ try (InputStream inputStream = Files.newInputStream(content.toPath());
+ JsonParser jsonParser = new JsonFactory().createParser(inputStream)) {
+ // skip opening '{'
+ jsonParser.nextToken();
+ return new JsonMetadataParser<>(managedClass, metadataProvider)
+ .loadJsonObject(managedClass, jsonParser);
+ }
+ }
+
/** The managed class for a metadata key, or null when no plugin registers
that key. */
private static Class<IHopMetadata> managedClassFor(
IHopMetadataProvider metadataProvider, String key) {
diff --git
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintCommand.java
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintCommand.java
index 371d4c7c5b..34f883b2d8 100644
--- a/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintCommand.java
+++ b/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintCommand.java
@@ -126,7 +126,9 @@ public class LintCommand implements Callable<Integer>,
IHopCommand, IHasHopMetad
@Option(
names = "--fail-on",
paramLabel = "<severity>",
- description = "Exit 1 when a finding reaches this severity. Default:
${DEFAULT-VALUE}.")
+ description =
+ "Exit 1 when a finding reaches this severity: ERROR, WARNING, INFO
or NONE. Default:"
+ + " ${DEFAULT-VALUE}.")
private String failOn = "ERROR";
@Option(
@@ -246,6 +248,11 @@ public class LintCommand implements Callable<Integer>,
IHopCommand, IHasHopMetad
}
private int run() throws Exception {
+ resolvePathOptions();
+ // Refused before anything runs: a typo here used to read as ERROR and
pass a build with
+ // warnings.
+ LintSeverity.parseFailOn(failOn);
+
if (installHook) {
installGitHook();
return 0;
@@ -272,7 +279,7 @@ public class LintCommand implements Callable<Integer>,
IHopCommand, IHasHopMetad
}
if (Utils.isEmpty(target)) {
- target = System.getProperty("target");
+ target = resolveAgainstOrigin(System.getProperty("target"));
}
if (Utils.isEmpty(target)) {
target = userDirectory();
@@ -603,6 +610,7 @@ public class LintCommand implements Callable<Integer>,
IHopCommand, IHasHopMetad
String envFailOn = System.getenv("HOP_LINT_FAIL_ON");
if (!Utils.isEmpty(envFailOn)) {
failOn = envFailOn.trim().toUpperCase();
+ LintSeverity.parseFailOn(failOn);
}
if (Utils.isEmpty(stagedFileList)) {
@@ -610,7 +618,7 @@ public class LintCommand implements Callable<Integer>,
IHopCommand, IHasHopMetad
}
prepare();
- List<File> stagedFiles =
+ List<PreCommitLintService.StagedFile> stagedFiles =
PreCommitLintService.readStagedFiles(stagedFileList, new
File(userDirectory()));
if (stagedFiles.isEmpty()) {
if (!quiet) {
@@ -618,28 +626,29 @@ public class LintCommand implements Callable<Integer>,
IHopCommand, IHasHopMetad
}
return 0;
}
+ File firstFile = stagedFiles.get(0).file();
// As strict as a plain run: a commit is not checked against default rules
because the
// project's hop-lint.yml or an installed pack could not be read.
LintRun.checkRulePacks();
HopLinter linter = new HopLinter();
- linter.loadConfigurationStrictly(stagedFiles.get(0));
+ linter.loadConfigurationStrictly(firstFile);
// The lint target for path-relative purposes is the project the staged
files live in.
- target = projectRootOf(stagedFiles.get(0));
- applyProjectTo(stagedFiles.get(0).getPath());
+ target = projectRootOf(firstFile);
+ applyProjectTo(firstFile.getPath());
IVariables variables = variables();
// Without a metadata provider a pipeline will not load at all, and
connection rules cannot
// resolve — the hook would pass commits it should have blocked.
- IHopMetadataProvider metadataProvider =
resolveMetadataProvider(stagedFiles.get(0), variables);
+ IHopMetadataProvider metadataProvider = resolveMetadataProvider(firstFile,
variables);
// A file the project excludes should not block a commit either.
- List<File> toLint = new ArrayList<>();
- for (File staged : stagedFiles) {
- if (linter.getPolicy().isExcluded(staged.getAbsolutePath(),
reportBaseDirectory())) {
+ List<PreCommitLintService.StagedFile> toLint = new ArrayList<>();
+ for (PreCommitLintService.StagedFile staged : stagedFiles) {
+ if (linter.getPolicy().isExcluded(staged.file().getAbsolutePath(),
reportBaseDirectory())) {
if (verbose) {
- System.out.println("Skipping excluded file: " + staged);
+ System.out.println("Skipping excluded file: " + staged.file());
}
continue;
}
@@ -653,7 +662,7 @@ public class LintCommand implements Callable<Integer>,
IHopCommand, IHasHopMetad
}
PreCommitLintService.Result result =
- PreCommitLintService.lintFiles(
+ PreCommitLintService.lintStagedFiles(
toLint, LintSeverity.parseFailOn(failOn), variables,
metadataProvider, true);
// The baseline matters most here: touching a legacy file would otherwise
block the commit
@@ -889,6 +898,29 @@ public class LintCommand implements Callable<Integer>,
IHopCommand, IHasHopMetad
return Utils.isEmpty(origin) ? System.getProperty("user.dir") : origin;
}
+ /**
+ * Make the target and every file option relative to where the command was
typed.
+ *
+ * <p>Java resolves a relative path against {@code user.dir}, which under
the launcher is the Hop
+ * installation: {@code hop lint .} linted the installation, {@code hop lint
naming.hpl} was not
+ * found, and {@code -o lint.sarif} wrote the report inside the installation.
+ */
+ void resolvePathOptions() {
+ target = resolveAgainstOrigin(target);
+ configFile = resolveAgainstOrigin(configFile);
+ outputFile = resolveAgainstOrigin(outputFile);
+ baselineFile = resolveAgainstOrigin(baselineFile);
+ writeBaselineFile = resolveAgainstOrigin(writeBaselineFile);
+ stagedFileList = resolveAgainstOrigin(stagedFileList);
+ }
+
+ private static String resolveAgainstOrigin(String path) {
+ if (Utils.isEmpty(path) || new File(path).isAbsolute()) {
+ return path;
+ }
+ return Paths.get(userDirectory()).resolve(path).normalize().toString();
+ }
+
private void installGitHook() throws IOException {
// Resolve from what is being linted, never from the working directory:
under the launcher that
// is the Hop installation, and the hook would land in whatever repository
Hop itself sits in.
@@ -931,18 +963,36 @@ public class LintCommand implements Callable<Integer>,
IHopCommand, IHasHopMetad
*/
String hookScript() {
// git lists staged files relative to the repository root, and the hop
launcher changes to the
- // Hop installation before it starts Java, so the paths are made absolute
here. The trap
- // removes the list however the script ends; the last command's status is
the hook's.
+ // Hop installation before it starts Java, so the paths are made absolute
here. Each line pairs
+ // a file with a copy of its staged version: the commit holds what is
staged, not what is on
+ // disk. Renamed files (R) are linted like added ones. A symlink's staged
blob is the link's
+ // target, and a filtered file's (Git LFS) is a pointer, so those are
linted on disk. The traps
+ // remove the copies however the script ends, Ctrl-C included; the last
command's status is the
+ // hook's.
return """
#!/bin/sh
ROOT="$(git rev-parse --show-toplevel)" || exit 1
- STAGED_LIST="$(mktemp)" || exit 1
- trap 'rm -f "$STAGED_LIST"' EXIT
- git diff --cached --name-only --diff-filter=ACM | while IFS= read -r
FILE; do
- case "$FILE" in
- *.hpl|*.hwf|metadata/*.json|*/metadata/*.json) printf '%s/%s\\n'
"$ROOT" "$FILE" ;;
- esac
- done > "$STAGED_LIST"
+ STAGED_DIR="$(mktemp -d)" || exit 1
+ trap 'rm -rf "$STAGED_DIR"' EXIT
+ trap 'exit 1' HUP INT TERM
+ STAGED_LIST="$STAGED_DIR/staged-files"
+ git -c core.quotePath=false diff --cached --name-only
--diff-filter=ACMR | {
+ N=0
+ while IFS= read -r FILE; do
+ case "$FILE" in
+ *.hpl|*.hwf|metadata/*.json|*/metadata/*.json)
+ MODE="$(git ls-files -s -- "$FILE" | head -n 1 | cut -d ' ' -f
1)"
+ case "$MODE:$(git check-attr filter -- "$FILE")" in
+ 120000:*) printf '%s/%s\\n' "$ROOT" "$FILE" ;;
+ *": filter: unspecified"|*": filter: unset")
+ N=$((N + 1))
+ git show ":$FILE" > "$STAGED_DIR/$N" || exit 1
+ printf '%s/%s\\t%s/%s\\n' "$ROOT" "$FILE" "$STAGED_DIR"
"$N" ;;
+ *) printf '%s/%s\\n' "$ROOT" "$FILE" ;;
+ esac ;;
+ esac
+ done
+ } > "$STAGED_LIST" || exit 1
if [ ! -s "$STAGED_LIST" ]; then
exit 0
fi
diff --git
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintSeverity.java
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintSeverity.java
index 7a7db6b3e5..78bde14ab9 100644
--- a/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintSeverity.java
+++ b/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintSeverity.java
@@ -16,6 +16,8 @@
*/
package org.apache.hop.lint;
+import java.util.Arrays;
+import java.util.stream.Collectors;
import org.apache.hop.core.ICheckResult;
import org.apache.hop.core.util.Utils;
import org.apache.hop.i18n.BaseMessages;
@@ -27,6 +29,7 @@ public final class LintSeverity {
public enum FailOn implements IEnumHasCodeAndDescription {
ERROR,
WARNING,
+ INFO,
NONE;
@Override
@@ -59,6 +62,15 @@ public final class LintSeverity {
private LintSeverity() {}
+ /**
+ * The fail-on threshold for a value given on the command line or in {@code
HOP_LINT_FAIL_ON}.
+ *
+ * <p>An unknown value is refused. Reading a typo such as {@code WARNNG} as
ERROR passed a build
+ * that should have failed on its warnings, without a word.
+ *
+ * @return ERROR when the value is empty
+ * @throws IllegalArgumentException when the value is not a threshold
+ */
public static FailOn parseFailOn(String value) {
if (Utils.isEmpty(value)) {
return FailOn.ERROR;
@@ -66,7 +78,12 @@ public final class LintSeverity {
try {
return FailOn.valueOf(value.trim().toUpperCase());
} catch (IllegalArgumentException e) {
- return FailOn.ERROR;
+ throw new IllegalArgumentException(
+ "Unknown fail-on severity '"
+ + value.trim()
+ + "'. Use one of: "
+ +
Arrays.stream(FailOn.values()).map(FailOn::name).collect(Collectors.joining(",
"))
+ + ".");
}
}
@@ -80,7 +97,10 @@ public final class LintSeverity {
if ("ERROR".equalsIgnoreCase(severity)) {
return true;
}
- return failOn == FailOn.WARNING && "WARNING".equalsIgnoreCase(severity);
+ if ("WARNING".equalsIgnoreCase(severity)) {
+ return failOn == FailOn.WARNING || failOn == FailOn.INFO;
+ }
+ return failOn == FailOn.INFO && "INFO".equalsIgnoreCase(severity);
}
public static int toCheckResultType(String severity) {
diff --git
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/PreCommitLintService.java
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/PreCommitLintService.java
index 34cd1c0136..022935755e 100644
---
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/PreCommitLintService.java
+++
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/PreCommitLintService.java
@@ -53,6 +53,21 @@ public final class PreCommitLintService {
}
}
+ /**
+ * A file in the commit, and the copy of its staged version that the git
hook made.
+ *
+ * @param file the file in the working tree, which decides the
configuration, the project and the
+ * path findings are reported against
+ * @param content the staged version to read, or null to read {@code file}
+ */
+ public record StagedFile(File file, File content) {
+
+ /** What is linted: the staged copy when there is one, the file on disk
otherwise. */
+ public File source() {
+ return content != null ? content : file;
+ }
+ }
+
private PreCommitLintService() {}
public static Result lintFiles(
@@ -75,20 +90,40 @@ public final class PreCommitLintService {
IVariables variables,
IHopMetadataProvider metadataProvider,
boolean strict) {
+ List<StagedFile> staged = new ArrayList<>();
+ for (File file : files) {
+ staged.add(new StagedFile(file, null));
+ }
+ return lintStagedFiles(staged, failOn, variables, metadataProvider,
strict);
+ }
+
+ /**
+ * Lint the staged version of each file where the git hook copied one.
+ *
+ * @see #lintFiles(List, LintSeverity.FailOn, IVariables,
IHopMetadataProvider, boolean)
+ */
+ public static Result lintStagedFiles(
+ List<StagedFile> stagedFiles,
+ LintSeverity.FailOn failOn,
+ IVariables variables,
+ IHopMetadataProvider metadataProvider,
+ boolean strict) {
List<LintResult> allResults = new ArrayList<>();
HopLinter linter = new HopLinter();
- for (File file : files) {
- if (file == null || !file.exists()) {
+ for (StagedFile staged : stagedFiles) {
+ if (staged == null || staged.file() == null ||
!staged.source().isFile()) {
continue;
}
+ File file = staged.file();
try {
if (strict) {
// lintFile() would load the configuration again, leniently.
linter.loadConfigurationStrictly(file);
- allResults.addAll(linter.processFile(file, metadataProvider,
variables));
+ allResults.addAll(
+ linter.processFile(file, staged.content(), metadataProvider,
variables));
} else {
- if (files.size() == 1 || allResults.isEmpty()) {
+ if (stagedFiles.size() == 1 || allResults.isEmpty()) {
linter.loadConfigurationForContext(file);
}
allResults.addAll(linter.lintFile(file.getAbsolutePath(),
metadataProvider, variables));
@@ -97,13 +132,13 @@ public final class PreCommitLintService {
throw e;
} catch (Exception e) {
log.logError("Error linting file " + file.getAbsolutePath() + ": " +
e.getMessage(), e);
- allResults.add(
- new LintResult(
- "SYSTEM-001",
- "File Processing Error",
- "ERROR",
+ linter
+ .systemFinding(
+ HopLinter.SYSTEM_FILE_PROCESSING,
"Failed to process file: " + e.getMessage(),
- LintPathUtils.normalizePath(file.getAbsolutePath())));
+ LintPathUtils.normalizePath(file.getAbsolutePath()),
+ null)
+ .ifPresent(allResults::add);
}
}
@@ -116,21 +151,25 @@ public final class PreCommitLintService {
return new Result(allResults, blocking);
}
- public static List<File> readStagedFiles(String stagedFileListPath) throws
HopException {
+ public static List<StagedFile> readStagedFiles(String stagedFileListPath)
throws HopException {
return readStagedFiles(stagedFileListPath, null);
}
/**
* The staged files to lint.
*
+ * <p>Each line is a file, optionally followed by a tab and the copy of its
staged version that
+ * the git hook wrote. With a copy the file itself need not exist: it can be
staged and then
+ * deleted or changed on disk, and the commit still holds the staged version.
+ *
* @param baseDirectory what a relative path in the list is relative to. git
lists staged files
* relative to the repository root, and the hop launcher changes to the
Hop installation
* before it starts Java, so resolving them against the working
directory found none of them
* and the hook let every commit through.
*/
- public static List<File> readStagedFiles(String stagedFileListPath, File
baseDirectory)
+ public static List<StagedFile> readStagedFiles(String stagedFileListPath,
File baseDirectory)
throws HopException {
- List<File> files = new ArrayList<>();
+ List<StagedFile> files = new ArrayList<>();
File listFile = new File(stagedFileListPath);
if (!listFile.isFile()) {
throw new HopException("Staged file list not found: " +
stagedFileListPath);
@@ -138,22 +177,25 @@ public final class PreCommitLintService {
try {
for (String line : java.nio.file.Files.readAllLines(listFile.toPath())) {
- String trimmed = line.trim();
+ String[] parts = line.split("\t", 2);
+ String trimmed = parts[0].trim();
if (trimmed.isEmpty()) {
continue;
}
- File candidate = new File(trimmed);
- if (!candidate.isAbsolute() && baseDirectory != null) {
- candidate = new File(baseDirectory, trimmed);
- }
+ File candidate = resolve(trimmed, baseDirectory);
if (!isLintablePath(candidate.getAbsolutePath())) {
continue;
}
- if (candidate.isFile()) {
- files.add(candidate);
+ File content =
+ parts.length > 1 && !parts[1].isBlank()
+ ? resolve(parts[1].trim(), baseDirectory)
+ : null;
+ StagedFile staged = new StagedFile(candidate, content);
+ if (staged.source().isFile()) {
+ files.add(staged);
} else {
// A staged file the hook cannot find is a hook that checks nothing,
so say so.
- System.err.println("Staged file not found, not linted: " +
candidate.getPath());
+ System.err.println("Staged file not found, not linted: " +
staged.source().getPath());
}
}
} catch (Exception e) {
@@ -162,6 +204,14 @@ public final class PreCommitLintService {
return files;
}
+ private static File resolve(String path, File baseDirectory) {
+ File file = new File(path);
+ if (!file.isAbsolute() && baseDirectory != null) {
+ file = new File(baseDirectory, path);
+ }
+ return file;
+ }
+
public static boolean isLintablePath(String path) {
if (path == null) {
return false;
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 7a01748541..9da5762e2b 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
@@ -546,9 +546,11 @@ public class RuleBuilderDialog extends Dialog {
populateTags();
loadClausesFromRule();
- // A native rule, such as HOP-CHECK, says how Hop's own verify remarks are
reported. It has no
- // target, field or condition, and the editor refused to save it, even a
severity change.
- if (rule.isNativeVerify()) {
+ // A native rule, such as HOP-CHECK, says how Hop's own verify remarks are
reported, and a
+ // system
+ // rule how a file the linter could not read is. They have no target,
field or condition, and
+ // the editor refused to save them, even a severity change.
+ if (rule.isReportOnly()) {
for (Control control :
new Control[] {
targetCombo,
@@ -730,7 +732,7 @@ public class RuleBuilderDialog extends Dialog {
return false;
}
}
- if (rule.isNativeVerify()) {
+ if (rule.isReportOnly()) {
return true;
}
if (targetCombo.getSelectionIndex() < 0) {
@@ -741,7 +743,11 @@ public class RuleBuilderDialog extends Dialog {
// Every clause, not only the one on show. Saving relies on each having a
field and condition.
captureWidgetsIntoSelectedClause();
for (int i = 0; i < editingClauses.size(); i++) {
- String problem = clauseProblem(editingClauses.get(i));
+ String problem =
+ clauseProblem(
+ editingClauses.get(i),
+ RuleTarget.values()[targetCombo.getSelectionIndex()],
+ !rule.getAppliesTo().isEmpty());
if (problem != null) {
clauseTable.setSelection(i);
loadSelectedClauseIntoWidgets();
@@ -788,6 +794,17 @@ public class RuleBuilderDialog extends Dialog {
return null;
}
+ /**
+ * What is missing from a clause or wrong with its value, or null when the
rule can use it.
+ *
+ * @param scoped whether the rule is narrowed with appliesTo
+ * @see RuleClause#valueProblem(RuleTarget, boolean)
+ */
+ static String clauseProblem(RuleClause clause, RuleTarget target, boolean
scoped) {
+ String problem = clauseProblem(clause);
+ return problem != null ? problem : clause.valueProblem(target, scoped);
+ }
+
private void saveRule() {
if (idText.getEditable()) {
rule.setId(idText.getText().trim());
@@ -799,7 +816,7 @@ public class RuleBuilderDialog extends Dialog {
rule.setHelpUri(
Utils.isEmpty(helpUriText.getText().trim()) ? null :
helpUriText.getText().trim());
rule.setTags(readTags());
- if (rule.isNativeVerify()) {
+ if (rule.isReportOnly()) {
return;
}
diff --git
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/RuleClause.java
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/RuleClause.java
index dda5041401..ef1bbbf6c8 100644
--- a/plugins/misc/lint/src/main/java/org/apache/hop/lint/RuleClause.java
+++ b/plugins/misc/lint/src/main/java/org/apache/hop/lint/RuleClause.java
@@ -17,7 +17,12 @@
package org.apache.hop.lint;
+import java.util.List;
import java.util.Objects;
+import java.util.regex.Pattern;
+import java.util.regex.PatternSyntaxException;
+import org.apache.hop.core.util.Utils;
+import org.apache.hop.i18n.BaseMessages;
/**
* One thing a rule checks: a field, a condition, and the value the condition
compares against.
@@ -29,6 +34,8 @@ import java.util.Objects;
*/
public class RuleClause {
+ private static final Class<?> PKG = RuleClause.class; // for i18n purposes
+
private String targetField;
private RuleCondition condition;
private String conditionValue;
@@ -65,6 +72,103 @@ public class RuleClause {
this.conditionValue = conditionValue;
}
+ /**
+ * Why this clause can never report anything, or null when it can be
evaluated.
+ *
+ * <p>A value the condition cannot use, such as {@code abc} for a maximum or
{@code [a-} for a
+ * pattern, made the rule report nothing at all and log an exception on
every run, which reads as
+ * a clean result. A missing field, condition or value is not judged here.
+ *
+ * @param target what the rule checks, for the fields the linter itself
provides
+ * @param scoped whether the rule is narrowed with {@code appliesTo}; such a
rule can read the
+ * plugin's own settings, whose types are not known here, so only the
value is judged
+ */
+ public String valueProblem(RuleTarget target, boolean scoped) {
+ if (condition == null || Utils.isEmpty(targetField)) {
+ return null;
+ }
+ if (!scoped && neverFires(RuleTargetFields.getValueKind(target,
targetField), condition)) {
+ return BaseMessages.getString(
+ PKG, "RuleClause.Problem.ConditionDoesNotFit", condition.name(),
targetField);
+ }
+ if (!condition.requiresValue() || Utils.isEmpty(conditionValue)) {
+ return null;
+ }
+ String value = conditionValue.trim();
+ switch (condition) {
+ case MAX_VALUE, MIN_VALUE, EXACT_VALUE:
+ try {
+ Double.parseDouble(value);
+ } catch (NumberFormatException e) {
+ return BaseMessages.getString(
+ PKG, "RuleClause.Problem.NotANumber", condition.name(),
conditionValue);
+ }
+ break;
+ case MAX_COLLECTION_SIZE, MIN_COLLECTION_SIZE:
+ try {
+ Integer.parseInt(value);
+ } catch (NumberFormatException e) {
+ return BaseMessages.getString(
+ PKG, "RuleClause.Problem.NotAWholeNumber", condition.name(),
conditionValue);
+ }
+ break;
+ case MATCHES_PATTERN, NOT_MATCHES_PATTERN:
+ try {
+ Pattern.compile(conditionValue);
+ } catch (PatternSyntaxException e) {
+ return BaseMessages.getString(
+ PKG,
+ "RuleClause.Problem.InvalidPattern",
+ condition.name(),
+ conditionValue,
+ e.getDescription());
+ }
+ break;
+ default:
+ break;
+ }
+ return null;
+ }
+
+ /**
+ * Whether the rule executor can never report this condition on a value of
this kind.
+ *
+ * <p>Only what it cannot fire on, not what the editor would suggest: the
text conditions read any
+ * value as text, so {@code port NO_HARDCODED} and {@code targetTransforms
CONTAINS} work.
+ */
+ private static boolean neverFires(RuleTargetFields.ValueKind kind,
RuleCondition condition) {
+ if (kind == null) {
+ return false;
+ }
+ boolean numeric =
+ condition == RuleCondition.MAX_VALUE
+ || condition == RuleCondition.MIN_VALUE
+ || condition == RuleCondition.EXACT_VALUE;
+ boolean collection =
+ condition == RuleCondition.NOT_EMPTY_COLLECTION
+ || condition == RuleCondition.MAX_COLLECTION_SIZE
+ || condition == RuleCondition.MIN_COLLECTION_SIZE;
+ return switch (kind) {
+ // A count is always there, and never reads as empty.
+ case COUNT ->
+ collection || condition == RuleCondition.NOT_NULL || condition ==
RuleCondition.NOT_EMPTY;
+ case FLAG -> numeric || collection;
+ case LIST -> numeric;
+ case TEXT -> collection;
+ };
+ }
+
+ /** The first problem {@link #valueProblem} finds in any of these clauses,
or null. */
+ public static String valueProblem(List<RuleClause> clauses, RuleTarget
target, boolean scoped) {
+ for (RuleClause clause : clauses) {
+ String problem = clause.valueProblem(target, scoped);
+ if (problem != null) {
+ return problem;
+ }
+ }
+ return null;
+ }
+
public RuleClause copy() {
return new RuleClause(targetField, condition, conditionValue);
}
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 56cea11c44..265f0ed85c 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
@@ -385,6 +385,15 @@ public class RuleManagerDialog extends Dialog {
editButton.setEnabled(hasSelection);
deleteButton.setEnabled(projectEditable);
toggleButton.setEnabled(hasSelection);
+ // "Toggle Enabled" read the same either way, and only the far-right
column said what it did.
+ toggleButton.setText(
+ BaseMessages.getString(
+ PKG,
+ !hasSelection
+ ? "RuleManagerDialog.Button.ToggleEnabled"
+ : selected.isEnabled()
+ ? "RuleManagerDialog.Button.DisableRule"
+ : "RuleManagerDialog.Button.EnableRule"));
// A project rule with a pack rule's id replaces it, so removing it brings
the pack rule back.
deleteButton.setText(
BaseMessages.getString(
diff --git
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/RuleTargetFields.java
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/RuleTargetFields.java
index bf367db4e6..50e3fffd5b 100644
--- a/plugins/misc/lint/src/main/java/org/apache/hop/lint/RuleTargetFields.java
+++ b/plugins/misc/lint/src/main/java/org/apache/hop/lint/RuleTargetFields.java
@@ -197,6 +197,85 @@ public class RuleTargetFields {
}
}
+ /** What kind of value a field the linter provides holds, as the rule
executor reads it. */
+ public enum ValueKind {
+ /** A number that is always there, such as {@code transformCount}. */
+ COUNT,
+ /** True or false. */
+ FLAG,
+ /** A list or a map, such as {@code targetTransforms} or a connection's
{@code attributes}. */
+ LIST,
+ /** Text, or null when not set. A connection's {@code port} is text. */
+ TEXT
+ }
+
+ private static final Map<String, ValueKind> VALUE_KINDS = new HashMap<>();
+
+ static {
+ for (String field :
+ List.of("transformCount", "actionCount", "hopCount", "noteCount",
"copies")) {
+ VALUE_KINDS.put(field, ValueKind.COUNT);
+ }
+ for (String field :
+ List.of(
+ "hasDisabledHops",
+ "hasOrphanedTransforms",
+ "hasOrphanedActions",
+ "hasNotes",
+ "isReferenced",
+ "distributes",
+ "errorHandling",
+ "isDummy",
+ "isOrphaned",
+ "isBlockingTransform",
+ "hasDefaultName",
+ "isStart",
+ "enabled",
+ "unconditional",
+ "evaluation")) {
+ VALUE_KINDS.put(field, ValueKind.FLAG);
+ }
+ for (String field : List.of("targetTransforms", "targetActions",
"attributes")) {
+ VALUE_KINDS.put(field, ValueKind.LIST);
+ }
+ for (String field :
+ List.of(
+ "name",
+ "description",
+ "filename",
+ "pluginId",
+ "databaseType",
+ "hostname",
+ "port",
+ "databaseName",
+ "username",
+ "password",
+ "servername",
+ "dataTablespace",
+ "indexTablespace",
+ "fromTransform",
+ "toTransform",
+ "fromAction",
+ "toAction")) {
+ VALUE_KINDS.put(field, ValueKind.TEXT);
+ }
+ }
+
+ /**
+ * The kind of value one of the target's own fields holds, or null when it
is not known: a field
+ * the target does not provide, or one whose value the linter does not read
itself, such as the
+ * secret fields of a transform.
+ */
+ public static ValueKind getValueKind(RuleTarget target, String field) {
+ if (target == RuleTarget.TRANSFORM || target == RuleTarget.ACTION) {
+ // Matched against every setting of the plugin by name, whatever its
type.
+ if (List.of("password", "secret", "credential", "apiKey",
"token").contains(field)) {
+ return null;
+ }
+ }
+ return getFieldsForTarget(target).contains(field) ? VALUE_KINDS.get(field)
: null;
+ }
+
/** Get compatible conditions for a field type */
public static List<RuleCondition> getCompatibleConditions(String field) {
// Determine field type and return compatible conditions
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 4bb9c5f4b1..04d66c312c 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
@@ -88,7 +88,7 @@ public final class EffectiveRuleSet {
public List<CustomLintRule> getEnabledPolicyRules() {
List<CustomLintRule> enabled = new ArrayList<>();
for (CustomLintRule rule : rules) {
- if (rule.isEnabled() && !rule.isNativeVerify()) {
+ if (rule.isEnabled() && !rule.isReportOnly()) {
enabled.add(rule);
}
}
@@ -101,6 +101,16 @@ public final class EffectiveRuleSet {
* <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.
*/
+ /** The rule with this id, or null. */
+ public CustomLintRule findRule(String ruleId) {
+ for (CustomLintRule rule : rules) {
+ if (ruleId != null && ruleId.equals(rule.getId())) {
+ return rule;
+ }
+ }
+ return null;
+ }
+
public List<CustomLintRule> getNativeVerifyRules() {
List<CustomLintRule> nativeRules = new ArrayList<>();
for (CustomLintRule rule : rules) {
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 e36e9b39a9..44448fca2b 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
@@ -248,7 +248,7 @@ public final class ProjectLintYamlExporter {
}
private static Map<String, Object> toFullCustomRuleMap(CustomLintRule rule) {
- if (rule.isNativeVerify()) {
+ if (rule.isReportOnly()) {
return toFullNativeRuleMap(rule);
}
Map<String, Object> ruleConfig = new LinkedHashMap<>();
@@ -316,7 +316,7 @@ public final class ProjectLintYamlExporter {
*/
private static Map<String, Object> toFullNativeRuleMap(CustomLintRule rule) {
Map<String, Object> ruleConfig = new LinkedHashMap<>();
- ruleConfig.put("type", CustomLintRule.TYPE_NATIVE);
+ ruleConfig.put("type", rule.getType());
ruleConfig.put("enabled", rule.isEnabled());
ruleConfig.put("severity", rule.getSeverity());
if (!rule.getAppliesTo().isEmpty()) {
diff --git
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/RulePackDiscovery.java
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/RulePackDiscovery.java
index 560f898b22..b4722d7882 100644
---
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/RulePackDiscovery.java
+++
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/RulePackDiscovery.java
@@ -219,7 +219,9 @@ public class RulePackDiscovery {
metadata.owner(),
metadata.priority(),
metadata.overrides());
- } catch (IOException e) {
+ } catch (IOException | RuntimeException e) {
+ // Loading the rules fails again, and the registry reports that with the
pack's name. A YAML
+ // error escaping from here stopped hop lint with only snakeyaml's line
and column.
LogChannel.GENERAL.logError(
"Failed to read pack metadata from "
+ yamlFile.getAbsolutePath()
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 fd807c1b6f..a4201c9708 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
@@ -28,6 +28,7 @@ import org.apache.hop.core.util.Utils;
import org.apache.hop.lint.CustomLintRule;
import org.apache.hop.lint.LinterConfig;
import org.apache.hop.lint.LinterConfigPlugin;
+import org.apache.hop.lint.RuleClause;
import org.apache.hop.lint.RuleConfig;
/**
@@ -219,12 +220,43 @@ public class RuleRegistry {
}
}
+ warnAboutRulesThatCannotReport(merged.values(), warnings);
+
LinterConfig config = buildLinterConfig(merged);
config.setEnabled(true);
return new EffectiveRuleSet(
new ArrayList<>(merged.values()), config, overlay.getPolicy(),
warnings);
}
+ /**
+ * Warn about each enabled rule whose value its condition cannot use, such
as a maximum of {@code
+ * abc} or a pattern of {@code [a-}. Such a rule never reports anything,
which reads as a clean
+ * result, so it is named the way an unknown rule id is.
+ */
+ private static void warnAboutRulesThatCannotReport(
+ Collection<CustomLintRule> rules, List<String> warnings) {
+ for (CustomLintRule rule : rules) {
+ if (!rule.isEnabled() || rule.isReportOnly() || rule.getTarget() ==
null) {
+ continue;
+ }
+ String problem =
+ RuleClause.valueProblem(
+ rule.getClauses(), rule.getTarget(),
!rule.getAppliesTo().isEmpty());
+ if (problem == null) {
+ continue;
+ }
+ String warning =
+ "Warning: rule '"
+ + rule.generateRuleId()
+ + "' "
+ + YamlRulePackParser.inPack(rule.getPackId())
+ + " cannot report anything: "
+ + problem;
+ warnings.add(warning);
+ LintWarnings.logOnce(warning, warning);
+ }
+ }
+
/** The rule of this id, ignoring case: {@code sql-002} in hop-lint.yml
means SQL-002. */
private static CustomLintRule findRule(Map<String, CustomLintRule> rules,
String ruleId) {
CustomLintRule exact = rules.get(ruleId);
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 16b97e756a..9d1c6620e0 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
@@ -39,6 +39,7 @@ import org.apache.hop.lint.RuleTarget;
import org.yaml.snakeyaml.LoaderOptions;
import org.yaml.snakeyaml.Yaml;
import org.yaml.snakeyaml.constructor.SafeConstructor;
+import org.yaml.snakeyaml.error.YAMLException;
/** Parses YAML rule pack and project overlay files. */
public final class YamlRulePackParser {
@@ -75,6 +76,10 @@ public final class YamlRulePackParser {
"tags",
"helpUri");
+ /** The keys a rule covering the linter's own findings about a run may
carry. */
+ static final Set<String> SYSTEM_RULE_KEYS =
+ Set.of("type", "name", "description", "enabled", "severity", "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(
@@ -157,9 +162,51 @@ public final class YamlRulePackParser {
}
}
+ /**
+ * The {@code rules:} block, checked to be a mapping of rule ids to mappings.
+ *
+ * <p>Written with dashes, like {@code exclude:} and {@code suppress:}, it
failed with "class
+ * java.util.ArrayList cannot be cast to class java.util.Map".
+ *
+ * @param source what is being read, for the error message
+ */
+ @SuppressWarnings("unchecked")
+ private static Map<String, Object> rulesSection(Map<String, Object>
yamlData, String source)
+ throws IOException {
+ Object rules = yamlData.get("rules");
+ if (rules == null) {
+ return null;
+ }
+ if (!(rules instanceof Map<?, ?> map)) {
+ throw new IOException(
+ source
+ + ": rules: maps each rule id to its settings, as in 'DB-001:'
followed by"
+ + " 'severity: WARNING' on an indented line, and is not a
list.");
+ }
+ for (Map.Entry<?, ?> entry : map.entrySet()) {
+ if (entry.getValue() != null && !(entry.getValue() instanceof Map)) {
+ throw new IOException(
+ source
+ + ": rule '"
+ + entry.getKey()
+ + "' has to be a mapping of settings such as severity: and
enabled:, not '"
+ + entry.getValue()
+ + "'.");
+ }
+ }
+ return (Map<String, Object>) map;
+ }
+
private static Map<String, Object> readYaml(InputStream inputStream, String
source)
throws IOException {
- Object loaded = new Yaml(new SafeConstructor(new
LoaderOptions())).load(inputStream);
+ Object loaded;
+ try {
+ loaded = new Yaml(new SafeConstructor(new
LoaderOptions())).load(inputStream);
+ } catch (YAMLException e) {
+ // Read as an I/O problem, so that a broken pack is reported as that
pack's error. The
+ // callers name the file or pack; snakeyaml only says where in "reader"
it gave up.
+ throw new IOException(e.getMessage(), e);
+ }
if (loaded == null) {
return Collections.emptyMap();
}
@@ -188,11 +235,11 @@ public final class YamlRulePackParser {
for (ClassLoader loader : classLoadersToTry(anchor)) {
try (InputStream inputStream = loader.getResourceAsStream(resourcePath))
{
if (inputStream != null) {
- return loadFromStream(inputStream, defaultPackId, defaultOwner);
+ return loadFromStream(inputStream, defaultPackId, defaultOwner,
resourcePath);
}
} catch (IOException e) {
- LogChannel.GENERAL.logDetailed(
- "Failed to read rule pack " + resourcePath + " from " + loader +
": " + e.getMessage());
+ // A pack that cannot be read is reported, with its name, rather than
loaded empty.
+ throw new IllegalStateException("Failed to load rule pack " +
resourcePath, e);
}
}
@@ -256,14 +303,23 @@ public final class YamlRulePackParser {
public static List<CustomLintRule> loadFromFile(
File file, String defaultPackId, RulePackOwner defaultOwner) throws
IOException {
try (InputStream inputStream = Files.newInputStream(file.toPath())) {
- return loadFromStream(inputStream, defaultPackId, defaultOwner);
+ return loadFromStream(inputStream, defaultPackId, defaultOwner,
file.getPath());
}
}
public static List<CustomLintRule> loadFromStream(
InputStream inputStream, String defaultPackId, RulePackOwner
defaultOwner)
throws IOException {
- Map<String, Object> yamlData = readYaml(inputStream, "The rule pack");
+ return loadFromStream(inputStream, defaultPackId, defaultOwner, "The rule
pack");
+ }
+
+ /**
+ * @param source what is being read, for the error messages: a file path
where there is one
+ */
+ private static List<CustomLintRule> loadFromStream(
+ InputStream inputStream, String defaultPackId, RulePackOwner
defaultOwner, String source)
+ throws IOException {
+ Map<String, Object> yamlData = readYaml(inputStream, source);
PackMetadata metadata = parsePackMetadata(yamlData, defaultPackId,
defaultOwner, 0);
return parseRulesSection(yamlData, metadata);
}
@@ -292,8 +348,7 @@ public final class YamlRulePackParser {
}
LintPolicy policy = parsePolicy(yamlData, projectYaml);
- @SuppressWarnings("unchecked")
- Map<String, Object> rulesSection = (Map<String, Object>)
yamlData.get("rules");
+ Map<String, Object> rulesSection = rulesSection(yamlData,
projectYaml.getPath());
if (rulesSection == null || rulesSection.isEmpty()) {
return new ProjectYamlOverlay(Collections.emptyList(),
Collections.emptyMap(), policy);
}
@@ -311,6 +366,10 @@ public final class YamlRulePackParser {
projectRules.add(
parseNativeRule(
ruleId, ruleData, RulePackIds.PROJECT, RulePackOwner.PROJECT,
location));
+ } else if (isSystemRuleDefinition(ruleData)) {
+ projectRules.add(
+ parseSystemRule(
+ ruleId, ruleData, RulePackIds.PROJECT, RulePackOwner.PROJECT,
location));
} else if (isCustomRuleDefinition(ruleData)) {
CustomLintRule rule =
parseCustomRule(ruleId, ruleData, RulePackIds.PROJECT,
RulePackOwner.PROJECT, location);
@@ -420,9 +479,9 @@ public final class YamlRulePackParser {
}
private static List<CustomLintRule> parseRulesSection(
- Map<String, Object> yamlData, PackMetadata metadata) {
- @SuppressWarnings("unchecked")
- Map<String, Object> rulesSection = (Map<String, Object>)
yamlData.get("rules");
+ Map<String, Object> yamlData, PackMetadata metadata) throws IOException {
+ Map<String, Object> rulesSection =
+ rulesSection(yamlData, "Rule pack '" + metadata.packId() + "'");
if (rulesSection == null || rulesSection.isEmpty()) {
return Collections.emptyList();
}
@@ -435,6 +494,12 @@ public final class YamlRulePackParser {
rules.add(parseNativeRule(ruleId, ruleData, metadata.packId(),
metadata.owner()));
continue;
}
+ if (isSystemRuleDefinition(ruleData)) {
+ rules.add(
+ parseSystemRule(
+ ruleId, ruleData, metadata.packId(), metadata.owner(),
inPack(metadata.packId())));
+ continue;
+ }
if (!isCustomRuleDefinition(ruleData)) {
continue;
}
@@ -510,6 +575,34 @@ public final class YamlRulePackParser {
return rule;
}
+ public static boolean isSystemRuleDefinition(Map<String, Object> ruleData) {
+ return ruleData != null &&
CustomLintRule.TYPE_SYSTEM.equals(ruleData.get("type"));
+ }
+
+ /**
+ * Read a rule that says how the linter reports a problem with the run
itself, such as a file it
+ * could not read. Like a native rule it has no target or condition.
+ */
+ private static CustomLintRule parseSystemRule(
+ String ruleId,
+ Map<String, Object> ruleData,
+ String packId,
+ RulePackOwner owner,
+ String location) {
+ CustomLintRule rule = new CustomLintRule();
+ rule.setId(ruleId);
+ rule.setPackId(packId);
+ rule.setPackOwner(owner);
+ rule.setType(CustomLintRule.TYPE_SYSTEM);
+ 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"), "ERROR"));
+ applyDocumentation(rule, ruleData, location);
+ warnUnknownKeys(ruleId, location, ruleData, SYSTEM_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));
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 2b2b032b1f..cc555bcadb 100644
--- a/plugins/misc/lint/src/main/resources/hop-lint-core.yml
+++ b/plugins/misc/lint/src/main/resources/hop-lint-core.yml
@@ -166,6 +166,36 @@ rules:
build; a comment stays a comment. 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.
+ # ------------------------------------------------------------------
+ # The linter's own findings about a run.
+ #
+ # A `system` rule does not check anything either: it says how the linter
+ # reports a file it could not read or a run it could not finish. They ship at
+ # ERROR, because a file that was not linted is a file nothing was checked
+ # against; a project can lower that, or switch one off, like any other rule.
+ # ------------------------------------------------------------------
+
+ SYSTEM-001:
+ type: system
+ enabled: true
+ severity: ERROR
+ name: "File Processing Error"
+ description: "A pipeline, workflow or metadata file could not be loaded,
so no rule was checked against it"
+
+ SYSTEM-002:
+ type: system
+ enabled: true
+ severity: ERROR
+ name: "Configuration Error"
+ description: "The lint run stopped before it finished, so the files it did
not reach were not checked"
+
+ SYSTEM-003:
+ type: system
+ enabled: true
+ severity: ERROR
+ name: "Unreadable Metadata"
+ description: "A metadata file, such as a connection, could not be read, so
no rule was checked against it"
+
# 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
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 7b3cc65bb9..e4ba795394 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
@@ -88,6 +88,10 @@ RuleBuilderDialog.Error.Clause=Clause {0}: {1}
RuleBuilderDialog.Error.FieldRequired=Field must be selected
RuleBuilderDialog.Error.ConditionRequired=Condition must be selected
RuleBuilderDialog.Error.ValueRequired=Value is required for this condition
+RuleClause.Problem.ConditionDoesNotFit={0} does not apply to {1}, so the rule
can never report anything
+RuleClause.Problem.NotANumber={0} compares with a number, and ''{1}'' is not
one. Variables are not resolved in rule values.
+RuleClause.Problem.NotAWholeNumber={0} compares with a whole number, and
''{1}'' is not one. Variables are not resolved in rule values.
+RuleClause.Problem.InvalidPattern={0} needs a regular expression, and ''{1}''
is not valid: {2}
# ---------------------------------------------------------------------------
# Rule manager dialog
@@ -108,6 +112,8 @@ RuleManagerDialog.Button.EditRule=Edit Rule
RuleManagerDialog.Button.DeleteRule=Delete Rule
RuleManagerDialog.Button.RestorePackRule=Restore Pack Rule
RuleManagerDialog.Button.ToggleEnabled=Toggle Enabled
+RuleManagerDialog.Button.DisableRule=Disable Rule
+RuleManagerDialog.Button.EnableRule=Enable Rule
RuleManagerDialog.Button.Close=Close
RuleManagerDialog.Dialog.ConfirmDelete.Title=Confirm Delete
RuleManagerDialog.Dialog.ConfirmDelete.Message=Delete project rule ''{0}''?
@@ -254,6 +260,7 @@ LintSeverity.Level.WARNING=Warning
LintSeverity.Level.INFO=Info
LintSeverity.FailOn.ERROR=Error
LintSeverity.FailOn.WARNING=Warning
+LintSeverity.FailOn.INFO=Info
LintSeverity.FailOn.NONE=None
LintReportFormat.TEXT=Text
LintReportFormat.JSON=JSON
diff --git
a/plugins/misc/lint/src/test/java/org/apache/hop/lint/HopMetadataFileLoaderTest.java
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/HopMetadataFileLoaderTest.java
index 3984a8844c..770b07643a 100644
---
a/plugins/misc/lint/src/test/java/org/apache/hop/lint/HopMetadataFileLoaderTest.java
+++
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/HopMetadataFileLoaderTest.java
@@ -77,6 +77,38 @@ public class HopMetadataFileLoaderTest {
assertEquals("SALES_PRD", HopMetadataFileLoader.metadataNameOf(file));
}
+ /**
+ * The git hook lints the staged version of a connection, which can differ
from the one on disk:
+ * the type and the name come from the file, the settings from the staged
copy.
+ *
+ * @see <a href="https://github.com/apache/hop/issues/9071";>#9071</a>
+ */
+ @Test
+ public void aStagedCopyIsReadInsteadOfTheFile(@TempDir Path dir) throws
Exception {
+ File file =
+ metadataFile(
+ dir,
+ "rdbms",
+ "SALES_PRD.json",
+ """
+ {"rdbms": {"POSTGRESQL": {"hostname": "on-disk", "password":
"${PG_PASSWORD}"}}}
+ """);
+ Path staged = dir.resolve("staged-copy");
+ Files.writeString(
+ staged,
+ """
+ {"rdbms": {"POSTGRESQL": {"hostname": "staged", "password":
"hunter2"}}}
+ """,
+ StandardCharsets.UTF_8);
+
+ HopMetadataFileLoader.MetadataLoad load =
+ HopMetadataFileLoader.read(file, staged.toFile(), null);
+
+ DatabaseMeta databaseMeta = (DatabaseMeta) load.object();
+ assertEquals("staged", databaseMeta.getHostname());
+ assertEquals("SALES_PRD", databaseMeta.getName());
+ }
+
@Test
public void relationalConnectionsLoadWithoutAProvider(@TempDir Path dir)
throws Exception {
File file =
diff --git
a/plugins/misc/lint/src/test/java/org/apache/hop/lint/LintCommandOptionsTest.java
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/LintCommandOptionsTest.java
new file mode 100644
index 0000000000..206f55553a
--- /dev/null
+++
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/LintCommandOptionsTest.java
@@ -0,0 +1,249 @@
+/*
+ * 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.assertTrue;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import java.io.ByteArrayOutputStream;
+import java.io.PrintStream;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import org.apache.hop.core.HopEnvironment;
+import org.apache.hop.core.variables.Variables;
+import org.apache.hop.pipeline.PipelineMeta;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+import picocli.CommandLine;
+
+/**
+ * {@code hop lint} paths, thresholds, system findings and the staged files
the git hook passes.
+ *
+ * @see <a href="https://github.com/apache/hop/issues/9071";>#9071</a>
+ */
+public class LintCommandOptionsTest {
+
+ @TempDir private Path dir;
+
+ private Path project;
+
+ private String originDir;
+
+ @BeforeAll
+ static void initHop() throws Exception {
+ HopEnvironment.init();
+ }
+
+ /** A project with a pipeline that cannot be read, which SYSTEM-001 reports.
*/
+ @BeforeEach
+ void createProject() throws Exception {
+ project = dir.resolve("project");
+ Files.createDirectories(project);
+ writePipeline(project.resolve("customers.hpl"), "Customers");
+ Files.writeString(project.resolve("broken.hpl"), "this is not a pipeline");
+ originDir = System.getProperty("hop.origin.dir");
+ }
+
+ @AfterEach
+ void restoreOrigin() {
+ if (originDir == null) {
+ System.clearProperty("hop.origin.dir");
+ } else {
+ System.setProperty("hop.origin.dir", originDir);
+ }
+ CustomRuleExecutor.setProjectIndex(null);
+ }
+
+ private static void writePipeline(Path file, String description) throws
Exception {
+ PipelineMeta pipelineMeta = new PipelineMeta();
+ pipelineMeta.setName(file.getFileName().toString().replace(".hpl", ""));
+ pipelineMeta.setDescription(description);
+ Files.writeString(file,
pipelineMeta.getXml(Variables.getADefaultVariableSpace()));
+ }
+
+ /** Runs hop lint; returns the exit code, with stdout and stderr in {@code
output}. */
+ private static int hopLint(StringBuilder output, String... args) {
+ ByteArrayOutputStream bytes = new ByteArrayOutputStream();
+ PrintStream capture = new PrintStream(bytes, true, StandardCharsets.UTF_8);
+ PrintStream systemOut = System.out;
+ PrintStream systemErr = System.err;
+ int exitCode;
+ try {
+ System.setOut(capture);
+ System.setErr(capture);
+ exitCode = new CommandLine(new LintCommand()).execute(args);
+ } finally {
+ System.setOut(systemOut);
+ System.setErr(systemErr);
+ }
+ output.append(bytes.toString(StandardCharsets.UTF_8));
+ return exitCode;
+ }
+
+ private static JsonNode report(Path file) throws Exception {
+ return new ObjectMapper().readTree(Files.readString(file));
+ }
+
+ // ------------------------------------------------------------------
relative paths
+
+ /**
+ * The launcher runs Java from the Hop installation, so {@code hop lint .}
linted the installation
+ * and {@code -o lint.json} wrote the report there.
+ */
+ @Test
+ public void relativePathsAreResolvedAgainstWhereHopLintWasRun() throws
Exception {
+ System.setProperty("hop.origin.dir", project.toString());
+
+ StringBuilder output = new StringBuilder();
+ int status = hopLint(output, "-f", "JSON", "-o", "lint.json", "--fail-on",
"NONE", ".");
+
+ assertEquals(0, status, output.toString());
+ Path written = project.resolve("lint.json");
+ assertTrue(Files.isRegularFile(written), "the report belongs in the
project: " + output);
+ assertEquals(1, report(written).get("summary").get("total").asInt(),
output.toString());
+ }
+
+ @Test
+ public void aRelativeFileTargetIsFound() throws Exception {
+ System.setProperty("hop.origin.dir", project.toString());
+
+ StringBuilder output = new StringBuilder();
+ int status = hopLint(output, "customers.hpl");
+
+ assertEquals(0, status, output.toString());
+ assertFalse(output.toString().contains("does not exist"),
output.toString());
+ }
+
+ @Test
+ public void baselineFilesAreRelativeToo() throws Exception {
+ System.setProperty("hop.origin.dir", project.toString());
+
+ StringBuilder output = new StringBuilder();
+ assertEquals(0, hopLint(output, "--write-baseline", "baseline.json", "."),
output.toString());
+ assertTrue(Files.isRegularFile(project.resolve("baseline.json")),
output.toString());
+ assertEquals(0, hopLint(output, "--baseline", "baseline.json", "."),
output.toString());
+ }
+
+ // ------------------------------------------------------------------
--fail-on
+
+ /** A typo used to read as ERROR, and a project with warnings passed. */
+ @Test
+ public void anUnknownFailOnIsRefused() {
+ StringBuilder output = new StringBuilder();
+
+ int status = hopLint(output, "--fail-on", "WARNNG", project.toString());
+
+ assertEquals(1, status, output.toString());
+ assertTrue(output.toString().contains("Unknown fail-on severity
'WARNNG'"), output.toString());
+ }
+
+ /** --fail-on INFO used to fail on errors only. SYSTEM-001 is lowered to
INFO to have one. */
+ @Test
+ public void failOnInfoFailsOnAnInfoFinding() throws Exception {
+ Files.writeString(
+ project.resolve("hop-lint.yml"), "rules:\n SYSTEM-001:\n severity:
INFO\n");
+
+ StringBuilder output = new StringBuilder();
+ assertEquals(0, hopLint(output, "--fail-on", "WARNING",
project.toString()), output.toString());
+ assertEquals(1, hopLint(output, "--fail-on", "INFO", project.toString()),
output.toString());
+ }
+
+ // ------------------------------------------------------------------ system
findings
+
+ @Test
+ public void aSystemFindingTakesTheProjectsSeverity() throws Exception {
+ Files.writeString(
+ project.resolve("hop-lint.yml"), "rules:\n SYSTEM-001:\n severity:
WARNING\n");
+
+ StringBuilder output = new StringBuilder();
+ int status =
+ hopLint(output, "-f", "JSON", "-o",
dir.resolve("out.json").toString(), project.toString());
+
+ assertEquals(0, status, "a warning does not fail on ERROR: " + output);
+ JsonNode finding = report(dir.resolve("out.json")).get("findings").get(0);
+ assertEquals("SYSTEM-001", finding.get("ruleId").asText(),
finding.toString());
+ assertEquals("WARNING", finding.get("severity").asText(),
finding.toString());
+ assertFalse(output.toString().contains("no installed rule pack defines"),
output.toString());
+ }
+
+ @Test
+ public void aSystemRuleCanBeSwitchedOff() throws Exception {
+ Files.writeString(
+ project.resolve("hop-lint.yml"), "rules:\n SYSTEM-001:\n enabled:
false\n");
+
+ StringBuilder output = new StringBuilder();
+
+ assertEquals(0, hopLint(output, project.toString()), output.toString());
+ }
+
+ @Test
+ public void systemRulesAreListed() {
+ StringBuilder output = new StringBuilder();
+
+ assertEquals(0, hopLint(output, "--list-rules"), output.toString());
+ for (String ruleId : new String[] {"SYSTEM-001", "SYSTEM-002",
"SYSTEM-003"}) {
+ assertTrue(output.toString().contains(ruleId), ruleId + " is not listed:
" + output);
+ }
+ }
+
+ // ------------------------------------------------------------------ the
staged version
+
+ /**
+ * The staged pipeline has no description, the one on disk has. LOCAL-001
asks for one, so only
+ * linting the staged copy blocks the commit.
+ */
+ @Test
+ public void thePreCommitCheckLintsTheStagedCopy() throws Exception {
+ Files.delete(project.resolve("broken.hpl"));
+ Files.writeString(
+ project.resolve("hop-lint.yml"),
+ """
+ rules:
+ LOCAL-001:
+ type: custom
+ enabled: true
+ severity: ERROR
+ target: PIPELINE
+ targetField: description
+ condition: NOT_EMPTY
+ name: Pipeline Description
+ """);
+ Path staged = dir.resolve("staged-copy");
+ writePipeline(staged, "");
+ Path list = dir.resolve("staged.txt");
+
+ Files.writeString(list, project.resolve("customers.hpl") + "\t" + staged +
"\n");
+ StringBuilder output = new StringBuilder();
+ assertEquals(
+ 1, hopLint(output, "--pre-commit", "--staged-file", list.toString()),
output.toString());
+ assertTrue(output.toString().contains("customers.hpl"), "reported as the
file: " + output);
+ assertFalse(output.toString().contains("staged-copy"), output.toString());
+
+ // Without a staged copy the file on disk, with its description, is what
is linted.
+ Files.writeString(list, project.resolve("customers.hpl") + "\n");
+ StringBuilder disk = new StringBuilder();
+ assertEquals(
+ 0, hopLint(disk, "--pre-commit", "--staged-file", list.toString()),
disk.toString());
+ }
+}
diff --git
a/plugins/misc/lint/src/test/java/org/apache/hop/lint/LintCommandTest.java
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/LintCommandTest.java
index 9d0a63c3a1..f2b6e4cdde 100644
--- a/plugins/misc/lint/src/test/java/org/apache/hop/lint/LintCommandTest.java
+++ b/plugins/misc/lint/src/test/java/org/apache/hop/lint/LintCommandTest.java
@@ -32,6 +32,7 @@ import java.nio.file.Files;
import java.nio.file.Path;
import java.util.List;
import java.util.Map;
+import java.util.TreeMap;
import org.apache.hop.core.HopVersionProvider;
import org.apache.hop.core.logging.HopLogStore;
import org.apache.hop.core.logging.LogChannel;
@@ -160,6 +161,7 @@ public class LintCommandTest {
if [ "$1" = "--staged-file" ]; then
cat "$2" > "$CAPTURE"
echo "$2" > "$CAPTURE.path"
+ cut -f2 "$2" | while IFS= read -r COPY; do cat "$COPY"; echo; done
> "$CAPTURE.content"
fi
shift
done
@@ -192,15 +194,113 @@ public class LintCommandTest {
assertEquals(1, status, "a failing lint has to block the commit");
String root = repo.toRealPath().toString();
- List<String> staged = Files.readAllLines(dir.resolve("capture"));
assertEquals(
- List.of(root + "/load/customers.hpl", root +
"/metadata/rdbms/crm.json"),
- staged.stream().map(line ->
Path.of(line).toString()).sorted().toList());
+ List.of(root + "/load/customers.hpl", root +
"/metadata/rdbms/crm.json"), stagedPaths());
String listFile = Files.readString(dir.resolve("capture.path")).trim();
assertFalse(Files.exists(Path.of(listFile)), "the staged list is left
behind: " + listFile);
}
+ /** The files the stand-in launcher was given, without the staged copies. */
+ private List<String> stagedPaths() throws Exception {
+ return Files.readAllLines(dir.resolve("capture")).stream()
+ .map(line -> Path.of(line.split("\t")[0]).toString())
+ .sorted()
+ .toList();
+ }
+
+ /**
+ * The hook linted the files on disk, so a password that was staged and then
removed only on disk
+ * was committed unchecked.
+ *
+ * @see <a href="https://github.com/apache/hop/issues/9071";>#9071</a>
+ */
+ @Test
+ public void theHookLintsTheStagedVersionNotTheOneOnDisk() throws Exception {
+ assumeTrue(canRunShell());
+ Path repo = repositoryWithStagedFiles("load/customers.hpl");
+ Files.writeString(repo.resolve("load/customers.hpl"), "changed on disk
only");
+
+ run(repo, hookEnvironment(), "/bin/sh",
dir.resolve("pre-commit").toString());
+
+ assertEquals("content",
Files.readString(dir.resolve("capture.content")).trim());
+ }
+
+ /** git mv lists a renamed file as R, which --diff-filter=ACM left out of
the check. */
+ @Test
+ public void theHookLintsARenamedFile() throws Exception {
+ assumeTrue(canRunShell());
+ Path repo = repositoryWithStagedFiles("load/customers.hpl");
+ Map<String, String> noHook = Map.of();
+ assertEquals(
+ 0,
+ run(
+ repo,
+ noHook,
+ "git",
+ "-c",
+ "user.name=lint",
+ "-c",
+ "[email protected]",
+ "commit",
+ "-q",
+ "-m",
+ "first"));
+ assertEquals(0, run(repo, noHook, "git", "mv", "load/customers.hpl",
"load/clients.hpl"));
+ Files.writeString(repo.resolve("load/clients.hpl"), "content");
+
+ int status = run(repo, hookEnvironment(), "/bin/sh",
dir.resolve("pre-commit").toString());
+
+ assertEquals(1, status, "the renamed file has to be linted");
+ assertEquals(List.of(repo.toRealPath() + "/load/clients.hpl"),
stagedPaths());
+ }
+
+ /** The lines the stand-in launcher was given, file and staged copy, keyed
by the file name. */
+ private Map<String, String[]> stagedLines() throws Exception {
+ Map<String, String[]> lines = new TreeMap<>();
+ for (String line : Files.readAllLines(dir.resolve("capture"))) {
+ String[] parts = line.split("\t");
+ lines.put(Path.of(parts[0]).getFileName().toString(), parts);
+ }
+ return lines;
+ }
+
+ /**
+ * The staged blob of a symlink is the path it points to, which is not a
pipeline. Such a file is
+ * linted on disk, as it was before the hook read staged versions.
+ */
+ @Test
+ public void aSymlinkIsLintedOnDisk() throws Exception {
+ assumeTrue(canRunShell());
+ Path repo = repositoryWithStagedFiles("p/customers.hpl");
+ Files.createSymbolicLink(repo.resolve("link.hpl"),
Path.of("p/customers.hpl"));
+ assertEquals(0, run(repo, Map.of(), "git", "add", "link.hpl"));
+
+ run(repo, hookEnvironment(), "/bin/sh",
dir.resolve("pre-commit").toString());
+
+ Map<String, String[]> lines = stagedLines();
+ assertEquals(1, lines.get("link.hpl").length, "no staged copy of a
symlink");
+ assertEquals(2, lines.get("customers.hpl").length, "a plain file still
gets one");
+ }
+
+ /**
+ * A file stored with Git LFS has a pointer as its staged blob, and so does
any file behind a
+ * clean filter. The filter here is not configured, so the test does not
depend on git-lfs.
+ */
+ @Test
+ public void aFilteredFileIsLintedOnDisk() throws Exception {
+ assumeTrue(canRunShell());
+ Path repo = repositoryWithStagedFiles("lfs/customers.hpl",
"plain/orders.hpl");
+ Files.writeString(repo.resolve(".gitattributes"), "lfs/*.hpl
filter=example\n");
+ assertEquals(0, run(repo, Map.of(), "git", "add", ".gitattributes"));
+
+ run(repo, hookEnvironment(), "/bin/sh",
dir.resolve("pre-commit").toString());
+
+ Map<String, String[]> lines = stagedLines();
+ assertEquals(1, lines.get("customers.hpl").length, "no staged copy of an
LFS file");
+ assertEquals(2, lines.get("orders.hpl").length);
+ }
+
@Test
public void theHookDoesNotStartHopWithoutHopFiles() throws Exception {
assumeTrue(canRunShell());
@@ -221,12 +321,35 @@ public class LintCommandTest {
Path list = dir.resolve("staged.txt");
Files.writeString(list,
"customers.hpl\nmetadata/rdbms/crm.json\nmissing.hpl\nnotes.txt\n");
- List<File> files = PreCommitLintService.readStagedFiles(list.toString(),
repo.toFile());
+ List<PreCommitLintService.StagedFile> files =
+ PreCommitLintService.readStagedFiles(list.toString(), repo.toFile());
+
+ assertEquals(
+ List.of(
+ new
PreCommitLintService.StagedFile(repo.resolve("customers.hpl").toFile(), null),
+ new PreCommitLintService.StagedFile(
+ repo.resolve("metadata/rdbms/crm.json").toFile(), null)),
+ files);
+ }
+
+ /** A staged file can be gone from disk; its staged copy is what the commit
holds. */
+ @Test
+ public void aStagedCopyIsReadEvenWhenTheFileIsGone() throws Exception {
+ Path repo = dir.resolve("repo");
+ Files.createDirectories(repo);
+ Path copy = dir.resolve("copy-1");
+ Files.writeString(copy, "staged");
+ Path list = dir.resolve("staged.txt");
+ Files.writeString(list, repo.resolve("deleted.hpl") + "\t" + copy + "\n");
+
+ List<PreCommitLintService.StagedFile> files =
+ PreCommitLintService.readStagedFiles(list.toString(), repo.toFile());
assertEquals(
List.of(
- repo.resolve("customers.hpl").toFile(),
- repo.resolve("metadata/rdbms/crm.json").toFile()),
+ new PreCommitLintService.StagedFile(
+ repo.resolve("deleted.hpl").toFile(), copy.toFile())),
files);
+ assertEquals(copy.toFile(), files.get(0).source());
}
}
diff --git
a/plugins/misc/lint/src/test/java/org/apache/hop/lint/LintSeverityThresholdTest.java
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/LintSeverityThresholdTest.java
index 9d5f387b63..46431660b2 100644
---
a/plugins/misc/lint/src/test/java/org/apache/hop/lint/LintSeverityThresholdTest.java
+++
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/LintSeverityThresholdTest.java
@@ -16,7 +16,9 @@
*/
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.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import org.junit.jupiter.api.Test;
@@ -53,6 +55,31 @@ public class LintSeverityThresholdTest {
assertFalse(LintSeverity.meetsFailOnThreshold("WARNING", threshold));
}
+ /**
+ * --fail-on INFO read as ERROR.
+ *
+ * @see <a href="https://github.com/apache/hop/issues/9071";>#9071</a>
+ */
+ @Test
+ public void failOnInfoFailsOnEverything() {
+ LintSeverity.FailOn threshold = LintSeverity.parseFailOn("info");
+
+ assertEquals(LintSeverity.FailOn.INFO, threshold);
+ assertTrue(LintSeverity.meetsFailOnThreshold("ERROR", threshold));
+ assertTrue(LintSeverity.meetsFailOnThreshold("WARNING", threshold));
+ assertTrue(LintSeverity.meetsFailOnThreshold("INFO", threshold));
+ }
+
+ /** A typo read as ERROR, so a project with warnings passed --fail-on
WARNNG. */
+ @Test
+ public void anUnknownThresholdIsRefused() {
+ IllegalArgumentException e =
+ assertThrows(IllegalArgumentException.class, () ->
LintSeverity.parseFailOn("WARNNG"));
+
+ assertTrue(e.getMessage().contains("ERROR, WARNING, INFO, NONE"),
e.getMessage());
+ assertEquals(LintSeverity.FailOn.ERROR, LintSeverity.parseFailOn(""));
+ }
+
/** An unrecognised severity must not be treated as harmless. */
@Test
public void unknownSeveritiesDoNotSlipPastAnErrorThreshold() {
diff --git
a/plugins/misc/lint/src/test/java/org/apache/hop/lint/RuleBuilderValidationTest.java
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/RuleBuilderValidationTest.java
index 8b5f8031f1..8c9728b202 100644
---
a/plugins/misc/lint/src/test/java/org/apache/hop/lint/RuleBuilderValidationTest.java
+++
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/RuleBuilderValidationTest.java
@@ -19,6 +19,7 @@ package org.apache.hop.lint;
import static org.junit.jupiter.api.Assertions.assertEquals;
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.Set;
import org.junit.jupiter.api.Test;
@@ -43,6 +44,128 @@ class RuleBuilderValidationTest {
assertNull(RuleBuilderDialog.clauseProblem(new RuleClause("name",
RuleCondition.NOT_NULL, "")));
}
+ /**
+ * A value its condition cannot use was accepted, and the rule never
reported anything.
+ *
+ * @see <a href="https://github.com/apache/hop/issues/9071";>#9071</a>
+ */
+ @Test
+ void aValueTheConditionCannotUseIsRefused() {
+ for (String value : new String[] {"abc", "${MAXT}", "3,0"}) {
+ assertNotNull(
+ RuleBuilderDialog.clauseProblem(
+ new RuleClause("transformCount", RuleCondition.MAX_VALUE, value),
+ RuleTarget.PIPELINE,
+ false),
+ value);
+ }
+ assertNull(
+ RuleBuilderDialog.clauseProblem(
+ new RuleClause("transformCount", RuleCondition.MAX_VALUE, "3.5"),
+ RuleTarget.PIPELINE,
+ false));
+ assertNotNull(
+ RuleBuilderDialog.clauseProblem(
+ new RuleClause("transforms", RuleCondition.MAX_COLLECTION_SIZE,
"2.5"),
+ RuleTarget.PIPELINE,
+ false));
+ String pattern =
+ RuleBuilderDialog.clauseProblem(
+ new RuleClause("name", RuleCondition.MATCHES_PATTERN, "[a-"),
+ RuleTarget.PIPELINE,
+ false);
+ assertNotNull(pattern);
+ assertTrue(pattern.contains("[a-"), pattern);
+ }
+
+ @Test
+ void aConditionThatDoesNotFitTheFieldIsRefused() {
+ String problem =
+ RuleBuilderDialog.clauseProblem(
+ new RuleClause("transformCount", RuleCondition.NOT_NULL, ""),
+ RuleTarget.PIPELINE,
+ false);
+
+ assertEquals(
+ "NOT_NULL does not apply to transformCount, so the rule can never
report anything",
+ problem);
+ }
+
+ /**
+ * The text conditions read any value as text, so they work on a
connection's port, which is text,
+ * and on a list of target transforms. These were refused although the rules
report.
+ */
+ @Test
+ void aConditionTheExecutorCanFireIsKept() {
+ assertNull(
+ RuleBuilderDialog.clauseProblem(
+ new RuleClause("port", RuleCondition.NO_HARDCODED, ""),
+ RuleTarget.DATABASE_CONNECTION,
+ false));
+ assertNull(
+ RuleBuilderDialog.clauseProblem(
+ new RuleClause("port", RuleCondition.MATCHES_PATTERN, "^\\d+$"),
+ RuleTarget.DATABASE_CONNECTION,
+ false));
+ assertNull(
+ RuleBuilderDialog.clauseProblem(
+ new RuleClause("targetTransforms", RuleCondition.CONTAINS,
"Abort"),
+ RuleTarget.TRANSFORM,
+ false));
+ assertNull(
+ RuleBuilderDialog.clauseProblem(
+ new RuleClause("transformCount", RuleCondition.MATCHES_PATTERN,
"^\\d$"),
+ RuleTarget.PIPELINE,
+ false));
+ }
+
+ /** What the executor can never report: each of these reads as a clean
result. */
+ @Test
+ void aConditionTheExecutorCannotFireIsRefused() {
+ assertNotNull(
+ RuleBuilderDialog.clauseProblem(
+ new RuleClause("transformCount", RuleCondition.NOT_EMPTY, ""),
+ RuleTarget.PIPELINE,
+ false));
+ assertNotNull(
+ RuleBuilderDialog.clauseProblem(
+ new RuleClause("hasNotes", RuleCondition.MAX_VALUE, "1"),
RuleTarget.PIPELINE, false));
+ assertNotNull(
+ RuleBuilderDialog.clauseProblem(
+ new RuleClause("attributes", RuleCondition.MIN_VALUE, "1"),
+ RuleTarget.DATABASE_CONNECTION,
+ false));
+ assertNotNull(
+ RuleBuilderDialog.clauseProblem(
+ new RuleClause("name", RuleCondition.MAX_COLLECTION_SIZE, "3"),
+ RuleTarget.PIPELINE,
+ false));
+ // The secret fields of a transform match every setting by name, whatever
its type.
+ assertNull(
+ RuleBuilderDialog.clauseProblem(
+ new RuleClause("password", RuleCondition.NOT_EMPTY_COLLECTION, ""),
+ RuleTarget.TRANSFORM,
+ false));
+ }
+
+ /**
+ * A rule narrowed with appliesTo can read a plugin's own setting of the
same name, whose type is
+ * not known, so only its value is judged.
+ */
+ @Test
+ void aScopedRuleKeepsAConditionTheEditorWouldNotSuggest() {
+ assertNull(
+ RuleBuilderDialog.clauseProblem(
+ new RuleClause("port", RuleCondition.MATCHES_PATTERN, "^\\d+$"),
+ RuleTarget.TRANSFORM,
+ true));
+ assertNull(
+ RuleBuilderDialog.clauseProblem(
+ new RuleClause("commitSize", RuleCondition.MAX_VALUE, "1000"),
+ RuleTarget.TRANSFORM,
+ false));
+ }
+
@Test
void aRuleIdMustBeUsableAndFree() {
Set<String> taken = Set.of("DOC-001");
diff --git
a/plugins/misc/lint/src/test/java/org/apache/hop/lint/RuleEditorChoicesTest.java
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/RuleEditorChoicesTest.java
index bcb368798c..119bfbafc6 100644
---
a/plugins/misc/lint/src/test/java/org/apache/hop/lint/RuleEditorChoicesTest.java
+++
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/RuleEditorChoicesTest.java
@@ -45,7 +45,7 @@ public class RuleEditorChoicesTest {
public void everyCoreRuleFitsTheEditor() {
List<String> problems = new ArrayList<>();
for (CustomLintRule rule : new HopCoreRulePack().loadRules()) {
- if (rule.isNativeVerify()) {
+ if (rule.isReportOnly()) {
continue;
}
for (RuleClause clause : rule.getClauses()) {
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 00cc602d31..9179cb2a97 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
@@ -79,7 +79,7 @@ public class HopCoreRulePackTest {
rules.stream().filter(CustomLintRule::isNativeVerify).noneMatch(rule
-> rule.isComposed()));
assertTrue(
rules.stream()
- .filter(rule -> !rule.isNativeVerify())
+ .filter(rule -> !rule.isReportOnly())
.allMatch(rule -> rule.getTarget() != null),
"every rule the linter evaluates has something to evaluate it
against");
}
@@ -113,7 +113,7 @@ public class HopCoreRulePackTest {
new HopCoreRulePack()
.loadRules().stream()
.filter(CustomLintRule::isEnabled)
- .filter(rule -> !rule.isNativeVerify())
+ .filter(rule -> !rule.isReportOnly())
.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 673c7cf6ca..ea23324f06 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
@@ -18,13 +18,17 @@ package org.apache.hop.lint.registry;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import java.io.File;
import java.nio.file.Files;
+import java.nio.file.Path;
import java.util.ArrayList;
+import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
+import org.apache.commons.io.FileUtils;
import org.apache.hop.core.logging.HopLogStore;
import org.apache.hop.core.logging.IHopLoggingEventListener;
import org.apache.hop.core.logging.LogLevel;
@@ -442,7 +446,7 @@ public class RuleRegistryTest {
*/
@Test
public void aPackCannotSilentlyReplaceAnotherPacksRule() {
- Map<String, CustomLintRule> merged = new java.util.LinkedHashMap<>();
+ Map<String, CustomLintRule> merged = new LinkedHashMap<>();
RuleRegistry.mergePack(
merged, packOf("hop-core", List.of(), ruleFrom("hop-core", "DB-001",
"ERROR")));
@@ -456,7 +460,7 @@ public class RuleRegistryTest {
/** Declaring the intent is what makes it allowed. */
@Test
public void aDeclaredOverrideIsApplied() {
- Map<String, CustomLintRule> merged = new java.util.LinkedHashMap<>();
+ Map<String, CustomLintRule> merged = new LinkedHashMap<>();
RuleRegistry.mergePack(
merged, packOf("hop-core", List.of(), ruleFrom("hop-core", "DB-001",
"ERROR")));
@@ -471,7 +475,7 @@ public class RuleRegistryTest {
/** A pack redefining its own rule is not a collision. */
@Test
public void aPackMayRedefineItsOwnRule() {
- Map<String, CustomLintRule> merged = new java.util.LinkedHashMap<>();
+ Map<String, CustomLintRule> merged = new LinkedHashMap<>();
RuleRegistry.mergePack(
merged, packOf("acme", List.of(), ruleFrom("acme", "ACME-001",
"WARNING")));
RuleRegistry.mergePack(
@@ -483,7 +487,7 @@ public class RuleRegistryTest {
/** Rule ids are matched without regard to case, as they are hand-written in
YAML. */
@Test
public void overrideDeclarationIgnoresCase() {
- Map<String, CustomLintRule> merged = new java.util.LinkedHashMap<>();
+ Map<String, CustomLintRule> merged = new LinkedHashMap<>();
RuleRegistry.mergePack(
merged, packOf("hop-core", List.of(), ruleFrom("hop-core", "DB-001",
"ERROR")));
@@ -691,6 +695,121 @@ public class RuleRegistryTest {
assertTrue(new RuleRegistry(discovery).getPackErrors().isEmpty());
}
+ /**
+ * A value the condition cannot use, from hop-lint.yml or a pack, made the
rule report nothing and
+ * log an exception on every run.
+ *
+ * @see <a href="https://github.com/apache/hop/issues/9071";>#9071</a>
+ */
+ @Test
+ public void aRuleThatCannotReportIsNamedInAWarning() throws Exception {
+ File projectYaml = File.createTempFile("hop-lint", ".yml");
+ Files.writeString(
+ projectYaml.toPath(),
+ """
+ rules:
+ STRUCT-001:
+ enabled: true
+ conditionValue: abc
+ LOCAL-001:
+ type: custom
+ enabled: true
+ target: PIPELINE
+ targetField: name
+ condition: MATCHES_PATTERN
+ conditionValue: "[a-"
+ LOCAL-002:
+ type: custom
+ enabled: true
+ target: PIPELINE
+ targetField: transformCount
+ condition: NOT_NULL
+ LOCAL-003:
+ type: custom
+ enabled: false
+ target: PIPELINE
+ targetField: transformCount
+ condition: MAX_VALUE
+ conditionValue: "${MAXT}"
+ """);
+ try {
+ List<String> warnings =
RuleRegistry.getInstance().resolve(projectYaml).getWarnings();
+
+ assertEquals(3, warnings.size(), warnings.toString());
+ assertTrue(
+ warnings.stream().anyMatch(w -> w.contains("'STRUCT-001'") &&
w.contains("'abc'")),
+ warnings.toString());
+ assertTrue(
+ warnings.stream().anyMatch(w -> w.contains("'LOCAL-001'") &&
w.contains("[a-")),
+ warnings.toString());
+ assertTrue(
+ warnings.stream().anyMatch(w -> w.contains("'LOCAL-002'") &&
w.contains("NOT_NULL")),
+ warnings.toString());
+ } finally {
+ Files.deleteIfExists(projectYaml.toPath());
+ }
+ }
+
+ @Test
+ public void theCorePackHasNoRuleThatCannotReport() {
+ assertTrue(
+ RuleRegistry.getInstance().resolve(null).getWarnings().isEmpty(),
+ RuleRegistry.getInstance().resolve(null).getWarnings().toString());
+ }
+
+ /** rules: written with dashes failed with "ArrayList cannot be cast to
class java.util.Map". */
+ @Test
+ public void rulesWrittenAsAListAreExplained() throws Exception {
+ File projectYaml = File.createTempFile("hop-lint", ".yml");
+ Files.writeString(projectYaml.toPath(), "rules:\n - DB-001:\n
enabled: false\n");
+ try {
+ LintConfigurationException e =
+ assertThrows(
+ LintConfigurationException.class,
+ () -> RuleRegistry.getInstance().resolve(projectYaml));
+ assertTrue(e.getMessage().contains("maps each rule id to its settings"),
e.getMessage());
+ assertFalse(e.getMessage().contains("ArrayList"), e.getMessage());
+ } finally {
+ Files.deleteIfExists(projectYaml.toPath());
+ }
+ }
+
+ /**
+ * A broken hop-lint-pack.yml in a plugin folder stopped hop lint with only
snakeyaml's "while
+ * parsing a flow sequence in 'reader'", naming neither the pack nor the
file.
+ */
+ @Test
+ public void aBrokenDiscoveredPackNamesItsFile() throws Exception {
+ Path root = Files.createTempDirectory("hop-lint-discovery");
+ Path engineDir = root.resolve("plugins/misc/hop-lint");
+ Path vendorDir = root.resolve("plugins/misc/acme-lint-rules");
+ Files.createDirectories(engineDir);
+ Files.createDirectories(vendorDir);
+ Path packFile = vendorDir.resolve("hop-lint-pack.yml");
+ Files.writeString(packFile, "pack:\n id: acme\nrules:\n ACME-001:
[unterminated\n");
+ try {
+ Map<String, IHopLintRulePack> packsById = new LinkedHashMap<>();
+ new RulePackDiscovery()
+ .registerInstalledPluginYamlPacksFromEngineDir(engineDir.toFile(),
packsById);
+ List<IHopLintRulePack> packs = new ArrayList<>(packsById.values());
+ RulePackDiscovery discovery =
+ new RulePackDiscovery() {
+ @Override
+ public List<IHopLintRulePack> discoverAll() {
+ return packs;
+ }
+ };
+
+ List<String> errors = new RuleRegistry(discovery).getPackErrors();
+
+ assertEquals(1, errors.size(), errors.toString());
+ assertTrue(errors.get(0).contains(packFile.toString()), errors.get(0));
+ assertTrue(errors.get(0).contains("line"), errors.get(0));
+ } finally {
+ FileUtils.deleteDirectory(root.toFile());
+ }
+ }
+
/** A cause chain that loops back on itself must not hang the registry while
it holds its lock. */
@Test
public void aLoopingCauseChainEnds() {
