diff --git a/CHANGELOG.md b/CHANGELOG.md index 357c0365..a00a45dc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,12 +2,33 @@ 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 +## v3.2.0 - TBD Fixed an issue where `AndFilter.equals()` and `OrFilter.equals()` could incorrectly evaluate to true. Updated Jackson dependencies to 2.17.2. +Added a property that allows ADD patch operations with value filters to target an existing value. +For example, for the following patch request: +```json +{ + "schemas": [ "urn:ietf:params:scim:api:messages:2.0:PatchOp" ], + "Operations": [ + { + "op": "add", + "path": "emails[type eq \"work\"].display", + "value": "apollo.j@example.com" + } + ] +} +``` +When the new `PatchOperation.APPEND_NEW_PATCH_VALUES_PROPERTY` property is set to `false`, this +operation will search the resource for an existing "work" email and add the value to that email. +This allows for better integration with SCIM provisioners that send individual requests such as +`emails[type eq "work"].display` followed by `emails[type eq "work"].value`, which are intended to +target the same email. The default value of `PatchOperation.APPEND_NEW_PATCH_VALUES_PROPERTY` is +`true`, which will always create a new value on the multi-valued attribute. + ## v3.1.0 - 2024-Jun-25 Updated all classes within the UnboundID SCIM 2 SDK to utilize `@Nullable` and `@NotNull` annotations for all non-primitive input parameters, member variables, and return values. These diff --git a/pom.xml b/pom.xml index a7d8b812..ef2f4ae6 100644 --- a/pom.xml +++ b/pom.xml @@ -18,7 +18,7 @@ 4.0.0 com.unboundid.product.scim2 scim2-parent - 3.1.1-SNAPSHOT + 3.2.0-SNAPSHOT pom UnboundID SCIM2 SDK Parent diff --git a/scim2-assembly/pom.xml b/scim2-assembly/pom.xml index e58e7aae..f3388754 100644 --- a/scim2-assembly/pom.xml +++ b/scim2-assembly/pom.xml @@ -19,7 +19,7 @@ scim2-parent com.unboundid.product.scim2 - 3.1.1-SNAPSHOT + 3.2.0-SNAPSHOT scim2-assembly pom diff --git a/scim2-sdk-client/pom.xml b/scim2-sdk-client/pom.xml index dcf5cdcf..7d32fdac 100644 --- a/scim2-sdk-client/pom.xml +++ b/scim2-sdk-client/pom.xml @@ -19,7 +19,7 @@ scim2-parent com.unboundid.product.scim2 - 3.1.1-SNAPSHOT + 3.2.0-SNAPSHOT scim2-sdk-client jar diff --git a/scim2-sdk-common/pom.xml b/scim2-sdk-common/pom.xml index 84fb74ff..485880f0 100644 --- a/scim2-sdk-common/pom.xml +++ b/scim2-sdk-common/pom.xml @@ -19,7 +19,7 @@ scim2-parent com.unboundid.product.scim2 - 3.1.1-SNAPSHOT + 3.2.0-SNAPSHOT scim2-sdk-common jar diff --git a/scim2-sdk-common/src/main/java/com/unboundid/scim2/common/messages/PatchOperation.java b/scim2-sdk-common/src/main/java/com/unboundid/scim2/common/messages/PatchOperation.java index 32d679f5..b9b62c18 100644 --- a/scim2-sdk-common/src/main/java/com/unboundid/scim2/common/messages/PatchOperation.java +++ b/scim2-sdk-common/src/main/java/com/unboundid/scim2/common/messages/PatchOperation.java @@ -42,6 +42,7 @@ import com.unboundid.scim2.common.filters.EqualFilter; import com.unboundid.scim2.common.filters.Filter; import com.unboundid.scim2.common.filters.FilterType; +import com.unboundid.scim2.common.utils.FilterEvaluator; import com.unboundid.scim2.common.utils.JsonUtils; import com.unboundid.scim2.common.utils.SchemaUtils; @@ -215,7 +216,6 @@ public void apply(@NotNull final ObjectNode node) throws ScimException Path path = (getPath() == null) ? Path.root() : getPath(); if (hasValueFilter(path)) { - validateAddOpWithFilter(path, value); applyAddWithValueFilter(path, node, value); } else @@ -377,8 +377,10 @@ private void applyAddWithValueFilter( @NotNull final Path path, @NotNull final ObjectNode existingResource, @NotNull final JsonNode value) - throws BadRequestException + throws ScimException { + validateAddOpWithFilter(path, value); + Filter valueFilter = path.getElement(0).getValueFilter(); String filterAttributeName = valueFilter.getAttributePath().toString(); ValueNode filterValue = valueFilter.getComparisonValue(); @@ -391,8 +393,8 @@ private void applyAddWithValueFilter( JsonNode jsonAttribute = existingResource.get(attributeName); if (jsonAttribute == null) { - // There are no existing values for the attribute, so we should add this - // value ourselves. + // There are no existing values for the attribute, so we should + // prepare to add this value ourselves. jsonAttribute = JsonUtils.getJsonNodeFactory().arrayNode(1); } if (!jsonAttribute.isArray()) @@ -405,13 +407,101 @@ private void applyAddWithValueFilter( } ArrayNode attribute = (ArrayNode) jsonAttribute; - // Construct the new attribute value that should be added to the resource. - ObjectNode newValue = JsonUtils.getJsonNodeFactory().objectNode(); - newValue.set(subAttributeName, value); - newValue.set(filterAttributeName, filterValue); + // When operations with a value filter add data, we can either append + // the data to a new value in the multi-valued attribute, or we can update + // an existing value. + // + // If the APPEND_NEW_PATCH_VALUES_PROPERTY property is enabled, the + // provided data should be added as a new value regardless of the existing + // resource's state (so we pretend that there are no matched values). + // Otherwise, any new data should update the existing value, if it is + // present. + ObjectNode matchedValue = null; + if (!APPEND_NEW_PATCH_VALUES_PROPERTY) + { + matchedValue = fetchExistingValue(attribute, valueFilter, attributeName); + } + + // If there are no existing values that match the filter, or if no values + // were fetched, then we should add the two attribute values to the array. + // In the former case, this indicates that something like a home address + // does not yet exist on the resource, so a new one should be added. + if (matchedValue == null) + { + ObjectNode newValue = JsonUtils.getJsonNodeFactory().objectNode(); + newValue.set(subAttributeName, value); + newValue.set(filterAttributeName, filterValue); + + attribute.add(newValue); + existingResource.replace(attributeName, attribute); + return; + } + + // Ensure that the data to be added is not already populated on the + // resource. For example, adding the "locality" sub-attribute to a home + // address should not be allowed if the home address already has a + // locality defined. + if (FilterEvaluator.evaluate(Filter.pr(subAttributeName), matchedValue)) + { + throw BadRequestException.invalidValue(String.format( + "The add operation attempted to add a new '%s' field, but the" + + " specified path already has a '%s' defined.", + subAttributeName, + subAttributeName + )); + } - attribute.add(newValue); - existingResource.replace(attributeName, attribute); + matchedValue.set(subAttributeName, value); + } + + /** + * Checks a multi-valued attribute for an existing value and returns a value + * based on the following conditions: + * + * + * @param attribute The multi-valued attribute. + * @param valueFilter The value selection filter provided with the patch + * add operation. + * @param attributeName The name of {@code attribute}. + * + * @return An ObjectNode representing the single value that matched the + * criteria of the {@code valueFilter}, or {@code null} if no + * attributes matched the filter. + * + * @throws ScimException If there was an error processing the filter, or + * if multiple values were matched. + */ + @Nullable + private static ObjectNode fetchExistingValue( + @NotNull final ArrayNode attribute, + @NotNull final Filter valueFilter, + @NotNull final String attributeName) + throws ScimException + { + ObjectNode matchedValue = null; + + for (var arrayVal : attribute) + { + if (FilterEvaluator.evaluate(valueFilter, arrayVal)) + { + if (matchedValue != null) + { + throw BadRequestException.noTarget( + "The operation could not be applied on the resource because the" + + " value filter matched more than one element in the '" + + attributeName + "' array of the resource."); + } + matchedValue = (ObjectNode) arrayVal; + } + } + + return matchedValue; } /** @@ -459,6 +549,7 @@ public int hashCode() result = 31 * result + value.hashCode(); return result; } + } static final class RemoveOperation extends PatchOperation @@ -1948,7 +2039,7 @@ private static void validateOperationValue(@Nullable final Path path, throws ScimException { if (value == null || value.isNull() || - (value.isObject() && value.size() == 0)) + (value.isObject() && value.isEmpty())) { throw BadRequestException.invalidSyntax( "The patch operation value must not be null or an empty object"); @@ -1961,4 +2052,47 @@ private static void validateOperationValue(@Nullable final Path path, + " attributes to " + type); } } + + /** + * This field represents a property that can customize behavior when + * processing ADD PATCH operations with a value filter. This is used for + * multi-valued attributes such as {@code emails}, and is not used for + * {@code remove} or {@code replace} operations. For example, consider the + * following patch request: + *
+   *   {
+   *     "schemas": [ "urn:ietf:params:scim:api:messages:2.0:PatchOp" ],
+   *     "Operations": [
+   *       {
+   *         "op": "add",
+   *         "path": "emails[type eq \"work\"].display",
+   *         "value": "apollo.j@example.com"
+   *       }
+   *     ]
+   *   }
+   * 
+ * + * When this property is enabled and the above patch request is applied, the + * following JSON will be appended to the {@code emails} of the user + * resource: + *
+   *   {
+   *     "type": "work",
+   *     "display": "apollo.j@example.com"
+   *   }
+   * 
+ * Note that this value is added regardless of the current state of the + * resource, so enabling this property for PATCH requests has the potential + * to result in multiple emails containing a {@code type} of {@code work}. + *

