Conversation
This commit adds phase I of support for SCIM bulk request management for client and server applications. This initial phase focuses on the object models for bulk entities. This also includes a BulkResourceMapper, which will help provide an API for obtaining the "data" and "response" fields of bulk requests/responses as Java POJOs, simplifying object handling for client applications. This commit also includes updates for the exception package. New errors related to bulk requests (HTTP 413 and HTTP 429) have been added. Additionally, HTTP status values can now be obtained from exception objects. Reviewer: braveulysses Reviewer: dougbulkley Reviewer: selliott512 Reviewer: vyhhuang JiraIssue: DS-39451
| * @throws ScimException If the SCIM service responded with an error. | ||
| */ | ||
| @NotNull | ||
| public BulkResponse invoke() throws ScimException |
There was a problem hiding this comment.
The layout of this class largely follows the other builders. However, the invoke() method is somewhat unique because any SCIM resources will be included within the BulkResponse, so the returned object is never typed. This is why we'll need to have the mapper class in Phase 2.
|
|
||
| package com.unboundid.scim2.common.bulk; | ||
|
|
||
| public abstract class BulkOperation |
There was a problem hiding this comment.
This class, as well as other related classes, will be filled out in Phase 2. They're included here so that they may be referenced by BulkRequestBuilder.
braveulysses
left a comment
There was a problem hiding this comment.
Thanks, Khalid — this looks like good, conscientious work, and it's great to see bulk operation support landing in the SDK.
| @NotNull | ||
| public BulkRequestBuilder bulkRequest() | ||
| { | ||
| return new BulkRequestBuilder(baseTarget.path("Bulk")); |
There was a problem hiding this comment.
Nit: I realize that this string literal is only used here, but consider defining it in ApiConstants alongside SCHEMAS_ENDPOINT, ME_ENDPOINT, et al.
There was a problem hiding this comment.
Thanks Jacob, good call. I've added this.
| */ | ||
| @NotNull | ||
| @SuppressWarnings("unchecked") | ||
| static <T extends ScimResource> Class<T> get(@NotNull final Set<String> s) |
There was a problem hiding this comment.
Nit: Consider giving the argument a descriptive name like schemas instead of s.
There was a problem hiding this comment.
I'll be honest, I only did this so that the method declaration could fit on one line. It felt harmless since the method is a one-liner and it is not public anyway. But if you feel strongly about it, let me know and I'll change it.
There was a problem hiding this comment.
I definitely don't feel strongly about it. 🙂
Updated add() and put() so that the methods are synchronized, offering some level of thread safety for writes to the map that occur during startup.
vyhhuang
left a comment
There was a problem hiding this comment.
These changes look good to me.
selliott512
left a comment
There was a problem hiding this comment.
This is an incomplete list of comments that I thought I'd share what I have for now.
| * as if the JSON body was too large. | ||
| */ | ||
| @NotNull | ||
| protected <C> C invoke(@NotNull final Class<C> cls) |
There was a problem hiding this comment.
Do you wanted this protected? Many of the other builds just have public.
There was a problem hiding this comment.
I set this as protected because there's virtually no reason I can think of that a caller would want anything other than a BulkResponse object returned. However, if they really need it, they could extend this class and achieve that result this way. Making it protected allows for this, even though I think it's unlikely.
| * @return This bulk request builder. | ||
| */ | ||
| @NotNull | ||
| public BulkRequestBuilder append(@Nullable final List<BulkOperation> ops) |
There was a problem hiding this comment.
The other builders have addOperation. Would addOperations be a good name for this method?
There was a problem hiding this comment.
I considered this, but the term "operation" felt like it was getting overloaded between "patch operation" and "bulk operation". This will become more noticeable in Phase 2, where there is a bulk operation type for patch requests, so the nomenclature has the potential to become confusing. So I decided to name this as "append" just to avoid confusion. What do you think?
| * {@link com.unboundid.scim2.common.bulk.BulkOperation BulkOperation}. | ||
| */ | ||
| @NotNull | ||
| public static final String BULK_PREFIX = "bulkId:"; |
There was a problem hiding this comment.
You added this, but you don't use it, which I think means callers will need to correlate results to their requests by index. That's probably ok because the caller probably has an ordered list of requests used to build the bulk request, but I just wanted to share my impression of what is happening.
There was a problem hiding this comment.
This is mostly placed so that client requests can easily reference another bulk operation without having to hard-code their own prefix. So they will have to correlate it themselves, yes.
| /** | ||
| * An enum representing possible bulk operation types. | ||
| */ | ||
| public enum BulkOpType |
There was a problem hiding this comment.
Is this phase 2? It does not seem to be used, at least internally. If you just want the constants there is Jakarta, but I understand that enums are helpful, and indicate a specific set of allowed value. In any case, if we need this it seems it could be simplified, pretty much:
public enum BulkOpType {
POST, PUT, PATCH, DELETE
}But I'm probably missing something.
There was a problem hiding this comment.
Great question, yes, this is needed for Phase 2.
Also, it's worth noting that the majority of these changes are placed in scim2-sdk-common since entities like bulk operations are model objects. The only dependency that common has is Jackson, so Jakarta utilities are not available to us in the common module anyway.
This commit adds phase I of support for SCIM bulk request management for client and server applications. This initial phase focuses on the object models for bulk entities. This also includes a BulkResourceMapper, which will help provide an API for obtaining the "data" and "response" fields of bulk requests/responses as Java POJOs, simplifying object handling for client applications.
This commit also includes updates for the exception package. New errors related to bulk requests (HTTP 413 and HTTP 429) have been added. Additionally, HTTP status values can now be obtained from exception objects.
Reviewer: braveulysses
Reviewer: dougbulkley
Reviewer: selliott512
Reviewer: vyhhuang
JiraIssue: DS-39451
Resolves #107