kjthorpe18 commented on code in PR #411: URL: https://github.com/apache/directory-scimple/pull/411#discussion_r1404579340
########## scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchGeneratorTest.java: ########## Review Comment: This covers ScimUser resources very well, but some basic tests for `ScimGroup` resources would be useful, too. - Modify `displayName` - add member(s) - remove member(s) - replace member(s) - when should this be used over multiple add/remove operations? e.g. `1 replace` = `1 add` + `1 remove` ########## scim-core/src/main/java/org/apache/directory/scim/core/repository/PatchGenerator.java: ########## @@ -0,0 +1,536 @@ +/* +* 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.apache.directory.scim.core.repository; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.node.*; +import com.flipkart.zjsonpatch.DiffFlags; +import com.flipkart.zjsonpatch.JsonDiff; +import org.apache.directory.scim.core.json.ObjectMapperFactory; +import org.apache.directory.scim.core.schema.SchemaRegistry; +import org.apache.directory.scim.spec.filter.AttributeComparisonExpression; +import org.apache.directory.scim.spec.filter.CompareOperator; +import org.apache.directory.scim.spec.filter.FilterExpression; +import org.apache.directory.scim.spec.filter.ValuePathExpression; +import org.apache.directory.scim.spec.filter.attribute.AttributeReference; +import org.apache.directory.scim.spec.patch.PatchOperation; +import org.apache.directory.scim.spec.patch.PatchOperationPath; +import org.apache.directory.scim.spec.resources.ScimExtension; +import org.apache.directory.scim.spec.resources.ScimResource; +import org.apache.directory.scim.spec.resources.TypedAttribute; +import org.apache.directory.scim.spec.schema.AttributeContainer; +import org.apache.directory.scim.spec.schema.Schema; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.*; +import java.util.stream.Collectors; + +/** + * Generates a diff (i.e. a list of {@link PatchOperation}s) between two SCIM resources. + * This class could be used by SCIM clients when they only want to send PATCH requests over the wire (and communicate + * only the delta between the original and the modified resource). + * This class can also be used when testing SCIM servers as an easy way to generate a list of {@link PatchOperation}s. + */ +public class PatchGenerator { + + private final SchemaRegistry schemaRegistry; + + public PatchGenerator(SchemaRegistry schemaRegistry) { + this.schemaRegistry = schemaRegistry; + } + + /** + * Returns a list of {@link PatchOperation}s that contain the differences between two {@link ScimResource}s. + * @param original an unmodified scim resource, as represented in a datastore. + * @param modified the modified version of the original scim resource. + * @return a list of {@link PatchOperation}s that contain the differences between two {@link ScimResource}s. + * @param <T> The type of the scim resource. + */ + public <T extends ScimResource> List<PatchOperation> diff(T original, T modified) { + return new UpdateRequest<T>(original, modified, schemaRegistry).getPatchOperations(); + } + + static private class UpdateRequest<T extends ScimResource> { + + private static final Logger log = LoggerFactory.getLogger(UpdateRequest.class); + + private static final String OPERATION = "op"; + private static final String PATH = "path"; + private static final String VALUE = "value"; + + // Create a Jackson ObjectMapper that reads JaxB annotations + private final ObjectMapper objectMapper = ObjectMapperFactory.getObjectMapper(); + + private final T resource; + private final T original; + + private final Schema schema; + + private final SchemaRegistry schemaRegistry; + + private final Map<Schema.Attribute, Integer> addRemoveOffsetMap = new HashMap<>(); + + private UpdateRequest(T original, T resource, SchemaRegistry schemaRegistry) { + this.schemaRegistry = schemaRegistry; + this.original = original; + this.resource = resource; + this.schema = schemaRegistry.getSchema(original.getBaseUrn()); + } + + public List<PatchOperation> getPatchOperations() { + try { + return createPatchOperations(); + } catch (IllegalArgumentException | IllegalAccessException | JsonProcessingException e) { + throw new IllegalStateException("Error creating the patch list", e); + } + } + + private void sortMultiValuedCollections(Object obj1, Object obj2, AttributeContainer ac) throws IllegalArgumentException { + for (Schema.Attribute attribute : ac.getAttributes()) { + Schema.AttributeAccessor accessor = attribute.getAccessor(); + if (attribute.isMultiValued()) { + @SuppressWarnings("unchecked") + List<Object> collection1 = obj1 != null ? (List<Object>) accessor.get(obj1) : null; + @SuppressWarnings("unchecked") + List<Object> collection2 = obj2 != null ? (List<Object>) accessor.get(obj2) : null; + + Set<Object> priorities = findCommonElements(collection1, collection2); + PrioritySortingComparator prioritySortingComparator = new PrioritySortingComparator(priorities); + if (collection1 != null) { + Collections.sort(collection1, prioritySortingComparator); + } + + if (collection2 != null) { + Collections.sort(collection2, prioritySortingComparator); + } + } else if (attribute.getType() == Schema.Attribute.Type.COMPLEX) { + Object nextObj1 = obj1 != null ? accessor.get(obj1) : null; + Object nextObj2 = obj2 != null ? accessor.get(obj2) : null; + sortMultiValuedCollections(nextObj1, nextObj2, attribute); + } + } + } + + private Set<Object> findCommonElements(List<Object> list1, List<Object> list2) { + if (list1 == null || list2 == null) { + return Collections.emptySet(); + } + + Set<Object> set1 = new HashSet<>(list1); + Set<Object> set2 = new HashSet<>(list2); + + set1 = set1.stream().map(PrioritySortingComparator::getComparableValue).collect(Collectors.toSet()); + set2 = set2.stream().map(PrioritySortingComparator::getComparableValue).collect(Collectors.toSet()); + + set1.retainAll(set2); + return set1; + } + + /** + * There is a know issue with the diffing tool that the tool will attempt to move empty arrays. By + * nulling out the empty arrays during comparison, this will prevent that error from occurring. Because + * deleting requires the parent node + * @param node Parent node. + */ + private static void nullEmptyLists(JsonNode node) { + List<String> objectsToDelete = new ArrayList<>(); + + if (node != null) { + Iterator<Map.Entry<String, JsonNode>> children = node.fields(); + while(children.hasNext()) { + Map.Entry<String, JsonNode> child = children.next(); + String name = child.getKey(); + JsonNode childNode = child.getValue(); + + //Attempt to delete children before analyzing + if (childNode.isContainerNode()) { + nullEmptyLists(childNode); + } + + if (childNode instanceof ArrayNode) { + ArrayNode ar = (ArrayNode)childNode; + if (ar.size() == 0) { + objectsToDelete.add(name); + } + } + } + + if (!objectsToDelete.isEmpty() && node instanceof ObjectNode) { + ObjectNode on = (ObjectNode)node; + for(String name : objectsToDelete) { + on.remove(name); + } + } + } + } + + private List<PatchOperation> createPatchOperations() throws IllegalArgumentException, IllegalAccessException, JsonProcessingException { + + sortMultiValuedCollections(this.original, this.resource, schema); + Map<String, ScimExtension> originalExtensions = this.original.getExtensions(); + Map<String, ScimExtension> resourceExtensions = this.resource.getExtensions(); + Set<String> keys = new HashSet<>(); + keys.addAll(originalExtensions.keySet()); + keys.addAll(resourceExtensions.keySet()); + + for(String key: keys) { + Schema extSchema = schemaRegistry.getSchema(key); + ScimExtension originalExtension = originalExtensions.get(key); + ScimExtension resourceExtension = resourceExtensions.get(key); + sortMultiValuedCollections(originalExtension, resourceExtension, extSchema); + } + + JsonNode node1 = objectMapper.valueToTree(original); + nullEmptyLists(node1); + JsonNode node2 = objectMapper.valueToTree(resource); + nullEmptyLists(node2); + JsonNode differences = JsonDiff.asJson(node1, node2, DiffFlags.dontNormalizeOpIntoMoveAndCopy()); + + List<PatchOperation> patchOps = convertToPatchOperations(differences); + return patchOps; + } + + List<PatchOperation> convertToPatchOperations(JsonNode node) throws IllegalArgumentException, IllegalAccessException, JsonProcessingException { + List<PatchOperation> operations = new ArrayList<>(); + if (node == null) { + return Collections.emptyList(); + } + + if (!(node instanceof ArrayNode)) { + throw new RuntimeException("Expecting an instance of a ArrayNode, but got: " + node.getClass()); + } + + ArrayNode root = (ArrayNode) node; + for (int i = 0; i < root.size(); i++) { + ObjectNode patchNode = (ObjectNode) root.get(i); + JsonNode operationNode = patchNode.get(OPERATION); + JsonNode pathNode = patchNode.get(PATH); + JsonNode valueNode = patchNode.get(VALUE); + + List<PatchOperation> nodeOperations = convertNodeToPatchOperations(operationNode.asText(), pathNode.asText(), valueNode); + if (!nodeOperations.isEmpty()) { + operations.addAll(nodeOperations); + } + } + + return operations; + } + + private List<PatchOperation> convertNodeToPatchOperations(String operationNode, String diffPath, JsonNode valueNode) throws IllegalArgumentException, IllegalAccessException, JsonProcessingException { + log.debug("convertNodeToPatchOperations: {} , {}", operationNode, diffPath); + List<PatchOperation> operations = new ArrayList<>(); + PatchOperation.Type patchOpType = PatchOperation.Type.valueOf(operationNode.toUpperCase(Locale.ROOT)); + + if (diffPath != null && diffPath.length() >= 1) { + ParseData parseData = new ParseData(diffPath); + + if (parseData.pathParts.isEmpty()) { + operations.add(handleExtensions(valueNode, patchOpType, parseData)); + } else { + operations.addAll(handleAttributes(valueNode, patchOpType, parseData)); + } + } + + return operations; + } + + private PatchOperation handleExtensions(JsonNode valueNode, PatchOperation.Type patchOpType, ParseData parseData) throws JsonProcessingException { + PatchOperation operation = new PatchOperation(); + operation.setOperation(patchOpType); + + AttributeReference attributeReference = new AttributeReference(parseData.pathUri, null); + PatchOperationPath patchOperationPath = new PatchOperationPath(); + ValuePathExpression valuePathExpression = new ValuePathExpression(attributeReference); + patchOperationPath.setValuePathExpression(valuePathExpression); + + operation.setPath(patchOperationPath); + operation.setValue(determineValue(patchOpType, valueNode, parseData)); + + return operation; + } + + @SuppressWarnings("unchecked") + private List<PatchOperation> handleAttributes(JsonNode valueNode, PatchOperation.Type patchOpType, ParseData parseData) throws IllegalAccessException, JsonProcessingException { + log.trace("in handleAttributes"); + List<PatchOperation> operations = new ArrayList<>(); + + List<String> attributeReferenceList = new ArrayList<>(); + FilterExpression valueFilterExpression = null; + List<String> subAttributes = new ArrayList<>(); + + boolean processingMultiValued = false; + boolean processedMultiValued = false; + boolean done = false; + + int i = 0; + for (String pathPart : parseData.pathParts) { + log.trace("path part: {}", pathPart); + if (done) { + throw new RuntimeException("Path should be done... Attribute not supported by the schema: " + pathPart); + } else if (processingMultiValued) { + parseData.traverseObjectsInArray(pathPart, patchOpType); + + if (!parseData.isLastIndex(i) || patchOpType != PatchOperation.Type.ADD) { + if (parseData.originalObject instanceof TypedAttribute) { + TypedAttribute typedAttribute = (TypedAttribute) parseData.originalObject; + String type = typedAttribute.getType(); + valueFilterExpression = new AttributeComparisonExpression(new AttributeReference("type"), CompareOperator.EQ, type); + } else if (parseData.originalObject instanceof String || parseData.originalObject instanceof Number) { + String toString = parseData.originalObject.toString(); + valueFilterExpression = new AttributeComparisonExpression(new AttributeReference("value"), CompareOperator.EQ, toString); + } else if(parseData.originalObject instanceof Enum) { + Enum<?> tempEnum = (Enum<?>)parseData.originalObject; + valueFilterExpression = new AttributeComparisonExpression(new AttributeReference("value"), CompareOperator.EQ, tempEnum.name()); + } else { + if (parseData.originalObject != null) { + log.debug("Attribute: {} doesn't implement TypedAttribute, can't create ValueFilterExpression", parseData.originalObject.getClass()); + } else { + log.debug("Attribute: null doesn't implement TypedAttribute, can't create ValueFilterExpression"); + } + valueFilterExpression = new AttributeComparisonExpression(new AttributeReference("value"), CompareOperator.EQ, "?"); + } + processingMultiValued = false; + processedMultiValued = true; + } + } else { + Schema.Attribute attribute = parseData.ac.getAttribute(pathPart); + + if (attribute != null) { + if (processedMultiValued) { + subAttributes.add(pathPart); + } else { + log.debug("Adding {} to attributeReferenceList", pathPart); + attributeReferenceList.add(pathPart); + } + + parseData.traverseObjects(pathPart, attribute); + + if (patchOpType == PatchOperation.Type.REPLACE && + (parseData.resourceObject != null && parseData.resourceObject instanceof Collection && !((Collection<?>)parseData.resourceObject).isEmpty()) && + (parseData.originalObject == null || + (parseData.originalObject instanceof Collection && ((Collection<?>)parseData.originalObject).isEmpty()))) { + patchOpType = PatchOperation.Type.ADD; + } + + if (attribute.isMultiValued()) { + processingMultiValued = true; + } else if (attribute.getType() != Schema.Attribute.Type.COMPLEX) { + done = true; + } + } + } + ++i; + } + + if (patchOpType == PatchOperation.Type.REPLACE && (parseData.resourceObject == null || + (parseData.resourceObject instanceof Collection && ((Collection<?>)parseData.resourceObject).isEmpty()))) { + patchOpType = PatchOperation.Type.REMOVE; + valueNode = null; + } + + if (patchOpType == PatchOperation.Type.REPLACE && parseData.originalObject == null) { + patchOpType = PatchOperation.Type.ADD; + } Review Comment: I see this is part what I mentioned in another comment - a replace operation actually being an add/remove depending on the original or target resource having `null` ########## scim-core/src/main/java/org/apache/directory/scim/core/repository/PatchGenerator.java: ########## @@ -0,0 +1,536 @@ +/* +* 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.apache.directory.scim.core.repository; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.node.*; +import com.flipkart.zjsonpatch.DiffFlags; +import com.flipkart.zjsonpatch.JsonDiff; +import org.apache.directory.scim.core.json.ObjectMapperFactory; +import org.apache.directory.scim.core.schema.SchemaRegistry; +import org.apache.directory.scim.spec.filter.AttributeComparisonExpression; +import org.apache.directory.scim.spec.filter.CompareOperator; +import org.apache.directory.scim.spec.filter.FilterExpression; +import org.apache.directory.scim.spec.filter.ValuePathExpression; +import org.apache.directory.scim.spec.filter.attribute.AttributeReference; +import org.apache.directory.scim.spec.patch.PatchOperation; +import org.apache.directory.scim.spec.patch.PatchOperationPath; +import org.apache.directory.scim.spec.resources.ScimExtension; +import org.apache.directory.scim.spec.resources.ScimResource; +import org.apache.directory.scim.spec.resources.TypedAttribute; +import org.apache.directory.scim.spec.schema.AttributeContainer; +import org.apache.directory.scim.spec.schema.Schema; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.*; +import java.util.stream.Collectors; + +/** + * Generates a diff (i.e. a list of {@link PatchOperation}s) between two SCIM resources. + * This class could be used by SCIM clients when they only want to send PATCH requests over the wire (and communicate + * only the delta between the original and the modified resource). + * This class can also be used when testing SCIM servers as an easy way to generate a list of {@link PatchOperation}s. + */ +public class PatchGenerator { + + private final SchemaRegistry schemaRegistry; + + public PatchGenerator(SchemaRegistry schemaRegistry) { + this.schemaRegistry = schemaRegistry; + } + + /** + * Returns a list of {@link PatchOperation}s that contain the differences between two {@link ScimResource}s. + * @param original an unmodified scim resource, as represented in a datastore. + * @param modified the modified version of the original scim resource. + * @return a list of {@link PatchOperation}s that contain the differences between two {@link ScimResource}s. + * @param <T> The type of the scim resource. + */ + public <T extends ScimResource> List<PatchOperation> diff(T original, T modified) { + return new UpdateRequest<T>(original, modified, schemaRegistry).getPatchOperations(); + } + + static private class UpdateRequest<T extends ScimResource> { + + private static final Logger log = LoggerFactory.getLogger(UpdateRequest.class); + + private static final String OPERATION = "op"; + private static final String PATH = "path"; + private static final String VALUE = "value"; + + // Create a Jackson ObjectMapper that reads JaxB annotations + private final ObjectMapper objectMapper = ObjectMapperFactory.getObjectMapper(); + + private final T resource; + private final T original; + + private final Schema schema; + + private final SchemaRegistry schemaRegistry; + + private final Map<Schema.Attribute, Integer> addRemoveOffsetMap = new HashMap<>(); + + private UpdateRequest(T original, T resource, SchemaRegistry schemaRegistry) { + this.schemaRegistry = schemaRegistry; + this.original = original; + this.resource = resource; + this.schema = schemaRegistry.getSchema(original.getBaseUrn()); + } + + public List<PatchOperation> getPatchOperations() { + try { + return createPatchOperations(); + } catch (IllegalArgumentException | IllegalAccessException | JsonProcessingException e) { + throw new IllegalStateException("Error creating the patch list", e); + } + } + + private void sortMultiValuedCollections(Object obj1, Object obj2, AttributeContainer ac) throws IllegalArgumentException { + for (Schema.Attribute attribute : ac.getAttributes()) { + Schema.AttributeAccessor accessor = attribute.getAccessor(); + if (attribute.isMultiValued()) { + @SuppressWarnings("unchecked") + List<Object> collection1 = obj1 != null ? (List<Object>) accessor.get(obj1) : null; + @SuppressWarnings("unchecked") + List<Object> collection2 = obj2 != null ? (List<Object>) accessor.get(obj2) : null; + + Set<Object> priorities = findCommonElements(collection1, collection2); + PrioritySortingComparator prioritySortingComparator = new PrioritySortingComparator(priorities); + if (collection1 != null) { + Collections.sort(collection1, prioritySortingComparator); + } + + if (collection2 != null) { + Collections.sort(collection2, prioritySortingComparator); + } + } else if (attribute.getType() == Schema.Attribute.Type.COMPLEX) { + Object nextObj1 = obj1 != null ? accessor.get(obj1) : null; + Object nextObj2 = obj2 != null ? accessor.get(obj2) : null; + sortMultiValuedCollections(nextObj1, nextObj2, attribute); + } + } + } + + private Set<Object> findCommonElements(List<Object> list1, List<Object> list2) { + if (list1 == null || list2 == null) { + return Collections.emptySet(); + } + + Set<Object> set1 = new HashSet<>(list1); + Set<Object> set2 = new HashSet<>(list2); + + set1 = set1.stream().map(PrioritySortingComparator::getComparableValue).collect(Collectors.toSet()); + set2 = set2.stream().map(PrioritySortingComparator::getComparableValue).collect(Collectors.toSet()); + + set1.retainAll(set2); + return set1; + } + + /** + * There is a know issue with the diffing tool that the tool will attempt to move empty arrays. By + * nulling out the empty arrays during comparison, this will prevent that error from occurring. Because + * deleting requires the parent node Review Comment: ```suggestion * There is a known issue with the diffing tool that the tool will attempt to move empty arrays. By * nulling out the empty arrays during comparison, this will prevent that error from occurring because * deleting requires the parent node. ``` -- 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: dev-unsubscr...@directory.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@directory.apache.org For additional commands, e-mail: dev-h...@directory.apache.org
