You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@directory.apache.org by "kjthorpe18 (via GitHub)" <gi...@apache.org> on 2023/12/12 14:38:59 UTC
[PR] Handle Azure style Group member remove operations [directory-scimple]
kjthorpe18 opened a new pull request, #448:
URL: https://github.com/apache/directory-scimple/pull/448
Azure sends PATCH operations to remove members from a group of the following form:
```
{
"schemas":[
"urn:ietf:params:scim:api:messages:2.0:PatchOp"
],
"Operations":[
{
"op":"remove",
"path":"members",
"value":[
{
"value":"aaebb169-b91d-481c-9957-097faeaf649a"
}
]
}
]
}
```
This is not how the SCIM spec says how to remove a single member from a group, but should be easy enough to handle this case of 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] Handle Azure style Group member remove operations [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers merged PR #448:
URL: https://github.com/apache/directory-scimple/pull/448
--
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] Handle Azure style Group member remove operations [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on code in PR #448:
URL: https://github.com/apache/directory-scimple/pull/448#discussion_r1424361352
##########
scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java:
##########
@@ -465,6 +467,39 @@ public void deleteItemWithComplexFilter() throws FilterParseException {
));
}
+ @Test
+ public void addItemToCollection() throws FilterParseException {
+ PatchOperation op = new PatchOperation();
+ op.setOperation(ADD);
+ op.setPath(PatchOperationPath.fromString("members"));
+ op.setValue(List.of(
+ Map.of(
+ "value", "9876",
+ "display", "testUser2",
+ "type", "User")
+ ));
+
+ ScimGroup updatedGroup = patchHandler.apply(group(), List.of(op));
+ assertThat(updatedGroup.getMembers().size()).isEqualTo(3);
Review Comment:
nit: may want to assert on the members of the to ensure member collection contains the expected new value
--
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] Handle Azure style Group member remove operations [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on code in PR #448:
URL: https://github.com/apache/directory-scimple/pull/448#discussion_r1424365505
##########
scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java:
##########
@@ -465,6 +467,39 @@ public void deleteItemWithComplexFilter() throws FilterParseException {
));
}
+ @Test
+ public void addItemToCollection() throws FilterParseException {
+ PatchOperation op = new PatchOperation();
+ op.setOperation(ADD);
+ op.setPath(PatchOperationPath.fromString("members"));
+ op.setValue(List.of(
+ Map.of(
+ "value", "9876",
+ "display", "testUser2",
+ "type", "User")
+ ));
+
+ ScimGroup updatedGroup = patchHandler.apply(group(), List.of(op));
+ assertThat(updatedGroup.getMembers().size()).isEqualTo(3);
+ }
+
+ /**
+ * This test covers Azure-style remove member operations, where a value is
+ * specified in the operation body, rather than in the path.
+ * For example: { "op": "remove", "path": "members", "value": [ { "value": "1234" } ] }
+ */
+ @Test
+ public void removeItemFromCollection() throws FilterParseException {
+ PatchOperation op = new PatchOperation();
+ op.setOperation(REMOVE);
+ op.setPath(PatchOperationPath.fromString("members"));
+ op.setValue(List.of(Map.of("value", "1234")));
+
+ ScimGroup updatedGroup = patchHandler.apply(group(), List.of(op));
+ assertThat(updatedGroup.getMembers()).isNotNull();
+ assertThat(updatedGroup.getMembers().size()).isEqualTo(1);
Review Comment:
As an aside, (and not part of this PR, selfishly asking for #435), do you think creating custom AssertJ assertions would be helpful? or do you think it's more readable to have multiple assertions?
Maybe something, that would read like:
```java
assertThat(updatedGroup)
.hasMemberCount(3)
.containsMember(<expected>)
```
Mostly thinking out loud, we have a util method in PatchGeneratorTest, but I've been thinking the messages would be more clear with custom assertions.
https://github.com/apache/directory-scimple/blob/ebb01f4d022fda36f9d28315ad0ac1d9427f9ac2/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchGeneratorTest.java#L754-L762
(I've been using Hamcrest for years, so its taken longer than it should have to get comfortable with AssertJ 🤣 )
##########
scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java:
##########
@@ -465,6 +467,39 @@ public void deleteItemWithComplexFilter() throws FilterParseException {
));
}
+ @Test
+ public void addItemToCollection() throws FilterParseException {
+ PatchOperation op = new PatchOperation();
+ op.setOperation(ADD);
+ op.setPath(PatchOperationPath.fromString("members"));
+ op.setValue(List.of(
+ Map.of(
+ "value", "9876",
+ "display", "testUser2",
+ "type", "User")
+ ));
+
+ ScimGroup updatedGroup = patchHandler.apply(group(), List.of(op));
+ assertThat(updatedGroup.getMembers().size()).isEqualTo(3);
+ }
+
+ /**
+ * This test covers Azure-style remove member operations, where a value is
+ * specified in the operation body, rather than in the path.
+ * For example: { "op": "remove", "path": "members", "value": [ { "value": "1234" } ] }
+ */
+ @Test
+ public void removeItemFromCollection() throws FilterParseException {
+ PatchOperation op = new PatchOperation();
+ op.setOperation(REMOVE);
+ op.setPath(PatchOperationPath.fromString("members"));
+ op.setValue(List.of(Map.of("value", "1234")));
+
+ ScimGroup updatedGroup = patchHandler.apply(group(), List.of(op));
+ assertThat(updatedGroup.getMembers()).isNotNull();
+ assertThat(updatedGroup.getMembers().size()).isEqualTo(1);
Review Comment:
As an aside, (and not part of this PR, selfishly asking for #435), do you think creating custom AssertJ assertions would be helpful? or do you think it's more readable to have multiple assertions?
Maybe something, that would read like:
```java
assertThat(updatedGroup)
.hasMemberCount(3)
.containsMember(<expected>)
```
Mostly thinking out loud, we have a util method in PatchGeneratorTest, but I've been thinking the messages would be more clear with custom assertions.
https://github.com/apache/directory-scimple/blob/ebb01f4d022fda36f9d28315ad0ac1d9427f9ac2/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchGeneratorTest.java#L754-L762
(I've been using Hamcrest for years, so its taken longer than it should have to get comfortable with AssertJ 🤣 )
--
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] Handle Azure style Group member remove operations [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on PR #448:
URL: https://github.com/apache/directory-scimple/pull/448#issuecomment-1861942949
@kjthorpe18 Can you rebase one more time?
After that 4ded7f3a971dc8b9441d1dc41bc1808bb36a49ac should apply clean, and we should be able to ship it!
--
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] Handle Azure style Group member remove operations [directory-scimple]
Posted by "kjthorpe18 (via GitHub)" <gi...@apache.org>.
kjthorpe18 commented on code in PR #448:
URL: https://github.com/apache/directory-scimple/pull/448#discussion_r1428820471
##########
scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java:
##########
@@ -465,6 +467,39 @@ public void deleteItemWithComplexFilter() throws FilterParseException {
));
}
+ @Test
+ public void addItemToCollection() throws FilterParseException {
+ PatchOperation op = new PatchOperation();
+ op.setOperation(ADD);
+ op.setPath(PatchOperationPath.fromString("members"));
+ op.setValue(List.of(
+ Map.of(
+ "value", "9876",
+ "display", "testUser2",
+ "type", "User")
+ ));
+
+ ScimGroup updatedGroup = patchHandler.apply(group(), List.of(op));
+ assertThat(updatedGroup.getMembers().size()).isEqualTo(3);
+ }
+
+ /**
+ * This test covers Azure-style remove member operations, where a value is
+ * specified in the operation body, rather than in the path.
+ * For example: { "op": "remove", "path": "members", "value": [ { "value": "1234" } ] }
+ */
+ @Test
+ public void removeItemFromCollection() throws FilterParseException {
+ PatchOperation op = new PatchOperation();
+ op.setOperation(REMOVE);
+ op.setPath(PatchOperationPath.fromString("members"));
+ op.setValue(List.of(Map.of("value", "1234")));
+
+ ScimGroup updatedGroup = patchHandler.apply(group(), List.of(op));
+ assertThat(updatedGroup.getMembers()).isNotNull();
+ assertThat(updatedGroup.getMembers().size()).isEqualTo(1);
Review Comment:
I've also really only used Hamcrest and JUnit assertions - this would be a good thing to add in a separate 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