Single result queries by Blackbaud-MikeLueders · Pull Request #480 · microsoft/spring-data-cosmosdb

Blackbaud-MikeLueders · 2020-01-31T18:57:20Z

This is a rebase of #381, I don't have permissions to update the branch so opening this as a new PR.

@kushagraThapar would you mind taking a look? Thanks!

This PR is a feature enhancement, adding the ability to have Repository methods that return a single object or Optional. Let's say I have the following...

class Repository extends CosmosRepository {
Foo findBySomething(String something);
}
Calling this method will currently result in a ClassCastException since it assumes a return type of List.
In addition, the findById method on CosmosRepository declares the following method - Optional findById(ID id, PartitionKey partitionKey); The contract says the Optional should contain the entity type but it does not - it contains a list of entity types. I would argue that the latter is a bug since the value returned in the Optional differs from what the method says should be returned.

The workaround would be to not use any methods that returned a single entity and to cast the returned value from the Optional to a List (since the actual value and the contract value are different) and deal with the single item list.

Does this help? Let me know if you'd like additional details.

kushagraThapar · 2020-01-31T19:14:56Z

@Blackbaud-MikeLueders Thanks for opening this PR. I will take a look at it.
Meanwhile, can you please update the summary of the PR with the following:

What this PR is for ?
What is the problem that this is trying to solve ?
What is the use case of the problem ?
Any workarounds as of now ?

Blackbaud-MikeLueders · 2020-02-03T21:52:24Z

@kushagraThapar this PR actually contains two separate changes, one is the feature change described in the summary and the other is to update the integration tests to respect cosmos.database if set. Would you prefer I split these changes into two separate PRs?

kushagraThapar · 2020-02-03T23:45:12Z

@kushagraThapar this PR actually contains two separate changes, one is the feature change described in the summary and the other is to update the integration tests to respect cosmos.database if set. Would you prefer I split these changes into two separate PRs?

Yes, I was about to comment that, specially after your last commit :)
P.S: It might take some time for me to review these, so please be patient, but be sure that I will review them soon, may be by end of this week.

Blackbaud-MikeLueders · 2020-02-04T04:13:13Z

@kushagraThapar I've split out the test updates to #482

kushagraThapar · 2020-02-04T04:45:08Z

@kushagraThapar I've split out the test updates to #482

Thanks for splitting it out, I will look at all 3 PRs soon.

kushagraThapar · 2020-02-06T20:51:18Z

/azp run

azure-pipelines · 2020-02-06T20:51:23Z

No pipelines are associated with this pull request.

kushagraThapar

Can you please add some example / test case for ReactiveCosmosRepository as well ?

kushagraThapar · 2020-02-07T23:06:25Z

Can you please add some example / test case for ReactiveCosmosRepository as well ?

@Blackbaud-MikeLueders I also looked at your statement regarding findbyId(id, partitionKey) -> returning List internally.
I don't think that is true. Because if you take a look at integration tests here: https://github.com/microsoft/spring-data-cosmosdb/blob/master/src/test/java/com/microsoft/azure/spring/data/cosmosdb/repository/integration/AddressRepositoryIT.java#L83
It is just returning a single entity. I am not sure why this seems to be a bug ?

Blackbaud-MikeLueders · 2020-02-08T14:31:34Z

@kushagraThapar you're right, findById seems to be special in that it uses a different execution path than a regular query. I've separated the changes in this branch to two commits - the first is the integration test SingleResponseRepositoryIT and the second are the changes required to make the integration test pass. The first commit includes the following changes to ContactRepository...

    Contact findOneByTitle(String title);
    Optional<Contact> findOptionallyByTitle(String title);

These are valid Spring Data methods but the tests which execute them will fail without the second commit. Any call to the first method will fail with java.lang.ClassCastException: java.util.ArrayList cannot be cast to com.microsoft.azure.spring.data.cosmosdb.domain.Contact and a call to the second method will return an Optional whose contents are a List rather than a single Contact which the contract requires. This seems like a bug.

Also, just FYI, the specific test you reference is wrong and should be failing but never will.

        final Optional<Address> addressById = ...
        assertThat(addressById.equals(TEST_ADDRESS1_PARTITION1));

