KOGITO-9964: Yard Validation: Masked rows check the mask wrong way ar…

…ound (apache#2068)

packages/yard-validator-worker/src/main/java/org/yard/validator/Parser.java

@@ -38,8 +38,7 @@ public ParserResult parse(final String yaml) {
 
             Logger.log("YaRD model has been read.");
             Logger.log("YaRD model name is " + model.getName());
-            final TreeMap<RowLocation, CustomTreeSet> result
-                    = visit(findTableStartRow(yaml), model);
+            final TreeMap<RowLocation, CustomTreeSet> result = visit(findTableStartRow(yaml), model);
 
             return new ParserResult(result, hitPolicy.toUpperCase());
         } catch (Exception e) {
@@ -72,16 +71,19 @@ private TreeMap<RowLocation, CustomTreeSet> visit(
                 hitPolicy = dt.getHitPolicy();
 
                 final List<Rule> rules = dt.getRules();
-                for (Rule rule : rules) {
-                    final RowLocation location = new RowLocation(
-                            rule.getRowNumber(),
-                            rule.getRowNumber() + rulesRow);
+                for (final Rule rule : rules) {
                     if (rule instanceof WhenThenRule) {
+                        final RowLocation location = new RowLocation(
+                                rule.getRowNumber(),
+                                rule.getRowNumber() * 2 - 1 + rulesRow);
                         final CustomTreeSet keys = getWhenThenKeys(dt, (WhenThenRule) rule, location);
                         if (!keys.isEmpty()) {
                             result.put(location, keys);
                         }
                     } else if (rule instanceof InlineRule) {
+                        final RowLocation location = new RowLocation(
+                                rule.getRowNumber(),
+                                rule.getRowNumber() + rulesRow);
                         final CustomTreeSet keys = getInlineRuleKeys(dt, (InlineRule) rule, location);
                         if (!keys.isEmpty()) {
                             result.put(location, keys);

packages/yard-validator-worker/src/main/java/org/yard/validator/checks/CheckProducer.java

@@ -28,6 +28,9 @@
 import java.util.*;
 import java.util.stream.Collectors;
 
+import static org.yard.validator.checks.Util.getHigherInTable;
+import static org.yard.validator.checks.Util.getLowerInTable;
+
 public class CheckProducer {
 
     public static List<Check> getChecks(final ParserResult parse) {
@@ -91,8 +94,8 @@ private static List<Check> getCheckList(
                 if (i == j) {
                     continue;
                 }
-                final RowLocation locationA = getHigher(locations, i, j);
-                final RowLocation locationB = getLower(locations, i, j);
+                final RowLocation locationA = getHigherInTable(locations, i, j);
+                final RowLocation locationB = getLowerInTable(locations, i, j);
 
                 final CheckItem checkItemA = new CheckItem(
                         locationA.getTableRowNumber(),
@@ -112,25 +115,4 @@ private static List<Check> getCheckList(
         return result;
     }
 
-    private static RowLocation getHigher(
-            final RowLocation[] locations,
-            final int i,
-            final int j) {
-        if (locations[i].getTableRowNumber() > locations[j].getTableRowNumber()) {
-            return locations[i];
-        } else {
-            return locations[j];
-        }
-    }
-
-    private static RowLocation getLower(
-            final RowLocation[] locations,
-            final int i,
-            final int j) {
-        if (locations[i].getTableRowNumber() < locations[j].getTableRowNumber()) {
-            return locations[i];
-        } else {
-            return locations[j];
-        }
-    }
 }

packages/yard-validator-worker/src/main/java/org/yard/validator/checks/SubsumptionCheck.java

@@ -32,43 +32,43 @@ public class SubsumptionCheck
         implements Check {
 
     private final String hitPolicy;
-    private final CheckItem checkItemA;
-    private final CheckItem checkItemB;
+    private final CheckItem higherCheckItem;
+    private final CheckItem lowerCheckItem;
 
     public SubsumptionCheck(
             final String hitPolicy,
-            final CheckItem checkItemA,
-            final CheckItem checkItemB) {
+            final CheckItem higherCheckItem,
+            final CheckItem lowerCheckItem) {
         this.hitPolicy = hitPolicy;
-        this.checkItemA = checkItemA;
-        this.checkItemB = checkItemB;
+        this.higherCheckItem = higherCheckItem;
+        this.lowerCheckItem = lowerCheckItem;
     }
 
     @Override
     public Optional<Issue> check() {
-        final Optional<Issue> aToB = getIssue(checkItemA, checkItemB);
+        final Optional<Issue> aToB = subsumes(higherCheckItem, lowerCheckItem);
 
         if (isFirstHitPolicy()) {
             if (aToB.isPresent()) {
-                // No need to check for redundancy. The first row masks the rest.
+                // No need to check for redundancy. The first row masks the other.
                 return Optional.of(new Issue(
                         "Masking row. The higher row prevents the activation of the other row.",
-                        checkItemA.getLocation(),
-                        checkItemB.getLocation()));
+                        higherCheckItem.getLocation(),
+                        lowerCheckItem.getLocation()));
             } else {
                 // No need to check the other row for subsumption,
                 // since subsumption is likely there by rule design
                 return Optional.empty();
             }
         } else {
-            final Optional<Issue> bToA = getIssue(checkItemB, checkItemA);
+            final Optional<Issue> bToA = subsumes(lowerCheckItem, higherCheckItem);
 
             // If the counterpart in the table is subsumptant, meaning the other item subsumes this, we have redundancy.
             if (aToB.isPresent() && bToA.isPresent()) {
                 return Optional.of(new Issue(
                         getRedundancyMessage(),
-                        checkItemA.getLocation(),
-                        checkItemB.getLocation()));
+                        higherCheckItem.getLocation(),
+                        lowerCheckItem.getLocation()));
             } else if (aToB.isPresent()) {
                 return aToB;
             } else {
@@ -100,7 +100,7 @@ private boolean isFirstHitPolicy() {
                 || Objects.equals("PRIORITY", hitPolicy);
     }
 
-    private Optional<Issue> getIssue(
+    private Optional<Issue> subsumes(
             final CheckItem checkItemA,
             final CheckItem checkItemB) {
         // All values and ranges in A are covered by B

packages/yard-validator-worker/src/main/java/org/yard/validator/checks/Util.java

@@ -0,0 +1,45 @@
+/*
+ * 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.yard.validator.checks;
+
+import org.yard.validator.key.RowLocation;
+
+public class Util {
+    public static RowLocation getHigherInTable(
+            final RowLocation[] locations,
+            final int i,
+            final int j) {
+        if (locations[i].getTableRowNumber() < locations[j].getTableRowNumber()) {
+            return locations[i];
+        } else {
+            return locations[j];
+        }
+    }
+
+    public static RowLocation getLowerInTable(
+            final RowLocation[] locations,
+            final int i,
+            final int j) {
+        if (locations[i].getTableRowNumber() > locations[j].getTableRowNumber()) {
+            return locations[i];
+        } else {
+            return locations[j];
+        }
+    }
+}

packages/yard-validator-worker/src/test/java/org/yard/validator/YardValidatorTest.java

@@ -70,7 +70,7 @@ public void subsumptionTheOtherWay() throws FileNotFoundException {
 
     @Test
     public void maskingRule() throws FileNotFoundException {
-        final String read = read("subsumption-the-other-way.yml", "First");
+        final String read = read("subsumption.yml", "First");
         final YardValidator validator = new YardValidator();
 
         final List<Issue> issues = new ArrayList<>();
@@ -81,6 +81,30 @@ public void maskingRule() throws FileNotFoundException {
         assertIssue(issues.get(0), "Masking row. The higher row prevents the activation of the other row.", 2, 3);
     }
 
+    @Test
+    public void subsumptionButFirstHPMakesItOK() throws FileNotFoundException {
+        final String read = read("subsumption-the-other-way.yml", "First");
+        final YardValidator validator = new YardValidator();
+
+        final List<Issue> issues = new ArrayList<>();
+
+        validator.validate(read, issues::add);
+
+        assertEquals(0, issues.size());
+    }
+
+    @Test
+    public void noIssues() throws FileNotFoundException {
+        final String read = read("package-prices.yml");
+        final YardValidator validator = new YardValidator();
+
+        final List<Issue> issues = new ArrayList<>();
+
+        validator.validate(read, issues::add);
+
+        assertEquals(0, issues.size());
+    }
+
     @Test
     public void redundancy() throws FileNotFoundException {
         final String read = read("redundancy.yml");
@@ -98,6 +122,24 @@ public void redundancy() throws FileNotFoundException {
         assertIssue(issues.get(0), "Redundancy found. If both rows return the same result, the other can be removed. If they return different results, the table fails to return a value.", 1, 2);
     }
 
+    @Test
+    public void checkCorrectIssueRowsWhenThen() throws FileNotFoundException {
+        final String read = read("when-then-redundancy.yml");
+        final YardValidator validator = new YardValidator();
+
+        final List<Issue> issues = new ArrayList<>();
+
+        validator.validate(read, issues::add);
+
+        assertEquals(1, issues.size());
+        final Issue issue = issues.get(0);
+        assertIssue(issue, "Redundancy found. If both rows return the same result, the other can be removed. If they return different results, the table fails to return a value.", 1, 2);
+        final RowLocation first = (RowLocation) issue.getLocations()[0];
+        final RowLocation second = (RowLocation) issue.getLocations()[1];
+        assertEquals(22, first.getActualRowNumberInFile());
+        assertEquals(24, second.getActualRowNumberInFile());
+    }
+
     @Test
     public void redundancyWithUniqueHP() throws FileNotFoundException {
         final String read = read("redundancy.yml", "Unique");
@@ -114,6 +156,7 @@ public void redundancyWithUniqueHP() throws FileNotFoundException {
         assertEquals(1, issues.size());
         assertIssue(issues.get(0), "Redundancy found. Unique hit policy fails when more than one row returns results.", 1, 2);
     }
+
     private void assertIssue(
             final Issue issue,
             final String message,

packages/yard-validator-worker/src/test/resources/org/yard/validator/package-prices.yml

@@ -0,0 +1,30 @@
+specVersion: alpha
+kind: YaRD
+name: "Traffic Violation"
+# expressionLang: FEEL
+inputs:
+  - name: "Length"
+    type: integer
+  - name: "Width"
+    type: number
+  - name: "Height"
+    type: number
+  - name: "Weight"
+    type: number
+elements:
+  - name: Package
+    type: Decision
+    logic:
+      type: DecisionTable
+      # First matching result will be picked
+      hitPolicy: FIRST
+      inputs: ["Height", "Width", "Length", "Weight"]
+      rules:
+        - when: ["<= 3", "<= 25", "<= 35", "<= 2"]
+          then: '{ "Size": "S", "Cost": 10.90 }'
+        - when: ["<= 11", "<= 32", "<= 42", "<=25"]
+          then: '{ "Size": "M", "Cost": 16.90 }'
+        - when: ["<= 19", "<= 36", "<= 60", "<= 25"]
+          then: '{ "Size": "L", "Cost": 18.90 }'
+        - when: ["<= 37", "<= 36", "<= 60", "<= 25"]
+          then: '{ "Size": "XL", "Cost": 25.90}'

packages/yard-validator-worker/src/test/resources/org/yard/validator/when-then-redundancy.yml

@@ -0,0 +1,25 @@
+specVersion: alpha
+kind: YaRD
+name: "Traffic Violation"
+# expressionLang: FEEL
+inputs:
+  - name: "Length"
+    type: integer
+  - name: "Width"
+    type: number
+  - name: "Height"
+    type: number
+  - name: "Weight"
+    type: number
+elements:
+  - name: Package
+    type: Decision
+    logic:
+      type: DecisionTable
+      # First matching result will be picked
+      inputs: ["Height", "Width", "Length", "Weight"]
+      rules:
+        - when: ["<= 3", "<= 25", "<= 35", "<= 2"]
+          then: '{ "Size": "S", "Cost": 10.90 }'
+        - when: ["<= 3", "<= 25", "<= 35", "<= 2"]
+          then: '{ "Size": "S", "Cost": 10.90 }'