You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@directory.apache.org by bd...@apache.org on 2023/01/03 18:45:59 UTC
[directory-scimple] 01/01: Enable UpdateRequest patch related tests
This is an automated email from the ASF dual-hosted git repository.
bdemers pushed a commit to branch patch-tests
in repository https://gitbox.apache.org/repos/asf/directory-scimple.git
commit f7ff76b340e54718b29024e5fef84424e2fd687f
Author: Brian Demers <bd...@apache.org>
AuthorDate: Tue Jan 3 13:45:50 2023 -0500
Enable UpdateRequest patch related tests
These tests were disabled, so they have been reworked with a new PatchOperationAssert
---
.../scim/core/repository/UpdateRequest.java | 17 +-
.../scim/core/repository/UpdateRequestTest.java | 211 ++++++++++++++-------
.../directory/scim/spec/resources/PhoneNumber.java | 2 +
3 files changed, 159 insertions(+), 71 deletions(-)
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 f7ea9208..35b5a069 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.POJONode;
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 @@ public class UpdateRequest<T extends ScimResource> {
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 @@ public class UpdateRequest<T extends ScimResource> {
}
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 @@ public class UpdateRequest<T extends ScimResource> {
@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 @@ public class UpdateRequest<T extends ScimResource> {
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 @@ public class UpdateRequest<T extends ScimResource> {
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 @@ public class UpdateRequest<T extends ScimResource> {
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 669e5f9e..d97df1ac 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.Photo;
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.Data;
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 @@ public class UpdateRequestTest {
}
@Test
- @Disabled
- //TODO: do asserts
public void testNonTypedAttributeListGetUseablePath() throws Exception {
ScimUser user1 = createUser();
ScimUser user2 = createUser();
@@ -539,15 +537,17 @@ public class UpdateRequestTest {
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 @@ public class UpdateRequestTest {
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 @@ public class UpdateRequestTest {
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 @@ public class UpdateRequestTest {
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 @@ public class UpdateRequestTest {
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 @@ public class UpdateRequestTest {
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 @@ public class UpdateRequestTest {
} 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 @@ public class UpdateRequestTest {
}
@Test
- //TODO: do parameterized test
+ @Disabled
public void offsetTest1() throws Exception {
ScimUser user1 = createUser();
ScimUser user2 = createUser();
@@ -733,7 +732,14 @@ public class UpdateRequestTest {
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 @@ public class UpdateRequestTest {
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 @@ public class UpdateRequestTest {
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 337038c4..917841db 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.XmlAccessorType;
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 @@ import lombok.Setter;
@XmlType
@XmlAccessorType(XmlAccessType.NONE)
+@ToString
public class PhoneNumber extends KeyedResource implements Serializable, TypedAttribute {
private static final long serialVersionUID = 607319505715224096L;