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 85a6b1328a Fixes #8871 : Lint rule editor and Rules Manager: clause
validation, … (#8877)
85a6b1328a is described below
commit 85a6b1328aab0a0639ab2a7caa94be749fbd10b5
Author: Bart Maertens <[email protected]>
AuthorDate: Tue Oct 13 13:29:01 2026 +0200
Fixes #8871 : Lint rule editor and Rules Manager: clause validation, …
(#8877)
* Fixes #8871 : Lint rule editor and Rules Manager: clause validation,
saved values, Rule ID, pack rule overrides, sorting
* Issue #8871 : Address review comments: later clause values, edits kept on
a target round trip, validation tests, comments
---
.../ROOT/pages/linting/lint-rule-packs.adoc | 8 +-
.../modules/ROOT/pages/linting/lint-rules.adoc | 11 +
.../org/apache/hop/lint/LintPolicyYamlWriter.java | 19 +-
.../org/apache/hop/lint/LinterConfigPlugin.java | 5 +-
.../org/apache/hop/lint/RuleBuilderDialog.java | 268 ++++++++++++++---
.../org/apache/hop/lint/RuleManagerDialog.java | 272 ++++++++++++++----
.../hop/lint/registry/ProjectLintYamlExporter.java | 78 ++++-
.../hop/lint/registry/ProjectYamlOverlay.java | 26 +-
.../hop/lint/registry/YamlRulePackParser.java | 10 +-
.../hop/lint/messages/messages_en_US.properties | 18 ++
.../apache/hop/lint/LintRuleYamlWriterTest.java | 36 +++
.../hop/lint/RuleBuilderDialogClauseTest.java | 316 +++++++++++++++++++++
.../apache/hop/lint/RuleBuilderValidationTest.java | 60 ++++
.../apache/hop/lint/registry/RuleRegistryTest.java | 124 ++++++++
14 files changed, 1146 insertions(+), 105 deletions(-)
diff --git
a/docs/hop-user-manual/modules/ROOT/pages/linting/lint-rule-packs.adoc
b/docs/hop-user-manual/modules/ROOT/pages/linting/lint-rule-packs.adoc
index 7096b430eb..cd339cd420 100644
--- a/docs/hop-user-manual/modules/ROOT/pages/linting/lint-rule-packs.adoc
+++ b/docs/hop-user-manual/modules/ROOT/pages/linting/lint-rule-packs.adoc
@@ -121,5 +121,11 @@ Rule ids are matched ignoring case.
An id that no installed pack defines, such as a typo, is reported as a warning
in the Hop log and by `hop lint`, with the closest id as a suggestion, and the
override is ignored.
The rule manager in Hop Gui writes these overrides for you.
-Editing a pack rule there never changes the pack: a change to its severity,
threshold, enabled state, tags or help URL is written to `hop-lint.yml` as an
override, and a change to what the rule actually looks at is written there as a
complete rule which replaces the pack rule of that id.
+Editing a pack rule there never changes the pack: a change to its name,
description, severity, threshold, enabled state, tags or help URL is written to
`hop-lint.yml` as an override, and the rule goes on following the pack.
+
+A change to what the rule actually looks at, its target, a field or a
condition, is written to `hop-lint.yml` as a complete rule which replaces the
pack rule of that id.
+So is a changed value in any clause of a composed rule but the first: an
override carries one `conditionValue`, the first clause's.
+From then on, changes to that rule in a newer version of the pack no longer
apply, so the rule manager asks before it saves such a change.
+The rule is then listed as a project rule, and *Restore Pack Rule* removes it
from `hop-lint.yml` and brings the pack's rule back.
+
Pack rules cannot be deleted, only switched off.
diff --git a/docs/hop-user-manual/modules/ROOT/pages/linting/lint-rules.adoc
b/docs/hop-user-manual/modules/ROOT/pages/linting/lint-rules.adoc
index 0b3eb71a04..ea016d7c01 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
@@ -497,6 +497,11 @@ rules:
enabled: true
conditionValue: "30"
+ # Use your own wording for a pack rule
+ DOC-002:
+ name: "Workflows need a description"
+ description: "Say what the workflow does and who to ask about it"
+
# A rule that exists only in this project
LOCAL-001:
type: custom
@@ -511,6 +516,12 @@ rules:
*Tools -> Lint -> Manage Custom Rules* writes this file for you.
It changes only the entry of the rule you added, edited, switched or deleted:
the other rules, the `exclude` and `suppress` sections and your comments stay
as you wrote them.
+A rule you add there gets its id in the *Rule ID* field, suggested as
`PROJECT-001`, `PROJECT-002` and so on.
+Findings, SARIF reports, `hop lint --list-rules` and `hop-lint.yml` all name
the rule by this id, so pick one that says what the rule is about.
+An id holds letters, digits, hyphens, underscores and dots, and no two rules
can share one, whatever the case.
+A project rule's id can be changed later; a `suppress` entry naming the old id
does not follow it.
+A pack rule keeps the pack's id.
+
A parse error in `hop-lint.yml` is reported rather than ignored, so that a
typo cannot leave you linting with the defaults while believing otherwise.
== Excluding files, and accepting findings
diff --git
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintPolicyYamlWriter.java
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintPolicyYamlWriter.java
index d5bbd45925..c961e80591 100644
---
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintPolicyYamlWriter.java
+++
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintPolicyYamlWriter.java
@@ -233,12 +233,29 @@ public final class LintPolicyYamlWriter {
*/
public static void putRule(Path yamlFile, String ruleId, Map<String, Object>
entry)
throws IOException {
+ putRule(yamlFile, ruleId, entry, null);
+ }
+
+ /**
+ * Write a rule under a new id and remove the entry under its old one, in a
single write: a
+ * refused write leaves the file as it was.
+ *
+ * @param previousId the id the rule was saved under before; null or the
same id changes nothing
+ * else
+ */
+ public static void putRule(
+ Path yamlFile, String ruleId, Map<String, Object> entry, String
previousId)
+ throws IOException {
if (isBlank(ruleId)) {
throw new IOException("A rule needs an id");
}
String original =
Files.exists(yamlFile) ? Files.readString(yamlFile,
StandardCharsets.UTF_8) : "";
- String updated = replaceRule(original, ruleId, entry);
+ String updated = original;
+ if (!isBlank(previousId) && !previousId.equalsIgnoreCase(ruleId)) {
+ updated = replaceRule(updated, previousId, null);
+ }
+ updated = replaceRule(updated, ruleId, entry);
if (updated.equals(original)) {
return;
}
diff --git
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/LinterConfigPlugin.java
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/LinterConfigPlugin.java
index b72ac48c6c..71cc6aca36 100644
---
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/LinterConfigPlugin.java
+++
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/LinterConfigPlugin.java
@@ -604,10 +604,7 @@ public class LinterConfigPlugin implements IConfigOptions,
IGuiPluginCompositeWi
public void saveProjectRule(CustomLintRule rule, String previousId) throws
IOException {
Path path = Paths.get(resolveProjectConfigPath());
String ruleId = rule.generateRuleId();
- if (!Utils.isEmpty(previousId) && !previousId.equalsIgnoreCase(ruleId)) {
- LintPolicyYamlWriter.removeRule(path, previousId);
- }
- LintPolicyYamlWriter.putRule(path, ruleId,
ProjectLintYamlExporter.entryFor(rule));
+ LintPolicyYamlWriter.putRule(path, ruleId,
ProjectLintYamlExporter.entryFor(rule), previousId);
log.logBasic("Saved lint rule " + ruleId + " to " + path);
}
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 2ba1d817c9..7a01748541 100644
--- a/plugins/misc/lint/src/main/java/org/apache/hop/lint/RuleBuilderDialog.java
+++ b/plugins/misc/lint/src/main/java/org/apache/hop/lint/RuleBuilderDialog.java
@@ -17,12 +17,20 @@
package org.apache.hop.lint;
import java.util.ArrayList;
+import java.util.Collection;
+import java.util.IdentityHashMap;
import java.util.LinkedHashMap;
import java.util.List;
+import java.util.Locale;
import java.util.Map;
+import java.util.Set;
+import java.util.TreeSet;
+import java.util.function.Consumer;
+import java.util.regex.Pattern;
import org.apache.hop.core.util.Utils;
import org.apache.hop.core.variables.Variables;
import org.apache.hop.i18n.BaseMessages;
+import org.apache.hop.lint.registry.ProjectLintYamlExporter;
import org.apache.hop.ui.core.PropsUi;
import org.apache.hop.ui.core.widget.ColumnInfo;
import org.apache.hop.ui.core.widget.TableView;
@@ -51,12 +59,16 @@ public class RuleBuilderDialog extends Dialog {
private static final Class<?> PKG = RuleBuilderDialog.class; // for i18n
purposes
+ /** A rule id is a YAML key, a SARIF rule id and a word on the command line.
*/
+ private static final Pattern RULE_ID =
Pattern.compile("[A-Za-z0-9][A-Za-z0-9_.-]*");
+
private Shell shell;
private Shell parent;
private CustomLintRule rule;
private boolean ok = false;
// UI components
+ private Text idText;
private Text nameText;
private Text descriptionText;
private Combo targetCombo;
@@ -84,11 +96,46 @@ public class RuleBuilderDialog extends Dialog {
/** Guards the widget listeners while the widgets are being loaded from a
clause. */
private boolean loadingClause = false;
+ /**
+ * Where validation errors go instead of a message box. A native message box
cannot be driven from
+ * a UI test, so the tests collect the messages here.
+ */
+ Consumer<String> errorReporter;
+
+ /** The target shown, so choosing it again changes nothing. */
+ private int shownTarget = -1;
+
+ /**
+ * Clauses a target change emptied, as they were just before, by the clause
they were taken from.
+ * Going back to that target puts a clause back while it is still empty.
+ */
+ private final Map<RuleClause, EmptiedClause> emptiedClauses = new
IdentityHashMap<>();
+
+ private record EmptiedClause(RuleTarget target, RuleClause clause) {}
+
+ /** The ids of the other rules, upper case, which this rule's id must not
clash with. */
+ private final Set<String> takenIds = new TreeSet<>();
+
public RuleBuilderDialog(Shell parent, CustomLintRule rule) {
super(parent, SWT.DIALOG_TRIM | SWT.RESIZE | SWT.APPLICATION_MODAL);
this.parent = parent;
this.rule = rule != null ? rule : new CustomLintRule();
- setText(rule != null ? "Edit Lint Rule" : "Add Lint Rule");
+ setText(
+ BaseMessages.getString(
+ PKG, rule != null ? "RuleBuilderDialog.Title.Edit" :
"RuleBuilderDialog.Title.Add"));
+ }
+
+ /**
+ * @param takenIds the ids of every other rule, compared ignoring case; the
rule's own id may be
+ * among them
+ */
+ public void setTakenIds(Collection<String> takenIds) {
+ this.takenIds.clear();
+ for (String id : takenIds) {
+ if (id != null && !id.equalsIgnoreCase(rule.getId())) {
+ this.takenIds.add(id.toUpperCase(Locale.ROOT));
+ }
+ }
}
public CustomLintRule open() {
@@ -134,20 +181,37 @@ public class RuleBuilderDialog extends Dialog {
int margin = 10;
int labelWidth = 120;
+ // Rule ID: what findings, SARIF, --list-rules and hop-lint.yml call the
rule.
+ Label idLabel = new Label(shell, SWT.RIGHT);
+ idLabel.setText(BaseMessages.getString(PKG,
"RuleBuilderDialog.Label.RuleId"));
+ FormData idLabelData = new FormData();
+ idLabelData.left = new FormAttachment(0, margin);
+ idLabelData.right = new FormAttachment(0, labelWidth);
+ idLabelData.top = new FormAttachment(0, margin);
+ idLabel.setLayoutData(idLabelData);
+
+ idText = new Text(shell, SWT.BORDER);
+ idText.setToolTipText(BaseMessages.getString(PKG,
"RuleBuilderDialog.RuleId.Tooltip"));
+ FormData idData = new FormData();
+ idData.left = new FormAttachment(idLabel, margin);
+ idData.right = new FormAttachment(100, -margin);
+ idData.top = new FormAttachment(0, margin);
+ idText.setLayoutData(idData);
+
// Rule Name
Label nameLabel = new Label(shell, SWT.RIGHT);
nameLabel.setText(BaseMessages.getString(PKG,
"RuleBuilderDialog.Label.RuleName"));
FormData nameLabelData = new FormData();
nameLabelData.left = new FormAttachment(0, margin);
nameLabelData.right = new FormAttachment(0, labelWidth);
- nameLabelData.top = new FormAttachment(0, margin);
+ nameLabelData.top = new FormAttachment(idText, margin);
nameLabel.setLayoutData(nameLabelData);
nameText = new Text(shell, SWT.BORDER);
FormData nameData = new FormData();
nameData.left = new FormAttachment(nameLabel, margin);
nameData.right = new FormAttachment(100, -margin);
- nameData.top = new FormAttachment(0, margin);
+ nameData.top = new FormAttachment(idText, margin);
nameText.setLayoutData(nameData);
// Description
@@ -184,7 +248,7 @@ public class RuleBuilderDialog extends Dialog {
new SelectionAdapter() {
@Override
public void widgetSelected(SelectionEvent e) {
- updateFieldCombo(null);
+ targetChanged();
}
});
FormData targetData = new FormData();
@@ -285,8 +349,7 @@ public class RuleBuilderDialog extends Dialog {
new SelectionAdapter() {
@Override
public void widgetSelected(SelectionEvent e) {
- updateConditionCombo(null);
- captureWidgetsIntoSelectedClause();
+ fieldChanged();
}
});
FormData fieldData = new FormData();
@@ -309,8 +372,7 @@ public class RuleBuilderDialog extends Dialog {
new SelectionAdapter() {
@Override
public void widgetSelected(SelectionEvent e) {
- updateValueField();
- captureWidgetsIntoSelectedClause();
+ conditionChanged();
}
});
FormData conditionData = new FormData();
@@ -455,6 +517,11 @@ public class RuleBuilderDialog extends Dialog {
}
private void populateControls() {
+ idText.setText(rule.getId() != null ? rule.getId() : "");
+ // A pack rule keeps the pack's id: hop-lint.yml names it to change it. So
does a project rule
+ // which redefines a pack rule, since another id would bring the pack rule
back beside it.
+ idText.setEditable(
+ rule.isProjectEditable() &&
ProjectLintYamlExporter.packRule(rule.getId()) == null);
if (rule.getName() != null) {
nameText.setText(rule.getName());
}
@@ -463,6 +530,7 @@ public class RuleBuilderDialog extends Dialog {
}
if (rule.getTarget() != null) {
targetCombo.select(rule.getTarget().ordinal());
+ shownTarget = rule.getTarget().ordinal();
updateFieldCombo(null);
}
// Selected by index: setText does nothing on a read-only combo, which
left WARNING showing for
@@ -542,15 +610,97 @@ public class RuleBuilderDialog extends Dialog {
private void updateValueField() {
RuleCondition condition = selectedCondition();
- if (condition != null) {
- valueLabel.setVisible(condition.requiresValue());
- valueText.setVisible(condition.requiresValue());
+ boolean usesValue = condition == null || condition.requiresValue();
+ valueLabel.setVisible(usesValue);
+ valueText.setVisible(usesValue);
+ if (condition != null && condition.requiresValue()) {
+ valueText.setToolTipText(condition.getDescription());
+ }
+ shell.layout(true, true);
+ }
- if (condition.requiresValue()) {
- valueText.setToolTipText(condition.getDescription());
+ /**
+ * A field the new target also has stays, with its condition and value; a
field it does not have
+ * empties its clause. Going back to the target a clause was emptied on puts
it back as it was
+ * then, edits included, as long as it is still empty. That covers a field
the list does not know,
+ * such as a Table Input's {@code sql}.
+ */
+ private void targetChanged() {
+ int targetIndex = targetCombo.getSelectionIndex();
+ if (targetIndex < 0 || targetIndex == shownTarget) {
+ return;
+ }
+ RuleTarget previousTarget = shownTarget < 0 ? null :
RuleTarget.values()[shownTarget];
+ shownTarget = targetIndex;
+ RuleTarget target = RuleTarget.values()[targetIndex];
+ List<String> fields = RuleTargetFields.getFieldsForTarget(target);
+ for (RuleClause clause : editingClauses) {
+ if (!Utils.isEmpty(clause.getTargetField()) &&
!fields.contains(clause.getTargetField())) {
+ emptiedClauses.put(clause, new EmptiedClause(previousTarget,
clause.copy()));
+ clause.setTargetField("");
+ clause.setCondition(null);
+ clause.setConditionValue("");
+ } else if (Utils.isEmpty(clause.getTargetField())) {
+ EmptiedClause emptied = emptiedClauses.get(clause);
+ if (emptied != null && emptied.target() == target) {
+ clause.setTargetField(emptied.clause().getTargetField());
+ clause.setCondition(emptied.clause().getCondition());
+ clause.setConditionValue(emptied.clause().getConditionValue());
+ emptiedClauses.remove(clause);
+ }
}
}
- shell.layout(true, true);
+ refreshClauseTable();
+ loadSelectedClauseIntoWidgets();
+ }
+
+ /**
+ * Choosing the field the clause already reads changes nothing. Another
field keeps the condition
+ * when that field offers it too.
+ */
+ private void fieldChanged() {
+ RuleClause clause = selectedClause();
+ if (clause == null) {
+ return;
+ }
+ String field = fieldCombo.getText();
+ if (field.equals(clause.getTargetField())) {
+ return;
+ }
+ RuleCondition condition = clause.getCondition();
+ if (condition != null &&
!RuleTargetFields.getCompatibleConditions(field).contains(condition)) {
+ condition = null;
+ }
+ clause.setTargetField(field);
+ clause.setCondition(condition);
+ if (condition == null || !condition.requiresValue()) {
+ clause.setConditionValue("");
+ }
+ refreshClauseTable();
+ loadSelectedClauseIntoWidgets();
+ }
+
+ /**
+ * A condition without a value clears the value: the value field is hidden
then, and a value
+ * nobody can see must not be saved.
+ */
+ private void conditionChanged() {
+ RuleCondition condition = selectedCondition();
+ if (condition != null && !condition.requiresValue()) {
+ loadingClause = true;
+ try {
+ valueText.setText("");
+ } finally {
+ loadingClause = false;
+ }
+ }
+ updateValueField();
+ captureWidgetsIntoSelectedClause();
+ }
+
+ private RuleClause selectedClause() {
+ int index = clauseTable.getSelectionIndex();
+ return index < 0 || index >= editingClauses.size() ? null :
editingClauses.get(index);
}
private void ok() {
@@ -562,8 +712,15 @@ public class RuleBuilderDialog extends Dialog {
}
private boolean validate() {
+ if (idText.getEditable()) {
+ String problem = ruleIdProblem(idText.getText().trim(), takenIds);
+ if (problem != null) {
+ showError(problem);
+ return false;
+ }
+ }
if (Utils.isEmpty(nameText.getText())) {
- showError("Rule name is required");
+ showError(BaseMessages.getString(PKG,
"RuleBuilderDialog.Error.NameRequired"));
return false;
}
// A row with only a key or only a value is not a tag. Saving would drop
it without a word.
@@ -577,29 +734,64 @@ public class RuleBuilderDialog extends Dialog {
return true;
}
if (targetCombo.getSelectionIndex() < 0) {
- showError("Target type must be selected");
- return false;
- }
- if (fieldCombo.getSelectionIndex() < 0) {
- showError("Field must be selected");
+ showError(BaseMessages.getString(PKG,
"RuleBuilderDialog.Error.TargetRequired"));
return false;
}
- if (conditionCombo.getSelectionIndex() < 0) {
- showError("Condition must be selected");
- return false;
+
+ // 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));
+ if (problem != null) {
+ clauseTable.setSelection(i);
+ loadSelectedClauseIntoWidgets();
+ showError(
+ editingClauses.size() == 1
+ ? problem
+ : BaseMessages.getString(
+ PKG, "RuleBuilderDialog.Error.Clause", String.valueOf(i +
1), problem));
+ return false;
+ }
}
+ return true;
+ }
- // Check if condition requires a value
- RuleCondition condition = selectedCondition();
- if (condition != null && condition.requiresValue() &&
Utils.isEmpty(valueText.getText())) {
- showError("Value is required for this condition");
- return false;
+ /**
+ * What is wrong with a rule id, or null when it can be used.
+ *
+ * @param takenIds the other rules' ids, upper case
+ */
+ static String ruleIdProblem(String id, Set<String> takenIds) {
+ if (id.isEmpty()) {
+ return BaseMessages.getString(PKG,
"RuleBuilderDialog.Error.RuleIdRequired");
+ }
+ if (!RULE_ID.matcher(id).matches()) {
+ return BaseMessages.getString(PKG,
"RuleBuilderDialog.Error.RuleIdInvalid", id);
+ }
+ if (takenIds.contains(id.toUpperCase(Locale.ROOT))) {
+ return BaseMessages.getString(PKG,
"RuleBuilderDialog.Error.RuleIdTaken", id);
}
+ return null;
+ }
- return true;
+ /** What is missing from a clause, or null when it is complete. */
+ static String clauseProblem(RuleClause clause) {
+ if (Utils.isEmpty(clause.getTargetField())) {
+ return BaseMessages.getString(PKG,
"RuleBuilderDialog.Error.FieldRequired");
+ }
+ if (clause.getCondition() == null) {
+ return BaseMessages.getString(PKG,
"RuleBuilderDialog.Error.ConditionRequired");
+ }
+ if (clause.getCondition().requiresValue() &&
Utils.isEmpty(clause.getConditionValue())) {
+ return BaseMessages.getString(PKG,
"RuleBuilderDialog.Error.ValueRequired");
+ }
+ return null;
}
private void saveRule() {
+ if (idText.getEditable()) {
+ rule.setId(idText.getText().trim());
+ }
rule.setName(nameText.getText());
rule.setDescription(descriptionText.getText());
rule.setSeverity(severityCombo.getText());
@@ -612,6 +804,12 @@ public class RuleBuilderDialog extends Dialog {
}
captureWidgetsIntoSelectedClause();
+ for (RuleClause clause : editingClauses) {
+ // No value where the condition takes none, and none rather than an
empty one.
+ if (!clause.getCondition().requiresValue() ||
Utils.isEmpty(clause.getConditionValue())) {
+ clause.setConditionValue(null);
+ }
+ }
rule.setTarget(RuleTarget.values()[targetCombo.getSelectionIndex()]);
rule.setCombinator(
combinatorCombo.getSelectionIndex() == 1 ? RuleCombinator.ANY_OF :
RuleCombinator.ALL_OF);
@@ -735,11 +933,11 @@ public class RuleBuilderDialog extends Dialog {
}
RuleClause clause = editingClauses.get(index);
clause.setTargetField(fieldCombo.getText());
+ // Null too, so the table never shows a condition the combo no longer has.
RuleCondition condition = selectedCondition();
- if (condition != null) {
- clause.setCondition(condition);
- }
- clause.setConditionValue(valueText.getText());
+ clause.setCondition(condition);
+ clause.setConditionValue(
+ condition == null || condition.requiresValue() ? valueText.getText() :
"");
refreshClauseTable();
}
@@ -767,6 +965,10 @@ public class RuleBuilderDialog extends Dialog {
}
private void showError(String message) {
+ if (errorReporter != null) {
+ errorReporter.accept(message);
+ return;
+ }
MessageBox messageBox = new MessageBox(shell, SWT.ICON_ERROR | SWT.OK);
messageBox.setText(
BaseMessages.getString(PKG,
"RuleBuilderDialog.Dialog.ValidationError.Title"));
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 fff060060e..56cea11c44 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
@@ -16,10 +16,16 @@
*/
package org.apache.hop.lint;
+import java.util.ArrayList;
+import java.util.Comparator;
import java.util.List;
+import java.util.Locale;
+import java.util.Set;
+import java.util.TreeSet;
import org.apache.hop.core.logging.ILogChannel;
import org.apache.hop.core.logging.LogChannel;
import org.apache.hop.i18n.BaseMessages;
+import org.apache.hop.lint.registry.ProjectLintYamlExporter;
import org.apache.hop.lint.registry.RulePackIds;
import org.apache.hop.lint.registry.RulePackOwner;
import org.eclipse.swt.SWT;
@@ -57,6 +63,14 @@ public class RuleManagerDialog extends Dialog {
private Button deleteButton;
private Button toggleButton;
+ /** The column the table is sorted on, or -1 for the order the rules were
loaded in. */
+ private int sortColumn = -1;
+
+ private boolean sortAscending = true;
+
+ /** The ids the packs define, looked up once rather than on every selection.
*/
+ private Set<String> packRuleIds;
+
public RuleManagerDialog(Shell parent, List<CustomLintRule> rules) {
super(parent, SWT.DIALOG_TRIM | SWT.RESIZE | SWT.APPLICATION_MODAL);
this.parent = parent;
@@ -175,6 +189,12 @@ public class RuleManagerDialog extends Dialog {
tagsColumn.setText(BaseMessages.getString(PKG,
"RuleManagerDialog.Column.Tags"));
tagsColumn.setWidth(200);
+ TableColumn[] columns = rulesTable.getColumns();
+ for (int i = 0; i < columns.length; i++) {
+ int index = i;
+ columns[i].addListener(SWT.Selection, e -> sortOn(index));
+ }
+
FormData tableData = new FormData();
tableData.left = new FormAttachment(0, margin);
tableData.right = new FormAttachment(100, -(BUTTON_PANEL_WIDTH + 2 *
margin));
@@ -283,34 +303,68 @@ public class RuleManagerDialog extends Dialog {
}
private void populateTable() {
+ CustomLintRule selected = rulesTable.getSelectionIndex() < 0 ? null :
getSelectedRule();
rulesTable.removeAll();
- for (CustomLintRule rule : rules) {
+ List<CustomLintRule> shown = new ArrayList<>(rules);
+ if (sortColumn >= 0) {
+ Comparator<CustomLintRule> order =
+ Comparator.comparing(
+ rule -> rowText(rule)[sortColumn].toLowerCase(Locale.ROOT),
+ Comparator.naturalOrder());
+ shown.sort(sortAscending ? order : order.reversed());
+ }
+ for (CustomLintRule rule : shown) {
TableItem item = new TableItem(rulesTable, SWT.NONE);
- item.setText(0, rule.getTarget() != null ? rule.generateRuleId() :
nullToEmpty(rule.getId()));
- item.setText(1, rule.getName() != null ? rule.getName() : "");
- item.setText(2, rule.getPackOwner() != null ?
rule.getPackOwner().getDisplayName() : "");
- item.setText(3, rule.getTarget() != null ?
rule.getTarget().getDisplayName() : "");
- item.setText(4, rule.getTargetField() != null ? rule.getTargetField() :
"");
- // A composed rule checks several things, and showing only its first
clause made it
- // indistinguishable from a rule that checks one. Say so in the columns
that would otherwise
- // be a half-truth.
- if (rule.isComposed()) {
- item.setText(
- 4, rule.getClauses().size() + " fields (" +
rule.getCombinator().getYamlKey() + ")");
- // A condition says what is required; the rule reports when the
conditions are not met.
- item.setText(
- 5, rule.getCombinator() == RuleCombinator.ALL_OF ? "All not met" :
"Any not met");
- item.setText(6, "");
- } else {
- item.setText(5, rule.getCondition() != null ?
rule.getCondition().getDisplayName() : "");
- item.setText(6, rule.getConditionValue() != null ?
rule.getConditionValue() : "");
- }
- item.setText(7, rule.getSeverity() != null ? rule.getSeverity() :
"WARNING");
- item.setText(8, rule.isEnabled() ? "✓" : "✗");
- item.setText(9, String.join(", ",
LintRuleDetails.flatTags(rule.getTags())));
+ item.setText(rowText(rule));
item.setData(rule);
}
+ if (selected != null) {
+ select(selected);
+ }
+ }
+
+ private static String[] rowText(CustomLintRule rule) {
+ String[] row = new String[10];
+ row[0] = rule.getTarget() != null ? rule.generateRuleId() :
nullToEmpty(rule.getId());
+ row[1] = nullToEmpty(rule.getName());
+ row[2] = rule.getPackOwner() != null ?
rule.getPackOwner().getDisplayName() : "";
+ row[3] = rule.getTarget() != null ? rule.getTarget().getDisplayName() : "";
+ // A composed rule checks several things, and showing only its first
clause made it
+ // indistinguishable from a rule that checks one. Say so in the columns
that would otherwise be
+ // a half-truth.
+ if (rule.isComposed()) {
+ row[4] = rule.getClauses().size() + " fields (" +
rule.getCombinator().getYamlKey() + ")";
+ // A condition says what is required; the rule reports when the
conditions are not met.
+ row[5] = rule.getCombinator() == RuleCombinator.ALL_OF ? "All not met" :
"Any not met";
+ row[6] = "";
+ } else {
+ row[4] = nullToEmpty(rule.getTargetField());
+ row[5] = rule.getCondition() != null ?
rule.getCondition().getDisplayName() : "";
+ row[6] = nullToEmpty(rule.getConditionValue());
+ }
+ row[7] = rule.getSeverity() != null ? rule.getSeverity() : "WARNING";
+ row[8] = rule.isEnabled() ? "✓" : "✗";
+ row[9] = String.join(", ", LintRuleDetails.flatTags(rule.getTags()));
+ return row;
+ }
+
+ /** Sort on a column, or turn the order round when it is already the one
sorted on. */
+ private void sortOn(int column) {
+ sortAscending = column != sortColumn || !sortAscending;
+ sortColumn = column;
+ rulesTable.setSortColumn(rulesTable.getColumn(column));
+ rulesTable.setSortDirection(sortAscending ? SWT.UP : SWT.DOWN);
+ populateTable();
+ }
+
+ private void select(CustomLintRule rule) {
+ int index = findTableIndex(rule);
+ if (index >= 0) {
+ rulesTable.setSelection(index);
+ rulesTable.showSelection();
+ }
+ updateButtonState();
}
private CustomLintRule getSelectedRule() {
@@ -331,20 +385,79 @@ public class RuleManagerDialog extends Dialog {
editButton.setEnabled(hasSelection);
deleteButton.setEnabled(projectEditable);
toggleButton.setEnabled(hasSelection);
+ // A project rule with a pack rule's id replaces it, so removing it brings
the pack rule back.
+ deleteButton.setText(
+ BaseMessages.getString(
+ PKG,
+ projectEditable && shadowsPackRule(selected)
+ ? "RuleManagerDialog.Button.RestorePackRule"
+ : "RuleManagerDialog.Button.DeleteRule"));
+ }
+
+ /**
+ * Compared exactly, as the rule registry does: a project rule {@code
trans-002} sits beside the
+ * pack's TRANS-002 rather than replacing it.
+ */
+ private boolean shadowsPackRule(CustomLintRule rule) {
+ if (packRuleIds == null) {
+ packRuleIds = ProjectLintYamlExporter.packRuleIds();
+ }
+ return packRuleIds.contains(rule.generateRuleId());
+ }
+
+ /** Every rule id but this rule's own, for the editor to check a new id
against. */
+ private List<String> idsOtherThan(CustomLintRule rule) {
+ List<String> ids = new ArrayList<>();
+ for (CustomLintRule other : rules) {
+ if (other != rule) {
+ ids.add(other.generateRuleId());
+ }
+ }
+ return ids;
+ }
+
+ /** PROJECT-001, or the first number after it that no rule uses yet. */
+ private String suggestRuleId() {
+ Set<String> taken = new TreeSet<>();
+ for (CustomLintRule rule : rules) {
+ taken.add(rule.generateRuleId().toUpperCase(Locale.ROOT));
+ }
+ for (int i = 1; ; i++) {
+ String id = String.format("PROJECT-%03d", i);
+ if (!taken.contains(id)) {
+ return id;
+ }
+ }
+ }
+
+ /** Put the saved version of a rule where the old one was. */
+ private void replaceInList(CustomLintRule old, CustomLintRule replacement) {
+ for (int i = 0; i < rules.size(); i++) {
+ if (rules.get(i) == old) {
+ rules.set(i, replacement);
+ return;
+ }
+ }
+ rules.add(replacement);
}
private void addRule() {
- RuleBuilderDialog dialog = new RuleBuilderDialog(shell, null);
+ CustomLintRule draft = new CustomLintRule();
+ draft.setId(suggestRuleId());
+ RuleBuilderDialog dialog = new RuleBuilderDialog(shell, draft);
+ dialog.setText(BaseMessages.getString(PKG, "RuleBuilderDialog.Title.Add"));
+ dialog.setTakenIds(idsOtherThan(draft));
CustomLintRule newRule = dialog.open();
- if (newRule != null) {
- newRule.setPackId(RulePackIds.PROJECT);
- newRule.setPackOwner(RulePackOwner.PROJECT);
- // Fixed now: without an id of its own the rule is named after a hash of
its field and
- // condition, so editing either would save it under a new id and leave
the old entry behind.
- newRule.setId(newRule.generateRuleId());
+ if (newRule == null) {
+ return;
+ }
+ newRule.setPackId(RulePackIds.PROJECT);
+ newRule.setPackOwner(RulePackOwner.PROJECT);
+ // Listed only once it is saved, so the table never shows a rule
hop-lint.yml does not have.
+ if (saveRule(newRule, null)) {
rules.add(newRule);
populateTable();
- saveRule(newRule, null);
+ select(newRule);
log.logBasic("Added project rule: " + newRule.getName());
}
}
@@ -359,16 +472,43 @@ public class RuleManagerDialog extends Dialog {
// the project's hop-lint.yml, as an override when it only tunes the rule
and as a full rule
// definition when it redefines it. That keeps the pack upgradeable and
the decision with the
// project.
+ //
+ // The editor works on a copy, so a change that fails to save leaves the
rule as it was.
String previousId = rule.generateRuleId();
- RuleBuilderDialog dialog = new RuleBuilderDialog(shell, rule);
+ RuleBuilderDialog dialog = new RuleBuilderDialog(shell, rule.copy());
+ dialog.setTakenIds(idsOtherThan(rule));
CustomLintRule editedRule = dialog.open();
- if (editedRule != null) {
+ if (editedRule == null) {
+ return;
+ }
+ boolean redefinesPackRule =
+ !rule.isProjectEditable() &&
ProjectLintYamlExporter.redefinesPackRule(editedRule);
+ if (redefinesPackRule && !confirmRedefine(editedRule)) {
+ return;
+ }
+ if (saveRule(editedRule, previousId)) {
+ if (redefinesPackRule) {
+ // Written out in full, it is the project's rule now, as it will be
when next loaded.
+ editedRule.setPackId(RulePackIds.PROJECT);
+ editedRule.setPackOwner(RulePackOwner.PROJECT);
+ }
+ replaceInList(rule, editedRule);
populateTable();
- saveRule(editedRule, previousId);
- log.logBasic("Edited rule: " + rule.getName());
+ select(editedRule);
+ log.logBasic("Edited rule: " + editedRule.getName());
}
}
+ private boolean confirmRedefine(CustomLintRule rule) {
+ MessageBox confirmBox = new MessageBox(shell, SWT.ICON_WARNING | SWT.YES |
SWT.NO);
+ confirmBox.setText(
+ BaseMessages.getString(PKG,
"RuleManagerDialog.Dialog.ConfirmRedefine.Title"));
+ confirmBox.setMessage(
+ BaseMessages.getString(
+ PKG, "RuleManagerDialog.Dialog.ConfirmRedefine.Message",
rule.generateRuleId()));
+ return confirmBox.open() == SWT.YES;
+ }
+
private void deleteRule() {
CustomLintRule rule = getSelectedRule();
if (rule == null) {
@@ -380,21 +520,40 @@ public class RuleManagerDialog extends Dialog {
return;
}
+ String ruleId = rule.generateRuleId();
+ CustomLintRule packRule = ProjectLintYamlExporter.packRule(ruleId);
MessageBox confirmBox = new MessageBox(shell, SWT.ICON_QUESTION | SWT.YES
| SWT.NO);
- confirmBox.setText(BaseMessages.getString(PKG,
"RuleManagerDialog.Dialog.ConfirmDelete.Title"));
- confirmBox.setMessage(
- BaseMessages.getString(
- PKG, "RuleManagerDialog.Dialog.ConfirmDelete.Message",
rule.getName()));
+ if (packRule != null) {
+ confirmBox.setText(
+ BaseMessages.getString(PKG,
"RuleManagerDialog.Dialog.ConfirmRestore.Title"));
+ confirmBox.setMessage(
+ BaseMessages.getString(PKG,
"RuleManagerDialog.Dialog.ConfirmRestore.Message", ruleId));
+ } else {
+ confirmBox.setText(
+ BaseMessages.getString(PKG,
"RuleManagerDialog.Dialog.ConfirmDelete.Title"));
+ confirmBox.setMessage(
+ BaseMessages.getString(
+ PKG, "RuleManagerDialog.Dialog.ConfirmDelete.Message",
rule.getName()));
+ }
+ if (confirmBox.open() != SWT.YES) {
+ return;
+ }
- if (confirmBox.open() == SWT.YES) {
+ try {
+ LinterConfigPlugin.getInstance().removeProjectRule(ruleId);
+ } catch (Exception e) {
+ showSaveError(rule, e);
+ return;
+ }
+ if (packRule != null) {
+ replaceInList(rule, packRule);
+ populateTable();
+ select(packRule);
+ log.logBasic("Restored pack rule: " + ruleId);
+ } else {
rules.remove(rule);
populateTable();
updateButtonState();
- try {
-
LinterConfigPlugin.getInstance().removeProjectRule(rule.generateRuleId());
- } catch (Exception e) {
- showSaveError(rule, e);
- }
log.logBasic("Deleted project rule: " + rule.getName());
}
}
@@ -406,13 +565,18 @@ public class RuleManagerDialog extends Dialog {
return;
}
- rule.setEnabled(!rule.isEnabled());
- populateTable();
- rulesTable.setSelection(findTableIndex(rule));
- updateButtonState();
- saveRule(rule, null);
- log.logBasic(
- "Toggled rule: " + rule.getName() + " to " + (rule.isEnabled() ?
"enabled" : "disabled"));
+ CustomLintRule toggled = rule.copy();
+ toggled.setEnabled(!rule.isEnabled());
+ if (saveRule(toggled, null)) {
+ replaceInList(rule, toggled);
+ populateTable();
+ select(toggled);
+ log.logBasic(
+ "Toggled rule: "
+ + toggled.getName()
+ + " to "
+ + (toggled.isEnabled() ? "enabled" : "disabled"));
+ }
}
private int findTableIndex(CustomLintRule rule) {
@@ -449,11 +613,13 @@ public class RuleManagerDialog extends Dialog {
* Write the one rule that changed. The rest of hop-lint.yml — other rules,
exclusions,
* suppressions and comments — stays as it is.
*/
- private void saveRule(CustomLintRule rule, String previousId) {
+ private boolean saveRule(CustomLintRule rule, String previousId) {
try {
LinterConfigPlugin.getInstance().saveProjectRule(rule, previousId);
+ return true;
} catch (Exception e) {
showSaveError(rule, e);
+ return false;
}
}
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 d5f3b34417..e36e9b39a9 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
@@ -19,9 +19,11 @@ package org.apache.hop.lint.registry;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
+import java.util.Set;
import org.apache.hop.lint.CustomLintRule;
import org.apache.hop.lint.RuleClause;
import org.yaml.snakeyaml.DumperOptions;
@@ -84,6 +86,30 @@ public final class ProjectLintYamlExporter {
return null;
}
+ /**
+ * Whether saving this rule writes it out in full over a pack rule of the
same id, so that it no
+ * longer follows the pack. A change to the name, description, severity,
value or tags is an
+ * override and keeps the rule tied to the pack; a change to what it checks
is not.
+ */
+ public static boolean redefinesPackRule(CustomLintRule desired) {
+ CustomLintRule packDefault = packDefaults().get(desired.generateRuleId());
+ return packDefault != null && structurallyDiffersFromPackDefault(desired,
packDefault);
+ }
+
+ /**
+ * The rule a pack defines under this id, before the project changes it, or
null when no pack
+ * defines one.
+ */
+ public static CustomLintRule packRule(String ruleId) {
+ CustomLintRule packDefault = packDefaults().get(ruleId);
+ return packDefault != null ? packDefault.copy() : null;
+ }
+
+ /** The ids of the rules the packs define, as they are written. */
+ public static Set<String> packRuleIds() {
+ return new LinkedHashSet<>(packDefaults().keySet());
+ }
+
/** The rules as the packs define them, before any project changes them. */
private static Map<String, CustomLintRule> packDefaults() {
Map<String, CustomLintRule> packDefaults = new LinkedHashMap<>();
@@ -98,9 +124,11 @@ public final class ProjectLintYamlExporter {
return desired.isEnabled() != packDefault.isEnabled()
|| !Objects.equals(desired.getSeverity(), packDefault.getSeverity())
|| !Objects.equals(desired.getAdditionalParameters(),
packDefault.getAdditionalParameters())
- || !Objects.equals(desired.getConditionValue(),
packDefault.getConditionValue())
+ || !sameText(desired.getConditionValue(),
packDefault.getConditionValue())
|| !Objects.equals(desired.getHelpUri(), packDefault.getHelpUri())
|| !Objects.equals(desired.getTags(), packDefault.getTags())
+ || !Objects.equals(desired.getName(), packDefault.getName())
+ || !sameText(desired.getDescription(), packDefault.getDescription())
|| structurallyDiffersFromPackDefault(desired, packDefault);
}
@@ -121,25 +149,40 @@ public final class ProjectLintYamlExporter {
|| !Objects.equals(desired.getCondition(), packDefault.getCondition())
|| !Objects.equals(desired.getAppliesTo(), packDefault.getAppliesTo())
|| !Objects.equals(desired.getType(), packDefault.getType())
- || !Objects.equals(desired.getMessageKey(),
packDefault.getMessageKey())
- || !Objects.equals(desired.getName(), packDefault.getName())
- || !Objects.equals(desired.getDescription(),
packDefault.getDescription());
+ || !Objects.equals(desired.getMessageKey(),
packDefault.getMessageKey());
}
/**
- * What a rule looks at, ignoring the values it compares against.
+ * What a rule looks at: every clause's field and condition, and the values
of the clauses after
+ * the first.
*
- * <p>A changed threshold is a tune and an override block carries it. A
changed field or condition
- * is a redefinition and it cannot, so only those count as structural.
+ * <p>An override block carries one {@code conditionValue}, the first
clause's, so a changed
+ * threshold there is a tune. A changed field or condition, or a changed
value in any later
+ * clause, can only be written as the whole rule.
*/
private static List<String> clauseShape(CustomLintRule rule) {
List<String> shape = new ArrayList<>();
- for (RuleClause clause : rule.getClauses()) {
- shape.add(clause.getTargetField() + "|" + clause.getCondition());
+ List<RuleClause> clauses = rule.getClauses();
+ for (int i = 0; i < clauses.size(); i++) {
+ RuleClause clause = clauses.get(i);
+ String value = i == 0 ? "" : emptyToNull(clause.getConditionValue());
+ shape.add(clause.getTargetField() + "|" + clause.getCondition() + "|" +
value);
}
return shape;
}
+ private static String emptyToNull(String text) {
+ return text == null || text.isEmpty() ? null : text;
+ }
+
+ /**
+ * An empty text and no text are the same thing: the editor cannot tell them
apart, and a pack
+ * reads a missing conditionValue as an empty one.
+ */
+ private static boolean sameText(String first, String second) {
+ return Objects.equals(emptyToNull(first), emptyToNull(second));
+ }
+
private static Map<String, Object> toOverrideMap(
CustomLintRule desired, CustomLintRule packDefault) {
Map<String, Object> ruleConfig = new LinkedHashMap<>();
@@ -149,7 +192,7 @@ public final class ProjectLintYamlExporter {
if (!Objects.equals(desired.getSeverity(), packDefault.getSeverity())) {
ruleConfig.put("severity", desired.getSeverity());
}
- if (!Objects.equals(desired.getConditionValue(),
packDefault.getConditionValue())
+ if (!sameText(desired.getConditionValue(), packDefault.getConditionValue())
&& desired.getConditionValue() != null) {
ruleConfig.put("conditionValue", desired.getConditionValue());
}
@@ -161,6 +204,16 @@ public final class ProjectLintYamlExporter {
// An empty value is how a project removes the pack's link.
ruleConfig.put("helpUri", desired.getHelpUri() != null ?
desired.getHelpUri() : "");
}
+ if (!Objects.equals(desired.getName(), packDefault.getName())
+ && desired.getName() != null
+ && !desired.getName().isEmpty()) {
+ ruleConfig.put("name", desired.getName());
+ }
+ if (!sameText(desired.getDescription(), packDefault.getDescription())) {
+ // An empty value is how a project clears the pack's description.
+ ruleConfig.put(
+ "description", desired.getDescription() != null ?
desired.getDescription() : "");
+ }
Map<String, Object> tags = tagOverrides(desired.getTags(),
packDefault.getTags());
if (!tags.isEmpty()) {
ruleConfig.put("tags", tags);
@@ -220,7 +273,10 @@ public final class ProjectLintYamlExporter {
} else {
ruleConfig.put("targetField", rule.getTargetField());
ruleConfig.put("condition", rule.getCondition().name());
- ruleConfig.put("conditionValue", rule.getConditionValue());
+ // Left out when there is none, as in a composed rule's clauses.
+ if (rule.getConditionValue() != null &&
!rule.getConditionValue().isEmpty()) {
+ ruleConfig.put("conditionValue", rule.getConditionValue());
+ }
}
ruleConfig.put("name", rule.getName());
ruleConfig.put("description", rule.getDescription());
diff --git
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/ProjectYamlOverlay.java
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/ProjectYamlOverlay.java
index cbb787c497..59fab55d93 100644
---
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/ProjectYamlOverlay.java
+++
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/ProjectYamlOverlay.java
@@ -69,6 +69,11 @@ public final class ProjectYamlOverlay {
private final Map<String, Object> parameters;
private final String helpUri;
+ /** A name and description of the project's own, for a pack rule it only
renames. */
+ private final String name;
+
+ private final String description;
+
/**
* Tags the project sets on the pack's rule, by key. A key replaces the
pack's values for that
* key and leaves its other tags alone; a key with no values removes the
pack's tag.
@@ -81,12 +86,16 @@ public final class ProjectYamlOverlay {
String conditionValue,
Map<String, Object> parameters,
String helpUri,
+ String name,
+ String description,
Map<String, List<String>> tags) {
this.enabled = enabled;
this.severity = severity;
this.conditionValue = conditionValue;
this.parameters = parameters;
this.helpUri = helpUri;
+ this.name = name;
+ this.description = description;
this.tags = tags != null ? tags : Collections.emptyMap();
}
@@ -112,9 +121,18 @@ public final class ProjectYamlOverlay {
Object value = ruleData.get("helpUri");
helpUri = value != null ? value.toString().trim() : "";
}
+ // A name or description renames the pack rule in place; it stays the
pack's rule.
+ String name = ruleData.get("name") != null ?
String.valueOf(ruleData.get("name")) : null;
+ String description =
+ ruleData.containsKey("description")
+ ? (ruleData.get("description") != null
+ ? String.valueOf(ruleData.get("description"))
+ : "")
+ : null;
Map<String, List<String>> tags =
YamlRulePackParser.tagsValue(ruleData.get("tags"), ruleId, location,
true);
- return new ProjectRuleOverlay(enabled, severity, conditionValue,
parameters, helpUri, tags);
+ return new ProjectRuleOverlay(
+ enabled, severity, conditionValue, parameters, helpUri, name,
description, tags);
}
public void applyTo(org.apache.hop.lint.CustomLintRule rule) {
@@ -133,6 +151,12 @@ public final class ProjectYamlOverlay {
if (helpUri != null) {
rule.setHelpUri(helpUri.isEmpty() ? null : helpUri);
}
+ if (name != null && !name.trim().isEmpty()) {
+ rule.setName(name);
+ }
+ if (description != null) {
+ rule.setDescription(description);
+ }
tags.forEach(
(key, values) -> {
if (values.isEmpty()) {
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 d93d1bf1a9..16b97e756a 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
@@ -77,7 +77,15 @@ public final class YamlRulePackParser {
/** The keys a project's hop-lint.yml may use to tune a rule from a pack. */
static final Set<String> OVERRIDE_KEYS =
- Set.of("enabled", "severity", "conditionValue", "parameters", "tags",
"helpUri");
+ Set.of(
+ "enabled",
+ "severity",
+ "conditionValue",
+ "parameters",
+ "tags",
+ "helpUri",
+ "name",
+ "description");
private YamlRulePackParser() {}
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 050fbcc81a..7b3cc65bb9 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
@@ -58,6 +58,8 @@ LinterConfigPlugin.Option.NativeChecks.ToolTip=Also run Hop's
built-in pipeline/
# ---------------------------------------------------------------------------
# Rule builder dialog
# ---------------------------------------------------------------------------
+RuleBuilderDialog.Label.RuleId=Rule ID:
+RuleBuilderDialog.RuleId.Tooltip=The id findings, SARIF reports, --list-rules
and hop-lint.yml use for this rule: letters, digits, hyphens, underscores and
dots.
RuleBuilderDialog.Label.RuleName=Rule Name:
RuleBuilderDialog.Label.Description=Description:
RuleBuilderDialog.Label.TargetType=Target Type:
@@ -75,6 +77,17 @@ RuleBuilderDialog.Tags.Incomplete=Each tag needs both a key
and a value.
RuleBuilderDialog.Button.Ok=OK
RuleBuilderDialog.Button.Cancel=Cancel
RuleBuilderDialog.Dialog.ValidationError.Title=Validation Error
+RuleBuilderDialog.Title.Add=Add Lint Rule
+RuleBuilderDialog.Title.Edit=Edit Lint Rule
+RuleBuilderDialog.Error.NameRequired=Rule name is required
+RuleBuilderDialog.Error.TargetRequired=Target type must be selected
+RuleBuilderDialog.Error.RuleIdRequired=Rule ID is required
+RuleBuilderDialog.Error.RuleIdInvalid=Rule ID ''{0}'' can only contain
letters, digits, hyphens, underscores and dots, and must start with a letter or
digit
+RuleBuilderDialog.Error.RuleIdTaken=Another rule already has ID ''{0}''
+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
# ---------------------------------------------------------------------------
# Rule manager dialog
@@ -93,10 +106,15 @@ RuleManagerDialog.Column.Tags=Tags
RuleManagerDialog.Button.AddProjectRule=Add Project Rule
RuleManagerDialog.Button.EditRule=Edit Rule
RuleManagerDialog.Button.DeleteRule=Delete Rule
+RuleManagerDialog.Button.RestorePackRule=Restore Pack Rule
RuleManagerDialog.Button.ToggleEnabled=Toggle Enabled
RuleManagerDialog.Button.Close=Close
RuleManagerDialog.Dialog.ConfirmDelete.Title=Confirm Delete
RuleManagerDialog.Dialog.ConfirmDelete.Message=Delete project rule ''{0}''?
+RuleManagerDialog.Dialog.ConfirmRestore.Title=Restore Pack Rule
+RuleManagerDialog.Dialog.ConfirmRestore.Message=Remove {0} from hop-lint.yml
and use the pack''s rule again?
+RuleManagerDialog.Dialog.ConfirmRedefine.Title=Replace Pack Rule
+RuleManagerDialog.Dialog.ConfirmRedefine.Message=This changes what {0} checks.
The whole rule is written to hop-lint.yml and replaces the pack''s rule, so
later changes to it in the pack no longer apply.\n\nSave anyway?
RuleManagerDialog.Dialog.PackRule.Title=Pack Rule
RuleManagerDialog.Dialog.SaveFailed.Title=Rule Not Saved
RuleManagerDialog.Dialog.SaveFailed.Message=Rule {0} could not be saved to
hop-lint.yml. The file was not changed.
diff --git
a/plugins/misc/lint/src/test/java/org/apache/hop/lint/LintRuleYamlWriterTest.java
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/LintRuleYamlWriterTest.java
index ca4fd65c91..081bf08c74 100644
---
a/plugins/misc/lint/src/test/java/org/apache/hop/lint/LintRuleYamlWriterTest.java
+++
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/LintRuleYamlWriterTest.java
@@ -108,6 +108,42 @@ public class LintRuleYamlWriterTest {
assertEquals(1, policy.getSuppressions().size());
}
+ /**
+ * A new id and the removal of the old one go into the file together.
Removing first and writing
+ * second lost the rule from the file when the write was refused.
+ */
+ @Test
+ public void aNewIdReplacesTheOldEntryInOneWrite() throws Exception {
+ Files.writeString(yaml(), PROJECT_FILE, StandardCharsets.UTF_8);
+
+ LintPolicyYamlWriter.putRule(
+ yaml(), "CRM-NAMING", entry("type", "custom", "enabled", false),
"CRM-001");
+
+ Map<String, ?> rules = rulesIn(Files.readString(yaml(),
StandardCharsets.UTF_8));
+ assertFalse(rules.containsKey("CRM-001"), "the old id is gone");
+ assertEquals(entry("type", "custom", "enabled", false),
rules.get("CRM-NAMING"));
+ assertTrue(rules.containsKey("DOC-001") && rules.containsKey("SEC-002"));
+ }
+
+ @Test
+ public void aRefusedNewIdLeavesTheFileAsItWas() throws Exception {
+ String inline = "rules: {CRM-001: {enabled: false}}\n";
+ Files.writeString(yaml(), inline, StandardCharsets.UTF_8);
+
+ assertThrows(
+ IOException.class,
+ () ->
+ LintPolicyYamlWriter.putRule(yaml(), "CRM-NAMING",
entry("enabled", true), "CRM-001"));
+
+ assertEquals(inline, Files.readString(yaml(), StandardCharsets.UTF_8));
+ }
+
+ @SuppressWarnings("unchecked")
+ private static Map<String, ?> rulesIn(String yaml) {
+ Map<String, ?> root = new org.yaml.snakeyaml.Yaml().load(yaml);
+ return (Map<String, ?>) root.get("rules");
+ }
+
@Test
public void aRuleWithAnInlineCommentIsReplacedWhole() throws Exception {
String after =
diff --git
a/plugins/misc/lint/src/test/java/org/apache/hop/lint/RuleBuilderDialogClauseTest.java
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/RuleBuilderDialogClauseTest.java
new file mode 100644
index 0000000000..31425bc21b
--- /dev/null
+++
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/RuleBuilderDialogClauseTest.java
@@ -0,0 +1,316 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hop.lint;
+
+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 java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.concurrent.atomic.AtomicReference;
+import org.apache.hop.ui.testing.SwtBotTestBase;
+import org.eclipse.swtbot.swt.finder.SWTBot;
+import org.eclipse.swtbot.swt.finder.waits.DefaultCondition;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+
+/** The field, condition and value widgets of the rule editor, and the clause
table above them. */
+@Tag("uitest")
+class RuleBuilderDialogClauseTest extends SwtBotTestBase {
+
+ private static CustomLintRule rule(
+ RuleTarget target, String field, RuleCondition condition, String value) {
+ CustomLintRule rule = new CustomLintRule("Naming", target, field,
condition);
+ rule.setId("PROJECT-001");
+ rule.setConditionValue(value);
+ return rule;
+ }
+
+ /** Hidden but kept, the old pattern was saved with NOT_NULL and came back
on the next switch. */
+ @Test
+ void aConditionWithoutValueDropsTheOldValue() {
+ CustomLintRule rule =
+ rule(RuleTarget.PIPELINE, "name", RuleCondition.MATCHES_PATTERN,
"^[A-Z][A-Z0-9_]*$");
+ AtomicReference<CustomLintRule> saved = new AtomicReference<>();
+
+ withDialog(
+ parent -> saved.set(new RuleBuilderDialog(parent, rule).open()),
+ bot -> {
+ SWTBot dialog = bot.shell("Edit Lint Rule").bot();
+ dialog.comboBoxWithLabel("Condition:").setSelection("Not Null");
+ dialog.button("OK").click();
+ });
+
+ assertNotNull(saved.get(), "the dialog refused to save");
+ assertEquals(RuleCondition.NOT_NULL, saved.get().getCondition());
+ assertNull(saved.get().getConditionValue());
+ }
+
+ /** Choosing the field again emptied the condition, and OK then refused to
save. */
+ @Test
+ void choosingTheSameFieldKeepsTheCondition() {
+ CustomLintRule rule =
+ rule(RuleTarget.TRANSFORM, "isOrphaned", RuleCondition.MUST_BE_FALSE,
null);
+ AtomicReference<CustomLintRule> saved = new AtomicReference<>();
+ AtomicReference<String> tableCondition = new AtomicReference<>();
+
+ withDialog(
+ parent -> saved.set(new RuleBuilderDialog(parent, rule).open()),
+ bot -> {
+ SWTBot dialog = bot.shell("Edit Lint Rule").bot();
+ dialog.comboBoxWithLabel("Field:").setSelection("isOrphaned");
+ tableCondition.set(dialog.table().cell(0, 1));
+ dialog.button("OK").click();
+ });
+
+ assertEquals("Must Be False", tableCondition.get());
+ assertNotNull(saved.get(), "the dialog refused to save");
+ assertEquals(RuleCondition.MUST_BE_FALSE, saved.get().getCondition());
+ }
+
+ /** A field with a condition both offer keeps the condition and its value. */
+ @Test
+ void anotherFieldKeepsAConditionItAlsoOffers() {
+ CustomLintRule rule = rule(RuleTarget.PIPELINE, "name",
RuleCondition.STARTS_WITH, "etl_");
+ AtomicReference<CustomLintRule> saved = new AtomicReference<>();
+
+ withDialog(
+ parent -> saved.set(new RuleBuilderDialog(parent, rule).open()),
+ bot -> {
+ SWTBot dialog = bot.shell("Edit Lint Rule").bot();
+ dialog.comboBoxWithLabel("Field:").setSelection("description");
+ dialog.button("OK").click();
+ });
+
+ assertNotNull(saved.get(), "the dialog refused to save");
+ assertEquals("description", saved.get().getTargetField());
+ assertEquals(RuleCondition.STARTS_WITH, saved.get().getCondition());
+ assertEquals("etl_", saved.get().getConditionValue());
+ }
+
+ /** Pipeline to Workflow cleared the field and condition, though workflows
have a name too. */
+ @Test
+ void anotherTargetKeepsAFieldItAlsoHas() {
+ CustomLintRule rule = rule(RuleTarget.PIPELINE, "name",
RuleCondition.NOT_NULL, null);
+ AtomicReference<CustomLintRule> saved = new AtomicReference<>();
+ AtomicReference<String> tableField = new AtomicReference<>();
+
+ withDialog(
+ parent -> saved.set(new RuleBuilderDialog(parent, rule).open()),
+ bot -> {
+ SWTBot dialog = bot.shell("Edit Lint Rule").bot();
+ dialog
+ .comboBoxWithLabel("Target Type:")
+ .setSelection(RuleTarget.WORKFLOW.getDisplayName());
+ tableField.set(dialog.table().cell(0, 0));
+ dialog.button("OK").click();
+ });
+
+ assertEquals("name", tableField.get());
+ assertNotNull(saved.get(), "the dialog refused to save");
+ assertEquals(RuleTarget.WORKFLOW, saved.get().getTarget());
+ assertEquals("name", saved.get().getTargetField());
+ assertEquals(RuleCondition.NOT_NULL, saved.get().getCondition());
+ }
+
+ /** A field the new target does not have goes, in the table as well as the
dropdowns. */
+ @Test
+ void anotherTargetClearsAFieldItDoesNotHave() {
+ CustomLintRule rule =
+ rule(RuleTarget.PIPELINE, "transformCount", RuleCondition.MAX_VALUE,
"30");
+ AtomicReference<String> tableField = new AtomicReference<>();
+ AtomicReference<String> tableCondition = new AtomicReference<>();
+
+ withDialog(
+ parent -> new RuleBuilderDialog(parent, rule).open(),
+ bot -> {
+ SWTBot dialog = bot.shell("Edit Lint Rule").bot();
+ dialog
+ .comboBoxWithLabel("Target Type:")
+ .setSelection(RuleTarget.WORKFLOW.getDisplayName());
+ tableField.set(dialog.table().cell(0, 0));
+ tableCondition.set(dialog.table().cell(0, 1));
+ dialog.button("Cancel").click();
+ });
+
+ assertEquals("", tableField.get());
+ assertEquals("", tableCondition.get());
+ }
+
+ /**
+ * A trip to another target and back gives a clause its field again, even
one the editor does not
+ * list, such as a Table Input's sql.
+ */
+ @Test
+ void backOnItsOwnTargetARuleGetsItsFieldsBack() {
+ CustomLintRule rule =
+ rule(RuleTarget.TRANSFORM, "sql", RuleCondition.NOT_MATCHES_PATTERN,
"(?i)select \\*");
+ AtomicReference<CustomLintRule> saved = new AtomicReference<>();
+ AtomicReference<String> emptiedField = new AtomicReference<>();
+
+ withDialog(
+ parent -> saved.set(new RuleBuilderDialog(parent, rule).open()),
+ bot -> {
+ SWTBot dialog = bot.shell("Edit Lint Rule").bot();
+ dialog
+ .comboBoxWithLabel("Target Type:")
+ .setSelection(RuleTarget.TRANSFORM.getDisplayName());
+ dialog
+ .comboBoxWithLabel("Target Type:")
+ .setSelection(RuleTarget.PIPELINE.getDisplayName());
+ emptiedField.set(dialog.table().cell(0, 0));
+ dialog
+ .comboBoxWithLabel("Target Type:")
+ .setSelection(RuleTarget.TRANSFORM.getDisplayName());
+ dialog.button("OK").click();
+ });
+
+ assertEquals("", emptiedField.get(), "pipelines have no sql");
+ assertNotNull(saved.get(), "the dialog refused to save");
+ assertEquals("sql", saved.get().getTargetField());
+ assertEquals(RuleCondition.NOT_MATCHES_PATTERN,
saved.get().getCondition());
+ assertEquals("(?i)select \\*", saved.get().getConditionValue());
+ }
+
+ /** An edit made before the trip to another target is what comes back, not
the rule as opened. */
+ @Test
+ void backOnItsOwnTargetAClauseKeepsItsEdits() {
+ CustomLintRule rule = rule(RuleTarget.TRANSFORM, "sql",
RuleCondition.NOT_MATCHES_PATTERN, "a");
+ AtomicReference<CustomLintRule> saved = new AtomicReference<>();
+
+ withDialog(
+ parent -> saved.set(new RuleBuilderDialog(parent, rule).open()),
+ bot -> {
+ SWTBot dialog = bot.shell("Edit Lint Rule").bot();
+ dialog.textWithLabel("Value:").setText("b");
+ dialog
+ .comboBoxWithLabel("Target Type:")
+ .setSelection(RuleTarget.PIPELINE.getDisplayName());
+ dialog
+ .comboBoxWithLabel("Target Type:")
+ .setSelection(RuleTarget.TRANSFORM.getDisplayName());
+ dialog.button("OK").click();
+ });
+
+ assertNotNull(saved.get(), "the dialog refused to save");
+ assertEquals("sql", saved.get().getTargetField());
+ assertEquals("b", saved.get().getConditionValue());
+ }
+
+ /** OK checks every clause, and shows the first incomplete one. */
+ @Test
+ void anEmptyClauseThatIsNotOnShowIsRefused() {
+ CustomLintRule rule = rule(RuleTarget.PIPELINE, "name",
RuleCondition.NOT_NULL, null);
+ rule.setAdditionalClauses(new ArrayList<>(List.of(new RuleClause("", null,
""))));
+
+ Refusal refusal = pressOkExpectingRefusal(rule);
+
+ assertEquals(List.of("Clause 2: Field must be selected"),
refusal.errors());
+ assertEquals("", refusal.selectedField(), "the empty clause is selected");
+ }
+
+ @Test
+ void aMissingValueInAClauseThatIsNotOnShowIsRefused() {
+ CustomLintRule rule = rule(RuleTarget.PIPELINE, "name",
RuleCondition.NOT_NULL, null);
+ rule.setAdditionalClauses(
+ new ArrayList<>(List.of(new RuleClause("description",
RuleCondition.MATCHES_PATTERN, ""))));
+
+ Refusal refusal = pressOkExpectingRefusal(rule);
+
+ assertEquals(List.of("Clause 2: Value is required for this condition"),
refusal.errors());
+ assertEquals("description", refusal.selectedField());
+ }
+
+ /** A value left on a condition that takes none is no reason to refuse; it
is just not saved. */
+ @Test
+ void aLeftoverValueOnAConditionWithoutValueIsDropped() {
+ CustomLintRule rule = rule(RuleTarget.PIPELINE, "name",
RuleCondition.NOT_NULL, "^X$");
+ rule.setAdditionalClauses(
+ new ArrayList<>(List.of(new RuleClause("description",
RuleCondition.NOT_EMPTY, "left"))));
+ AtomicReference<CustomLintRule> saved = new AtomicReference<>();
+ List<String> errors = new CopyOnWriteArrayList<>();
+
+ withDialog(
+ parent -> {
+ RuleBuilderDialog dialog = new RuleBuilderDialog(parent, rule);
+ dialog.errorReporter = errors::add;
+ saved.set(dialog.open());
+ },
+ bot -> bot.shell("Edit Lint Rule").bot().button("OK").click());
+
+ assertEquals(List.of(), errors);
+ assertNotNull(saved.get(), "the dialog refused to save");
+ assertNull(saved.get().getConditionValue());
+ assertNull(saved.get().getAdditionalClauses().get(0).getConditionValue());
+ }
+
+ private record Refusal(List<String> errors, String selectedField) {}
+
+ /** Press OK, wait for the refusal, note the selected clause's field and
cancel. */
+ private Refusal pressOkExpectingRefusal(CustomLintRule rule) {
+ List<String> errors = new CopyOnWriteArrayList<>();
+ AtomicReference<CustomLintRule> saved = new AtomicReference<>();
+ AtomicReference<String> selectedField = new AtomicReference<>();
+
+ withDialog(
+ parent -> {
+ RuleBuilderDialog dialog = new RuleBuilderDialog(parent, rule);
+ dialog.errorReporter = errors::add;
+ saved.set(dialog.open());
+ },
+ bot -> {
+ SWTBot dialog = bot.shell("Edit Lint Rule").bot();
+ dialog.button("OK").click();
+ bot.waitUntil(
+ new DefaultCondition() {
+ @Override
+ public boolean test() {
+ return !errors.isEmpty();
+ }
+
+ @Override
+ public String getFailureMessage() {
+ return "OK was not refused";
+ }
+ });
+ selectedField.set(dialog.table().selection().get(0, 0));
+ dialog.button("Cancel").click();
+ });
+
+ assertNull(saved.get(), "a refused rule is not saved");
+ return new Refusal(errors, selectedField.get());
+ }
+
+ @Test
+ void aProjectRuleIdCanBeChanged() {
+ CustomLintRule rule = rule(RuleTarget.PIPELINE, "name",
RuleCondition.NOT_NULL, null);
+ AtomicReference<CustomLintRule> saved = new AtomicReference<>();
+
+ withDialog(
+ parent -> saved.set(new RuleBuilderDialog(parent, rule).open()),
+ bot -> {
+ SWTBot dialog = bot.shell("Edit Lint Rule").bot();
+ dialog.textWithLabel("Rule ID:").setText("NAMING-PIPE");
+ dialog.button("OK").click();
+ });
+
+ assertNotNull(saved.get(), "the dialog refused to save");
+ assertEquals("NAMING-PIPE", saved.get().getId());
+ }
+}
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
new file mode 100644
index 0000000000..8b5f8031f1
--- /dev/null
+++
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/RuleBuilderValidationTest.java
@@ -0,0 +1,60 @@
+/*
+ * 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.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import java.util.Set;
+import org.junit.jupiter.api.Test;
+
+/** The checks the rule editor runs on OK, before anything is written to
hop-lint.yml. */
+class RuleBuilderValidationTest {
+
+ /**
+ * An empty extra clause passed the editor, which only checked the clause on
show, and the save
+ * then failed with a NullPointerException message.
+ */
+ @Test
+ void anEmptyClauseIsIncomplete() {
+ assertEquals(
+ "Field must be selected", RuleBuilderDialog.clauseProblem(new
RuleClause("", null, "")));
+ assertEquals(
+ "Condition must be selected",
+ RuleBuilderDialog.clauseProblem(new RuleClause("name", null, "")));
+ assertEquals(
+ "Value is required for this condition",
+ RuleBuilderDialog.clauseProblem(new RuleClause("name",
RuleCondition.MATCHES_PATTERN, "")));
+ assertNull(RuleBuilderDialog.clauseProblem(new RuleClause("name",
RuleCondition.NOT_NULL, "")));
+ }
+
+ @Test
+ void aRuleIdMustBeUsableAndFree() {
+ Set<String> taken = Set.of("DOC-001");
+
+ assertNull(RuleBuilderDialog.ruleIdProblem("PROJECT-001", taken));
+ assertNull(RuleBuilderDialog.ruleIdProblem("naming.pipelines_2", taken));
+ assertNotNull(RuleBuilderDialog.ruleIdProblem("", taken));
+ assertNotNull(RuleBuilderDialog.ruleIdProblem("my rule", taken));
+ assertNotNull(RuleBuilderDialog.ruleIdProblem("-001", taken));
+ assertEquals(
+ "Another rule already has ID 'doc-001'",
+ RuleBuilderDialog.ruleIdProblem("doc-001", taken),
+ "ids are compared ignoring case, as hop-lint.yml matches them");
+ }
+}
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 45a27a37b0..673c7cf6ca 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
@@ -284,6 +284,130 @@ public class RuleRegistryTest {
assertFalse(yaml.contains("type: custom"), "a retuned pack rule stays an
override");
}
+ /**
+ * A new name or description is not a new rule. Written out in full, the
pack rule was replaced
+ * for good and stopped following the pack, with no warning in the editor.
+ */
+ @Test
+ public void renamingAPackRuleIsAnOverride() throws Exception {
+ CustomLintRule renamed = packRule("DOC-001");
+ String packName = renamed.getName();
+ renamed.setName("Pipelines need a description");
+ renamed.setDescription("Our own wording");
+
+ assertFalse(ProjectLintYamlExporter.redefinesPackRule(renamed));
+ String yaml = ProjectLintYamlExporter.export(List.of(renamed));
+ assertFalse(yaml.contains("type: custom"), "a renamed pack rule stays an
override");
+ assertFalse(yaml.contains("targetField"), yaml);
+
+ File projectYaml = File.createTempFile("hop-lint", ".yml");
+ Files.writeString(projectYaml.toPath(), yaml);
+ try {
+ CustomLintRule loaded =
+ RuleRegistry.getInstance().resolve(projectYaml).getRules().stream()
+ .filter(rule -> "DOC-001".equals(rule.generateRuleId()))
+ .findFirst()
+ .orElseThrow();
+ assertEquals("Pipelines need a description", loaded.getName());
+ assertEquals("Our own wording", loaded.getDescription());
+ assertFalse(loaded.isProjectEditable(), "still the pack's rule, not " +
packName + " copied");
+ } finally {
+ Files.deleteIfExists(projectYaml.toPath());
+ }
+ }
+
+ /**
+ * A pack reads a missing conditionValue as empty, and the editor saves none
as null. OK on an
+ * unchanged rule must not count as a change.
+ */
+ @Test
+ public void anUnchangedPackRuleSavedFromTheEditorNeedsNoEntry() {
+ CustomLintRule unchanged = packRule("DOC-001");
+ unchanged.setConditionValue(null);
+
+ assertEquals(null, ProjectLintYamlExporter.entryFor(unchanged));
+
assertFalse(ProjectLintYamlExporter.export(List.of(unchanged)).contains("DOC-001"));
+ }
+
+ /** Project rules replace a pack rule only under the very same id, as the
registry stores them. */
+ @Test
+ public void aPackRuleIsFoundByItsExactId() {
+ assertTrue(ProjectLintYamlExporter.packRule("TRANS-002") != null);
+ assertEquals(null, ProjectLintYamlExporter.packRule("trans-002"));
+ }
+
+ @Test
+ public void changingWhatAPackRuleChecksRedefinesIt() {
+ CustomLintRule redefined = packRule("TRANS-002");
+ redefined.setTargetField("hasDisabledHops");
+
+ assertTrue(ProjectLintYamlExporter.redefinesPackRule(redefined));
+ assertTrue(ProjectLintYamlExporter.packRuleIds().contains("TRANS-002"));
+ assertEquals("TRANS-002",
ProjectLintYamlExporter.packRule("TRANS-002").generateRuleId());
+ }
+
+ /**
+ * An override carries the first clause's value only, so a changed value in
a later clause makes
+ * the rule a redefinition, written whole with every clause.
+ */
+ @Test
+ public void aChangedValueInALaterClauseRedefinesTheRule() throws Exception {
+ CustomLintRule edited = packRule("SQL-002");
+ edited.getAdditionalClauses().get(0).setConditionValue("^\\s*0\\s*$");
+
+ assertTrue(ProjectLintYamlExporter.redefinesPackRule(edited));
+ String yaml = ProjectLintYamlExporter.export(List.of(edited));
+ assertTrue(yaml.contains("type: custom"), yaml);
+
+ File projectYaml = File.createTempFile("hop-lint", ".yml");
+ Files.writeString(projectYaml.toPath(), yaml);
+ try {
+ CustomLintRule loaded =
+ RuleRegistry.getInstance().resolve(projectYaml).getRules().stream()
+ .filter(rule -> "SQL-002".equals(rule.generateRuleId()))
+ .findFirst()
+ .orElseThrow();
+ assertEquals(edited.getClauses(), loaded.getClauses(), "both patterns
are written");
+ } finally {
+ Files.deleteIfExists(projectYaml.toPath());
+ }
+ }
+
+ @Test
+ public void aChangedValueInTheFirstClauseStaysAnOverride() {
+ CustomLintRule edited = packRule("SQL-002");
+ edited.setConditionValue("(?is).*\\bselect\\s+\\*\\s+from.*");
+
+ assertFalse(ProjectLintYamlExporter.redefinesPackRule(edited));
+ Map<String, Object> entry = ProjectLintYamlExporter.entryFor(edited);
+ assertEquals(Map.of("conditionValue", edited.getConditionValue()), entry);
+ }
+
+ /** {@code conditionValue: ''} was written for every rule whose condition
takes no value. */
+ @Test
+ public void aRuleWithoutAValueIsWrittenWithoutConditionValue() {
+ CustomLintRule rule =
+ new CustomLintRule(
+ "Named",
+ org.apache.hop.lint.RuleTarget.PIPELINE,
+ "name",
+ org.apache.hop.lint.RuleCondition.NOT_NULL);
+ rule.setId("PROJECT-001");
+
+ Map<String, Object> entry = ProjectLintYamlExporter.entryFor(rule);
+
+ assertEquals("NOT_NULL", entry.get("condition"));
+ assertFalse(entry.containsKey("conditionValue"), entry.toString());
+ }
+
+ private static CustomLintRule packRule(String ruleId) {
+ return RuleRegistry.getInstance().resolve(null).getRules().stream()
+ .filter(rule -> ruleId.equals(rule.generateRuleId()))
+ .findFirst()
+ .orElseThrow()
+ .copy();
+ }
+
/**
* Pack rules are cached, so each caller has to get its own copies. Handing
out the cached objects
* would let one caller's edit — the rule manager toggling a rule, a project
overlay disabling one
