search

microsoft / spring-data-cosmosdb

Access data with Azure Cosmos DB
MIT License
94 stars 64 forks source link

Delete respects etag #544

Closed Blackbaud-MikeLueders closed 4 years ago

Blackbaud-MikeLueders commented 4 years ago

@kushagraThapar

SimpleCosmosRepository:delete should respect the etag value if the input object is versioned.

I would like to get your thoughts on something else related to @Version but not addressed in this PR. Currently, there is a strict dependency on the specific name of the @Version field - it must be "_etag". IMO, this is problematic for two reasons. First, this seems like an implementation detail of Cosmos and not something that necessarily needs to be exposed at the bean level. Second, some companies have naming standards that disallow underscores in variable names.

More importantly, as currently implemented, someone could annotate a field that is named something other than "_etag" with @Version and the library will allow it, it just won't actually do optimistic locking. This could be fixed by looking for @Version and disallowing it on fields not named "_etag" but I would rather the library transparently handle converting the field annotated with @Version to/from the property "_etag" when sending/receiving. The problem is that to do it cleanly would require minor (but breaking) changes to the CosmosOperations class. Let me know if this is something that you would be interested in and I'll throw up another PR outlining the approach.

kushagraThapar commented 4 years ago

@Blackbaud-MikeLueders - This is great feedback. We should definitely bring that change where any field can be annotated with @Version annotation and it should behave like one. I totally agree with naming standard constraints as well.

Since we are working on moving spring-data-cosmosdb to new azure-cosmos V4 SDK. We are preparing for breaking changes anyway. So this could be a great time to bring this in.

Though as I mentioned previously, the repo is still in movement, so any new PRs should be against the new repo. It is close to completion at this point, but not completely done. I will update on this PR once the repo is moved completely, and then you can create this PR and the new PR against new repo.

Blackbaud-MikeLueders commented 4 years ago

@kushagraThapar I understand re: new repository, I just wanted to get this up and available for review in case there were any issues that needed to be addressed, will move it over once the new repo is complete. Thanks!

kushagraThapar commented 4 years ago

@Blackbaud-MikeLueders - The repo movement is getting to completion now. Here is my PR which moves spring-data-cosmos to azure-cosmos V4 dependency: https://github.com/Azure/azure-sdk-for-java/pull/13229

This PR will be merged soon (latest by tomorrow).

Once this is in, you can move this PR against the new repo and we can fix this issue.

Let me know if you have any questions.

kushagraThapar commented 4 years ago

@Blackbaud-MikeLueders - my PR is in to master now - you can rebase it to latest master branch here: https://github.com/Azure/azure-sdk-for-java/tree/master/sdk/cosmos/azure-spring-data-cosmos and create a PR in the new repo.

Let me know if you have any questions / concerns.

kushagraThapar commented 4 years ago

@Blackbaud-MikeLueders - Any updates on new PR agains the new repo ?

I can manually bring this delete respects etag PR to the new repo if you would like, but the @Version change that you were talking about has to be brought by you as I am unaware of that change.

Let me know your thoughts on above. For now, I am closing this PR.

Blackbaud-EricSlater commented 4 years ago

@kushagraThapar i will be porting this PR over to the new repo shortly as well as resuming the work on getting @version changes discussed here fully working. We have Work in progress branch but its not totally done yet. I am pretty sure we can get it done by the end of this week. Since there is a deadline I will prioritize on getting the WIP branch done and opened for review.

kushagraThapar commented 4 years ago

@Blackbaud-EricSlater sounds good, thank you very much!