You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@directory.apache.org by "jasonfagerberg-toast (via GitHub)" <gi...@apache.org> on 2025/01/17 14:37:15 UTC
[PR] Fixed issue with PATCH incorrectly replacing entire complex attr [directory-scimple]
jasonfagerberg-toast opened a new pull request, #726:
URL: https://github.com/apache/directory-scimple/pull/726
Fix for: https://github.com/apache/directory-scimple/issues/725
--
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] Fixed issue with PATCH incorrectly replacing entire complex attr [directory-scimple]
Posted by "jasonfagerberg-toast (via GitHub)" <gi...@apache.org>.
jasonfagerberg-toast commented on PR #726:
URL: https://github.com/apache/directory-scimple/pull/726#issuecomment-2608168489
@bdemers hey just wondering on the release process for this library? Since the CI passed is this available on maven for me to pull down once merged?
--
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] Fixed issue with PATCH incorrectly replacing entire complex attr [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on PR #726:
URL: https://github.com/apache/directory-scimple/pull/726#issuecomment-2667473299
@jasonfagerberg-toast still working on it! I'm working through an access issue when publishing the release.
However, we are now publishing snapshots: https://repository.apache.org/content/repositories/snapshots/
And I've temporarily staged the release here: https://repository.apache.org/content/repositories/orgapachedirectory-1241/
> [!IMPORTANT]
> This staging repo is temporary, and it's possible the artifact may change before the actual release.
That said, please test out the build and let us know if you run into any issues!
--
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] Fixed issue with PATCH incorrectly replacing entire complex attr [directory-scimple]
Posted by "jasonfagerberg-toast (via GitHub)" <gi...@apache.org>.
jasonfagerberg-toast commented on PR #726:
URL: https://github.com/apache/directory-scimple/pull/726#issuecomment-2598519161
I think one issue here is if a complex attribute is within a complex attribute, i.e. this request
```json
{
"schemas": [
"urn:ietf:params:scim:api:messages:2.0:PatchOp"
],
"Operations": [
{
"op": "replace",
"value": {
"name": {
"givenName": "New Given Name"
}
}
}
]
}
```
I think the behavior will still be that `name` is replaced entirely but ill write a test to see
--
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] Fixed issue with PATCH incorrectly replacing entire complex attr [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on PR #726:
URL: https://github.com/apache/directory-scimple/pull/726#issuecomment-2608232953
Hey @jasonfagerberg-toast I should be able to get the release kicked off tomorrow or Friday.
The ASF release process requires the release to be voted on over the course of a few days.
Community feedback is always welcome (even if it's just a quick +1)
The vote will be held on the 'dev' mailing list: https://directory.apache.org/mailing-lists-and-irc.html#existing-mailing-lists
Thanks again for the PRs!
--
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] Fixed issue with PATCH incorrectly replacing entire complex attr [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on PR #726:
URL: https://github.com/apache/directory-scimple/pull/726#issuecomment-2626294271
@jasonfagerberg-toast I haven't forgot about the release.
I was tracking down another issue (that's been resolved so we should be good to go with the release!)
--
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] Fixed issue with PATCH incorrectly replacing entire complex attr [directory-scimple]
Posted by "jasonfagerberg-toast (via GitHub)" <gi...@apache.org>.
jasonfagerberg-toast commented on PR #726:
URL: https://github.com/apache/directory-scimple/pull/726#issuecomment-2598523178
> I think the behavior will still be that `name` is replaced entirely but ill write a test to see
Added a test and its passing so nice work everyone, take off early for the day
--
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] Fixed issue with PATCH incorrectly replacing entire complex attr [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on code in PR #726:
URL: https://github.com/apache/directory-scimple/pull/726#discussion_r1920522921
##########
scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java:
##########
@@ -56,6 +56,46 @@ public PatchHandlerTest() {
this.patchHandler = new DefaultPatchHandler(schemaRegistry);
}
+ @Test
+ public void applyReplaceComplexAttribute() {
+ // setup existing user
+ Name existingName = new Name();
+ existingName.setFamilyName("Family Name");
+ existingName.setGivenName("Given Name");
+ ScimUser user = user();
+ user.setName(existingName);
+
+ // setup patch ops
+ Map<String, String> newName = Map.of("familyName", "New Family Name");
+ PatchOperation op = patchOperation(REPLACE, "name", newName);
+
+ //execute
+ ScimUser updatedUser = patchHandler.apply(user, List.of(op));
+ assertThat(updatedUser.getName().getFamilyName()).isEqualTo("New Family Name");
+ // assert that PATCH did not update fields not provided in PatchOperation
+ assertThat(updatedUser.getName().getGivenName()).isNotNull();
Review Comment:
nit: we may want to assert the original value here instead of null.
```suggestion
assertThat(updatedUser.getName().getGivenName()).isEqualTo("Given Name");
```
##########
scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java:
##########
@@ -56,6 +56,46 @@ public PatchHandlerTest() {
this.patchHandler = new DefaultPatchHandler(schemaRegistry);
}
+ @Test
+ public void applyReplaceComplexAttribute() {
+ // setup existing user
+ Name existingName = new Name();
+ existingName.setFamilyName("Family Name");
+ existingName.setGivenName("Given Name");
+ ScimUser user = user();
+ user.setName(existingName);
+
+ // setup patch ops
+ Map<String, String> newName = Map.of("familyName", "New Family Name");
+ PatchOperation op = patchOperation(REPLACE, "name", newName);
+
+ //execute
+ ScimUser updatedUser = patchHandler.apply(user, List.of(op));
+ assertThat(updatedUser.getName().getFamilyName()).isEqualTo("New Family Name");
+ // assert that PATCH did not update fields not provided in PatchOperation
+ assertThat(updatedUser.getName().getGivenName()).isNotNull();
+ }
+
+ @Test
+ public void applyReplaceNestedComplexAttribute() {
+ // setup existing user
+ Name existingName = new Name();
+ existingName.setFamilyName("Family Name");
+ existingName.setGivenName("Given Name");
+ ScimUser user = user();
+ user.setName(existingName);
+
+ // setup patch ops
+ Map<String, Map<String, String>> newName = Map.of("name", Map.of("familyName", "New Family Name"));
+ PatchOperation op = patchOperation(REPLACE, null, newName);
+
+ //execute
+ ScimUser updatedUser = patchHandler.apply(user, List.of(op));
+ assertThat(updatedUser.getName().getFamilyName()).isEqualTo("New Family Name");
+ // assert that PATCH did not update fields not provided in PatchOperation
+ assertThat(updatedUser.getName().getGivenName()).isNotNull();
Review Comment:
```suggestion
assertThat(updatedUser.getName().getGivenName()).isEqualTo("Given Name");
```
--
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] Fixed issue with PATCH incorrectly replacing entire complex attr [directory-scimple]
Posted by "jasonfagerberg-toast (via GitHub)" <gi...@apache.org>.
jasonfagerberg-toast commented on PR #726:
URL: https://github.com/apache/directory-scimple/pull/726#issuecomment-2666240550
> @jasonfagerberg-toast I haven't forgot about the release. I was tracking down another issue (that's been resolved so we should be good to go with the release!)
Hey @bdemers just checking in again for when you think this will be released?
--
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] Fixed issue with PATCH incorrectly replacing entire complex attr [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers merged PR #726:
URL: https://github.com/apache/directory-scimple/pull/726
--
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