From 8122a60d490b2bba5e7f2d29c18c52219ce06658 Mon Sep 17 00:00:00 2001 From: Khalid Qarryzada Date: Fri, 9 Aug 2024 15:36:05 -0500 Subject: [PATCH] Update equals() for combining filters Resolved an issue where combining filters were not evaluating equivalency correctly since the size of the lists were not compared. Reviewer: vyhhuang Reviewer: dougbulkley JiraIssue: DS-49092 --- CHANGELOG.md | 2 + .../scim2/common/filters/AndFilter.java | 9 ++- .../scim2/common/filters/CombiningFilter.java | 3 +- .../scim2/common/filters/OrFilter.java | 9 ++- .../scim2/common/FilterEvaluatorTestCase.java | 65 +++++++++++++++++++ 5 files changed, 76 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c97780bd..cedc9b57 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,8 @@ All notable changes to this project will be documented in this file. This project adheres to [Semantic Versioning](https://semver.org/). ## v3.1.1 - TBD +Fixed an issue where `AndFilter.equals()` and `OrFilter.equals()` could incorrectly evaluate to +true. ## v3.1.0 - 2024-Jun-25 Updated all classes within the UnboundID SCIM 2 SDK to utilize `@Nullable` and `@NotNull` diff --git a/scim2-sdk-common/src/main/java/com/unboundid/scim2/common/filters/AndFilter.java b/scim2-sdk-common/src/main/java/com/unboundid/scim2/common/filters/AndFilter.java index 6ec4f9f4..37a5e60d 100644 --- a/scim2-sdk-common/src/main/java/com/unboundid/scim2/common/filters/AndFilter.java +++ b/scim2-sdk-common/src/main/java/com/unboundid/scim2/common/filters/AndFilter.java @@ -101,19 +101,18 @@ public boolean equals(@Nullable final Object o) { return true; } - if (o == null || getClass() != o.getClass()) + if (!(o instanceof AndFilter)) { return false; } - CombiningFilter that = (CombiningFilter) o; - - if (!getCombinedFilters().containsAll(that.getCombinedFilters())) + AndFilter that = (AndFilter) o; + if (getCombinedFilters().size() != that.getCombinedFilters().size()) { return false; } - return true; + return getCombinedFilters().containsAll(that.getCombinedFilters()); } /** diff --git a/scim2-sdk-common/src/main/java/com/unboundid/scim2/common/filters/CombiningFilter.java b/scim2-sdk-common/src/main/java/com/unboundid/scim2/common/filters/CombiningFilter.java index 9a7cafd8..e362fc77 100644 --- a/scim2-sdk-common/src/main/java/com/unboundid/scim2/common/filters/CombiningFilter.java +++ b/scim2-sdk-common/src/main/java/com/unboundid/scim2/common/filters/CombiningFilter.java @@ -18,7 +18,6 @@ package com.unboundid.scim2.common.filters; import com.unboundid.scim2.common.annotations.NotNull; -import com.unboundid.scim2.common.annotations.Nullable; import java.util.Collections; import java.util.List; @@ -66,7 +65,7 @@ public boolean isCombiningFilter() * {@inheritDoc} */ @Override - @Nullable + @NotNull public List getCombinedFilters() { return filterComponents; diff --git a/scim2-sdk-common/src/main/java/com/unboundid/scim2/common/filters/OrFilter.java b/scim2-sdk-common/src/main/java/com/unboundid/scim2/common/filters/OrFilter.java index 0822539b..97f19356 100644 --- a/scim2-sdk-common/src/main/java/com/unboundid/scim2/common/filters/OrFilter.java +++ b/scim2-sdk-common/src/main/java/com/unboundid/scim2/common/filters/OrFilter.java @@ -100,19 +100,18 @@ public boolean equals(@Nullable final Object o) { return true; } - if (o == null || getClass() != o.getClass()) + if (!(o instanceof OrFilter)) { return false; } - CombiningFilter that = (CombiningFilter) o; - - if (!getCombinedFilters().containsAll(that.getCombinedFilters())) + OrFilter that = (OrFilter) o; + if (getCombinedFilters().size() != that.getCombinedFilters().size()) { return false; } - return true; + return getCombinedFilters().containsAll(that.getCombinedFilters()); } /** diff --git a/scim2-sdk-common/src/test/java/com/unboundid/scim2/common/FilterEvaluatorTestCase.java b/scim2-sdk-common/src/test/java/com/unboundid/scim2/common/FilterEvaluatorTestCase.java index fdd94ddc..f5e3d278 100644 --- a/scim2-sdk-common/src/test/java/com/unboundid/scim2/common/FilterEvaluatorTestCase.java +++ b/scim2-sdk-common/src/test/java/com/unboundid/scim2/common/FilterEvaluatorTestCase.java @@ -32,6 +32,7 @@ import java.util.Date; import java.util.TimeZone; +import static org.assertj.core.api.Assertions.assertThat; import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertFalse; import static org.testng.Assert.assertTrue; @@ -337,6 +338,70 @@ public void testGreaterOrEqualFilter() throws Exception } + /** + * Validates the {@link com.unboundid.scim2.common.filters.AndFilter#equals} + * and {@link com.unboundid.scim2.common.filters.OrFilter#equals} methods. + */ + @Test + @SuppressWarnings({"EqualsWithItself", "ConstantValue"}) + public void testCombiningFilterEquals() throws Exception + { + Filter andFilter = Filter.and( + Filter.eq("userName", "Ganon"), + Filter.eq("nickName", "Ganon") + ); + Filter orFilter = Filter.or( + Filter.eq("userName", "Ganon"), + Filter.eq("nickName", "Ganon") + ); + + // A filter instance should always be equivalent to itself. + assertThat(andFilter.equals(andFilter)).isTrue(); + assertThat(orFilter.equals(orFilter)).isTrue(); + + // An initialized AND/OR filter should never be equivalent to 'null'. + assertThat(andFilter.equals(null)).isFalse(); + assertThat(orFilter.equals(null)).isFalse(); + + // An AND filter should not be equivalent to an OR filter, even if it has + // the same subordinate filters. + assertThat(andFilter.equals(orFilter)).isFalse(); + assertThat(orFilter.equals(andFilter)).isFalse(); + + // When the one filter is a superset of the other filter, the filters should + // not be considered equivalent. + Filter andFilterSuperset = Filter.and( + Filter.eq("userName", "Ganon"), + Filter.eq("nickName", "Ganon"), + Filter.sw("title", "Mr") + ); + assertThat(andFilter.equals(andFilterSuperset)).isFalse(); + assertThat(andFilterSuperset.equals(andFilter)).isFalse(); + + Filter orFilterSuperset = Filter.or( + Filter.eq("userName", "Ganon"), + Filter.eq("nickName", "Ganon"), + Filter.sw("title", "Mr") + ); + assertThat(orFilter.equals(orFilterSuperset)).isFalse(); + assertThat(orFilterSuperset.equals(orFilter)).isFalse(); + + // The order of the subordinate filters should not affect equivalency. + Filter andFilterDifferentOrder = Filter.and( + Filter.eq("nickName", "Ganon"), + Filter.eq("userName", "Ganon") + ); + assertThat(andFilter.equals(andFilterDifferentOrder)).isTrue(); + assertThat(andFilterDifferentOrder.equals(andFilter)).isTrue(); + Filter orFilterDifferentOrder = Filter.or( + Filter.eq("nickName", "Ganon"), + Filter.eq("userName", "Ganon") + ); + assertThat(orFilter.equals(orFilterDifferentOrder)).isTrue(); + assertThat(orFilterDifferentOrder.equals(orFilter)).isTrue(); + } + + /** * Test that filters matching.