From 7df05d9ace03342b8798d67d015caaa86c2c6029 Mon Sep 17 00:00:00 2001 From: Captain-P-Goldfish Date: Fri, 26 Jan 2024 20:34:24 +0100 Subject: [PATCH] Add new constructor to PatchRequestHandler and fix patch-bug If a multivalued complex attribute with several existing elements was patched with a patch-operation that contained the same identical elements only the first element was kept and all other elements were lost after the patch-operation. This is fixed now. --- .../patch/DefaultPatchOperationHandler.java | 1 + .../sdk/server/patch/PatchRequestHandler.java | 25 ++- .../sdk/server/patch/PatchTargetHandler.java | 111 ++++++++++--- .../patch/PatchAddResourceHandlerTest.java | 4 +- .../MultivaluedComplexAttributeTests.java | 154 ++++++++++++++++++ 5 files changed, 262 insertions(+), 33 deletions(-) diff --git a/scim-sdk-server/src/main/java/de/captaingoldfish/scim/sdk/server/patch/DefaultPatchOperationHandler.java b/scim-sdk-server/src/main/java/de/captaingoldfish/scim/sdk/server/patch/DefaultPatchOperationHandler.java index b3aa3a0c..a8ef6974 100644 --- a/scim-sdk-server/src/main/java/de/captaingoldfish/scim/sdk/server/patch/DefaultPatchOperationHandler.java +++ b/scim-sdk-server/src/main/java/de/captaingoldfish/scim/sdk/server/patch/DefaultPatchOperationHandler.java @@ -226,6 +226,7 @@ public T getUpdatedResource(String resourceId, T patchedResource, boolean wasRes RequestResourceValidator requestResourceValidator = new RequestResourceValidator(serviceProvider, resourceType, HttpMethod.PATCH); validatedResource = (T)requestResourceValidator.validateDocument(patchedResource); + validatedResource.setId(resourceId); Supplier oldResourceSupplier = getOldResourceSupplier(resourceId, Collections.emptyList(), Collections.emptyList()); diff --git a/scim-sdk-server/src/main/java/de/captaingoldfish/scim/sdk/server/patch/PatchRequestHandler.java b/scim-sdk-server/src/main/java/de/captaingoldfish/scim/sdk/server/patch/PatchRequestHandler.java index 63640637..df54cdb2 100644 --- a/scim-sdk-server/src/main/java/de/captaingoldfish/scim/sdk/server/patch/PatchRequestHandler.java +++ b/scim-sdk-server/src/main/java/de/captaingoldfish/scim/sdk/server/patch/PatchRequestHandler.java @@ -3,6 +3,7 @@ import java.time.LocalDateTime; import java.time.temporal.ChronoUnit; import java.util.AbstractMap; +import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Optional; @@ -88,11 +89,6 @@ public class PatchRequestHandler implements ScimAttribut */ private final PatchConfig patchConfig; - /** - * this resource-handler will accept the handled patch-operations - */ - private final ResourceHandler resourceHandler; - /** * the definition of the resource that is being patched */ @@ -142,7 +138,6 @@ public PatchRequestHandler(String resourceId, Context context) { this.resourceId = resourceId; - this.resourceHandler = resourceHandler; this.serviceProvider = resourceHandler.getServiceProvider(); this.patchConfig = resourceHandler.getServiceProvider().getPatchConfig(); this.resourceType = resourceHandler.getResourceType(); @@ -153,6 +148,22 @@ public PatchRequestHandler(String resourceId, this.validationContext = new ValidationContext(resourceType); } + public PatchRequestHandler(String resourceId, + ServiceProvider serviceProviderConfig, + ResourceType resourceType, + PatchOperationHandler patchOperationHandler) + { + this.resourceId = resourceId; + this.serviceProvider = serviceProviderConfig; + this.patchConfig = serviceProviderConfig.getPatchConfig(); + this.resourceType = resourceType; + this.patchWorkarounds = Collections.emptyList(); + this.mainSchema = resourceType.getMainSchema(); + this.extensionSchemas = resourceType.getAllSchemaExtensions(); + this.patchOperationHandler = patchOperationHandler; + this.validationContext = new ValidationContext(resourceType); + } + /** * delegate method to {@link PatchOperationHandler#getOldResourceSupplier(String, List, List)} */ @@ -290,7 +301,7 @@ private AttributePathRoot getAttributePath(PatchRequestOperation fixedOperation) { try { - return RequestUtils.parsePatchPath(resourceHandler.getResourceType(), fixedOperation.getPath().orElse(null)); + return RequestUtils.parsePatchPath(resourceType, fixedOperation.getPath().orElse(null)); } catch (InvalidFilterException ex) { diff --git a/scim-sdk-server/src/main/java/de/captaingoldfish/scim/sdk/server/patch/PatchTargetHandler.java b/scim-sdk-server/src/main/java/de/captaingoldfish/scim/sdk/server/patch/PatchTargetHandler.java index e16f5934..df1f4e52 100644 --- a/scim-sdk-server/src/main/java/de/captaingoldfish/scim/sdk/server/patch/PatchTargetHandler.java +++ b/scim-sdk-server/src/main/java/de/captaingoldfish/scim/sdk/server/patch/PatchTargetHandler.java @@ -4,6 +4,7 @@ import java.util.List; import java.util.Optional; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.stream.Collectors; import org.apache.commons.lang3.StringUtils; @@ -31,7 +32,6 @@ import de.captaingoldfish.scim.sdk.server.utils.RequestUtils; import de.captaingoldfish.scim.sdk.server.utils.ScimAttributeHelper; import lombok.AccessLevel; -import lombok.AllArgsConstructor; import lombok.Getter; import lombok.extern.slf4j.Slf4j; @@ -621,11 +621,20 @@ private boolean handleInnerComplexAttribute(SchemaAttribute subAttribute, Object { if (subAttribute.isMultiValued()) { - ArrayNode arrayNode = (ArrayNode)complexNode.get(subAttribute.getName()); - if (arrayNode == null) + ArrayNode arrayNode; + JsonNode jsonNode = complexNode.get(subAttribute.getName()); + if (jsonNode == null || jsonNode.isNull() || !jsonNode.isArray()) { arrayNode = new ScimArrayNode(subAttribute); complexNode.set(subAttribute.getName(), arrayNode); + if (jsonNode != null && !jsonNode.isNull()) + { + arrayNode.add(jsonNode); + } + } + else + { + arrayNode = (ArrayNode)jsonNode; } if (PatchOp.REPLACE.equals(patchOp)) { @@ -777,40 +786,47 @@ private boolean handleDirectMultiValuedComplexPathReference(ArrayNode multiValue null, ScimType.RFC7644.NO_TARGET); } } + + List valueNodes = values.stream().map(val -> { + try + { + return JsonHelper.readJsonDocument(val, ObjectNode.class); + } + catch (IOException ex) + { + throw new BadRequestException("The value must be a whole complex type json structure but was: '" + val + "'", + ex, ScimType.RFC7644.INVALID_VALUE); + } + }).collect(Collectors.toList()); + + List originalNodes = new ArrayList<>(); if (PatchOp.REPLACE.equals(patchOp)) { for ( int i = matchingComplexNodes.size() - 1 ; i >= 0 ; i-- ) { IndexNode indexNode = matchingComplexNodes.get(i); - boolean isEquals = indexNode.getObjectNode().equals(JsonHelper.readJsonDocument(values.get(0))); - if (isEquals) + if (path.isWithFilter()) { - return false; + indexNode.createCopy(); } + originalNodes.add((ObjectNode)multiValued.get(indexNode.getIndex())); multiValued.remove(indexNode.getIndex()); } } - for ( String value : values ) + + boolean isResourceChanged = false; + for ( ObjectNode complexNode : valueNodes ) { - ObjectNode complexNode; - try - { - complexNode = (ObjectNode)JsonHelper.readJsonDocument(value); - } - catch (IOException ex) - { - throw new BadRequestException("The value must be a whole complex type json structure but was: '" + value + "'", - ex, ScimType.RFC7644.INVALID_VALUE); - } JsonNode primary = complexNode.get(AttributeNames.RFC7643.PRIMARY); checkForPrimary(multiValued, primary != null && primary.booleanValue()); if (path.isWithFilter() && !matchingComplexNodes.isEmpty()) { - complexNode.fields().forEachRemaining(entry -> { - for ( IndexNode matchingComplexIndexNode : matchingComplexNodes ) - { - ObjectNode matchingNode = matchingComplexIndexNode.getObjectNode(); + for ( IndexNode matchingComplexIndexNode : matchingComplexNodes ) + { + ObjectNode matchingNode = matchingComplexIndexNode.getObjectNode(); + ObjectNode originalNode = JsonHelper.readJsonDocument(matchingNode.toString(), ObjectNode.class); + complexNode.fields().forEachRemaining(entry -> { if (PatchOp.REPLACE.equals(patchOp)) { matchingNode.set(entry.getKey(), entry.getValue()); @@ -829,12 +845,14 @@ private boolean handleDirectMultiValuedComplexPathReference(ArrayNode multiValue originalArray.addAll(newValue); } } - } - }); + }); + isResourceChanged = isResourceChanged || !originalNode.equals(matchingNode); + } if (PatchOp.REPLACE.equals(patchOp)) { for ( IndexNode matchingComplexNode : matchingComplexNodes ) { + isResourceChanged = isResourceChanged || matchingComplexNode.isResultUnchanged(); multiValued.add(matchingComplexNode.getObjectNode()); } } @@ -842,9 +860,32 @@ private boolean handleDirectMultiValuedComplexPathReference(ArrayNode multiValue else if (!path.isWithFilter() || matchingComplexNodes.isEmpty()) { multiValued.add(complexNode); + isResourceChanged = isResourceChanged || !hasEqualObject(originalNodes, complexNode); } } - return true; + return isResourceChanged; + } + + /** + * jackson is failing on int and long comparison even if the nodes do have the same values. for this reason we + * are overriding the comparison here in case for number nodes + * + * @return true if the list contains a node that is equal to given complexNode, false else + */ + private boolean hasEqualObject(List originalObjects, ObjectNode complexNode) + { + return originalObjects.parallelStream().anyMatch(originalNode -> { + return originalNode.equals((o1, o2) -> { + if (o1.isNumber() && o2.isNumber()) + { + return o1.longValue() == o2.longValue() ? 0 : 1; + } + else + { + return o1.equals(o2) ? 0 : 1; + } + }, complexNode); + }); } /** @@ -978,7 +1019,6 @@ private SchemaAttribute getSchemaAttribute() * a helper class that is used in case of filtering. We will also hold the index of the filtered nodes */ @Getter - @AllArgsConstructor private static class IndexNode { @@ -991,5 +1031,26 @@ private static class IndexNode * a filtered node */ private ObjectNode objectNode; + + /** + * a copy of this node that must explicitly be created + */ + private ObjectNode copyNode; + + public IndexNode(int index, ObjectNode objectNode) + { + this.index = index; + this.objectNode = objectNode; + } + + public void createCopy() + { + copyNode = JsonHelper.readJsonDocument(objectNode.toString(), ObjectNode.class); + } + + public boolean isResultUnchanged() + { + return objectNode.equals(copyNode); + } } } diff --git a/scim-sdk-server/src/test/java/de/captaingoldfish/scim/sdk/server/patch/PatchAddResourceHandlerTest.java b/scim-sdk-server/src/test/java/de/captaingoldfish/scim/sdk/server/patch/PatchAddResourceHandlerTest.java index 095a01c5..cb46824a 100644 --- a/scim-sdk-server/src/test/java/de/captaingoldfish/scim/sdk/server/patch/PatchAddResourceHandlerTest.java +++ b/scim-sdk-server/src/test/java/de/captaingoldfish/scim/sdk/server/patch/PatchAddResourceHandlerTest.java @@ -161,7 +161,6 @@ public void testFieldsAreSet() resourceEndpoint.getPatchWorkarounds(), new Context(null)); Assertions.assertNotNull(patchRequestHandler.getPatchConfig()); - Assertions.assertNotNull(patchRequestHandler.getResourceHandler()); Assertions.assertNotNull(patchRequestHandler.getPatchWorkarounds()); Assertions.assertFalse(patchRequestHandler.getPatchWorkarounds().isEmpty()); Assertions.assertNotNull(patchRequestHandler.getMainSchema()); @@ -1613,12 +1612,15 @@ public void testReplaceEmailsWithPrimary() .valueNode(allTypeChanges) .build()); PatchOpRequest patchOpRequest = PatchOpRequest.builder().operations(operations).build(); + log.debug(patchOpRequest.toPrettyString()); addAllTypesToProvider(allTypes); + log.info(allTypes.toPrettyString()); PatchRequestHandler patchRequestHandler = new PatchRequestHandler(allTypes.getId().get(), allTypesResourceType.getResourceHandlerImpl(), resourceEndpoint.getPatchWorkarounds(), new Context(null)); AllTypes patchedAllTypes = patchRequestHandler.handlePatchRequest(patchOpRequest); + log.warn(patchedAllTypes.toPrettyString()); Assertions.assertTrue(patchRequestHandler.isResourceChanged()); Assertions.assertEquals(emailArray, patchedAllTypes.get(AttributeNames.RFC7643.EMAILS)); } diff --git a/scim-sdk-server/src/test/java/de/captaingoldfish/scim/sdk/server/patch/validationtests/MultivaluedComplexAttributeTests.java b/scim-sdk-server/src/test/java/de/captaingoldfish/scim/sdk/server/patch/validationtests/MultivaluedComplexAttributeTests.java index d25069bb..099bb29a 100644 --- a/scim-sdk-server/src/test/java/de/captaingoldfish/scim/sdk/server/patch/validationtests/MultivaluedComplexAttributeTests.java +++ b/scim-sdk-server/src/test/java/de/captaingoldfish/scim/sdk/server/patch/validationtests/MultivaluedComplexAttributeTests.java @@ -1200,6 +1200,89 @@ public void testAddOneMultivaluedComplexWithPrevious(String attributeName) } } + /** + * replace a multiComplex with identical value + */ + @DisplayName("success: Replace multiComplex with identical value") + @ParameterizedTest + @ValueSource(strings = {"multiComplex", "urn:gold:params:scim:schemas:custom:2.0:AllTypes:multiComplex"}) + public void testReplaceMultiComplexWithIdenticalValue(String attributeName) + { + PatchRequestOperation patchRequestOperation; + { + ArrayNode values = new ArrayNode(JsonNodeFactory.instance); + { + AllTypes complex = new AllTypes(false); + complex.setString("hello world"); + complex.setNumber(5L); + values.add(complex); + } + { + AllTypes complex = new AllTypes(false); + complex.setStringArray(Arrays.asList("hello", "world")); + complex.setNumberArray(Arrays.asList(6L, 7L)); + values.add(complex); + } + patchRequestOperation = PatchRequestOperation.builder() + .op(PatchOp.REPLACE) + .path(attributeName) + .valueNode(values) + .build(); + } + + List operations = Arrays.asList(patchRequestOperation); + PatchOpRequest patchOpRequest = PatchOpRequest.builder().operations(operations).build(); + + AllTypes allTypes = new AllTypes(true); + allTypes.set("multiComplex", patchRequestOperation.getValueNode().get()); + addAllTypesToProvider(allTypes); + + PatchRequestHandler patchRequestHandler = new PatchRequestHandler(allTypes.getId().get(), + allTypesResourceType.getResourceHandlerImpl(), + resourceEndpoint.getPatchWorkarounds(), + new Context(null)); + AllTypes patchedResource = Assertions.assertDoesNotThrow(() -> patchRequestHandler.handlePatchRequest(patchOpRequest)); + Assertions.assertFalse(patchRequestHandler.isResourceChanged()); + Assertions.assertEquals(2, patchedResource.getMultiComplex().size()); + { + AllTypes patchedComplex = patchedResource.getMultiComplex().get(0); + Assertions.assertEquals(2, patchedComplex.size()); + Assertions.assertEquals("hello world", patchedComplex.getString().get()); + Assertions.assertEquals(5L, patchedComplex.getNumber().get()); + } + { + AllTypes patchedComplex = patchedResource.getMultiComplex().get(1); + Assertions.assertEquals(2, patchedComplex.size()); + Assertions.assertEquals(2, patchedComplex.getStringArray().size()); + Assertions.assertEquals("hello", patchedComplex.getStringArray().get(0)); + Assertions.assertEquals("world", patchedComplex.getStringArray().get(1)); + Assertions.assertEquals(2, patchedComplex.getNumberArray().size()); + Assertions.assertEquals(6L, patchedComplex.getNumberArray().get(0)); + Assertions.assertEquals(7L, patchedComplex.getNumberArray().get(1)); + } + // must be called + { + Mockito.verify(defaultPatchOperationHandler) + .handleOperation(Mockito.any(), Mockito.any(MultivaluedComplexAttributeOperation.class)); + } + // must not be called + { + Mockito.verify(defaultPatchOperationHandler, Mockito.never()) + .handleOperation(Mockito.any(), Mockito.any(RemoveExtensionRefOperation.class)); + Mockito.verify(defaultPatchOperationHandler, Mockito.never()) + .handleOperation(Mockito.any(), Mockito.any(SimpleAttributeOperation.class)); + Mockito.verify(defaultPatchOperationHandler, Mockito.never()) + .handleOperation(Mockito.any(), Mockito.any(MultivaluedSimpleAttributeOperation.class)); + Mockito.verify(defaultPatchOperationHandler, Mockito.never()) + .handleOperation(Mockito.any(), Mockito.any(RemoveComplexAttributeOperation.class)); + Mockito.verify(defaultPatchOperationHandler, Mockito.never()) + .handleOperation(Mockito.any(), Mockito.any(MultivaluedComplexSimpleSubAttributeOperation.class)); + Mockito.verify(defaultPatchOperationHandler, Mockito.never()) + .handleOperation(Mockito.any(), + Mockito.any(MultivaluedComplexMultivaluedSubAttributeOperation.class)); + } + } + /* *************************************************************************************** */ } @DisplayName("With Filter Tests") @@ -1731,7 +1814,78 @@ public void testAddOneMultivaluedComplexWithFilter(String attributeName) Mockito.any(MultivaluedComplexMultivaluedSubAttributeOperation.class)); } } + + /** + * add operation does not change multiComplex with identical value + */ + @DisplayName("success: Add multiComplex with identical value") + @ParameterizedTest + @ValueSource(strings = {"multiComplex", "urn:gold:params:scim:schemas:custom:2.0:AllTypes:multiComplex"}) + public void testAddMultiComplexWithIdenticalValue(String attributeName) + { + PatchRequestOperation patchRequestOperation; + { + ArrayNode values = new ArrayNode(JsonNodeFactory.instance); + { + AllTypes complex = new AllTypes(false); + complex.setString("hello world"); + complex.setNumber(5L); + values.add(complex); + } + + final String filterPath = String.format("%s[string eq \"hello world\"]", attributeName); + patchRequestOperation = PatchRequestOperation.builder() + .op(PatchOp.ADD) + .path(filterPath) + .valueNode(values) + .build(); + } + + List operations = Arrays.asList(patchRequestOperation); + PatchOpRequest patchOpRequest = PatchOpRequest.builder().operations(operations).build(); + + AllTypes allTypes = new AllTypes(true); + allTypes.set("multiComplex", patchRequestOperation.getValueNode().get()); + addAllTypesToProvider(allTypes); + + PatchRequestHandler patchRequestHandler = new PatchRequestHandler(allTypes.getId().get(), + allTypesResourceType.getResourceHandlerImpl(), + resourceEndpoint.getPatchWorkarounds(), + new Context(null)); + AllTypes patchedResource = Assertions.assertDoesNotThrow(() -> patchRequestHandler.handlePatchRequest(patchOpRequest)); + Assertions.assertFalse(patchRequestHandler.isResourceChanged()); + Assertions.assertEquals(1, patchedResource.getMultiComplex().size()); + { + AllTypes patchedComplex = patchedResource.getMultiComplex().get(0); + Assertions.assertEquals(2, patchedComplex.size()); + Assertions.assertEquals("hello world", patchedComplex.getString().get()); + Assertions.assertEquals(5L, patchedComplex.getNumber().get()); + } + // must be called + { + Mockito.verify(defaultPatchOperationHandler) + .handleOperation(Mockito.any(), Mockito.any(MultivaluedComplexAttributeOperation.class)); + } + // must not be called + { + Mockito.verify(defaultPatchOperationHandler, Mockito.never()) + .handleOperation(Mockito.any(), Mockito.any(RemoveExtensionRefOperation.class)); + Mockito.verify(defaultPatchOperationHandler, Mockito.never()) + .handleOperation(Mockito.any(), Mockito.any(SimpleAttributeOperation.class)); + Mockito.verify(defaultPatchOperationHandler, Mockito.never()) + .handleOperation(Mockito.any(), Mockito.any(MultivaluedSimpleAttributeOperation.class)); + Mockito.verify(defaultPatchOperationHandler, Mockito.never()) + .handleOperation(Mockito.any(), Mockito.any(RemoveComplexAttributeOperation.class)); + Mockito.verify(defaultPatchOperationHandler, Mockito.never()) + .handleOperation(Mockito.any(), Mockito.any(MultivaluedComplexSimpleSubAttributeOperation.class)); + Mockito.verify(defaultPatchOperationHandler, Mockito.never()) + .handleOperation(Mockito.any(), + Mockito.any(MultivaluedComplexMultivaluedSubAttributeOperation.class)); + } + } + /* **************************************************************************************** */ } + /* **************************************************************************************** */ } @DisplayName("Extension Resource Tests")