Skip to content

Commit

Permalink
Add new constructor to PatchRequestHandler and fix patch-bug
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@Captain-P-Goldfish
Captain-P-Goldfish committed Jan 26, 2024
1 parent 557e8f3 commit 7df05d9
Show file tree
Hide file tree
Showing 5 changed files with 262 additions and 33 deletions.
1 change: 1 addition & 0 deletions .../src/main/java/de/captaingoldfish/scim/sdk/server/patch/DefaultPatchOperationHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<T> oldResourceSupplier = getOldResourceSupplier(resourceId,
Collections.emptyList(),
Collections.emptyList());
Expand Down
25 changes: 18 additions & 7 deletions ...dk-server/src/main/java/de/captaingoldfish/scim/sdk/server/patch/PatchRequestHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -88,11 +89,6 @@ public class PatchRequestHandler<T extends ResourceNode> implements ScimAttribut
*/
private final PatchConfig patchConfig;

/**
* this resource-handler will accept the handled patch-operations
*/
private final ResourceHandler<T> resourceHandler;

/**
* the definition of the resource that is being patched
*/
Expand Down Expand Up @@ -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();
Expand All @@ -153,6 +148,22 @@ public PatchRequestHandler(String resourceId,
this.validationContext = new ValidationContext(resourceType);
}

public PatchRequestHandler(String resourceId,
ServiceProvider serviceProviderConfig,
ResourceType resourceType,
PatchOperationHandler<T> 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)}
*/
Expand Down Expand Up @@ -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)
{
Expand Down
111 changes: 86 additions & 25 deletions ...sdk-server/src/main/java/de/captaingoldfish/scim/sdk/server/patch/PatchTargetHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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))
{
Expand Down Expand Up @@ -777,40 +786,47 @@ private boolean handleDirectMultiValuedComplexPathReference(ArrayNode multiValue
null, ScimType.RFC7644.NO_TARGET);
}
}

List<ObjectNode> 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<ObjectNode> 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());
Expand All @@ -829,22 +845,47 @@ 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());
}
}
}
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<ObjectNode> 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);
});
}

/**
Expand Down Expand Up @@ -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
{

Expand All @@ -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);
}
}
}
4 changes: 3 additions & 1 deletion ...r/src/test/java/de/captaingoldfish/scim/sdk/server/patch/PatchAddResourceHandlerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -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<AllTypes> 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));
}
Expand Down
Loading

0 comments on commit 7df05d9

Please sign in to comment.