You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hop.apache.org by "bamaer (via GitHub)" <gi...@apache.org> on 2026/10/12 14:11:30 UTC
[PR] Fixes #8871 : Lint rule editor and Rules Manager: clause validation, … (hop)
bamaer opened a new pull request, #8877:
URL: https://github.com/apache/hop/pull/8877
…saved values, Rule ID, pack rule overrides, sorting
**Please** add a meaningful description for your change here
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [x] Run `mvn clean install apache-rat:check` to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
- [x] If you have a group of commits related to the same change, please squash your commits into one and force push your branch using `git rebase -i`.
- [x] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable.
To make clear that you license your contribution under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [x] I hereby declare this contribution to be licensed under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [ ] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes #8871 : Lint rule editor and Rules Manager: clause validation, … (hop)
Posted by "mattcasters (via GitHub)" <gi...@apache.org>.
mattcasters commented on code in PR #8877:
URL: https://github.com/apache/hop/pull/8877#discussion_r4249528191
##########
plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/ProjectLintYamlExporter.java:
##########
@@ -98,9 +124,11 @@ private static boolean differsFromPackDefault(
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())
Review Comment:
**[bug]** A value change is only noticed on the rule's first clause. `differsFromPackDefault` compares `getConditionValue()`, and `toOverrideMap` (line 188) writes that one key. `clauseShape`, which `structurallyDiffersFromPackDefault` uses, is `field|condition` and ignores values, and an override block cannot carry a second clause anyway (`allOf` / `anyOf` is parsed as a full custom rule). Pack rule SQL-002 is `allOf` with a `sql` pattern and a `rowLimit` pattern. Changing only the row-limit pattern leaves every compared field equal to the pack default, so `entryFor` returns null. `putRule` then either leaves `hop-lint.yml` untouched or, when that null entry is the whole edit, deletes the rule's project block. `RuleManagerDialog` line 490 treats a non-throwing save as success and shows the new value until the manager is reopened. A name or severity edit in the same save keeps those overrides and still drops the later clause's value. The new registry test only changes `TRANS-002`'
s field, so this path is untested.
**Suggestion:** If any clause after the first has a different `conditionValue` than the pack clause, treat it as a redefinition (`redefinesPackRule` / `toFullCustomRuleMap`) so the confirm dialog runs and every clause is written. Add a test that changes SQL-002's second clause value and asserts `type: custom` plus both patterns, and that a first-clause-only value change stays an override.
##########
plugins/misc/lint/src/main/java/org/apache/hop/lint/RuleBuilderDialog.java:
##########
@@ -542,15 +594,94 @@ private RuleCondition selectedCondition() {
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. Back on the rule's own target, an emptied clause gets the rule's clause
+ * back, so a trip to another target does not lose 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;
+ }
+ shownTarget = targetIndex;
+ RuleTarget target = RuleTarget.values()[targetIndex];
+ List<String> fields = RuleTargetFields.getFieldsForTarget(target);
+ List<RuleClause> original = target == rule.getTarget() ? rule.getClauses() : List.of();
Review Comment:
**[suggestion]** Coming back to the rule's own target restores an emptied clause from `rule.getClauses()`, which is the rule as the dialog was opened, not the clause the user had just edited. `saveRule` is what copies the table back onto the rule, so an edit to a Table Input `sql` pattern, or a field change away from `sql`, is thrown away if that clause is cleared on the way through another target and then restored. The table shows the old clause before OK, but the round trip undoes work the user already did in this dialog.
**Suggestion:** When a clause is about to be cleared, remember that editing clause and restore the snapshot. `rule.getClauses()` is the right fallback only for a clause that was never edited.
##########
plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/ProjectYamlOverlay.java:
##########
@@ -112,9 +121,18 @@ public static ProjectRuleOverlay fromMap(
Object value = ruleData.get("helpUri");
helpUri = value != null ? value.toString().trim() : "";
}
+ // Renaming a pack rule used to write it out in full, which replaced the pack rule for good.
Review Comment:
**[suggestion]** The new comment records the old bug ("Renaming a pack rule used to write it out in full...") instead of a constraint the next reader cannot see in the code. The same history shows up on the clause loop (`RuleBuilderDialog` line 722: an empty extra clause used to pass and then NPE) and on `LintPolicyYamlWriter.putRule` ("Removing first and writing second lost the rule...").
**Suggestion:** Drop the history. Where a comment is still needed, state the invariant only, for example that a `name` key on an override renames the pack rule without copying it.
##########
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() {
Review Comment:
**[suggestion]** `anEmptyClauseIsIncomplete` only calls `clauseProblem` on single clauses. The bug it names was that `validate()` checked the clause on show and let an empty extra row reach save. Deleting the loop at `RuleBuilderDialog` line 725 and going back to checking the selected row would not fail this test, or any UI test in the PR.
**Suggestion:** Drive `validate()` with two clauses, the visible one complete and the other empty, and assert it rejects the rule and selects the empty row. The same for a value required on a non-selected row (`MATCHES_PATTERN` with an empty value) and for `NOT_NULL` with a leftover value, which must still pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes #8871 : Lint rule editor and Rules Manager: clause validation, … (hop)
Posted by "bamaer (via GitHub)" <gi...@apache.org>.
bamaer commented on code in PR #8877:
URL: https://github.com/apache/hop/pull/8877#discussion_r4250909133
##########
plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/ProjectLintYamlExporter.java:
##########
@@ -98,9 +124,11 @@ private static boolean differsFromPackDefault(
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())
Review Comment:
Fixed. `clauseShape` now includes the values of every clause after the first, so changing one makes the rule a redefinition: the Rules Manager asks first and the whole rule is written with every clause. A first-clause value change stays an override. Tests: `aChangedValueInALaterClauseRedefinesTheRule` changes SQL-002's row-limit pattern and reads the file back with both patterns, and `aChangedValueInTheFirstClauseStaysAnOverride`. The docs page on rule packs says the same.
##########
plugins/misc/lint/src/main/java/org/apache/hop/lint/RuleBuilderDialog.java:
##########
@@ -542,15 +594,94 @@ private RuleCondition selectedCondition() {
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. Back on the rule's own target, an emptied clause gets the rule's clause
+ * back, so a trip to another target does not lose 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;
+ }
+ shownTarget = targetIndex;
+ RuleTarget target = RuleTarget.values()[targetIndex];
+ List<String> fields = RuleTargetFields.getFieldsForTarget(target);
+ List<RuleClause> original = target == rule.getTarget() ? rule.getClauses() : List.of();
Review Comment:
Fixed. A target change that empties a clause now keeps a copy of that editing clause and its target. Going back to that target restores the copy while the clause is still empty, and `rule.getClauses()` is no longer used. Test: `backOnItsOwnTargetAClauseKeepsItsEdits`.
##########
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() {
Review Comment:
Added SWTBot tests that drive `validate()` through OK, with errors collected through a package-private `errorReporter` because a native message box can't be driven from a test: `anEmptyClauseThatIsNotOnShowIsRefused` (also asserts the empty row is selected), `aMissingValueInAClauseThatIsNotOnShowIsRefused` and `aLeftoverValueOnAConditionWithoutValueIsDropped`. Checking only the selected row makes the first two fail.
##########
plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/ProjectYamlOverlay.java:
##########
@@ -112,9 +121,18 @@ public static ProjectRuleOverlay fromMap(
Object value = ruleData.get("helpUri");
helpUri = value != null ? value.toString().trim() : "";
}
+ // Renaming a pack rule used to write it out in full, which replaced the pack rule for good.
Review Comment:
Done. The history is gone from this comment and from the others like it in the PR (the clause loop, `putRule`, the Rules Manager and the exporter); where a comment is still needed it states the invariant.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes #8871 : Lint rule editor and Rules Manager: clause validation, … (hop)
Posted by "hansva (via GitHub)" <gi...@apache.org>.
hansva merged PR #8877:
URL: https://github.com/apache/hop/pull/8877
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org