You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@directory.apache.org by "bdemers (via GitHub)" <gi...@apache.org> on 2023/11/29 15:35:19 UTC
[I] Add ScimGroup tests for PatchGenerator, (previously)UpdateRequest [directory-scimple]
bdemers opened a new issue, #427:
URL: https://github.com/apache/directory-scimple/issues/427
There are a couple gaps in the test coverage for `PatchGenerator`, specifically in the area of generating diffs for collections.
Add tests common for ScimGroup patch operations:
Modify displayName - REPLACE op
add member(s) - ADD op
remove member(s) - REMOVE op
replace member(s) - when should this be used over multiple add/remove operations? e.g. 1 replace = 1 add + 1 remove
^ This is the tricky one, after a little poking around it looks like we need to change how we are using the JSON diff library.
Take this use case for example:
The original Group Resource has member refs:
``` yaml
...
value: user1
```
After being modifed, it has:
``` yaml
...
value: user2
```
This is actually the removal of User1, and the addition of User2, and NOT the replacement of the `value` field.
For reference see:
- https://github.com/apache/directory-scimple/pull/411#pullrequestreview-1748295568
- https://github.com/apache/directory-scimple/pull/411#discussion_r1404587890
--
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.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: [I] Add ScimGroup tests for PatchGenerator, (previously)UpdateRequest [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on issue #427:
URL: https://github.com/apache/directory-scimple/issues/427#issuecomment-1846315481
Digging into the `REPLACE` vs `REMOVE`/`ADD` a bit more.
I hacked up some (really ugly) code to try to understand how the logic in the `PatchGenerator` (previously `UpdateRequest`) works.
As @kjthorpe18 mentioned in #411: https://github.com/apache/directory-scimple/pull/411#discussion_r1404579340
I think any time collections are diffed, changes should result in a remove/add (instead of a replace).
I don't think it's possible to accurately make assumptions about when a replace could be used.
We have a test that expects a `replace` when updating an email address.
original:
```json
"emails": [{
"type": "work",
"value": "jxa123@psu.edu"}
]
```
new:
```json
"emails": [{
"type": "work",
"value": "nobody@example.com"}
]
```
This could result in:
```
type: REPLACE
path: emails[type EQ "work"].value
value: nobody@example.com
```
Or this:
```
type: REMOVE
path: emails[value EQ "jxa123@psu.edu"]
type: ADD
path: emails
value: {
"type": "work",
"value": "nobody@example.com"}
```
I think it's _safer_ to assume the latter.
> **NOTE:** Generating patches is not the same as applying patches, which does need to support both REMOVE/ADD and REPLACE 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
Re: [I] Add ScimGroup tests for PatchGenerator, (previously)UpdateRequest [directory-scimple]
Posted by "joshuapsteele (via GitHub)" <gi...@apache.org>.
joshuapsteele commented on issue #427:
URL: https://github.com/apache/directory-scimple/issues/427#issuecomment-1840934004
@bdemers I'm having issues getting the `GroupIT` tests (or any ITs, for that matter) to run locally. `./mvnw package` seems to skip right over them, unless I've missed something! (Perhaps this merits another issue.)
Anyway, the important point in this thread is that we need a test that covers the following PATCH operation to remove a user from a group. Instead of specifying the user ID in the `path` (as is currently covered in the tests), Azure, for example, specifies the user ID in the `value` like so:
```
String patchBody = "{" +
"\"schemas\": [\"urn:ietf:params:scim:api:messages:2.0:PatchOp\"]," +
"\"Operations\": [{" +
"\"op\": \"remove\"," +
"\"path\": \"members\"," +
"\"value\": [{\"value\": \"" + userId + "\"}]" +
"}]}";
```
--
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: [I] Add ScimGroup tests for PatchGenerator, (previously)UpdateRequest [directory-scimple]
Posted by "kjthorpe18 (via GitHub)" <gi...@apache.org>.
kjthorpe18 commented on issue #427:
URL: https://github.com/apache/directory-scimple/issues/427#issuecomment-1850082458
I looked at the RFC again and I believe I was misunderstanding `replace` ops with Group members. I don't think it's possible to replace one `member` value with another, but rather a `replace` on `members` replaces _all_ of the members with a specified list.
```
The following example shows how to replace all of the members of a
group with a different members list in a single replace operation.
Some text was removed for readability (indicated by "..."):
PATCH /Groups/acbf3ae7-8463-4692-b4fd-9b4da3f908ce
Host: example.com
Accept: application/scim+json
Content-Type: application/scim+json
Authorization: Bearer h480djs93hd8
If-Match: W/"a330bc54f0671c9"
{
"schemas":
["urn:ietf:params:scim:api:messages:2.0:PatchOp"],
"Operations": [{
"op":"replace",
"path":"members",
"value":[
{
"display": "Babs Jensen",
"$ref":
"https://example.com/v2/Users/2819c223...413861904646",
"value": "2819c223...413861904646"
},
{
"display": "James Smith",
"$ref":
"https://example.com/v2/Users/08e1d05d...473d93df9210",
"value": "08e1d05d...473d93df9210"
}
]
}]
}
```
So, at least with group membership PATCH generation, `replace` is a bit more nuanced. You could still `remove` every user and `add` another, but it's not as I though `1 add + `1 remove` = `1 replace`.
--
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: [I] Add ScimGroup tests for PatchGenerator, (previously)UpdateRequest [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on issue #427:
URL: https://github.com/apache/directory-scimple/issues/427#issuecomment-1874648623
Added tests in #411
--
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: [I] Add ScimGroup tests for PatchGenerator, (previously)UpdateRequest [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on issue #427:
URL: https://github.com/apache/directory-scimple/issues/427#issuecomment-1841422206
@joshuapsteele starting the thread here works, if we figure out it's something else, we can move it to another issue later.
Here is some background, hopefully this helps you troubleshoot, we can try to collect more info.
The IT's are are created in the `scim-compliance-tests` module and are executed in other modules (Spring support, other examples, etc).
**NOTE:** IIRC, the Quarkus example is a special case to ensure everything works with the Quarkus tools
Because of this, no tests are executed as part of the `scim-compliance-tests` module. But if you run the full build, you should see the failsafe plugin execute the ITs in the other module, for example you should see the test result file:
`scim-server-examples/scim-server-jersey/target/failsafe-reports/org.apache.directory.scim.compliance.tests.GroupsIT.txt`
The other thing you may see is the build cache, after running a clean build once, your next clean build (assuming you didn't make any changes) would get a lot of cache hits.
You can disable the build cache by using the flag: `-Dgradle.cache.local.enabled`, e.g.:
```sh
./mvnw clean install -Dgradle.cache.local.enabled=false
```
Hopefully that helps, and if that was the problem, I can add this info to the README or some sort of CONTRIB guide (interested in your thoughts)
---
One other note, you _should_ be able to run a single IT from your IDE (from the `scim-compliance-tests` module), by default that test will try to run against a SCIM server at: `http://localhost:8080/v2`
If that doesn't match your server under test URL, you can quickly hack up:
https://github.com/apache/directory-scimple/blob/develop/scim-compliance-tests/src/main/java/org/apache/directory/scim/compliance/tests/ScimpleITSupport.java#L51-L52
But to make that more flexible, we could add something here:
https://github.com/apache/directory-scimple/blob/develop/scim-compliance-tests/src/main/java/org/apache/directory/scim/compliance/junit/EmbeddedServerExtension.java#L56
Maybe read from a system property `scimple.it.url` or something 🤷
The goal of this is to facility quickly running individual tests or while iterating on adding a new IT (without needing to run the full test-suite or build)
Hopefully this helps!!
--
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: [I] Add ScimGroup tests for PatchGenerator, (previously)UpdateRequest [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers closed issue #427: Add ScimGroup tests for PatchGenerator, (previously)UpdateRequest
URL: https://github.com/apache/directory-scimple/issues/427
--
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