Conversation
kqarryzada
requested review from
braveulysses,
dougbulkley,
selliott512 and
vyhhuang
|
|
||
| // Recursively traverse the node and replace all text bulk ID references to | ||
| // the new value. This approach minimizes the amount of garbage generated. | ||
| JsonUtils.replaceAllTextValues(data, BULK_PREFIX + bulkId, realValue); |
There was a problem hiding this comment.
Even though this eliminates the read-only model for the object that is good for threading, I preferred this approach because it has less potential to cause severe GC pressure.
| names = {"DELETE", "Delete", "delete"}), | ||
| }) | ||
| @JsonPropertyOrder({ "method", "path", "bulkId", "version", "data" }) | ||
| public abstract class BulkOperation |
There was a problem hiding this comment.
These annotations (and the deserialization structure) are modelled after the PatchOperation class, which also define subclasses for distinct operation types. This allows all callers to just worry about a basic reusable class name (PatchOperation, BulkOperation).
| * | ||
| * Unlike {@link BulkOperation}, this {@link BulkOperationResult} does not use | ||
| * static helper methods since all result types are formatted the same and have | ||
| * similar restrictions. There are, however, alternate constructors available |
There was a problem hiding this comment.
I tried this model at first for consistency, but it just felt clunky. It works better for BulkOperation since the subclasses need to be hidden. I'm happier with using constructors for this class.
| * avoid attempting any of the remaining bulk operations present within the | ||
| * client bulk request. However, it is best to consult with the documentation | ||
| * for your SCIM service to be certain of the expected behavior, as some SCIM | ||
| * services may treat this value differently. |
There was a problem hiding this comment.
The RFC almost appears to give conflicting info on how this is treated. It took me several re-reads before I realized it was consistent in what it was trying to say. I added this warning to make sure that the behavior of other SCIM service implementations are checked first in case this is not true everywhere.
selliott512
left a comment
selliott512
left a comment
There was a problem hiding this comment.
Looks good. Nice job. I don't know that any of my comments require changes, but they may be interesting or helpful.
scim2-sdk-common/src/main/java/com/unboundid/scim2/common/bulk/BulkOperation.java
Show resolved
Hide resolved
scim2-sdk-common/src/main/java/com/unboundid/scim2/common/bulk/BulkOperation.java
Outdated
Show resolved
Hide resolved
scim2-sdk-common/src/main/java/com/unboundid/scim2/common/bulk/BulkOperation.java
Outdated
Show resolved
Hide resolved
scim2-sdk-common/src/main/java/com/unboundid/scim2/common/utils/JsonUtils.java
Outdated
Show resolved
Hide resolved
scim2-sdk-common/src/main/java/com/unboundid/scim2/common/utils/JsonUtils.java
Outdated
Show resolved
Hide resolved
scim2-sdk-common/src/test/java/com/unboundid/scim2/common/bulk/BulkRequestTest.java
Show resolved
Hide resolved
scim2-sdk-common/src/test/java/com/unboundid/scim2/common/bulk/BulkResponseTest.java
Show resolved
Hide resolved
dougbulkley
left a comment
dougbulkley
left a comment
There was a problem hiding this comment.
Nice thorough testing. Looking forward to working with this once we incorporate it into our PingDirectory server (DS-51295, no fixVersion yet).
This commit adds the second and final phase for supporting SCIM bulk request workflows for client and server applications. These changes build upon phase I by introducing the remaining support necessary for serialization/deserialization of bulk JSON data into Java objects, along with providing an interface for supporting these requests within applications. The primary reference point for information regarding bulk workflows has been placed in the BulkRequest class-level Javadoc. This provides the necessary context for what bulk requests are for, how to use them, and considerations to keep in mind. Special notes have been added for useful utilities in the API, such as a way to replace temporary bulk ID values after resources are created, as well as a powerful interface for extracting embedded SCIM resources and converting the JSON data into a usable Java object. Server applications are still responsible for handling bulk ID dependencies within a request, as well as evaluating whether a circular reference in the bulk request is unresolvable. However, a starting point for server-based processing logic has been provided in the BulkRequest documentation. Suggestions for features such as rate limiting have also been added to ensure server implementations consider these options. Reviewer: braveulysses Reviewer: dougbulkley Reviewer: selliott512 Reviewer: vyhhuang JiraIssue: DS-39451
kqarryzada
force-pushed
the
DS-39451-bulk-operation-support-phase-II
branch
from
40a92cc to
ee08f68
Compare
vyhhuang
left a comment
vyhhuang
left a comment
There was a problem hiding this comment.
These changes look good to me. I only had a minor comment
scim2-sdk-common/src/main/java/com/unboundid/scim2/common/bulk/BulkOperation.java
Outdated
Show resolved
Hide resolved
5 participants
This commit adds the second and final phase for supporting SCIM bulk request workflows for client and server applications. These changes build upon phase I by introducing the remaining support necessary for serialization/deserialization of bulk JSON data into Java objects, along with providing an interface for supporting these requests within applications.
The primary reference point for information regarding bulk workflows has been placed in the BulkRequest class-level Javadoc. This provides the necessary context for what bulk requests are for, how to use them, and considerations to keep in mind. Special notes have been added for useful utilities in the API, such as a way to replace temporary bulk ID values after resources are created, as well as a powerful interface for extracting embedded SCIM resources and converting the JSON data into a usable Java object.
Server applications are still responsible for handling bulk ID dependencies within a request, as well as evaluating whether a circular reference in the bulk request is unresolvable. However, a starting point for server-based processing logic has been provided in the BulkRequest documentation.
Reviewer: braveulysses
Reviewer: dougbulkley
Reviewer: selliott512
Reviewer: vyhhuang
JiraIssue: DS-39451
Resolves #107