Skip to content
This repository was archived by the owner on Aug 29, 2024. It is now read-only.

Conversation

@Blackbaud-MikeLueders
Copy link
Contributor

@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
Copy link
Collaborator

@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
Copy link
Contributor Author

@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
Copy link
Collaborator

@Blackbaud-MikeLueders - The repo movement is getting to completion now. Here is my PR which moves spring-data-cosmos to azure-cosmos V4 dependency: Azure/azure-sdk-for-java#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
Copy link
Collaborator

@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
Copy link
Collaborator

kushagraThapar commented Jul 28, 2020

@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
Copy link

@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
Copy link
Collaborator

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants