cosmos spring data - patch support #32630

TheovanKraay · 2022-12-16T19:03:41Z

Description

This PR adds an overload to Respository.save() method that allows entity to be patched (partial update). When this is called, only the fields defined in CosmosPatchOperations will be sent to Cosmos DB for update.

Note: RU cost for patch() is equivalent to the RU for replace() because the physics in terms of physical resources (cpu, memory, etc.) are roughly the same. The benefits from using patch come from being able to limit the number of updated fields sent over the wire, avoiding the need to retrieve the to-be-patched record from the database before patching, and avoiding the need to manually handle etag contention from concurrent updates.

CosmosPatchOperations patchOperations = CosmosPatchOperations
    .create()
    .replace("/size", 5)
    .add("/color", "blue");

User patchedUser = userRepository.save(user.getId(), new PartitionKey(user.getLastName()), User.class, patchOperations);

All SDK Contribution checklist:

The pull request does not introduce [breaking changes]
CHANGELOG is updated for new features, bug fixes or other significant changes.
I have read the contribution guidelines.

General Guidelines and Best Practices

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

Pull request includes test coverage for the included changes.

...ta-cosmos-test/src/test/java/com/azure/spring/data/cosmos/core/ReactiveCosmosTemplateIT.java

...azure-spring-data-cosmos/src/main/java/com/azure/spring/data/cosmos/core/CosmosTemplate.java

xinlian12 · 2023-01-09T18:58:39Z

...ring-data-cosmos/src/main/java/com/azure/spring/data/cosmos/repository/CosmosRepository.java

+     * @return the patched entity
+     * @throws IllegalArgumentException in case the given {@code id} is {@literal null}.
+     */
+    <S extends T> S save(S entityToPatch, CosmosPatchOperations patchOperations, CosmosPatchItemRequestOptions options);


should we also introduce an API without the requestOptions? save(S entityToPatch, CosmosPatchOperations patchOperations);

No we will not, we want to maintain the least amount of API functions possible. Allowing the requestOptions to be null is acceptable and follows our best practices in Spring.

trande4884 · 2023-01-10T16:29:26Z

...pring-data-cosmos-test/src/test/java/com/azure/spring/data/cosmos/core/CosmosTemplateIT.java

