microsoft / spring-data-cosmosdb

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

cosmos will reject a document if it doesn't have an id so auto create one if needed #481

Closed Blackbaud-MikeLueders closed 4 years ago

Blackbaud-MikeLueders commented 4 years ago

If save is invoked and the id of the object is null, cosmos will reject the call. SimpleCosmosRepository#save specifically checks for this case and calls either CosmosOperations#insert or CosmosOperations#upsert. Since there is a separate branching path for inserts in the case where id is null, I assume this is supposed to work (I'm guessing Cosmos supported this at some point but apparently no longer). This PR replaces a null id with a random UUID.

kushagraThapar commented 4 years ago

@Blackbaud-MikeLueders I am not sure if this is a change that we can accept. I would rather like to fail in case the id is null, instead of creating one in the SDK. Doesn't make sense for the SDK to take the responsibility / ownership of creating ids for client applications. Also, you are right, CosmosDB supported it earlier, but not anymore.

Blackbaud-MikeLueders commented 4 years ago

@kushagraThapar I have added the annotation com.microsoft.azure.spring.data.cosmosdb.annotation.GeneratedValue which is in line with javax.persistence.GeneratedValue which is respected by the Spring Data JPA implementation. If the id is annotated, a value will be automatically generated. If not, an exception will be thrown with a readable message before the request is sent to Cosmos. If this is acceptable, let me know and I will update the Readme.

kushagraThapar commented 4 years ago

@kirankumarkolli Can you please provide your input on this issue ?

kirankumarkolli commented 4 years ago

Document/Item in Cosmos is uniquely identified by (pk, id). (pk, id) context is required for any future (read, update, delete) scenario's. Hence its best to avoid auto generation inside SDK and let the application made the choice (Also the required code changes would be straight forward anyways).

Blackbaud-MikeLueders commented 4 years ago

@kirankumarkolli I don't understand what pk has to do with it. The partition key is not a random identifier, it will always need to be set, just like all the other data values on the entity. Id is different, it will generally be a randomly-generated value and I do not understand the desire to force all users of this library to manually set the value. Also, this change does allow the application to make the choice - it can choose to add the @GeneratedValue annotation to the Entity, or not. And by having the annotation on the Entity, I can easily see that the id column is generated rather than derived, which can be useful.

There are mechanisms to auto-generate ids in spring-data-jpa and spring-data-mongo, I don't understand what the issue is with adding a corresponding mechanism here. As an actual user of this library, it's a feature I would like.

kirankumarkolli commented 4 years ago

PartitionKey and Id are equally important documnet/item context. Autogenerate if-not-exists semantics are harder to reason about when its applied or not.

Tooling or convenience layer (like Spring-data or any tools) can make that choice if-needed. In this specific case can the sprint layer make the auto-generation choice right?

mlueders commented 4 years ago

@kirankumarkolli I'm not sure what you mean by Autogenerate if-not-exists semantics are harder to reason about when its applied or not. - in this case, if the @GeneratedValue annotation is applied to a field, the value of that field (and only that field) will be auto-generated. I don't think it ever makes sense to auto-generate a partition key but in many cases it does make sense to auto-generate an id.

Also, re: In this specific case can the sprint layer make the auto-generation choice right? - can you elaborate on this? spring-data-jpa uses javax.persistence.GeneratedValue and with spring-data-mongo you can achieve a similar result by implementing a listener that extends AbstractMongoEventListener, though to me it seems like the spring-data-jpa approach with an annotation on the class is much better b/c it's clear when you're looking at the class that the intent is for the system to generate the id.

kushagraThapar commented 4 years ago

This feature is introduced in new version of spring-data-cosmos SDK v3. Closing this PR now.