DATACOUCH-625 - Save doesn't return updated entity if immutable. #274
Conversation
| @@ -541,6 +541,7 @@ public void doWithPersistentProperty(final CouchbasePersistentProperty prop) { | |||
| generatedValueInfo = idProperty.findAnnotation(GeneratedValue.class); | |||
| String generatedId = generateId(generatedValueInfo, prefixes, suffixes, idAttributes); | |||
| target.setId(generatedId); | |||
| // this is not effective if id is Immutable, and accessor.setProperty() returns a new object in getBean() | |||
There was a problem hiding this comment.
Id population should happen on the Template API level, see MongoDB for reference. Note, newer versions have the Id population extracted because it happened in multiple places.
There was a problem hiding this comment.
After adding the update via CouchbaseTemplateSupport.applyUpdatedId() ( called by Reactive[Insert/Upsert]ByIdSupport.one(entity) which is called from the template), I left the update to the sourceObject here for two reasons - the first being that a number of unit tests depend on it (those can be fixed), the second is that if it is removed from here, it would appear to be a regression for anyone calling repository.save( sourceObject ), and expecting sourceObject to contain the id. They should be calling as savedObject = repository.save( sourceObject ). I could remove this now in hope that will result in fewer customers using it incorrectly, or wait to do it in the 4.1 release because it could be a breaking change for some.
My first idea was to have the Couchbase implementation of EntityWriter.write(source, target) return the updated object - but the signature returns void. (updating here and returning accessor.getBean() would have worked for that).
There could be one benefit from updating the source object with the id. For async/reactive calls - the generated id would be available immediately from the source object. The id from the saved object is not available until the save operation completes.
There was a problem hiding this comment.
Let's schedule any removal for the next version. We're in the RC phase with the GA release coming up Oct 28.
In any case in an async/reactive API usage scenario, it's hard to reason about when the model is being updated. In-place updates could have nasty side effects therefore we generally recommend using immutable types if you want to work with the saved/updated object instance that is returned by save(…).
| return template.getCollection(collection).reactive() | ||
| .upsert(converted.getId(), converted.export(), buildUpsertOptions()).map(result -> { | ||
| template.support().applyUpdatedCas(object, result.cas()); |
There was a problem hiding this comment.
would it make sense to move both apply methods into the upsert map callback so they are done in one spot?
There was a problem hiding this comment.
For both id and cas updates, they are done as soon as the value is available.
There is no efficiency benefit to doing them at the same time even for immutable entities - each update will require that a new object be created.
There was a problem hiding this comment.
After a lot of thinking, I made this change.
Modified applyUpdatedCas() to return accessor.getBean() which will occur if the version property is Immutable. Created applyUpdateId() that is analogous to applyUpdatedCas() Factored out the equals() method that was used by a number of test entity objects.
mikereiche
force-pushed
the
g625_save_doesnt_return_updated_entity_if_immutable
branch
from
a00e997 to
41fa412
Compare
mikereiche
deleted the
g625_save_doesnt_return_updated_entity_if_immutable
branch
Modified applyUpdatedCas() to return accessor.getBean() which will
occur if the version property is Immutable.
Created applyUpdateId() that is analogous to applyUpdatedCas()
Factored out the equals() method that was used by a number of
test entity objects.