Single result queries - Githubissues

Blackbaud-MikeLueders commented 4 years ago

This is a rebase of https://github.com/microsoft/spring-data-cosmosdb/pull/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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@kushagraThapar I've split out the test updates to https://github.com/microsoft/spring-data-cosmosdb/pull/482

kushagraThapar commented 4 years ago

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

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

kushagraThapar commented 4 years ago

/azp run

azure-pipelines[bot] commented 4 years ago

No pipelines are associated with this pull request.

kushagraThapar commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

/azp run

azure-pipelines[bot] commented 4 years ago

No pipelines are associated with this pull request.

kushagraThapar commented 4 years ago

@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 commented 4 years ago

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

kushagraThapar commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@Blackbaud-EricSlater FYI

kushagraThapar commented 4 years ago

@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 :)

microsoft / spring-data-cosmosdb

Single result queries #480