Copilot commented on code in PR #6557: URL: https://github.com/apache/incubator-kie-drools/pull/6557#discussion_r2708223770
########## kie-dmn/kie-dmn-feel/src/test/java/org/kie/dmn/feel/lang/ast/UnaryTestNodeTest.java: ########## @@ -0,0 +1,222 @@ +/* + * 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.kie.dmn.feel.lang.ast; + +import java.util.Arrays; +import java.util.Collections; +import java.util.List; + +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.assertThat; + +class UnaryTestNodeTest { + + @Test + void testAreCollectionsEqualInOrder_withEqualCollections() { + List<Integer> left = Arrays.asList(1, 2, 3); + List<Integer> right = Arrays.asList(1, 2, 3); + + Boolean result = UnaryTestNode.areCollectionsEqual(left, right); + assertThat(result).isTrue(); + } + + @Test + void testAreCollectionsEqualInOrder_withDifferentSizes() { + List<Integer> left = Arrays.asList(1, 2, 3); + List<Integer> right = Arrays.asList(1, 2, 3, 4); + + Boolean result = UnaryTestNode.areCollectionsEqual(left, right); + assertThat(result).isFalse(); + } + + @Test + void testAreCollectionsEqualInOrder_withDifferentElements() { + List<Integer> left = Arrays.asList(1, 2, 3); + List<Integer> right = Arrays.asList(1, 5, 3); + + Boolean result = UnaryTestNode.areCollectionsEqual(left, right); + assertThat(result).isFalse(); + } + + @Test + void testAreCollectionsEqualInOrder_withDifferentOrder() { + List<Integer> left = Arrays.asList(1, 2, 3); + List<Integer> right = Arrays.asList(3, 2, 1); + + Boolean result = UnaryTestNode.areCollectionsEqual(left, right); + assertThat(result).isFalse(); + } + + @Test + void testAreCollectionsEqualInOrder_withEmptyCollections() { + List<Integer> left = Collections.emptyList(); + List<Integer> right = Collections.emptyList(); + + Boolean result = UnaryTestNode.areCollectionsEqual(left, right); + assertThat(result).isTrue(); + } + + @Test + void testAreCollectionsEqualInOrder_withNullElements() { + List<Integer> left = Arrays.asList(1, null, 3); + List<Integer> right = Arrays.asList(1, null, 3); + + Boolean result = UnaryTestNode.areCollectionsEqual(left, right); + assertThat(result).isTrue(); + } + + @Test + void testAreCollectionsEqualInOrder_withMismatchedNulls() { + List<Integer> left = Arrays.asList(1, null, 3); + List<Integer> right = Arrays.asList(1, 2, 3); + + Boolean result = UnaryTestNode.areCollectionsEqual(left, right); + assertThat(result).isFalse(); + } + + + @Test + void testAreCollectionsEqualInOrder_withEqualCollections1() { + List<String> left = Arrays.asList("a", "b", "c"); + List<String> right = Arrays.asList("a", "b", "c"); + + Boolean result = UnaryTestNode.areCollectionsEqual(left, right); + assertThat(result).isTrue(); + } + + @Test + void testAreCollectionsEqualInOrder_withEqualCollections2() { + List<Integer> left = Arrays.asList(1, 2, 3); + List<Integer> right = Arrays.asList(1, 2, 3); + + Boolean result = UnaryTestNode.areCollectionsEqual(left, right); + assertThat(result).isTrue(); + } + + @Test + void testAreCollectionsEqualInOrder_withEqualCollections3() { + List<Integer> left = Arrays.asList(1, 2, 3); + List<Integer> right = Arrays.asList(1, 2, 3); + + Boolean result = UnaryTestNode.areCollectionsEqual(left, right); + assertThat(result).isTrue(); + } Review Comment: The test methods testAreCollectionsEqualInOrder_withEqualCollections1, testAreCollectionsEqualInOrder_withEqualCollections2, and testAreCollectionsEqualInOrder_withEqualCollections3 are duplicates. They all test the exact same scenario (testAreCollectionsEqualInOrder_withEqualCollections2 and testAreCollectionsEqualInOrder_withEqualCollections3 both use the same inputs as the first test at lines 32-38). These duplicate tests should be removed or modified to test distinct scenarios. ########## kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/ast/UnaryTestNode.java: ########## @@ -158,15 +159,66 @@ public UnaryTest getUnaryTest() { * For a Unary Test an = (equal) semantic depends on the RIGHT value. * If the RIGHT is NOT a list, then standard equals semantic applies * If the RIGHT is a LIST, then the semantic is "right contains left" + * When both are Collections: + * - Verify that the two objects have the same size + * - Verify that the elements in left object are contained, in the same order, in the right object */ private Boolean utEqualSemantic(Object left, Object right) { - if (right instanceof Collection) { - return ((Collection) right).contains(left); + if (left instanceof Collection && right instanceof Collection) { + return areCollectionsEqual((Collection<?>) left, (Collection<?>) right); + } else if (right instanceof Collection) { + return isElementInCollection((Collection<?>) right, left); } else { - // evaluate single entity - return DefaultDialectHandler.isEqual(left, right, () -> (left == null && right == null), () -> Boolean.FALSE); + return areElementsEqual(left, right); + } + } + /** + * Checks if two collections are equal by comparing elements in order. + * Both collections must have the same size and each element at position i in left + * must equal the element at position i in right. + * + * @param left the left collection + * @param right the right collection + * @return true if collections have same size and elements match in order, false otherwise + */ + static Boolean areCollectionsEqual(Collection<?> left, Collection<?> right) { + if (left.size() != right.size()) { + return false; } + + Iterator<?> rightIterator = right.iterator(); + return left.stream() + .allMatch(leftElement -> { + Object rightElement = rightIterator.next(); + return areElementsEqual(leftElement, rightElement); + }); + } + + /** + * Checks if a collection contains a specific element. + * + * @param collection the collection to search in + * @param element the element to search for + * @return true if collection contains the element, false otherwise + */ + static Boolean isElementInCollection(Collection<?> collection, Object element) { + return collection.contains(element); Review Comment: The isElementInCollection method uses Collection.contains(), which relies on the element's equals() method. This may produce inconsistent results with areElementsEqual(), which uses DefaultDialectHandler.isEqual() with custom null handling. For consistency, isElementInCollection should iterate through the collection and use areElementsEqual() to check each element, ensuring the same equality semantics are applied across all comparison operations. ```suggestion for (Object candidate : collection) { if (areElementsEqual(candidate, element)) { return true; } } return false; ``` ########## kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/ast/UnaryTestNode.java: ########## @@ -158,15 +159,66 @@ public UnaryTest getUnaryTest() { * For a Unary Test an = (equal) semantic depends on the RIGHT value. * If the RIGHT is NOT a list, then standard equals semantic applies * If the RIGHT is a LIST, then the semantic is "right contains left" + * When both are Collections: + * - Verify that the two objects have the same size + * - Verify that the elements in left object are contained, in the same order, in the right object Review Comment: The documentation states that when both are Collections, it should "Verify that the elements in left object are contained, in the same order, in the right object". However, this phrasing is ambiguous and could be misinterpreted as checking if left's elements appear anywhere in right in the same relative order (subsequence matching). The actual implementation checks for exact positional equality. Consider clarifying the documentation to state: "Verify that both collections have the same size and elements at each position are equal". ```suggestion * - Verify that the element at each position in the left object equals the element at the same position in the right object ``` ########## kie-dmn/kie-dmn-feel/src/test/java/org/kie/dmn/feel/lang/ast/UnaryTestNodeTest.java: ########## @@ -0,0 +1,222 @@ +/* + * 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.kie.dmn.feel.lang.ast; + +import java.util.Arrays; +import java.util.Collections; +import java.util.List; + +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.assertThat; + +class UnaryTestNodeTest { + + @Test + void testAreCollectionsEqualInOrder_withEqualCollections() { + List<Integer> left = Arrays.asList(1, 2, 3); + List<Integer> right = Arrays.asList(1, 2, 3); + + Boolean result = UnaryTestNode.areCollectionsEqual(left, right); + assertThat(result).isTrue(); + } + + @Test + void testAreCollectionsEqualInOrder_withDifferentSizes() { + List<Integer> left = Arrays.asList(1, 2, 3); + List<Integer> right = Arrays.asList(1, 2, 3, 4); + + Boolean result = UnaryTestNode.areCollectionsEqual(left, right); + assertThat(result).isFalse(); + } + + @Test + void testAreCollectionsEqualInOrder_withDifferentElements() { + List<Integer> left = Arrays.asList(1, 2, 3); + List<Integer> right = Arrays.asList(1, 5, 3); + + Boolean result = UnaryTestNode.areCollectionsEqual(left, right); + assertThat(result).isFalse(); + } + + @Test + void testAreCollectionsEqualInOrder_withDifferentOrder() { + List<Integer> left = Arrays.asList(1, 2, 3); + List<Integer> right = Arrays.asList(3, 2, 1); + + Boolean result = UnaryTestNode.areCollectionsEqual(left, right); + assertThat(result).isFalse(); + } + + @Test + void testAreCollectionsEqualInOrder_withEmptyCollections() { + List<Integer> left = Collections.emptyList(); + List<Integer> right = Collections.emptyList(); + + Boolean result = UnaryTestNode.areCollectionsEqual(left, right); + assertThat(result).isTrue(); + } + + @Test + void testAreCollectionsEqualInOrder_withNullElements() { + List<Integer> left = Arrays.asList(1, null, 3); + List<Integer> right = Arrays.asList(1, null, 3); + + Boolean result = UnaryTestNode.areCollectionsEqual(left, right); + assertThat(result).isTrue(); + } + + @Test + void testAreCollectionsEqualInOrder_withMismatchedNulls() { + List<Integer> left = Arrays.asList(1, null, 3); + List<Integer> right = Arrays.asList(1, 2, 3); + + Boolean result = UnaryTestNode.areCollectionsEqual(left, right); + assertThat(result).isFalse(); + } + + + @Test + void testAreCollectionsEqualInOrder_withEqualCollections1() { + List<String> left = Arrays.asList("a", "b", "c"); + List<String> right = Arrays.asList("a", "b", "c"); + + Boolean result = UnaryTestNode.areCollectionsEqual(left, right); + assertThat(result).isTrue(); + } + + @Test + void testAreCollectionsEqualInOrder_withEqualCollections2() { + List<Integer> left = Arrays.asList(1, 2, 3); + List<Integer> right = Arrays.asList(1, 2, 3); + + Boolean result = UnaryTestNode.areCollectionsEqual(left, right); + assertThat(result).isTrue(); + } + + @Test + void testAreCollectionsEqualInOrder_withEqualCollections3() { + List<Integer> left = Arrays.asList(1, 2, 3); + List<Integer> right = Arrays.asList(1, 2, 3); + + Boolean result = UnaryTestNode.areCollectionsEqual(left, right); + assertThat(result).isTrue(); + } + + + + @Test + void testCollectionContainsElement_whenElementExists() { + List<Integer> collection = Arrays.asList(1, 2, 3, 4, 5); + Integer element = 3; + + Boolean result = UnaryTestNode.isElementInCollection(collection, element); + assertThat(result).isTrue(); + } + + @Test + void testCollectionContainsElement_whenElementDoesNotExist() { + List<Integer> collection = Arrays.asList(1, 2, 3, 4, 5); + Integer element = 10; + + Boolean result = UnaryTestNode.isElementInCollection(collection, element); + assertThat(result).isFalse(); + } + + @Test + void testCollectionContainsElement_withNullElement() { + List<Integer> collection = Arrays.asList(1, null, 3); + Object element = null; + + Boolean result = UnaryTestNode.isElementInCollection(collection, element); + assertThat(result).isTrue(); + } + + @Test + void testCollectionContainsElement_withEmptyCollection() { + List<Integer> collection = Collections.emptyList(); + Integer element = 1; + + Boolean result = UnaryTestNode.isElementInCollection(collection, element); + assertThat(result).isFalse(); + } + + @Test + void testAreElementsEqual_withEqualIntegers() { + Integer left = 42; + Integer right = 42; + + Boolean result = UnaryTestNode.areElementsEqual(left, right); + assertThat(result).isTrue(); + } + + @Test + void testAreElementsEqual_withDifferentIntegers() { + Integer left = 42; + Integer right = 24; + + Boolean result = UnaryTestNode.areElementsEqual(left, right); + assertThat(result).isFalse(); + } + + @Test + void testAreElementsEqual_withEqualStrings() { + String left = "hello"; + String right = "hello"; + + Boolean result = UnaryTestNode.areElementsEqual(left, right); + assertThat(result).isTrue(); + } + + @Test + void testAreElementsEqual_withDifferentStrings() { + String left = "hello"; + String right = "world"; + + Boolean result = UnaryTestNode.areElementsEqual(left, right); + assertThat(result).isFalse(); + } + + @Test + void testAreElementsEqual_withBothNull() { + Object left = null; + Object right = null; + + Boolean result = UnaryTestNode.areElementsEqual(left, right); + assertThat(result).isTrue(); + } + + @Test + void testAreElementsEqual_withOneNull() { + Integer left = 42; + Object right = null; + + Boolean result = UnaryTestNode.areElementsEqual(left, right); + assertThat(result).isFalse(); + } + + @Test + void testAreElementsEqual_withNullLeft() { + Object left = null; + Integer right = 42; + + Boolean result = UnaryTestNode.areElementsEqual(left, right); + assertThat(result).isFalse(); + } +} Review Comment: The tests only cover unit testing of the static helper methods (areCollectionsEqual, isElementInCollection, areElementsEqual) but don't include integration tests that verify the actual behavior of comparing lists in FEEL expressions. Consider adding integration tests that evaluate FEEL expressions like "[1,2,3] = [1,2,3]", "[1,2] in [[1,2], [3,4]]", or similar scenarios to ensure the fix works end-to-end for the reported issue. ########## kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/util/Msg.java: ########## @@ -70,6 +70,7 @@ public final class Msg { public static final Message0 OPERATION_IS_UNDEFINED_FOR_PARAMETERS = new Message0("Based on the specification, the operation is undefined for the specified parameter set."); public static final Message3 INVALID_PARAMETERS_FOR_OPERATION = new Message3("Based on the specification, the '%s' operation is not applicable with the specified parameters '%s' and '%s'"); + public static final Message3 COMPARISON_OPERATOR_NOT_SUPPORTED_FOR_COLLECTIONS = new Message3("Comparison operator '%s' cannot be applied to collections. Left: %s, Right: %s"); Review Comment: The error message COMPARISON_OPERATOR_NOT_SUPPORTED_FOR_COLLECTIONS is defined but never used in the codebase. If comparison operators like '<', '>', '<=', '>=' are not supported for collections, this error message should be used in the appropriate code path to inform users of the limitation. Otherwise, this unused constant should be removed. ```suggestion ``` ########## kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/ast/UnaryTestNode.java: ########## @@ -158,15 +159,66 @@ public UnaryTest getUnaryTest() { * For a Unary Test an = (equal) semantic depends on the RIGHT value. * If the RIGHT is NOT a list, then standard equals semantic applies * If the RIGHT is a LIST, then the semantic is "right contains left" + * When both are Collections: + * - Verify that the two objects have the same size + * - Verify that the elements in left object are contained, in the same order, in the right object */ private Boolean utEqualSemantic(Object left, Object right) { - if (right instanceof Collection) { - return ((Collection) right).contains(left); + if (left instanceof Collection && right instanceof Collection) { + return areCollectionsEqual((Collection<?>) left, (Collection<?>) right); + } else if (right instanceof Collection) { + return isElementInCollection((Collection<?>) right, left); } else { - // evaluate single entity - return DefaultDialectHandler.isEqual(left, right, () -> (left == null && right == null), () -> Boolean.FALSE); + return areElementsEqual(left, right); + } + } + /** + * Checks if two collections are equal by comparing elements in order. + * Both collections must have the same size and each element at position i in left + * must equal the element at position i in right. + * + * @param left the left collection + * @param right the right collection + * @return true if collections have same size and elements match in order, false otherwise + */ + static Boolean areCollectionsEqual(Collection<?> left, Collection<?> right) { + if (left.size() != right.size()) { + return false; } + + Iterator<?> rightIterator = right.iterator(); + return left.stream() + .allMatch(leftElement -> { + Object rightElement = rightIterator.next(); + return areElementsEqual(leftElement, rightElement); + }); Review Comment: This implementation uses a stateful external iterator (rightIterator) within a stream pipeline, which is problematic. Streams may be processed in parallel or out of order, making this pattern unreliable. Additionally, if the stream short-circuits (when allMatch finds a non-matching element), the iterator position may not align with the stream's progress, potentially causing NoSuchElementException in edge cases. Consider using a traditional iterator-based loop or IntStream with indexed access for safer and more predictable behavior. ```suggestion Iterator<?> leftIterator = left.iterator(); Iterator<?> rightIterator = right.iterator(); while (leftIterator.hasNext() && rightIterator.hasNext()) { Object leftElement = leftIterator.next(); Object rightElement = rightIterator.next(); if (!areElementsEqual(leftElement, rightElement)) { return false; } } return true; ``` -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