In this instance, TEST_ADDRESS1_PARTITION1's type is Address, not Optional. Optional:equals will only return true if the object being compared is also an Optional. org.assertj.core.api.Assertions.assertThat does no assertion itself, it relies on calling another method to make the actual assertion (e.g. assertThat(something).isTrue().

Incarnation-p-lee · 2020-02-08T14:55:11Z

Here is an overview of what got changed by this pull request:

Issues
======
- Added 1
           

Complexity increasing per file
==============================
- src/test/java/com/microsoft/azure/spring/data/cosmosdb/repository/integration/SingleResponseRepositoryIT.java  1

See the complete overview on Codacy

Blackbaud-MikeLueders · 2020-02-08T15:03:39Z

@kushagraThapar as requested, I've added a reactive test. However, codacy is failing b/c of missing asserts. This test exactly mirrors the existing reactive tests in ReactiveCourseRepositoryIT. Please advise.

kushagraThapar · 2020-02-10T17:21:01Z

/azp run

azure-pipelines · 2020-02-10T17:21:07Z

No pipelines are associated with this pull request.

kushagraThapar · 2020-02-10T17:40:34Z

@kushagraThapar you're right, findById seems to be special in that it uses a different execution path than a regular query. I've separated the changes in this branch to two commits - the first is the integration test SingleResponseRepositoryIT and the second are the changes required to make the integration test pass. The first commit includes the following changes to ContactRepository...
    Contact findOneByTitle(String title);
    Optional<Contact> findOptionallyByTitle(String title);
These are valid Spring Data methods but the tests which execute them will fail without the second commit. Any call to the first method will fail with java.lang.ClassCastException: java.util.ArrayList cannot be cast to com.microsoft.azure.spring.data.cosmosdb.domain.Contact and a call to the second method will return an Optional whose contents are a List rather than a single Contact which the contract requires. This seems like a bug.

Also, just FYI, the specific test you reference is wrong and should be failing but never will.
        final Optional<Address> addressById = ...
        assertThat(addressById.equals(TEST_ADDRESS1_PARTITION1));
In this instance, TEST_ADDRESS1_PARTITION1's type is Address, not Optional. Optional:equals will only return true if the object being compared is also an Optional. org.assertj.core.api.Assertions.assertThat does no assertion itself, it relies on calling another method to make the actual assertion (e.g. assertThat(something).isTrue().

Thanks for pointing out the assertThat issue. I have fixed it in another PR.

kushagraThapar · 2020-02-10T18:48:56Z


    Flux<Course> findByDepartmentIn(Collection<String> departments);
+
+    Mono<Course> findByName(String name);


What happens when there are multiple courses with same name ?

kushagraThapar · 2020-02-10T18:49:27Z

+
+    Contact findOneByTitle(String title);
+
+    Optional<Contact> findOptionallyByTitle(String title);


Same question as below, what if there are multiple Contacts with same title ?
How would these two APIs behave (findByName and this one)..

it will fail, as demonstrated by the test testShouldFailIfMultipleResultsReturned in the first commit. it is expected that the client understands their data model and would not add a findOneBy or findOptionallyBy for queries that could return more than one item.

kushagraThapar · 2020-02-10T18:51:41Z

        final String collection = ((CosmosEntityMetadata) method.getEntityInformation()).getCollectionName();

-        final CosmosQueryExecution execution = getExecution(accessor);
+        final CosmosQueryExecution execution = getExecution(accessor, processor.getReturnedType());


When I mentioned for ReactiveCosmosRepository, I actually meant to have this functionality on ReactiveQuery pipeline as well.
For example, this code needs to be updated in AbstractReactiveCosmosQuery as well.

kushagraThapar · 2020-02-10T18:52:31Z

        }
    }

