You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@directory.apache.org by "bdemers (via GitHub)" <gi...@apache.org> on 2023/11/13 17:05:39 UTC
[PR] Move attribute and etag logic into Repository [directory-scimple]
bdemers opened a new pull request, #411:
URL: https://github.com/apache/directory-scimple/pull/411
Breaks up the current `update` method into new `update` and `patch` methods.
(Breaking change)
- ETag/Version logic must be managed by Repositories, except for the case of `ServiceProviderConfiguration` (which is likely stored in memory)
- Implementations should set `scimResource.meta.version`, how this version is defined is up to the implementation
- The `UpdateRequest` class has been removed, the `getPatchOperations` method has been moved into a new class `PatchGenerator`
- Current examples have been updated and demo simple in memory patch application. e.g. Get/apply patch/persist.
- Other implementations may want generate update queries directly from the list of PatchOperations (skipping intermediate get step)
--
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-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] Move attribute and etag logic into Repository [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on code in PR #411:
URL: https://github.com/apache/directory-scimple/pull/411#discussion_r1408553999
##########
scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchGeneratorTest.java:
##########
Review Comment:
These are great!
The first 3 are pretty easy, the last one is a bit more complex, and will require a bit more work (and likely changes as to the JSON diff lib is invoked)
Since this logic didn't change in this PR (it was moved from a different class)
I'll create a separate issue to track this (and I'll stick the current tests i have in a new PR, as a starting point)
--
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-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] Move attribute and etag logic into Repository [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on PR #411:
URL: https://github.com/apache/directory-scimple/pull/411#issuecomment-1812909404
> However, can we get some better comments/documentation about what, specifically, about the incoming PUT or PATCH requests determines whether the `update()` or `patch()` flows are used? In general, I tend to get lost in SCIMple when looking for the endpoint/controller-level logic. In this PR as well, I've missed (at least on a first pass) where the main logic is for SCIMple to decide whether to use `update()` or `patch()` based on the incoming request that it's handling.
Absolutely!
IMHO, the high level goal is that folks _shouldn't_ need to know any of the detail of the REST layer, and only need to implement the `Repository` API.
This will need to be expanded on in the javadoc and other docs, but here are some key points:
- There are two ways to modify an existing resource:
- a replacement of a resource, `PUT` request, which calls `repository.update(...)`
- a partial update of a resource, `PATCH` request which calls `repository.patch(...)`
Maybe it would be helpful to have a table mapping the SCIM protocol requests back to the repository method:
(not completely accurate, mostly thinking out loud for how this might look)
| REST Endpoint | REST method | Repository method |
|----------------|---------------|--------------------|
| /Users | `GET` | `repository.query()` |
| /Users/{id} ` | `PUT` | `repository.update()` |
| /Users/{id} ` | `PATCH` | `repository.patch()` |
...
As an aside, while talking about the javadocs, I'm not sure the Swagger/OpenAPI annotations in the REST layer add value to the user. (As the OpenAPI spec for SCIM could be defined by the body managing the RFC, or other project, that OpenAPI spec doesn't change unless the RFC is updated)
Anyway, I've wondered if removing it would make it easier to read/understand the code. (Or if I'm just over thinking it and that's not a problem at all)
--
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-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] Move attribute and etag logic into Repository [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on PR #411:
URL: https://github.com/apache/directory-scimple/pull/411#issuecomment-1832194061
@joshuapsteele All the reviews here are great!
I just cleaned up the git history a little, I'll merge it once CI passes!
--
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-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] Move attribute and etag logic into Repository [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on code in PR #411:
URL: https://github.com/apache/directory-scimple/pull/411#discussion_r1401218712
##########
scim-core/src/main/java/org/apache/directory/scim/core/repository/Repository.java:
##########
@@ -60,14 +63,31 @@ public interface Repository<T extends ScimResource> {
/**
* Allows the SCIM server's REST implementation to update and existing
* resource via a PUT to a valid end-point.
- *
- * @param updateRequest The ScimResource to update and persist.
+ *
+ * @param id the identifier of the ScimResource to update and persist.
+ * @param version an optional version (usually used as an ETag) that be used to optimize update requests, may be compared against, the current {@code ScimResource.meta.version}.
+ * @param resource an updated resource to persist
+ * @param includedAttributes optional set of attributes to include from ScimResource, may be used to optimize queries.
+ * @param excludedAttributes optional set of attributes to exclude from ScimResource, may be used to optimize queries.
* @return The newly updated ScimResource.
- * @throws ResourceException When the ScimResource cannot be
- * updated.
+ * @throws ResourceException When the ScimResource cannot be updated.
*/
- T update(UpdateRequest<T> updateRequest) throws ResourceException;
-
+ T update(String id, String version, T resource, Set<AttributeReference> includedAttributes, Set<AttributeReference> excludedAttributes) throws ResourceException;
+
+ /**
+ * Allows the SCIM server's REST implementation to update and existing
Review Comment:
Thanks!
##########
scim-server/src/main/java/org/apache/directory/scim/server/rest/BaseResourceTypeResourceImpl.java:
##########
@@ -382,35 +358,51 @@ private ScimException notFoundException(String id) {
return new ScimException(Status.NOT_FOUND, "Resource " + id + " not found");
}
- private void validatePreconditions(String id, EntityTag etag) {
- ResponseBuilder response = request.evaluatePreconditions(etag);
- if (response != null) {
- throw new WebApplicationException(response
- .entity(new ErrorResponse(Status.PRECONDITION_FAILED, "Failed to update record, backing record has changed - " + id))
- .build());
- }
- }
+// private void validatePreconditions(String id, EntityTag etag) {
+// ResponseBuilder response = request.evaluatePreconditions(etag);
+// if (response != null) {
+// throw new WebApplicationException(response
+// .entity(new ErrorResponse(Status.PRECONDITION_FAILED, "Failed to update record, backing record has changed - " + id))
+// .build());
+// }
+// }
+//
+// private EntityTag requireEtag(ScimResource resource) throws ScimException {
+// try {
+// return etagGenerator.generateEtag(resource);
+// } catch (EtagGenerationException e) {
+// throw new ScimException(Status.INTERNAL_SERVER_ERROR, "Failed to generate the etag", e);
+// }
+// }
+//
+// private EntityTag etag(ScimResource resource) {
+// try {
+// return etagGenerator.generateEtag(resource);
+// } catch (EtagGenerationException e) {
+// log.warn("Failed to generate etag for resource", e);
+// return null;
+// }
+// }
Review Comment:
Removed
--
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-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] Move attribute and etag logic into Repository [directory-scimple]
Posted by "kjthorpe18 (via GitHub)" <gi...@apache.org>.
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-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] Move attribute and etag logic into Repository [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on PR #411:
URL: https://github.com/apache/directory-scimple/pull/411#issuecomment-1836374354
@joshuapsteele keep doing what you doing, I'll try to get that ball rolling!
In the short term, if you haven't seen this article take a look!
https://community.apache.org/contributors/
--
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-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] Move attribute and etag logic into Repository [directory-scimple]
Posted by "joshuapsteele (via GitHub)" <gi...@apache.org>.
joshuapsteele commented on code in PR #411:
URL: https://github.com/apache/directory-scimple/pull/411#discussion_r1394096752
##########
scim-server/src/main/java/org/apache/directory/scim/server/rest/BaseResourceTypeResourceImpl.java:
##########
@@ -382,35 +358,51 @@ private ScimException notFoundException(String id) {
return new ScimException(Status.NOT_FOUND, "Resource " + id + " not found");
}
- private void validatePreconditions(String id, EntityTag etag) {
- ResponseBuilder response = request.evaluatePreconditions(etag);
- if (response != null) {
- throw new WebApplicationException(response
- .entity(new ErrorResponse(Status.PRECONDITION_FAILED, "Failed to update record, backing record has changed - " + id))
- .build());
- }
- }
+// private void validatePreconditions(String id, EntityTag etag) {
+// ResponseBuilder response = request.evaluatePreconditions(etag);
+// if (response != null) {
+// throw new WebApplicationException(response
+// .entity(new ErrorResponse(Status.PRECONDITION_FAILED, "Failed to update record, backing record has changed - " + id))
+// .build());
+// }
+// }
+//
+// private EntityTag requireEtag(ScimResource resource) throws ScimException {
+// try {
+// return etagGenerator.generateEtag(resource);
+// } catch (EtagGenerationException e) {
+// throw new ScimException(Status.INTERNAL_SERVER_ERROR, "Failed to generate the etag", e);
+// }
+// }
+//
+// private EntityTag etag(ScimResource resource) {
+// try {
+// return etagGenerator.generateEtag(resource);
+// } catch (EtagGenerationException e) {
+// log.warn("Failed to generate etag for resource", e);
+// return null;
+// }
+// }
Review Comment:
Still getting my footing in reviewing this PR. What's the reason for having these lines commented-out at the moment?
--
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-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] Move attribute and etag logic into Repository [directory-scimple]
Posted by "erant10 (via GitHub)" <gi...@apache.org>.
erant10 commented on code in PR #411:
URL: https://github.com/apache/directory-scimple/pull/411#discussion_r1397873778
##########
scim-core/src/main/java/org/apache/directory/scim/core/repository/Repository.java:
##########
@@ -60,14 +63,31 @@ public interface Repository<T extends ScimResource> {
/**
* Allows the SCIM server's REST implementation to update and existing
* resource via a PUT to a valid end-point.
- *
- * @param updateRequest The ScimResource to update and persist.
+ *
+ * @param id the identifier of the ScimResource to update and persist.
+ * @param version an optional version (usually used as an ETag) that be used to optimize update requests, may be compared against, the current {@code ScimResource.meta.version}.
Review Comment:
also nit: that *can* be
##########
scim-core/src/main/java/org/apache/directory/scim/core/repository/PatchGenerator.java:
##########
@@ -0,0 +1,523 @@
+/*
+* 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;
+
+public class PatchGenerator {
Review Comment:
I don't see this class used anywhere, so just making sure I understand - what exactly is the purpose of PatchGenerator? is it meant to be used optionally by anyone who wants to convert an `update` request into a `patch` request in their repo implementation?
Perhaps a short comment on this class and its intended use could be helpful for future adopters?
##########
scim-core/src/main/java/org/apache/directory/scim/core/repository/Repository.java:
##########
@@ -60,14 +63,31 @@ public interface Repository<T extends ScimResource> {
/**
* Allows the SCIM server's REST implementation to update and existing
* resource via a PUT to a valid end-point.
- *
- * @param updateRequest The ScimResource to update and persist.
+ *
+ * @param id the identifier of the ScimResource to update and persist.
+ * @param version an optional version (usually used as an ETag) that be used to optimize update requests, may be compared against, the current {@code ScimResource.meta.version}.
Review Comment:
I'm a little confused by the `version <-> etag` relationship.
are they are equivalent?
if etag an http concept then it makes sense to distinguish the two, but just curious if there was more to it than that
##########
scim-server-examples/scim-server-jersey/src/main/java/org/apache/directory/scim/example/jersey/service/InMemoryUserService.java:
##########
@@ -130,17 +134,21 @@ public ScimUser create(ScimUser resource) throws UnableToCreateResourceException
return resource;
}
- /**
- * @see Repository#update(UpdateRequest)
- */
@Override
- public ScimUser update(UpdateRequest<ScimUser> updateRequest) throws UnableToUpdateResourceException {
- String id = updateRequest.getId();
- ScimUser resource = updateRequest.getResource();
+ public ScimUser update(String id, String version, ScimUser resource, Set<AttributeReference> includedAttributeReferences, Set<AttributeReference> excludedAttributeReferences) throws ResourceException {
+ users.put(id, resource);
+ return resource;
+ }
+
+ @Override
+ public ScimUser patch(String id, String version, List<PatchOperation> patchOperations, Set<AttributeReference> includedAttributeReferences, Set<AttributeReference> excludedAttributeReferences) throws ResourceException {
+ PatchHandler patchHandler = new PatchHandlerImpl(schemaRegistry);
Review Comment:
would it make sense to inject PatchHandlerImpl to the Repositories instead of instantiating per request?
--
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-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] Move attribute and etag logic into Repository [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers merged PR #411:
URL: https://github.com/apache/directory-scimple/pull/411
--
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-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] Move attribute and etag logic into Repository [directory-scimple]
Posted by "joshuapsteele (via GitHub)" <gi...@apache.org>.
joshuapsteele commented on code in PR #411:
URL: https://github.com/apache/directory-scimple/pull/411#discussion_r1394093329
##########
scim-core/src/main/java/org/apache/directory/scim/core/repository/Repository.java:
##########
@@ -60,14 +63,31 @@ public interface Repository<T extends ScimResource> {
/**
* Allows the SCIM server's REST implementation to update and existing
* resource via a PUT to a valid end-point.
- *
- * @param updateRequest The ScimResource to update and persist.
+ *
+ * @param id the identifier of the ScimResource to update and persist.
+ * @param version an optional version (usually used as an ETag) that be used to optimize update requests, may be compared against, the current {@code ScimResource.meta.version}.
+ * @param resource an updated resource to persist
+ * @param includedAttributes optional set of attributes to include from ScimResource, may be used to optimize queries.
+ * @param excludedAttributes optional set of attributes to exclude from ScimResource, may be used to optimize queries.
* @return The newly updated ScimResource.
- * @throws ResourceException When the ScimResource cannot be
- * updated.
+ * @throws ResourceException When the ScimResource cannot be updated.
*/
- T update(UpdateRequest<T> updateRequest) throws ResourceException;
-
+ T update(String id, String version, T resource, Set<AttributeReference> includedAttributes, Set<AttributeReference> excludedAttributes) throws ResourceException;
+
+ /**
+ * Allows the SCIM server's REST implementation to update and existing
Review Comment:
```suggestion
* Allows the SCIM server's REST implementation to update an existing
```
--
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-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] Move attribute and etag logic into Repository [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on code in PR #411:
URL: https://github.com/apache/directory-scimple/pull/411#discussion_r1401219607
##########
scim-server-examples/scim-server-jersey/src/main/java/org/apache/directory/scim/example/jersey/service/InMemoryUserService.java:
##########
@@ -130,17 +134,21 @@ public ScimUser create(ScimUser resource) throws UnableToCreateResourceException
return resource;
}
- /**
- * @see Repository#update(UpdateRequest)
- */
@Override
- public ScimUser update(UpdateRequest<ScimUser> updateRequest) throws UnableToUpdateResourceException {
- String id = updateRequest.getId();
- ScimUser resource = updateRequest.getResource();
+ public ScimUser update(String id, String version, ScimUser resource, Set<AttributeReference> includedAttributeReferences, Set<AttributeReference> excludedAttributeReferences) throws ResourceException {
+ users.put(id, resource);
+ return resource;
+ }
+
+ @Override
+ public ScimUser patch(String id, String version, List<PatchOperation> patchOperations, Set<AttributeReference> includedAttributeReferences, Set<AttributeReference> excludedAttributeReferences) throws ResourceException {
+ PatchHandler patchHandler = new PatchHandlerImpl(schemaRegistry);
Review Comment:
I just pushed an updated, and injected the PatchHandler, please take a look!
--
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-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] Move attribute and etag logic into Repository [directory-scimple]
Posted by "joshuapsteele (via GitHub)" <gi...@apache.org>.
joshuapsteele commented on PR #411:
URL: https://github.com/apache/directory-scimple/pull/411#issuecomment-1836176581
> @joshuapsteele All the reviews here are great! I just cleaned up the git history a little, I'll merge it once CI passes!
@bdemers Sounds good! @kjthorpe18 @zalbert02 and I are all eagerly anticipating this improvement! BTW, how can we get added as contributors to this SCIMple project? We're quite motivated to see it succeed and continue to be developed over time.
--
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-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] Move attribute and etag logic into Repository [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on code in PR #411:
URL: https://github.com/apache/directory-scimple/pull/411#discussion_r1409472656
##########
scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchGeneratorTest.java:
##########
Review Comment:
Created #427
--
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-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] Move attribute and etag logic into Repository [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on code in PR #411:
URL: https://github.com/apache/directory-scimple/pull/411#discussion_r1394463027
##########
scim-server/src/main/java/org/apache/directory/scim/server/rest/BaseResourceTypeResourceImpl.java:
##########
@@ -382,35 +358,51 @@ private ScimException notFoundException(String id) {
return new ScimException(Status.NOT_FOUND, "Resource " + id + " not found");
}
- private void validatePreconditions(String id, EntityTag etag) {
- ResponseBuilder response = request.evaluatePreconditions(etag);
- if (response != null) {
- throw new WebApplicationException(response
- .entity(new ErrorResponse(Status.PRECONDITION_FAILED, "Failed to update record, backing record has changed - " + id))
- .build());
- }
- }
+// private void validatePreconditions(String id, EntityTag etag) {
+// ResponseBuilder response = request.evaluatePreconditions(etag);
+// if (response != null) {
+// throw new WebApplicationException(response
+// .entity(new ErrorResponse(Status.PRECONDITION_FAILED, "Failed to update record, backing record has changed - " + id))
+// .build());
+// }
+// }
+//
+// private EntityTag requireEtag(ScimResource resource) throws ScimException {
+// try {
+// return etagGenerator.generateEtag(resource);
+// } catch (EtagGenerationException e) {
+// throw new ScimException(Status.INTERNAL_SERVER_ERROR, "Failed to generate the etag", e);
+// }
+// }
+//
+// private EntityTag etag(ScimResource resource) {
+// try {
+// return etagGenerator.generateEtag(resource);
+// } catch (EtagGenerationException e) {
+// log.warn("Failed to generate etag for resource", e);
+// return null;
+// }
+// }
Review Comment:
Good catch, i though I deleted all of the dead code, I'll take another pass through this PR!
--
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-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] Move attribute and etag logic into Repository [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on code in PR #411:
URL: https://github.com/apache/directory-scimple/pull/411#discussion_r1401234044
##########
scim-core/src/main/java/org/apache/directory/scim/core/repository/PatchGenerator.java:
##########
@@ -0,0 +1,523 @@
+/*
+* 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;
+
+public class PatchGenerator {
Review Comment:
💯
Added Javadoc comments!
##########
scim-core/src/main/java/org/apache/directory/scim/core/repository/PatchGenerator.java:
##########
@@ -0,0 +1,523 @@
+/*
+* 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;
+
+public class PatchGenerator {
Review Comment:
💯
Added Javadoc comments!
--
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-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org