+    public void testPatchPreConditionSuccess() {
+        options.setFilterPredicate("FROM person p WHERE p.lastName = '"+LAST_NAME+"'");
+        Person patchedPerson = cosmosTemplate.patch(insertedPerson.getId(), new PartitionKey(insertedPerson.getLastName()), Person.class, operations, options);
+        assertEquals(patchedPerson.getAge(), patchedPerson.getAge());


This check will always succeed as you are checking "a = a". Let's change the second param to the "Age" variable.

Corrected this

trande4884 · 2023-01-10T16:29:39Z

...pring-data-cosmos-test/src/test/java/com/azure/spring/data/cosmos/core/CosmosTemplateIT.java

+        try {
+            options.setFilterPredicate("FROM person p WHERE p.lastName = 'dummy'");
+            Person patchedPerson = cosmosTemplate.patch(insertedPerson.getId(), new PartitionKey(insertedPerson.getLastName()), Person.class,  operations, options);
+            assertEquals(patchedPerson.getAge(), patchedPerson.getAge());


Same as above.

TheovanKraay · 2023-01-10T19:49:28Z

/azp run java - cosmos - tests

azure-pipelines · 2023-01-10T19:49:43Z

Azure Pipelines successfully started running 1 pipeline(s).

TheovanKraay · 2023-01-10T21:35:40Z

/check-enforcer override

FabianMeiswinkel · 2023-01-11T13:15:41Z

...ure-spring-data-cosmos/src/main/java/com/azure/spring/data/cosmos/core/CosmosOperations.java

+     * @param <T> type class of domain type
+     * @return the patched item
+     */
+    <T> T patch(Object id, PartitionKey partitionKey, Class<T> domainType, CosmosPatchOperations patchOperations, CosmosPatchItemRequestOptions options);


If this has been discussed in the API review please just add brief comment and resolve this. But why are we adding an overload with request options here when none of the other operations allow specifying request options? I am not against it - just curious why we now decide t add it and skipped it for all other operations? Is there a more common scenario that would need it? Or is the idea to eventually add overload to other operations with request options as well?

Most of the discussion was around whether to add an overload, or force caller to pass null (we went with overload, so that the method call in most cases will be less verbose). But I think here you are asking why we add request options at all since we don't expose that elsewhere, which is a good question? It is purely to facilitate setFilterPredicate for patch. If we generally don't want to expose request options, then I guess we should encapsulate this and just have a string parameter in the overload, something like String filterPredicate.

my 2c here is to avoid adding overloads with individual parameters and keep one with options if needed.

@lmolkova - actually that's what we had earlier :)
We had one API with requestOptions included and requestOptions can be nullable / optional. However, we got initial customer feedback from the PR review that if they need to pass in null, why are we forcing them to pass null and we should expose one API for simple use case. That's why we switched back to having 2 APIs.

two thanks for the clarification, @kushagraThapar ! Two methods are fine (one with options, one without). I meant that the verbose one (with extra options), should take an extendable options class and not an individual filterPredicate parameter.

I think that is just in the example / doc on the API, not an actual API itself, no?

FabianMeiswinkel · 2023-01-11T13:19:31Z

...ng-data-cosmos/src/main/java/com/azure/spring/data/cosmos/core/ReactiveCosmosOperations.java

+     * @param <T> type class of domain type
+     * @return Mono with the patched item
+     */
+    <T> Mono<T> patch(Object id, PartitionKey partitionKey, Class<T> domainType, CosmosPatchOperations patchOperations, CosmosPatchItemRequestOptions options);


See comment in CosmosOperations

FabianMeiswinkel · 2023-01-11T13:22:17Z

and avoiding possible etag contention from highly concurrent updates
This statement isn't true - the etag condition failures will just result in 449 which will be automatically retried - but it doesn't allow for any higher contention

Thanks for catching this! Is it sufficient to replace the word “avoiding” with “mitigating”? The point being that afaik 449s aren’t retried at all if doing read/replace vs patch - so this is still a benefit of using patch, correct?

Sorry - I seem to have been misunderstood - when using read+replace under high concurrent access you pass the etad form the read as pre-condition failure to the replace and would get a 420 if somebody else updated the document. This needs to be manually retried by the app. With patch instead you would get a 449 if the backend hits the same pre-codnition failure. And because the 449 is retried automatically the dev does not need to do anything. But if there is high contention it would still result in unreasonable latency etc. Bottom-line the contention you can achieve is the same - but concurrent updates (with low contention risk) are easier with patch because the headache of the 412 etag-precondition dance is done). My main point was that we should not set the expectation that highly concurrent updates are a good idea ever with cosmos db. patch or read+replace

Ok - how about "and avoiding the need to manually handle etag contention from highly concurrent updates"? Or you prefer that we remove this completely so as to avoid the suggestion that somehow it will be better with patch?
How about "and avoiding the need to manually handle etag contention from concurrent updates"

TheovanKraay · 2023-01-11T14:09:33Z

/check-enforcer override

TheovanKraay · 2023-01-11T15:30:23Z

/check-enforcer override

lmolkova · 2023-01-11T18:53:44Z

...ure-spring-data-cosmos/src/main/java/com/azure/spring/data/cosmos/core/CosmosOperations.java

+     * @param <T> type class of domain type
+     * @return the patched item
+     */
+    <T> T patch(Object id, PartitionKey partitionKey, Class<T> domainType, CosmosPatchOperations patchOperations, CosmosPatchItemRequestOptions options);


my 2c here is to avoid adding overloads with individual parameters and keep one with options if needed.

lmolkova · 2023-01-11T19:03:38Z

...ure-spring-data-cosmos/src/main/java/com/azure/spring/data/cosmos/core/CosmosOperations.java

+     * patches item
+     * @param id must not be {@literal null}
+     * @param partitionKey must not be {@literal null}
+     * @param domainType must not be {@literal null}


would be nice to add a description on what domainType is

kushagraThapar

Looks great @TheovanKraay
We should add more tests to repository tests, like AddressRepositoryIT, etc.

...azure-spring-data-cosmos/src/main/java/com/azure/spring/data/cosmos/core/CosmosTemplate.java

kushagraThapar · 2023-01-11T19:43:02Z

...ure-spring-data-cosmos/src/main/java/com/azure/spring/data/cosmos/core/CosmosOperations.java

+     * @param <T> type class of domain type
+     * @return the patched item
+     */
+    <T> T patch(Object id, PartitionKey partitionKey, Class<T> domainType, CosmosPatchOperations patchOperations, CosmosPatchItemRequestOptions options);


@lmolkova - actually that's what we had earlier :)
We had one API with requestOptions included and requestOptions can be nullable / optional. However, we got initial customer feedback from the PR review that if they need to pass in null, why are we forcing them to pass null and we should expose one API for simple use case. That's why we switched back to having 2 APIs.

kushagraThapar · 2023-01-11T19:43:10Z

...ure-spring-data-cosmos/src/main/java/com/azure/spring/data/cosmos/core/CosmosOperations.java

+     * patches item
+     * @param id must not be {@literal null}
+     * @param partitionKey must not be {@literal null}
+     * @param domainType must not be {@literal null}


TheovanKraay · 2023-01-11T21:55:04Z

Looks great @TheovanKraay We should add more tests to repository tests, like AddressRepositoryIT, etc.

Added

trande4884 · 2023-01-12T01:27:13Z

/azp run java - cosmos - tests

azure-pipelines · 2023-01-12T01:27:26Z

Azure Pipelines successfully started running 1 pipeline(s).

trande4884 · 2023-01-12T02:14:54Z

/check-enforcer override

Theo van Kraay added 7 commits December 9, 2022 22:48

initial patches changes

0e6522d

initial patch changes

492efef

edits

5bd32c0

add unit tests and Trevor multitenant pieces

2d0040b

edits after Trevor review

83b2170

edits after Trevor review

60605eb

remove uneccessary line

7ec75e1

TheovanKraay requested review from kushagraThapar, FabianMeiswinkel, backwind1233, chenrujun, hui1110, Netyyyy, saragluna, stliu, yiliuTo, xinlian12, moarychan, aayush3011, simorenoh, fangjian0423, jeet1995 and Pilchie as code owners December 16, 2022 19:03

ghost added Cosmos azure-spring All azure-spring related issues labels Dec 16, 2022

trande4884 linked an issue Dec 16, 2022 that may be closed by this pull request

Spring Cosmos - Support patch/partial update #21433

Closed

Theo van Kraay added 4 commits December 16, 2022 22:15

re-factor to fit Spring Data contract better

37dc1c2

fix javadocs errors

9fa7ed6

fix javadocs errors

0bcd7a0

amend contracts to be more consistent

b1dbccb

xinlian12 reviewed Jan 9, 2023

View reviewed changes

...ta-cosmos-test/src/test/java/com/azure/spring/data/cosmos/core/ReactiveCosmosTemplateIT.java Show resolved Hide resolved

xinlian12 reviewed Jan 9, 2023

View reviewed changes

...azure-spring-data-cosmos/src/main/java/com/azure/spring/data/cosmos/core/CosmosTemplate.java Show resolved Hide resolved

xinlian12 reviewed Jan 9, 2023

View reviewed changes

Theo van Kraay added 2 commits January 10, 2023 14:53

revert design + Annie/Trevor review

9dd8c39

fix build errors

e9e2261

trande4884 reviewed Jan 10, 2023

View reviewed changes

Theo van Kraay added 2 commits January 10, 2023 16:44

fix unit tests

e40896e

update revapi.json

b65aaa2

FabianMeiswinkel reviewed Jan 11, 2023

View reviewed changes

expand options descriptions in templates

4352685

lmolkova reviewed Jan 11, 2023

View reviewed changes

kushagraThapar requested changes Jan 11, 2023

View reviewed changes

add respository test cases

e930de9

Adding reactive tests and changing one assert.

3a2e90b

kushagraThapar approved these changes Jan 12, 2023

View reviewed changes

trande4884 merged commit 0033c98 into Azure:main Jan 12, 2023

TheovanKraay mentioned this pull request Jan 19, 2023

How to use cosmos client directly along with spring data repository as I could not find few operations like partial updates in spring data cosmos #32390

Closed

MapFilterMagic mentioned this pull request Aug 26, 2024

[QUERY] Azure Spring Data Cosmos Partial Document Update: Do I need Id and _etag? #41643

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cosmos spring data - patch support #32630

cosmos spring data - patch support #32630

TheovanKraay commented Dec 16, 2022 •

edited

Loading

xinlian12 Jan 9, 2023

trande4884 Jan 9, 2023

trande4884 Jan 10, 2023

TheovanKraay Jan 10, 2023

trande4884 Jan 10, 2023

TheovanKraay Jan 10, 2023

TheovanKraay commented Jan 10, 2023

azure-pipelines bot commented Jan 10, 2023

TheovanKraay commented Jan 10, 2023

FabianMeiswinkel Jan 11, 2023

TheovanKraay Jan 11, 2023 •

edited

Loading

lmolkova Jan 11, 2023

kushagraThapar Jan 11, 2023

lmolkova Jan 11, 2023

kushagraThapar Jan 11, 2023

FabianMeiswinkel Jan 11, 2023

FabianMeiswinkel commented Jan 11, 2023

TheovanKraay commented Jan 11, 2023

TheovanKraay commented Jan 11, 2023

lmolkova Jan 11, 2023

lmolkova Jan 11, 2023

kushagraThapar Jan 11, 2023

kushagraThapar left a comment

kushagraThapar Jan 11, 2023

kushagraThapar Jan 11, 2023

TheovanKraay commented Jan 11, 2023

trande4884 commented Jan 12, 2023

azure-pipelines bot commented Jan 12, 2023

trande4884 commented Jan 12, 2023

cosmos spring data - patch support #32630

cosmos spring data - patch support #32630

Conversation

TheovanKraay commented Dec 16, 2022 • edited Loading

Description

All SDK Contribution checklist:

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

Choose a reason for hiding this comment

TheovanKraay commented Jan 10, 2023

azure-pipelines bot commented Jan 10, 2023

TheovanKraay commented Jan 10, 2023

Choose a reason for hiding this comment

TheovanKraay Jan 11, 2023 • edited Loading

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

Choose a reason for hiding this comment

FabianMeiswinkel commented Jan 11, 2023

TheovanKraay commented Jan 11, 2023

TheovanKraay commented Jan 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kushagraThapar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheovanKraay commented Jan 11, 2023

trande4884 commented Jan 12, 2023

azure-pipelines bot commented Jan 12, 2023

trande4884 commented Jan 12, 2023

TheovanKraay commented Dec 16, 2022 •

edited

Loading

TheovanKraay Jan 11, 2023 •

edited

Loading