Enable UpdateRequest patch related tests These tests were disabled, so they have been reworked with a new PatchOperationAssert
diff --git a/scim-core/src/main/java/org/apache/directory/scim/core/repository/UpdateRequest.java b/scim-core/src/main/java/org/apache/directory/scim/core/repository/UpdateRequest.java index f7ea920..35b5a06 100644 --- a/scim-core/src/main/java/org/apache/directory/scim/core/repository/UpdateRequest.java +++ b/scim-core/src/main/java/org/apache/directory/scim/core/repository/UpdateRequest.java
@@ -38,6 +38,7 @@ import com.fasterxml.jackson.databind.node.TextNode; import com.fasterxml.jackson.module.jakarta.xmlbind.JakartaXmlBindAnnotationIntrospector; import com.fasterxml.jackson.module.jakarta.xmlbind.JakartaXmlBindAnnotationModule; +import com.flipkart.zjsonpatch.DiffFlags; import com.flipkart.zjsonpatch.JsonDiff; import lombok.EqualsAndHashCode; import lombok.Getter; @@ -251,7 +252,7 @@ nullEmptyLists(node1); JsonNode node2 = objectMapper.valueToTree(resource); nullEmptyLists(node2); - JsonNode differences = JsonDiff.asJson(node1, node2); + JsonNode differences = JsonDiff.asJson(node1, node2, DiffFlags.dontNormalizeOpIntoMoveAndCopy()); /* @@ -308,7 +309,7 @@ } private List<PatchOperation> convertNodeToPatchOperations(String operationNode, String diffPath, JsonNode valueNode) throws IllegalArgumentException, IllegalAccessException, JsonProcessingException { - log.info(operationNode + ", " + diffPath); + log.debug("convertNodeToPatchOperations: {} , {}", operationNode, diffPath); List<PatchOperation> operations = new ArrayList<>(); PatchOperation.Type patchOpType = PatchOperation.Type.valueOf(operationNode.toUpperCase()); @@ -342,7 +343,7 @@ @SuppressWarnings("unchecked") private List<PatchOperation> handleAttributes(JsonNode valueNode, PatchOperation.Type patchOpType, ParseData parseData) throws IllegalAccessException, JsonProcessingException { - log.info("in handleAttributes"); + log.trace("in handleAttributes"); List<PatchOperation> operations = new ArrayList<>(); List<String> attributeReferenceList = new ArrayList<>(); @@ -355,7 +356,7 @@ int i = 0; for (String pathPart : parseData.pathParts) { - log.info(pathPart); + log.trace("path part: {}", pathPart); if (done) { throw new RuntimeException("Path should be done... Attribute not supported by the schema: " + pathPart); } else if (processingMultiValued) { @@ -373,7 +374,11 @@ Enum<?> tempEnum = (Enum<?>)parseData.originalObject; valueFilterExpression = new AttributeComparisonExpression(new AttributeReference("value"), CompareOperator.EQ, tempEnum.name()); } else { - log.info("Attribute: {} doesn't implement TypedAttribute, can't create ValueFilterExpression", parseData.originalObject.getClass()); + 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; @@ -386,7 +391,7 @@ if (processedMultiValued) { subAttributes.add(pathPart); } else { - log.info("Adding " + pathPart + " to attributeReferenceList"); + log.debug("Adding {} to attributeReferenceList", pathPart); attributeReferenceList.add(pathPart); }
diff --git a/scim-core/src/test/java/org/apache/directory/scim/core/repository/UpdateRequestTest.java b/scim-core/src/test/java/org/apache/directory/scim/core/repository/UpdateRequestTest.java index 669e5f9..d97df1a 100644 --- a/scim-core/src/test/java/org/apache/directory/scim/core/repository/UpdateRequestTest.java +++ b/scim-core/src/test/java/org/apache/directory/scim/core/repository/UpdateRequestTest.java
@@ -19,15 +19,12 @@ package org.apache.directory.scim.core.repository; +import static org.apache.directory.scim.core.repository.UpdateRequestTest.PatchOperationCondition.op; import static org.assertj.core.api.Assertions.assertThat; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertNull; -import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.ArrayList; import java.util.List; +import java.util.Objects; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -49,6 +46,8 @@ import org.apache.directory.scim.spec.resources.ScimUser; import org.apache.directory.scim.core.schema.SchemaRegistry; import org.apache.directory.scim.spec.schema.Schemas; +import org.assertj.core.api.AbstractAssert; +import org.assertj.core.api.Condition; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; @@ -60,6 +59,7 @@ import lombok.extern.slf4j.Slf4j; import static org.assertj.core.groups.Tuple.tuple; +import static org.junit.jupiter.api.Assertions.*; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -523,8 +523,6 @@ } @Test - @Disabled - //TODO: do asserts public void testNonTypedAttributeListGetUseablePath() throws Exception { ScimUser user1 = createUser(); ScimUser user2 = createUser(); @@ -539,15 +537,17 @@ UpdateRequest<ScimUser> updateRequest = new UpdateRequest<>("1234", user1, user2, schemaRegistry); List<PatchOperation> operations = updateRequest.getPatchOperations(); - System.out.println("Number of operations: "+operations.size()); - operations.stream().forEach(op -> System.out.println(op)); - - //TODO: perform assert that proper add and remove paths are correct + + assertThat(operations) + .hasSize(1); + PatchOperation patchOp = operations.get(0); + PatchOperationAssert.assertThat(patchOp) + .hasType(Type.REPLACE) + .hashPath(ExampleObjectExtension.URN + ":list[value EQ \"third\"]") + .hasValue(FOURTH); } @Test - @Disabled - //TODO: do asserts public void testMoveFormatNameToNicknamePart1() throws Exception { ScimUser user1 = createUser(); ScimUser user2 = createUser(); @@ -559,13 +559,13 @@ UpdateRequest<ScimUser> updateRequest = new UpdateRequest<>("1234", user1, user2, schemaRegistry); List<PatchOperation> operations = updateRequest.getPatchOperations(); - System.out.println("Number of operations: "+operations.size()); - operations.stream().forEach(op -> System.out.println(op)); + + assertThat(operations).hasSize(2); + assertThat(operations).filteredOn(op(Type.ADD, "name.formatted", nickname)).hasSize(1); + assertThat(operations).filteredOn(op(Type.REMOVE, "nickName")).hasSize(1); } @Test - @Disabled - //TODO: do asserts public void testMoveFormatNameToNicknamePart2() throws Exception { ScimUser user1 = createUser(); ScimUser user2 = createUser(); @@ -573,19 +573,19 @@ String nickname = "John Xander Anyman"; user1.setNickName(nickname); user2.setNickName(""); - - user2.getName().setFormatted(nickname); + user1.getName().setFormatted(""); + user2.getName().setFormatted(nickname); UpdateRequest<ScimUser> updateRequest = new UpdateRequest<>("1234", user1, user2, schemaRegistry); List<PatchOperation> operations = updateRequest.getPatchOperations(); - System.out.println("Number of operations: "+operations.size()); - operations.stream().forEach(op -> System.out.println(op)); + + assertThat(operations).hasSize(2); + assertThat(operations).filteredOn(op(Type.REPLACE, "name.formatted", nickname)).hasSize(1); + assertThat(operations).filteredOn(op(Type.REPLACE, "nickName", "")).hasSize(1); } @Test - @Disabled - //TODO: do asserts public void testMoveFormatNameToNicknamePart3() throws Exception { ScimUser user1 = createUser(); ScimUser user2 = createUser(); @@ -593,19 +593,19 @@ String nickname = "John Xander Anyman"; user1.setNickName(nickname); user2.setNickName(null); - - user2.getName().setFormatted(nickname); + user1.getName().setFormatted(""); + user2.getName().setFormatted(nickname); UpdateRequest<ScimUser> updateRequest = new UpdateRequest<>("1234", user1, user2, schemaRegistry); List<PatchOperation> operations = updateRequest.getPatchOperations(); - System.out.println("Number of operations: "+operations.size()); - operations.stream().forEach(op -> System.out.println(op)); + + assertThat(operations).hasSize(2); + assertThat(operations).filteredOn(op(Type.REPLACE, "name.formatted", nickname)).hasSize(1); + assertThat(operations).filteredOn(op(Type.REMOVE, "nickName")).hasSize(1); } @Test - @Disabled - //TODO: do asserts public void testMoveFormatNameToNicknamePart4() throws Exception { ScimUser user1 = createUser(); @@ -614,19 +614,19 @@ String nickname = "John Xander Anyman"; user1.setNickName(nickname); user2.setNickName(""); - - user2.getName().setFormatted(nickname); + user1.getName().setFormatted(null); + user2.getName().setFormatted(nickname); UpdateRequest<ScimUser> updateRequest = new UpdateRequest<>("1234", user1, user2, schemaRegistry); List<PatchOperation> operations = updateRequest.getPatchOperations(); - System.out.println("Number of operations: "+operations.size()); - operations.stream().forEach(op -> System.out.println(op)); + + assertThat(operations).hasSize(2); + assertThat(operations).filteredOn(op(Type.ADD, "name.formatted", nickname)).hasSize(1); + assertThat(operations).filteredOn(op(Type.REPLACE, "nickName", "")).hasSize(1); } @Test - @Disabled - //TODO: do asserts public void testMoveFormatNameToNicknamePart5() throws Exception { ScimUser user1 = createUser(); ScimUser user2 = createUser(); @@ -634,14 +634,16 @@ String nickname = "John Xander Anyman"; user1.setNickName(""); user2.setNickName(nickname); - - user2.getName().setFormatted(null); + user1.getName().setFormatted(nickname); + user2.getName().setFormatted(null); UpdateRequest<ScimUser> updateRequest = new UpdateRequest<>("1234", user1, user2, schemaRegistry); List<PatchOperation> operations = updateRequest.getPatchOperations(); - System.out.println("Number of operations: "+operations.size()); - operations.stream().forEach(op -> System.out.println(op)); + + assertThat(operations).hasSize(2); + assertThat(operations).filteredOn(op(Type.REMOVE, "name.formatted")).hasSize(1); + assertThat(operations).filteredOn(op(Type.REPLACE, "nickName", nickname)).hasSize(1); } @ParameterizedTest @@ -671,10 +673,7 @@ } else { assertEquals(expectedOp.getValue(), actualOp.getValue().toString()); } - } - System.out.println("Number of operations: "+operations.size()); - operations.stream().forEach(op -> System.out.println(op)); } @SuppressWarnings("unused") @@ -715,7 +714,7 @@ } @Test - //TODO: do parameterized test + @Disabled public void offsetTest1() throws Exception { ScimUser user1 = createUser(); ScimUser user2 = createUser(); @@ -733,7 +732,14 @@ List<PatchOperation> operations = updateRequest.getPatchOperations(); System.out.println("Number of operations: "+operations.size()); operations.stream().forEach(op -> System.out.println(op)); - + + // TODO this is likely a flaky test, below, "A" replaces one of the "Z" values, but per order of the lists it should be "D" + assertThat(operations).hasSize(7); + assertThat(operations).filteredOn(op(Type.REPLACE, ExampleObjectExtension.URN + ":list[value EQ \"Z\"]", "A")).hasSize(1); + assertThat(operations).filteredOn(op(Type.REMOVE, ExampleObjectExtension.URN + ":list[value EQ \"Z\"]")).hasSize(3); + assertThat(operations).filteredOn(op(Type.REMOVE, ExampleObjectExtension.URN + ":list[value EQ \"D\"]")).hasSize(1); + assertThat(operations).filteredOn(op(Type.REMOVE, ExampleObjectExtension.URN + ":list[value EQ \"M\"]")).hasSize(1); + assertThat(operations).filteredOn(op(Type.REMOVE, ExampleObjectExtension.URN + ":list[value EQ \"Y\"]")).hasSize(1); } @Test @@ -745,41 +751,47 @@ String nickname = "John Xander Anyman"; user1.setNickName(null); user2.setNickName(nickname); - - user2.getName().setFormatted(""); + user1.getName().setFormatted(nickname); + user2.getName().setFormatted(""); UpdateRequest<ScimUser> updateRequest = new UpdateRequest<>("1234", user1, user2, schemaRegistry); List<PatchOperation> operations = updateRequest.getPatchOperations(); - System.out.println("Number of operations: "+operations.size()); - operations.stream().forEach(op -> System.out.println(op)); + + assertThat(operations).hasSize(2); + assertThat(operations).filteredOn(op(Type.REPLACE, "name.formatted", "")).hasSize(1); + assertThat(operations).filteredOn(op(Type.ADD, "nickName", nickname)).hasSize(1); } /** * This is used to test an error condition. In this scenario a user has multiple phone numbers where home is marked primary and work is not. A SCIM update - * is performed in which the new user only contains a work phone number where the type is null. When this happens it should only only be a single DELETE + * is performed in which the new user only contains a work phone number where the type is null. When this happens it should only be a single DELETE * operation. Instead it creates four operations: replace value of the home number with the work number value, replace the home type to work, * remove the primary flag, and remove the work number */ @Test @Disabled public void testShowBugWhereDeleteIsTreatedAsMultipleReplace() throws Exception { -// final int expectedNumberOfOperationsWithoutBug = 1; -// final int expectedNumberOfOperationsWithBug = 4; -// -// ScimUser user1 = createUser1(); -// ScimUser user2 = createUser(); -// user2.getPhoneNumbers().removeIf(p -> p.getType().equals("home")); -// -// PhoneNumber workNumber = user2.getPhoneNumbers().stream().filter(p -> p.getType().equals("work")).findFirst().orElse(null); -// workNumber.setType("home"); -// assertNotNull(workNumber); -// -// UpdateRequest<ScimUser> updateRequest = new UpdateRequest<>("1234", user1, user2, registry); -// List<PatchOperation> operations = updateRequest.getPatchOperations(); -// assertNotNull(operations); -// assertEquals(expectedNumberOfOperationsWithBug, operations.size()); -// assertNotEquals(expectedNumberOfOperationsWithoutBug, operations.size()); + final int expectedNumberOfOperationsWithoutBug = 1; + final int expectedNumberOfOperationsWithBug = 4; + + ScimUser user1 = createUser(); + ScimUser user2 = createUser(); + user2.getPhoneNumbers().removeIf(p -> p.getType().equals("home")); + + PhoneNumber workNumber = user2.getPhoneNumbers().stream().filter(p -> p.getType().equals("work")).findFirst().orElse(null); + assertNotNull(workNumber); + workNumber.setType(null); + + UpdateRequest<ScimUser> updateRequest = new UpdateRequest<>("1234", user1, user2, schemaRegistry); + List<PatchOperation> operations = updateRequest.getPatchOperations(); + assertNotNull(operations); + + System.out.println("Number of operations: "+operations.size()); + operations.stream().forEach(op -> System.out.println(op)); + + assertEquals(expectedNumberOfOperationsWithBug, operations.size()); + assertNotEquals(expectedNumberOfOperationsWithoutBug, operations.size()); } private PatchOperation assertSingleResult(List<PatchOperation> result) { @@ -916,4 +928,73 @@ return patchOperations; } + static class PatchOperationAssert extends AbstractAssert<PatchOperationAssert, PatchOperation> { + + public PatchOperationAssert(PatchOperation actual) { + super(actual, PatchOperationAssert.class); + } + + public PatchOperationAssert hashPath(String path) { + isNotNull(); + PatchOperationPath actualPath = actual.getPath(); + if (actualPath == null || !actualPath.toString().equals(path)) { + failWithMessage("Expecting path in:\n <%s>\nto be: <%s>\nbut was: <%s>", actual, actualPath, path); + } + return this; + } + + public PatchOperationAssert hasType(Type opType) { + isNotNull(); + Type actualType = actual.getOperation(); + if (!Objects.equals(actualType, opType)) { + failWithMessage("Expecting operation type in:\n <%s>\nto be: <%s>\nbut was: <%s>", actual, actualType, opType); + } + return this; + } + + public PatchOperationAssert hasValue(Object value) { + isNotNull(); + Object actualValue = actual.getValue(); + if (!Objects.equals(actualValue, value)) { + failWithMessage("Expecting value in:\n <%s>\nto be: <%s>\nbut was: <%s>", actual, actualValue, value); + } + return this; + } + + public PatchOperationAssert nullValue() { + return hasValue(null); + } + + public static PatchOperationAssert assertThat(PatchOperation actual) { + return new PatchOperationAssert(actual); + } + } + + static class PatchOperationCondition extends Condition<PatchOperation> { + + private final Type type; + private final String path; + private final Object value; + + public PatchOperationCondition(Type type, String path, Object value) { + this.type = type; + this.path = path; + this.value = value; + } + + @Override + public boolean matches(PatchOperation patchOperation){ + return Objects.equals(type, patchOperation.getOperation()) && + Objects.equals(path, Objects.toString(patchOperation.getPath().toString())) && + Objects.equals(value, patchOperation.getValue()); + } + + static PatchOperationCondition op(Type type, String path, Object value) { + return new PatchOperationCondition(type, path, value); + } + + static PatchOperationCondition op(Type type, String path) { + return op(type, path, null); + } + } }
diff --git a/scim-spec/scim-spec-schema/src/main/java/org/apache/directory/scim/spec/resources/PhoneNumber.java b/scim-spec/scim-spec-schema/src/main/java/org/apache/directory/scim/spec/resources/PhoneNumber.java index 337038c..917841d 100644 --- a/scim-spec/scim-spec-schema/src/main/java/org/apache/directory/scim/spec/resources/PhoneNumber.java +++ b/scim-spec/scim-spec-schema/src/main/java/org/apache/directory/scim/spec/resources/PhoneNumber.java
@@ -32,6 +32,7 @@ import jakarta.xml.bind.annotation.XmlElement; import jakarta.xml.bind.annotation.XmlType; +import lombok.ToString; import org.antlr.v4.runtime.ANTLRInputStream; import org.antlr.v4.runtime.BaseErrorListener; import org.antlr.v4.runtime.CommonTokenStream; @@ -61,6 +62,7 @@ @XmlType @XmlAccessorType(XmlAccessType.NONE) +@ToString public class PhoneNumber extends KeyedResource implements Serializable, TypedAttribute { private static final long serialVersionUID = 607319505715224096L;