Skip to content

DATACOUCH-625 - Save doesn't return updated entity if immutable. #274

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -89,14 +89,28 @@ public <T> T decodeEntity(String id, String source, long cas, Class<T> entityCla
return accessor.getBean();
}

public void applyUpdatedCas(final Object entity, final long cas) {
public Object applyUpdatedCas(final Object entity, final long cas) {
final ConvertingPropertyAccessor<Object> accessor = getPropertyAccessor(entity);
final CouchbasePersistentEntity<?> persistentEntity = mappingContext.getRequiredPersistentEntity(entity.getClass());
final CouchbasePersistentProperty versionProperty = persistentEntity.getVersionProperty();

if (versionProperty != null) {
accessor.setProperty(versionProperty, cas);
return accessor.getBean();
}
return entity;
}

public Object applyUpdatedId(final Object entity, Object id) {
final ConvertingPropertyAccessor<Object> accessor = getPropertyAccessor(entity);
final CouchbasePersistentEntity<?> persistentEntity = mappingContext.getRequiredPersistentEntity(entity.getClass());
final CouchbasePersistentProperty idProperty = persistentEntity.getIdProperty();

if (idProperty != null) {
accessor.setProperty(idProperty, id);
return accessor.getBean();
}
return entity;
}

public long getCas(final Object entity) {
Expand All @@ -106,9 +120,9 @@ public long getCas(final Object entity) {

long cas = 0;
if (versionProperty != null) {
Object casObject = (Number)accessor.getProperty(versionProperty);
if (casObject instanceof Number){
cas = ((Number)casObject).longValue();
Object casObject = (Number) accessor.getProperty(versionProperty);
if (casObject instanceof Number) {
cas = ((Number) casObject).longValue();
}
}
return cas;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,8 @@ public Mono<T> one(T object) {
CouchbaseDocument converted = template.support().encodeEntity(o);
return template.getCollection(collection).reactive()
.insert(converted.getId(), converted.export(), buildInsertOptions()).map(result -> {
template.support().applyUpdatedCas(object, result.cas());
return o;
Object updatedObject = template.support().applyUpdatedId(o, converted.getId());
return (T) template.support().applyUpdatedCas(updatedObject, result.cas());
});
}).onErrorMap(throwable -> {
if (throwable instanceof RuntimeException) {
Expand All @@ -97,7 +97,7 @@ private InsertOptions buildInsertOptions() {
} else if (durabilityLevel != DurabilityLevel.NONE) {
options.durability(durabilityLevel);
}
if (expiry != null && ! expiry.isZero()) {
if (expiry != null && !expiry.isZero()) {
options.expiry(expiry);
} else if (domainType.isAnnotationPresent(Document.class)) {
Document documentAnn = domainType.getAnnotation(Document.class);
Expand All @@ -110,26 +110,30 @@ private InsertOptions buildInsertOptions() {
@Override
public TerminatingInsertById<T> inCollection(final String collection) {
Assert.hasText(collection, "Collection must not be null nor empty.");
return new ReactiveInsertByIdSupport<>(template, domainType, collection, persistTo, replicateTo, durabilityLevel, expiry);
return new ReactiveInsertByIdSupport<>(template, domainType, collection, persistTo, replicateTo, durabilityLevel,
expiry);
}

@Override
public InsertByIdWithCollection<T> withDurability(final DurabilityLevel durabilityLevel) {
Assert.notNull(durabilityLevel, "Durability Level must not be null.");
return new ReactiveInsertByIdSupport<>(template, domainType, collection, persistTo, replicateTo, durabilityLevel, expiry);
return new ReactiveInsertByIdSupport<>(template, domainType, collection, persistTo, replicateTo, durabilityLevel,
expiry);
}

@Override
public InsertByIdWithCollection<T> withDurability(final PersistTo persistTo, final ReplicateTo replicateTo) {
Assert.notNull(persistTo, "PersistTo must not be null.");
Assert.notNull(replicateTo, "ReplicateTo must not be null.");
return new ReactiveInsertByIdSupport<>(template, domainType, collection, persistTo, replicateTo, durabilityLevel, expiry);
return new ReactiveInsertByIdSupport<>(template, domainType, collection, persistTo, replicateTo, durabilityLevel,
expiry);
}

@Override
public InsertByIdWithDurability<T> withExpiry(final Duration expiry) {
Assert.notNull(expiry, "expiry must not be null.");
return new ReactiveInsertByIdSupport<>(template, domainType, collection, persistTo, replicateTo, durabilityLevel, expiry);
return new ReactiveInsertByIdSupport<>(template, domainType, collection, persistTo, replicateTo, durabilityLevel,
expiry);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
*/
package org.springframework.data.couchbase.core;

import org.springframework.data.couchbase.core.mapping.CouchbasePersistentEntity;
import org.springframework.data.couchbase.core.mapping.CouchbasePersistentProperty;
import org.springframework.data.couchbase.core.mapping.Document;
import reactor.core.publisher.Flux;
import reactor.core.publisher.Mono;
Expand Down Expand Up @@ -73,8 +75,8 @@ public Mono<T> one(T object) {
CouchbaseDocument converted = template.support().encodeEntity(o);
return template.getCollection(collection).reactive()
.upsert(converted.getId(), converted.export(), buildUpsertOptions()).map(result -> {
template.support().applyUpdatedCas(object, result.cas());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it make sense to move both apply methods into the upsert map callback so they are done in one spot?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a lot of thinking, I made this change.

return o;
Object updatedObject = template.support().applyUpdatedId(o, converted.getId());
return (T) template.support().applyUpdatedCas(updatedObject, result.cas());
});
}).onErrorMap(throwable -> {
if (throwable instanceof RuntimeException) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@mikereiche mikereiche Oct 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(…).

accessor.setProperty(idProperty, generatedId);
} else {
target.setId(id);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
/*
* Copyright 2020 the original author or authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.springframework.data.couchbase.domain;

import java.lang.reflect.Field;

/**
* Comparable entity base class for tests
*
* @author Michael Reiche
*/
public class ComparableEntity {

/**
* equals() method that recursively calls equals on on fields
*
* @param that
* @return
* @throws RuntimeException
*/
@Override
public boolean equals(Object that) throws RuntimeException {
if (this == that) {
return true;
}
if (that == null
|| !(this.getClass().isAssignableFrom(that.getClass()) || that.getClass().isAssignableFrom(this.getClass()))) {
return false;
}
// check that all the fields in this have an equal field in that
for (Field f : this.getClass().getFields()) {
if (!same(f, this, that)) {
return false;
}
}
// check that all the fields in that have an equal field in this
for (Field f : that.getClass().getFields()) {
if (!same(f, that, this)) {
return false;
}
}
// check that all the declared fields in this have an equal field in that
for (Field f : this.getClass().getDeclaredFields()) {
if (!same(f, this, that)) {
return false;
}
}
// check that all the declared fields in that have an equal field in this
for (Field f : that.getClass().getDeclaredFields()) {
if (!same(f, that, this)) {
return false;
}
}
return true;
}

private static boolean same(Field f, Object a, Object b) {
Object thisField = null;
Object thatField = null;

try {
thisField = f.get(a);
thatField = f.get(b);
} catch (IllegalAccessException e) {
// assume that the important fields are in toString()
thisField = a.toString();
thatField = b.toString();
}
if (thisField == null && thatField == null) {
return true;
}
if (thisField == null && thatField != null) {
return false;
}
if (!thisField.equals(thatField)) {
return false;
}
return true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
*
* @author Michael Reiche
*/
public class Course {
public class Course extends ComparableEntity {
@Id private final String id;
private final String userId;
private final String room;
Expand All @@ -52,42 +52,4 @@ public String toString() {
return sb.toString();
}

@Override
public boolean equals(Object that) throws RuntimeException {
if (this == that)
return true;
if (that == null || getClass() != that.getClass())
return false;
for (Field f : getClass().getFields()) {
if (!same(f, this, that))
return false;
}
for (Field f : getClass().getDeclaredFields()) {
if (!same(f, this, that))
return false;
}
return true;
}

private static boolean same(Field f, Object a, Object b) {
Object thisField = null;
Object thatField = null;

try {
f.get(a);
f.get(b);
} catch (IllegalAccessException e) {
throw new RuntimeException(e);
}
if (thisField == null && thatField == null) {
return true;
}
if (thisField == null && thatField != null) {
return false;
}
if (!thisField.equals(thatField)) {
return false;
}
return true;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/*
* Copyright 2020 the original author or authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.springframework.data.couchbase.domain;

import lombok.Value;
import lombok.With;
import org.springframework.data.annotation.Id;
import org.springframework.data.annotation.Version;
import org.springframework.data.couchbase.core.mapping.Document;
import org.springframework.data.couchbase.core.mapping.Field;
import org.springframework.data.couchbase.core.mapping.id.GeneratedValue;
import org.springframework.data.couchbase.core.mapping.id.GenerationStrategy;

/**
* PersonValue entity for tests
*
* @author Michael Reiche
*/

@Value
@Document
public class PersonValue {
@Id @GeneratedValue(strategy = GenerationStrategy.UNIQUE)
@With String id;
// @Version @With
long version;
@Field String firstname;
@Field String lastname;

public PersonValue(String id, long version, String firstname, String lastname) {
this.id = id;
this.version = version;
this.firstname = firstname;
this.lastname = lastname;
}

public String toString() {
StringBuilder sb = new StringBuilder();
sb.append("PersonValue : {");
sb.append(" id : " + getId());
sb.append(", version : " + version);
sb.append(", firstname : " + firstname);
sb.append(", lastname : " + lastname);
sb.append(" }");
return sb.toString();
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
* Copyright 2020 the original author or authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.springframework.data.couchbase.domain;

import org.springframework.data.repository.CrudRepository;

/**
* PersonValue repository for tests
*
* @author Michael Reiche
*/
public interface PersonValueRepository extends CrudRepository<PersonValue, String> {

}
Loading