Applying Patches to resources #213
Conversation
These tests were disabled, so they have been reworked with a new PatchOperationAssert
|
Related PR for cleaning up/enabling patch related tests: #217 |
| import com.fasterxml.jackson.module.jakarta.xmlbind.JakartaXmlBindAnnotationIntrospector; | ||
| import com.fasterxml.jackson.module.jakarta.xmlbind.JakartaXmlBindAnnotationModule; | ||
|
|
||
| public class ObjectMapperProvider { |
There was a problem hiding this comment.
I recently refactored some of the JSON/ObjectMapper bits.
I'm not 100% that will help with this usage, but it is something we can investigate.
This may come into play when dealing with patches that contain values from SCIM Extensions.
That said, with your approach using objectAsMap it may not make a difference.
(Mostly a note to myself, to go back and look at the ObjectMapper-related bits 😄)
There was a problem hiding this comment.
of course, feel free to merge :)
Yeah I got the map conversion idea from this pr - #29
which might not be the most efficient but it seems simple enough...
|
I almost forgot, but if you haven't already, can you fill out an Apache ICLA (or CCLA in needed) |
|
@bdemers My main concern with this implementation is that PatchHandler is coupled with InMemoryFilterExpression, which is only meant to be used with the in-memory implementation... I wonder if there is a way to abstract-ify this FilterExpression so that it can be easily modified for non-demo purposes... |
|
@erant10 good call out on the I think it's use is appropriate as the patch only affects a single resource (or possibly a small subset), so there shouldn't be any major perf issues. (which was mostly my original concern, e.g. return a large collection from a datastore and then filter it down in memory) I like your idea of somehow abstracting the useful bits out, but I'll throw out some related ideas too:
Mostly thinking out loud, I'm not 100% sure that the last bit adds much value, as its very implementation-specific, and anyone who creates a Thoughts? |
# Conflicts: # scim-core/src/main/java/org/apache/directory/scim/core/repository/UpdateRequest.java
|
I merged in #217 and resolved a couple of minor conflicts
|
|
Note: if needed, we can clean up the history before merging, but there is probably more value in hacking on all the patch-related bits in one branch 💥 |
|
@erant10 I just used this branch to do a quick test with Okta's group membership patch requests and it worked great! Awesome work! |
|
@bdemers Sorry for the delay, just got back from vacation :) |
|
Related I created #233, (only the last commit is relevant) |
I like that change. do you think it makes sense to merge it as a separate PR, or rather include it in this one? |
|
@erant10 if we can split it up that would be ideal... but, it's all related to getting the patch bits working, so I personally wouldn't loose any sleep over getting everything in a single branch. We could clean up the history a little before merging (but IMHO, that not required unless it makes the change set more readable) |
| if (actual instanceof ScimResource) { | ||
| // actual is the top level scim resource - need to extract the top attribute. | ||
| // otherwise we can move on directly to the sub-attribute | ||
| actual = schemaAttribute.getAccessor().get(actual); | ||
| } |
There was a problem hiding this comment.
@erant10
🤔 This bit has me confused.
Locally, I reverted this back the previous try/catch/todo, and all the tests pass.
But I haven't been able to figure out what condition causes this error. Any ideas?
There was a problem hiding this comment.
actually this is a bug I just fixed in my latest commit. My original thought was that we should only "drill down" into the object if actual is a ScimResource (i.e. User/Group) but not if its an Attribute (e.g. Email). But I was wrong, and thats what was breaking the test lol. Anyway I added a method to the Schema interface which will check if a field is accessible. I think this fixes it, let me know if it makes more sense now
erant10
changed the title
Detect complex multivalue fitlers in InMemoryScimFitlerMatcher
| } | ||
| else { | ||
| Attribute attribute = baseSchema.getAttribute(attributeReference.getAttributeName()); | ||
| checkMutability(attribute); |
Add test cases for patch replace/remove with filter
Create Patch Operation handlers for each type of OP
…esource This allows for some complexity reduction and has a side benefit of traversing the object graph once per request vs once per patch op The `checkMutabilty` logic was also tweaked to allow setting of an immutable attribute if there is no existing value per rfc7644 sec 3.5.2 > a client MUST NOT modify an attribute that has mutability "readOnly" or "immutable". However, a client MAY "add" a value to an "immutable" attribute if the attribute had no previous value.
| } | ||
| } | ||
|
|
||
| private static void checkMutability(Attribute attribute, Object currentValue) throws IllegalArgumentException { |
There was a problem hiding this comment.
My last change litters the code with a few more checkMutability calls, to account for rfc7644 sec 3.5.2
a client MUST NOT modify an attribute that has mutability "readOnly" or "immutable". However, a client MAY "add" a value to an "immutable" attribute if the attribute had no previous value.
I'm not really a fan of spreading of how I spread this logic around, but needing the object's current value makes it tricky.
I went down the rabbit hole of creating a Collection/Map/Iterator/etc that could wrap a delegate and then make a mutability check if/when the attribute was mutated in the map. It was a neat bit of code, but it was ugly and required special handling for extensions so I threw it away. There is probably something more elegant that could be done, but I'm out of ideas 😆
It's not critical to get this in, but if you have any thoughts let me know!
Improved a bit of javadoc along the way too
|
@erant10 are we ready to merge this? |
|
@bdemers Yes I believe so! |
|
@erant10 perfect! I'll merge it! |
2 participants
No description provided.