You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@directory.apache.org by "erant10 (via GitHub)" <gi...@apache.org> on 2023/02/06 22:49:50 UTC
[GitHub] [directory-scimple] erant10 opened a new pull request, #251: patch fixes and test cases
erant10 opened a new pull request, #251:
URL: https://github.com/apache/directory-scimple/pull/251
This PR addresses the following PATCH issues/cases:
- Apply PATCH to single complex attribute (e.g. User.name)
- Apply PATCH Add to a MultiValue attribute that is missing (should create the list and then add the object)
- Fixes issue where adding an item to a missing attribute create an unmodifiable map
- address the following RFC requirement [source](https://www.rfc-editor.org/rfc/rfc7644#section-3.5.2):
> For multi-valued attributes, a PATCH operation that sets a value's "primary" sub-attribute to "true" SHALL cause the server to automatically set "primary" to "false" for any other values in the array.
--
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
[GitHub] [directory-scimple] erant10 commented on a diff in pull request #251: patch fixes and test cases
Posted by "erant10 (via GitHub)" <gi...@apache.org>.
erant10 commented on code in PR #251:
URL: https://github.com/apache/directory-scimple/pull/251#discussion_r1098050861
##########
scim-core/src/main/java/org/apache/directory/scim/core/repository/PatchHandlerImpl.java:
##########
@@ -279,11 +300,14 @@ public <T extends ScimResource> void applyMultiValue(T source, Map<String, Objec
throw new UnsupportedFilterException("Attribute cannot be added, only comparison expressions are supported when the existing item does not exist.");
}
AttributeComparisonExpression comparisonExpression = (AttributeComparisonExpression) valuePathExpression.getAttributeExpression();
+ checkPrimary(subAttributeName, items, value);
- items.add(Map.of(
+ items.add(new HashMap<>(Map.of(
Review Comment:
No I suppose it doesn't. Thing is `Map.of` creates an Immutable map by default, which later causes trouble when we try to call `put`... I was trying to solve it when I came across [this](https://stackoverflow.com/questions/54963337/create-mutable-collections-map-of-and-list-of)....
But I am open to any suggestions of course
--
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
[GitHub] [directory-scimple] bdemers merged pull request #251: patch fixes and test cases
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers merged PR #251:
URL: https://github.com/apache/directory-scimple/pull/251
--
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
[GitHub] [directory-scimple] erant10 commented on pull request #251: patch fixes and test cases
Posted by "erant10 (via GitHub)" <gi...@apache.org>.
erant10 commented on PR #251:
URL: https://github.com/apache/directory-scimple/pull/251#issuecomment-1458342661
@bdemers rebased. Thanks for fixing the test :)
--
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
[GitHub] [directory-scimple] bdemers commented on a diff in pull request #251: patch fixes and test cases
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on code in PR #251:
URL: https://github.com/apache/directory-scimple/pull/251#discussion_r1098127530
##########
scim-core/src/main/java/org/apache/directory/scim/core/repository/PatchHandlerImpl.java:
##########
@@ -279,11 +300,14 @@ public <T extends ScimResource> void applyMultiValue(T source, Map<String, Objec
throw new UnsupportedFilterException("Attribute cannot be added, only comparison expressions are supported when the existing item does not exist.");
}
AttributeComparisonExpression comparisonExpression = (AttributeComparisonExpression) valuePathExpression.getAttributeExpression();
+ checkPrimary(subAttributeName, items, value);
- items.add(Map.of(
+ items.add(new HashMap<>(Map.of(
Review Comment:
```suggestion
// Add a new mutable map
items.add(new HashMap<>(Map.of(
```
--
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
[GitHub] [directory-scimple] bdemers commented on pull request #251: patch fixes and test cases
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on PR #251:
URL: https://github.com/apache/directory-scimple/pull/251#issuecomment-1450956721
@erant10 I found (and fixed) a flaky test that was causing this PR (and a few others to fail).
Can you rebase 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
[GitHub] [directory-scimple] bdemers commented on a diff in pull request #251: patch fixes and test cases
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on code in PR #251:
URL: https://github.com/apache/directory-scimple/pull/251#discussion_r1098042150
##########
scim-core/src/main/java/org/apache/directory/scim/core/repository/PatchHandlerImpl.java:
##########
@@ -279,11 +300,14 @@ public <T extends ScimResource> void applyMultiValue(T source, Map<String, Objec
throw new UnsupportedFilterException("Attribute cannot be added, only comparison expressions are supported when the existing item does not exist.");
}
AttributeComparisonExpression comparisonExpression = (AttributeComparisonExpression) valuePathExpression.getAttributeExpression();
+ checkPrimary(subAttributeName, items, value);
- items.add(Map.of(
+ items.add(new HashMap<>(Map.of(
Review Comment:
Does this need to be a HashMap?
--
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
[GitHub] [directory-scimple] erant10 commented on a diff in pull request #251: patch fixes and test cases
Posted by "erant10 (via GitHub)" <gi...@apache.org>.
erant10 commented on code in PR #251:
URL: https://github.com/apache/directory-scimple/pull/251#discussion_r1098052982
##########
scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java:
##########
@@ -127,6 +127,57 @@ public void applyAddToMultiValuedComplexAttribute() {
assertThat(updatedUser.getAddresses()).isEqualTo(expectedAddresses);
}
+ @Test
+ public void applyAddSingleComplexAttribute() {
+ ScimUser user = user();
+ PatchOperation op = patchOperation(ADD, "name.honorificSuffix", "II");
+ ScimUser updatedUser = patchHandler.apply(user, List.of(op));
+ Name expectedName = new Name()
+ .setFormatted(user.getName().getFormatted())
+ .setHonorificSuffix("II");
+ assertThat(updatedUser.getName()).isEqualTo(expectedName);
+ }
+
+ @Test
+ public void applyReplaceSingleComplexAttribute() {
+ ScimUser user = user();
+ PatchOperation op = patchOperation(REPLACE, "name.formatted", "Charlie");
+ ScimUser updatedUser = patchHandler.apply(user, List.of(op));
+ Name expectedName = new Name()
+ .setFormatted("Charlie");
+ assertThat(updatedUser.getName()).isEqualTo(expectedName);
+ }
+
+ @Test
+ public void applyAddToMissingSingleComplexAttribute() {
+ ScimUser user = user();
+ PatchOperation op = patchOperation(ADD, "addresses[type eq \"work\"].postalCode", "ko4 8qq");
+ ScimUser updatedUser = patchHandler.apply(user, List.of(op));
+ List<Address> expectedAddresses = List.of(
+ new Address()
+ .setType("work")
+ .setPostalCode("ko4 8qq"));
+ assertThat(updatedUser.getAddresses()).isEqualTo(expectedAddresses);
+ }
+
+ @Test
+ public void settingPrimaryOnMultiValuedShouldResetAllOthersToFalse() {
+ // https://www.rfc-editor.org/rfc/rfc7644#section-3.5.2
+ ScimUser user = user();
+ PatchOperation op = patchOperation(REPLACE, "emails[type eq \"home\"].primary", true);
Review Comment:
Neither did I lol
I'll pass the message on to my teammate @photonicworld
--
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
[GitHub] [directory-scimple] erant10 commented on a diff in pull request #251: patch fixes and test cases
Posted by "erant10 (via GitHub)" <gi...@apache.org>.
erant10 commented on code in PR #251:
URL: https://github.com/apache/directory-scimple/pull/251#discussion_r1098050861
##########
scim-core/src/main/java/org/apache/directory/scim/core/repository/PatchHandlerImpl.java:
##########
@@ -279,11 +300,14 @@ public <T extends ScimResource> void applyMultiValue(T source, Map<String, Objec
throw new UnsupportedFilterException("Attribute cannot be added, only comparison expressions are supported when the existing item does not exist.");
}
AttributeComparisonExpression comparisonExpression = (AttributeComparisonExpression) valuePathExpression.getAttributeExpression();
+ checkPrimary(subAttributeName, items, value);
- items.add(Map.of(
+ items.add(new HashMap<>(Map.of(
Review Comment:
No I suppose it doesn't. Thing is `Map.of` creates an Immutable map by default, which later causes trouble when we try to call `items.put`... I was trying to solve it when I came across [this](https://stackoverflow.com/questions/54963337/create-mutable-collections-map-of-and-list-of)....
But I am open to any suggestions of course
--
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
[GitHub] [directory-scimple] bdemers commented on a diff in pull request #251: patch fixes and test cases
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on code in PR #251:
URL: https://github.com/apache/directory-scimple/pull/251#discussion_r1098042811
##########
scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java:
##########
@@ -127,6 +127,57 @@ public void applyAddToMultiValuedComplexAttribute() {
assertThat(updatedUser.getAddresses()).isEqualTo(expectedAddresses);
}
+ @Test
+ public void applyAddSingleComplexAttribute() {
+ ScimUser user = user();
+ PatchOperation op = patchOperation(ADD, "name.honorificSuffix", "II");
+ ScimUser updatedUser = patchHandler.apply(user, List.of(op));
+ Name expectedName = new Name()
+ .setFormatted(user.getName().getFormatted())
+ .setHonorificSuffix("II");
+ assertThat(updatedUser.getName()).isEqualTo(expectedName);
+ }
+
+ @Test
+ public void applyReplaceSingleComplexAttribute() {
+ ScimUser user = user();
+ PatchOperation op = patchOperation(REPLACE, "name.formatted", "Charlie");
+ ScimUser updatedUser = patchHandler.apply(user, List.of(op));
+ Name expectedName = new Name()
+ .setFormatted("Charlie");
+ assertThat(updatedUser.getName()).isEqualTo(expectedName);
+ }
+
+ @Test
+ public void applyAddToMissingSingleComplexAttribute() {
+ ScimUser user = user();
+ PatchOperation op = patchOperation(ADD, "addresses[type eq \"work\"].postalCode", "ko4 8qq");
+ ScimUser updatedUser = patchHandler.apply(user, List.of(op));
+ List<Address> expectedAddresses = List.of(
+ new Address()
+ .setType("work")
+ .setPostalCode("ko4 8qq"));
+ assertThat(updatedUser.getAddresses()).isEqualTo(expectedAddresses);
+ }
+
+ @Test
+ public void settingPrimaryOnMultiValuedShouldResetAllOthersToFalse() {
+ // https://www.rfc-editor.org/rfc/rfc7644#section-3.5.2
+ ScimUser user = user();
+ PatchOperation op = patchOperation(REPLACE, "emails[type eq \"home\"].primary", true);
Review Comment:
This is a great catch I hadn't noticed that in the spec!
--
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
[GitHub] [directory-scimple] bdemers commented on a diff in pull request #251: patch fixes and test cases
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on code in PR #251:
URL: https://github.com/apache/directory-scimple/pull/251#discussion_r1098042150
##########
scim-core/src/main/java/org/apache/directory/scim/core/repository/PatchHandlerImpl.java:
##########
@@ -279,11 +300,14 @@ public <T extends ScimResource> void applyMultiValue(T source, Map<String, Objec
throw new UnsupportedFilterException("Attribute cannot be added, only comparison expressions are supported when the existing item does not exist.");
}
AttributeComparisonExpression comparisonExpression = (AttributeComparisonExpression) valuePathExpression.getAttributeExpression();
+ checkPrimary(subAttributeName, items, value);
- items.add(Map.of(
+ items.add(new HashMap<>(Map.of(
Review Comment:
nit: Does this need to be a HashMap?
--
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
[GitHub] [directory-scimple] erant10 commented on a diff in pull request #251: patch fixes and test cases
Posted by "erant10 (via GitHub)" <gi...@apache.org>.
erant10 commented on code in PR #251:
URL: https://github.com/apache/directory-scimple/pull/251#discussion_r1098056446
##########
scim-core/src/main/java/org/apache/directory/scim/core/repository/PatchHandlerImpl.java:
##########
@@ -171,6 +171,18 @@ private static void checkMutability(Attribute attribute, Object currentValue) th
}
}
+ private static void checkPrimary(String subAttributeName, Collection<Map<String, Object>> items, Object value)
+ {
+ if (subAttributeName.equals("primary") && value.equals(true)) {
Review Comment:
@bdemers where would be a good place to declare `static final PRIMARY = "primary"`?
`Schema` maybe?
--
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
[GitHub] [directory-scimple] bdemers commented on a diff in pull request #251: patch fixes and test cases
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on code in PR #251:
URL: https://github.com/apache/directory-scimple/pull/251#discussion_r1098129412
##########
scim-core/src/main/java/org/apache/directory/scim/core/repository/PatchHandlerImpl.java:
##########
@@ -171,6 +171,18 @@ private static void checkMutability(Attribute attribute, Object currentValue) th
}
}
+ private static void checkPrimary(String subAttributeName, Collection<Map<String, Object>> items, Object value)
+ {
+ if (subAttributeName.equals("primary") && value.equals(true)) {
Review Comment:
🤔 for now, just stick it in `PatchHandlerImpl`, The special rules for `primary` seems to only apply to PATCH operations.
--
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
[GitHub] [directory-scimple] bdemers commented on pull request #251: patch fixes and test cases
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on PR #251:
URL: https://github.com/apache/directory-scimple/pull/251#issuecomment-1464547337
Thanks again @erant10!
--
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