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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