+    final class SingleEntityExecution implements CosmosQueryExecution {


Same as above comment: ReactiveCosmosQueryExecution changes ?

kushagraThapar · 2020-02-10T18:53:34Z

+
+@RunWith(SpringJUnit4ClassRunner.class)
+@ContextConfiguration(classes = TestRepositoryConfig.class)
+public class SingleResponseRepositoryIT {


It would be much better to have separate test cases for CosmosRepository and ReactiveCosmosRepository. That's the convention we follow and all the test cases have been designed like that .

kushagraThapar · 2020-02-24T04:01:07Z

@Blackbaud-MikeLueders Any updates on the above PR comments / feedback ?

kushagraThapar · 2020-02-24T04:39:15Z

@Blackbaud-MikeLueders - We have a release scheduled for this Tuesday - 25th Feb. It would be great if we can get this in before then. Would like this feature to get in spring-data-cosmsodb - 2.2.3 release.
Can you please implement the Reactive version of changes by then ?
If not for some reason, this can go in later releases. Let me know so I can plan accordingly.

mlueders · 2020-06-22T17:18:45Z

@kushagraThapar apologies for losing track of this, we're going to pick this up in the next couple weeks and begin adding the reactive support.

kushagraThapar · 2020-06-25T16:22:24Z

@Blackbaud-MikeLueders - We are currently in development of new spring-data-cosmosdb module - which will take a dependency on azure-cosmos V4 SDK (new GA SDK, which is better in terms of latency and throughput than V3 SDK).
You can read more about cosmos V4 SDK here : https://devblogs.microsoft.com/cosmosdb/java-sdk-v4-ga/

This repo has been moved to https://github.com/Azure/azure-sdk-for-java/tree/master/sdk/cosmos/azure-spring-data-cosmosdb

If you want this fix to go into the new repo, I would suggest opening this PR against the new repo.
Since the development / migration is still going on, the best way would be to wait for a week and once the repo has been moved completely to azure-sdk-for-java repo, then open the PR.

However, if you want this fix to go in earlier versions, then we can keep this fix here as it is and target it against master.
In the latter case, the fix will go in v2.3.0 release train - which is still targeting azure-cosmos v3 SDK.

Blackbaud-MikeLueders · 2020-06-25T18:30:28Z

@kushagraThapar thanks for the heads up, i'll go ahead and close this PR and target the new repo. What's the best way to know when the repo has been completely moved?

kushagraThapar · 2020-06-25T19:03:39Z

@Blackbaud-MikeLueders - all the source code has already been merged, and samples are currently being merged.
Though one more heads up for you. Currently we have merged the repo as it is. Work to move spring-data-cosmosdb to V4 SDK is still remaining. So I would suggest wait until I have made an initial check-in with updating spring-data-cosmosdb with V4 cosmos SDK.

The best way to know would be me telling you :)
How about I update here when appropriate time comes for you to open this PR against new repo ?
Also, we are planning to do GA for new spring sometime mid to late July. And I believe we want to pick this change in our GA version release. So once I give you the heads up, we can aggressively target this PR and get it merged on the new repo with V4 cosmos SDK as base for spring-data-cosmosdb .

I will update you once I have moved spring-data-cosmosdb to V4 SDK, as implementation code will change.

Blackbaud-MikeLueders · 2020-06-25T19:41:47Z

@kushagraThapar That would be great, thank you! FYI, we have some additional changes as well

add support for auditing (@CreatedDate, @createdby, @LastModifiedDate, @LastModifiedBy)
fix optimistic locking (there are issues with the current implementation, or at least there were 6 months ago, not sure if they've been addressed)

I was waiting on raising those until this PR was addressed to avoid too much at once but should we raise those items as PRs against this repo now so they can be reviewed (and we'd move them to the new repo next week) so we can bet those in as well before GA?

Blackbaud-MikeLueders · 2020-06-25T19:43:01Z

@Blackbaud-EricSlater FYI

kushagraThapar · 2020-06-25T19:53:09Z

@Blackbaud-MikeLueders Sounds good, all those features sounds cool and will be very good to have.
Will update you as soon as the repo is open for development.

Meanwhile, I will highly encourage you guys to go over cosmos V4 SDK changes - https://docs.microsoft.com/en-us/azure/cosmos-db/sql-api-sdk-java-v4
Since v4 SDK is quite different from v3 SDK, it has major public surface API changes, which could affect this and other PRs you are planning in a big way, but in a better way :)

Blackbaud-ErikBudtke mentioned this pull request Jan 31, 2020

Single result queries #381

Closed

Blackbaud-MikeLueders force-pushed the single-result-queries branch 3 times, most recently from 992269d to 7d86cc3 Compare February 4, 2020 04:10

kushagraThapar suggested changes Feb 6, 2020

View reviewed changes

Blackbaud-MikeLueders force-pushed the single-result-queries branch from 7d86cc3 to c869f35 Compare February 8, 2020 14:15

microsoft deleted a comment from Incarnation-p-lee Feb 10, 2020

kushagraThapar suggested changes Feb 10, 2020

View reviewed changes

Blackbaud-MikeLueders force-pushed the single-result-queries branch from 6c54615 to 0a4d7da Compare June 24, 2020 19:38

Blackbaud-MikeLueders added 3 commits June 24, 2020 14:40

add tests which demonstrate broken jpa contracts

e4c11a2

support single result queries

53426d8

added reactive test

e6686f0

Blackbaud-MikeLueders force-pushed the single-result-queries branch from 0a4d7da to e6686f0 Compare June 24, 2020 19:41

Blackbaud-MikeLueders closed this Jun 25, 2020

Blackbaud-EricSlater mentioned this pull request Jul 16, 2020

Single result queries Azure/azure-sdk-for-java#13251

Merged


		Flux<Course> findByDepartmentIn(Collection<String> departments);

		Mono<Course> findByName(String name);


		Contact findOneByTitle(String title);

		Optional<Contact> findOptionallyByTitle(String title);

Uh oh!

Conversation

Blackbaud-MikeLueders commented Jan 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kushagraThapar commented Jan 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Blackbaud-MikeLueders commented Feb 3, 2020

Uh oh!

kushagraThapar commented Feb 3, 2020

Uh oh!

Blackbaud-MikeLueders commented Feb 4, 2020

Uh oh!

kushagraThapar commented Feb 4, 2020

Uh oh!

kushagraThapar commented Feb 6, 2020

Uh oh!

azure-pipelines Bot commented Feb 6, 2020

Uh oh!

kushagraThapar left a comment

Choose a reason for hiding this comment

Uh oh!

kushagraThapar commented Feb 7, 2020

Uh oh!

Blackbaud-MikeLueders commented Feb 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Incarnation-p-lee commented Feb 8, 2020

Uh oh!

Blackbaud-MikeLueders commented Feb 8, 2020

Uh oh!

kushagraThapar commented Feb 10, 2020

Uh oh!

azure-pipelines Bot commented Feb 10, 2020

Uh oh!

kushagraThapar commented Feb 10, 2020

Uh oh!

kushagraThapar Feb 10, 2020

Choose a reason for hiding this comment

Uh oh!

kushagraThapar Feb 10, 2020

Choose a reason for hiding this comment

Uh oh!

Blackbaud-MikeLueders Feb 10, 2020

Choose a reason for hiding this comment

Uh oh!

kushagraThapar Feb 10, 2020

Choose a reason for hiding this comment

Uh oh!

kushagraThapar Feb 10, 2020

Choose a reason for hiding this comment

Uh oh!

kushagraThapar Feb 10, 2020

Choose a reason for hiding this comment

Uh oh!

kushagraThapar commented Feb 24, 2020

Uh oh!

kushagraThapar commented Feb 24, 2020

Uh oh!

mlueders commented Jun 22, 2020

Uh oh!

kushagraThapar commented Jun 25, 2020

Uh oh!

Blackbaud-MikeLueders commented Jun 25, 2020

Uh oh!

kushagraThapar commented Jun 25, 2020

Uh oh!

Blackbaud-MikeLueders commented Jun 25, 2020

Uh oh!

Blackbaud-MikeLueders commented Jun 25, 2020

Uh oh!

kushagraThapar commented Jun 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Blackbaud-MikeLueders commented Jan 31, 2020 •

edited

Loading

kushagraThapar commented Jan 31, 2020 •

edited

Loading

Blackbaud-MikeLueders commented Feb 8, 2020 •

edited

Loading