Move attribute and etag logic into Repository #411

bdemers · 2023-11-13T17:05:36Z

Breaks up the current update method into new update and patch methods.
(Breaking change)

ETag/Version logic must be managed by Repositories, except for the case of ServiceProviderConfiguration (which is likely stored in memory)
- Implementations should set scimResource.meta.version, how this version is defined is up to the implementation
The UpdateRequest class has been removed, the getPatchOperations method has been moved into a new class PatchGenerator
Current examples have been updated and demo simple in memory patch application. e.g. Get/apply patch/persist.
- Other implementations may want generate update queries directly from the list of PatchOperations (skipping intermediate get step)

joshuapsteele · 2023-11-15T11:48:09Z

scim-core/src/main/java/org/apache/directory/scim/core/repository/Repository.java

+  T update(String id, String version, T resource, Set<AttributeReference> includedAttributes, Set<AttributeReference> excludedAttributes) throws ResourceException;
+
+  /**
+   * Allows the SCIM server's REST implementation to update and existing


Suggested change

* Allows the SCIM server's REST implementation to update and existing

* Allows the SCIM server's REST implementation to update an existing

...server/src/main/java/org/apache/directory/scim/server/rest/BaseResourceTypeResourceImpl.java

joshuapsteele

Overall, this is looking good to me. Will definitely help us optimize database operations on our end when IdPs like Azure send us a bunch of single-user PATCH /Groups requests!

However, can we get some better comments/documentation about what, specifically, about the incoming PUT or PATCH requests determines whether the update() or patch() flows are used? In general, I tend to get lost in SCIMple when looking for the endpoint/controller-level logic. In this PR as well, I've missed (at least on a first pass) where the main logic is for SCIMple to decide whether to use update() or patch() based on the incoming request that it's handling.

bdemers · 2023-11-15T16:55:23Z

However, can we get some better comments/documentation about what, specifically, about the incoming PUT or PATCH requests determines whether the update() or patch() flows are used? In general, I tend to get lost in SCIMple when looking for the endpoint/controller-level logic. In this PR as well, I've missed (at least on a first pass) where the main logic is for SCIMple to decide whether to use update() or patch() based on the incoming request that it's handling.

Absolutely!

IMHO, the high level goal is that folks shouldn't need to know any of the detail of the REST layer, and only need to implement the Repository API.

This will need to be expanded on in the javadoc and other docs, but here are some key points:

There are two ways to modify an existing resource:
- a replacement of a resource, PUT request, which calls repository.update(...)
- a partial update of a resource, PATCH request which calls repository.patch(...)

Maybe it would be helpful to have a table mapping the SCIM protocol requests back to the repository method:

(not completely accurate, mostly thinking out loud for how this might look)

REST Endpoint	REST method	Repository method
/Users	`GET`	`repository.query()`
/Users/{id} `	`PUT`	`repository.update()`
/Users/{id} `	`PATCH`	`repository.patch()`
...

As an aside, while talking about the javadocs, I'm not sure the Swagger/OpenAPI annotations in the REST layer add value to the user. (As the OpenAPI spec for SCIM could be defined by the body managing the RFC, or other project, that OpenAPI spec doesn't change unless the RFC is updated)
Anyway, I've wondered if removing it would make it easier to read/understand the code. (Or if I'm just over thinking it and that's not a problem at all)

joshuapsteele

Just a couple nits, really (typo, remove commented-out lines). Otherwise, this approach looks good to me. And it would really help us optimize group membership updates on our end!

erant10 · 2023-11-17T19:44:46Z

...rsey/src/main/java/org/apache/directory/scim/example/jersey/service/InMemoryUserService.java

+
+  @Override
+  public ScimUser patch(String id, String version, List<PatchOperation> patchOperations, Set<AttributeReference> includedAttributeReferences, Set<AttributeReference> excludedAttributeReferences) throws ResourceException {
+    PatchHandler patchHandler = new PatchHandlerImpl(schemaRegistry);


would it make sense to inject PatchHandlerImpl to the Repositories instead of instantiating per request?

I just pushed an updated, and injected the PatchHandler, please take a look!

erant10 · 2023-11-17T20:12:05Z

scim-core/src/main/java/org/apache/directory/scim/core/repository/PatchGenerator.java

+import java.util.*;
+import java.util.stream.Collectors;
+
+public class PatchGenerator {


I don't see this class used anywhere, so just making sure I understand - what exactly is the purpose of PatchGenerator? is it meant to be used optionally by anyone who wants to convert an update request into a patch request in their repo implementation?

Perhaps a short comment on this class and its intended use could be helpful for future adopters?

💯
Added Javadoc comments!

scim-core/src/main/java/org/apache/directory/scim/core/repository/Repository.java

kjthorpe18

LGTM! Just a few small nits. Nice work here!

scim-core/src/main/java/org/apache/directory/scim/core/repository/PatchGenerator.java

kjthorpe18 · 2023-11-24T18:20:26Z

scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchGeneratorTest.java

This covers ScimUser resources very well, but some basic tests for ScimGroup resources would be useful, too.

Modify displayName

add member(s)

remove member(s)

replace member(s) - when should this be used over multiple add/remove operations? e.g. 1 replace = 1 add + 1 remove

These are great!

The first 3 are pretty easy, the last one is a bit more complex, and will require a bit more work (and likely changes as to the JSON diff lib is invoked)

Since this logic didn't change in this PR (it was moved from a different class)
I'll create a separate issue to track this (and I'll stick the current tests i have in a new PR, as a starting point)

Created #427

kjthorpe18 · 2023-11-24T18:39:57Z

scim-core/src/main/java/org/apache/directory/scim/core/repository/PatchGenerator.java

+      if (patchOpType == PatchOperation.Type.REPLACE && (parseData.resourceObject == null ||
+        (parseData.resourceObject instanceof Collection && ((Collection<?>)parseData.resourceObject).isEmpty()))) {
+        patchOpType = PatchOperation.Type.REMOVE;
+        valueNode = null;
+      }
+
+      if (patchOpType == PatchOperation.Type.REPLACE && parseData.originalObject == null) {
+        patchOpType = PatchOperation.Type.ADD;
+      }


I see this is part what I mentioned in another comment - a replace operation actually being an add/remove depending on the original or target resource having null

joshuapsteele

@bdemers Still think these changes look great and I'm eager to see them incorporated into the project! Who else do we need to bug so that they can review this PR? :)

Breaks up the current `update` method into new `update` and `patch` methods. (Breaking change) - ETag/Version logic must be managed by Repositories, except for the case of `ServiceProviderConfiguration` (which is likely stored in memory) - Implementations should set `scimResource.meta.version`, how this version is defined is up to the implementation - The `UpdateRequest` class has been removed, the `getPatchOperations` method has been moved into a new class `PatchGenerator` - Current examples have been updated and demo simple in memory patch application. e.g. Get/apply patch/persist. - Other implementations may want generate update queries directly from the list of PatchOperations (skipping intermediate get step)

Rename PatchHandlerImpl to DefaultPatchHandler (users of SCIMple could create different implementations) Update examples to inject PatchHandler

NOTE: This section does not describe the supporting endpoints that SCIMple handles automatically

bdemers · 2023-11-29T15:59:59Z

@joshuapsteele All the reviews here are great!
I just cleaned up the git history a little, I'll merge it once CI passes!

joshuapsteele · 2023-12-01T14:07:28Z

@joshuapsteele All the reviews here are great! I just cleaned up the git history a little, I'll merge it once CI passes!

@bdemers Sounds good! @kjthorpe18 @zalbert02 and I are all eagerly anticipating this improvement! BTW, how can we get added as contributors to this SCIMple project? We're quite motivated to see it succeed and continue to be developed over time.

bdemers · 2023-12-01T16:12:04Z

@joshuapsteele keep doing what you doing, I'll try to get that ball rolling!
In the short term, if you haven't seen this article take a look!
https://community.apache.org/contributors/

bdemers mentioned this pull request Nov 13, 2023

Idea: Pass all search information into the repository implementation #410

Closed

joshuapsteele reviewed Nov 15, 2023

View reviewed changes

...server/src/main/java/org/apache/directory/scim/server/rest/BaseResourceTypeResourceImpl.java Outdated Show resolved Hide resolved

joshuapsteele reviewed Nov 15, 2023

View reviewed changes

joshuapsteele approved these changes Nov 17, 2023

View reviewed changes

erant10 reviewed Nov 17, 2023

View reviewed changes

erant10 approved these changes Nov 22, 2023

View reviewed changes

joshuapsteele approved these changes Nov 22, 2023

View reviewed changes

zalbert02 approved these changes Nov 22, 2023

View reviewed changes

kjthorpe18 approved these changes Nov 24, 2023

View reviewed changes

joshuapsteele reviewed Nov 28, 2023

View reviewed changes

bdemers mentioned this pull request Nov 29, 2023

Add ScimGroup tests for PatchGenerator, (previously)UpdateRequest #427

Closed

bdemers force-pushed the update-patch-attributes branch from 509fa8a to ffcebe3 Compare November 29, 2023 15:56

bdemers added 3 commits November 29, 2023 10:57

Inject PatchHandler into exmaple constructors

8d3a055

Rename PatchHandlerImpl to DefaultPatchHandler (users of SCIMple could create different implementations) Update examples to inject PatchHandler

Adding section in readme that maps HTTP requests to Repository methods.

630d2f3

NOTE: This section does not describe the supporting endpoints that SCIMple handles automatically

bdemers force-pushed the update-patch-attributes branch from ffcebe3 to 630d2f3 Compare November 29, 2023 15:58

bdemers merged commit 65e5f13 into develop Dec 1, 2023
2 checks passed

bdemers deleted the update-patch-attributes branch December 1, 2023 16:09

bdemers mentioned this pull request Mar 11, 2024

eTag support with If-Match #534

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move attribute and etag logic into Repository #411

Move attribute and etag logic into Repository #411

bdemers commented Nov 13, 2023

joshuapsteele Nov 15, 2023

bdemers Nov 21, 2023

joshuapsteele left a comment

bdemers commented Nov 15, 2023

joshuapsteele left a comment

erant10 Nov 17, 2023

bdemers Nov 21, 2023

erant10 Nov 17, 2023

bdemers Nov 21, 2023

kjthorpe18 left a comment

kjthorpe18 Nov 24, 2023

bdemers Nov 28, 2023

bdemers Nov 29, 2023

kjthorpe18 Nov 24, 2023

joshuapsteele left a comment

bdemers commented Nov 29, 2023

joshuapsteele commented Dec 1, 2023

bdemers commented Dec 1, 2023

	* Allows the SCIM server's REST implementation to update and existing
	* Allows the SCIM server's REST implementation to update an existing

Move attribute and etag logic into Repository #411

Move attribute and etag logic into Repository #411

Conversation

bdemers commented Nov 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuapsteele left a comment

Choose a reason for hiding this comment

bdemers commented Nov 15, 2023

joshuapsteele left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjthorpe18 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuapsteele left a comment

Choose a reason for hiding this comment

bdemers commented Nov 29, 2023

joshuapsteele commented Dec 1, 2023

bdemers commented Dec 1, 2023