+ * + * If this property is disabled, then in the above example, the + * {@code display} field will be added to the resource's existing work email, + * if it exists. If the work email does not exist, then a new value will be + * appended. If there is already more than one existing work email on the + * resource, a {@link BadRequestException} will be thrown. + * + * @since 3.2.0 + */ + public static boolean APPEND_NEW_PATCH_VALUES_PROPERTY = true; } diff --git a/scim2-sdk-common/src/test/java/com/unboundid/scim2/common/AddOperationValueFilterTestCase.java b/scim2-sdk-common/src/test/java/com/unboundid/scim2/common/AddOperationValueFilterTestCase.java index 0684c288..64772cba 100644 --- a/scim2-sdk-common/src/test/java/com/unboundid/scim2/common/AddOperationValueFilterTestCase.java +++ b/scim2-sdk-common/src/test/java/com/unboundid/scim2/common/AddOperationValueFilterTestCase.java @@ -30,6 +30,7 @@ import com.unboundid.scim2.common.types.PhoneNumber; import com.unboundid.scim2.common.types.UserResource; import com.unboundid.scim2.common.utils.JsonUtils; +import org.testng.annotations.AfterMethod; import org.testng.annotations.Test; import java.util.List; @@ -51,6 +52,15 @@ */ public class AddOperationValueFilterTestCase { + /** + * Reset the configurable "append" property to the default value. + */ + @AfterMethod + public void tearDown() + { + PatchOperation.APPEND_NEW_PATCH_VALUES_PROPERTY = true; + } + /** * Ensure that patch ADD operations with a value selection filter are not * permitted for filter types other than equality filters. @@ -97,6 +107,9 @@ public void testBasic() throws Exception PatchRequest request; UserResource resource = new UserResource(); + // Unset the property to use the new behavior. + PatchOperation.APPEND_NEW_PATCH_VALUES_PROPERTY = false; + // Add a work email to a list of existing emails. resource.setEmails( new Email().setValue("existing@example.com").setType("home"), @@ -122,32 +135,20 @@ public void testBasic() throws Exception .containsOnly( new Address().setStreetAddress("The Batcave").setType("secret")); - // Add a 'mobile' phone number to a user when an existing 'mobile' phone - // number already exists. This should not be rejected. - resource = new UserResource(); - resource.setPhoneNumbers( - new PhoneNumber().setValue("+1 314-159-2653").setType("mobile") + // An operation should be able to append data to another field. This + // resource begins with the street address populated, and the patch request + // should be able to update the "formatted" field when using a filter. + resource = new UserResource().setAddresses( + new Address().setType("home").setStreetAddress("8 Mile Rd.") ); - path = Path.fromString("phoneNumbers[type eq \"mobile\"].value"); - request = createAddRequest(path, "+1 271-828-1828"); + path = Path.fromString("addresses[type eq \"home\"].country"); + request = createAddRequest(path, "US"); resource = applyPatchRequest(request, resource); - assertThat(resource.getPhoneNumbers()) - .hasSize(2) - .containsExactly( - new PhoneNumber().setValue("+1 314-159-2653").setType("mobile"), - new PhoneNumber().setValue("+1 271-828-1828").setType("mobile")); - - // Add two photos with the same 'type' value within a single patch request. - resource = new UserResource(); - path = Path.fromString("photos[type eq \"thumbnail\"].value"); - request = new PatchRequest( - PatchOperation.add(path, TextNode.valueOf("https://example.com/1.png")), - PatchOperation.add(path, TextNode.valueOf("https://example.com/2.png")) - ); - resource = applyPatchRequest(request, resource); - assertThat(resource.getPhotos()) - .filteredOn(photo -> photo.getType().equals("thumbnail")) - .hasSize(2); + assertThat(resource.getAddresses()).hasSize(1); + var address = resource.getAddresses().get(0); + assertThat(address.getCountry()).isEqualTo("US"); + assertThat(address.getType()).isEqualTo("home"); + assertThat(address.getStreetAddress()).isEqualTo("8 Mile Rd."); // Only a single value selection filter should be permitted. Path multipleFilter = Path.fromString( @@ -177,6 +178,47 @@ public void testBasic() throws Exception .isInstanceOf(BadRequestException.class) .hasMessageContaining("needs to be 'attribute[filter].subAttribute'"); + // Attempt adding a 'streetAddress' field to a home address in the case + // where the streetAddress is already populated. This should be rejected, + // since the add operation should not act as a replace operation. + UserResource userWithStreet = new UserResource().setAddresses( + new Address().setType("home").setStreetAddress("8 Mile Rd.") + ); + path = Path.fromString("addresses[type eq \"home\"].streetAddress"); + PatchRequest existingSubAttrRequest = createAddRequest(path, "7 Mile Rd."); + assertThatThrownBy(() -> applyPatchRequest(existingSubAttrRequest, userWithStreet)) + .isInstanceOf(BadRequestException.class) + .hasMessageContaining("The add operation attempted to add a new 'streetAddress' field") + .hasMessageContaining("already has a 'streetAddress' defined"); + + // Adding two photos with the same 'type' value within a single patch + // request should also be forbidden. + path = Path.fromString("photos[type eq \"thumbnail\"].value"); + PatchRequest requestWithConflict = new PatchRequest( + PatchOperation.add(path, TextNode.valueOf("https://example.com/1.png")), + PatchOperation.add(path, TextNode.valueOf("https://example.com/2.png")) + ); + assertThatThrownBy(() -> applyPatchRequest(requestWithConflict, new UserResource())) + .isInstanceOf(BadRequestException.class) + .hasMessageContaining("The add operation attempted to add a new 'value' field") + .hasMessageContaining("already has a 'value' defined"); + + // Attempt to apply an operation when the existing resource already has + // multiple matching elements. + UserResource existingUser = new UserResource().setAddresses( + new Address().setStreetAddress("street1").setType("home"), + new Address().setStreetAddress("street2").setType("home") + ); + path = Path.fromString("addresses[type eq \"home\"].streetAddress"); + PatchRequest requestOnInvalidResource = new PatchRequest( + PatchOperation.add(path, TextNode.valueOf("aThirdStreet")) + ); + assertThatThrownBy(() -> applyPatchRequest(requestOnInvalidResource, existingUser)) + .isInstanceOf(BadRequestException.class) + .hasMessageContaining("The operation could not be applied on the resource because") + .hasMessageContaining("the value filter matched more than one element in") + .hasMessageContaining("the 'addresses' array"); + // Assemble an invalid patch request by placing the value in an array, as // opposed to providing it as a single string value. path = Path.fromString("emails[type eq \"home\"].value"); @@ -264,6 +306,50 @@ public void testDeserializedObject() throws Exception assertThat(resourceString).isEqualTo(expected); } + /** + * Tests the behavior of the + * {@link PatchOperation#APPEND_NEW_PATCH_VALUES_PROPERTY} when it is + * configured to always append data targeted with a value filter. + */ + @Test + public void testUseAppendMode() throws Exception + { + UserResource resource; + Path path; + PatchRequest request; + + // Set the property so that the patch operation logic will always append new + // values for add operations with a filtered path. + PatchOperation.APPEND_NEW_PATCH_VALUES_PROPERTY = true; + + // Add a 'mobile' phone number to a user when an existing 'mobile' phone + // number already exists. This should not be rejected. + resource = new UserResource(); + resource.setPhoneNumbers( + new PhoneNumber().setValue("+1 314-159-2653").setType("mobile") + ); + path = Path.fromString("phoneNumbers[type eq \"mobile\"].value"); + request = createAddRequest(path, "+1 271-828-1828"); + resource = applyPatchRequest(request, resource); + assertThat(resource.getPhoneNumbers()) + .hasSize(2) + .containsExactly( + new PhoneNumber().setValue("+1 314-159-2653").setType("mobile"), + new PhoneNumber().setValue("+1 271-828-1828").setType("mobile")); + + // Add two photos with the same 'type' value within a single patch request. + resource = new UserResource(); + path = Path.fromString("photos[type eq \"thumbnail\"].value"); + request = new PatchRequest( + PatchOperation.add(path, TextNode.valueOf("https://example.com/1.png")), + PatchOperation.add(path, TextNode.valueOf("https://example.com/2.png")) + ); + resource = applyPatchRequest(request, resource); + assertThat(resource.getPhotos()) + .filteredOn(photo -> "thumbnail".equals(photo.getType())) + .hasSize(2); + } + /** * This helper method is shorthand for a new patch request that contains a * single add operation with a string value. diff --git a/scim2-sdk-server/pom.xml b/scim2-sdk-server/pom.xml index 8410d172..8cad8347 100644 --- a/scim2-sdk-server/pom.xml +++ b/scim2-sdk-server/pom.xml @@ -19,7 +19,7 @@ scim2-parent com.unboundid.product.scim2 - 3.1.1-SNAPSHOT + 3.2.0-SNAPSHOT scim2-sdk-server jar diff --git a/scim2-ubid-extensions/pom.xml b/scim2-ubid-extensions/pom.xml index aca88911..821a21a1 100644 --- a/scim2-ubid-extensions/pom.xml +++ b/scim2-ubid-extensions/pom.xml @@ -19,7 +19,7 @@ scim2-parent com.unboundid.product.scim2 - 3.1.1-SNAPSHOT + 3.2.0-SNAPSHOT scim2-ubid-extensions jar