Skip to content

Commit 865adfc

Browse files
emilienbevemilienbev
authored
Remove unnecessary intermediate String deserialization of document bodies (#2122)
* Remove unnecessary intermediate String deserialization of document in favour of the raw bytes * Fix intermittent test failure where deleteById(id).block() is called immediately after subscribe(), before the document has potentially been saved. * Fix typo "expiryTIme" * Provide default fallback implementation of decodeEntity(Object id, byte[] ...) (back to String). * Ensure that if both parsing and close throw, the close exception is suppressed rather than masking the original error. * Add ByteUtils utility static class for byte array/string conversions * Re-order author tags correctly * Remove excessive final modifiers * Remove decodeEntityBase logic duplication by using a shared function Adjust comments * Remove final modifiers in var declaration and method parameters * Use String.formatted() for logging --------- Signed-off-by: Emilien Bevierre <emilien.bevierre@couchbase.com> Signed-off-by: Emilien Bevierre <44171454+emilienbev@users.noreply.github.com>
1 parent 7e781a2 commit 865adfc

21 files changed

Lines changed: 386 additions & 144 deletions

‎.gitignore‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@ target/
55
.project
66
.settings
77

8+
src/test/resources/integration.local.properties
9+
810
*.iml
911
.idea
1012

‎src/main/java/org/springframework/data/couchbase/cache/CouchbaseCacheConfiguration.java‎

Lines changed: 25 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616

1717
package org.springframework.data.couchbase.cache;
1818

19-
import java.nio.charset.StandardCharsets;
19+
import org.springframework.data.couchbase.core.util.ByteUtils;
2020
import java.time.Duration;
2121

2222
import org.springframework.cache.Cache;
@@ -60,7 +60,8 @@ public static CouchbaseCacheConfiguration defaultCacheConfig() {
6060
}
6161

6262
/**
63-
* Registers default cache key converters. The following converters get registered:
63+
* Registers default cache key converters. The following converters get
64+
* registered:
6465
* <ul>
6566
* <li>{@link String} to byte using UTF-8 encoding.</li>
6667
* <li>{@link SimpleKey} to {@link String}</li>
@@ -70,12 +71,13 @@ public static CouchbaseCacheConfiguration defaultCacheConfig() {
7071
*/
7172
public static void registerDefaultConverters(final ConverterRegistry registry) {
7273
Assert.notNull(registry, "ConverterRegistry must not be null!");
73-
registry.addConverter(String.class, byte[].class, source -> source.getBytes(StandardCharsets.UTF_8));
74+
registry.addConverter(String.class, byte[].class, ByteUtils::getBytes);
7475
registry.addConverter(SimpleKey.class, String.class, SimpleKey::toString);
7576
}
7677

7778
/**
78-
* Set the expiry to apply for cache entries. Use {@link Duration#ZERO} to declare an eternal cache.
79+
* Set the expiry to apply for cache entries. Use {@link Duration#ZERO} to
80+
* declare an eternal cache.
7981
*
8082
* @param expiry must not be {@literal null}.
8183
* @return new {@link CouchbaseCacheConfiguration}.
@@ -112,9 +114,13 @@ public CouchbaseCacheConfiguration valueTranscoder(final Transcoder valueTransco
112114

113115
/**
114116
* Disable caching {@literal null} values. <br />
115-
* <strong>NOTE</strong> any {@link org.springframework.cache.Cache#put(Object, Object)} operation involving
116-
* {@literal null} value will error. Nothing will be written to Couchbase, nothing will be removed. An already
117-
* existing key will still be there afterwards with the very same value as before.
117+
* <strong>NOTE</strong> any
118+
* {@link org.springframework.cache.Cache#put(Object, Object)} operation
119+
* involving
120+
* {@literal null} value will error. Nothing will be written to Couchbase,
121+
* nothing will be removed. An already
122+
* existing key will still be there afterwards with the very same value as
123+
* before.
118124
*
119125
* @return new {@link CouchbaseCacheConfiguration}.
120126
*/
@@ -124,8 +130,10 @@ public CouchbaseCacheConfiguration disableCachingNullValues() {
124130
}
125131

126132
/**
127-
* Prefix the {@link CouchbaseCache#getName() cache name} with the given value. <br />
128-
* The generated cache key will be: {@code prefix + cache name + "::" + cache entry key}.
133+
* Prefix the {@link CouchbaseCache#getName() cache name} with the given value.
134+
* <br />
135+
* The generated cache key will be:
136+
* {@code prefix + cache name + "::" + cache entry key}.
129137
*
130138
* @param prefix the prefix to prepend to the cache name.
131139
* @return this.
@@ -137,7 +145,8 @@ public CouchbaseCacheConfiguration prefixCacheNameWith(final String prefix) {
137145
}
138146

139147
/**
140-
* Use the given {@link CacheKeyPrefix} to compute the prefix for the actual Couchbase {@literal key} given the
148+
* Use the given {@link CacheKeyPrefix} to compute the prefix for the actual
149+
* Couchbase {@literal key} given the
141150
* {@literal cache name} as function input.
142151
*
143152
* @param cacheKeyPrefix must not be {@literal null}.
@@ -165,14 +174,16 @@ public boolean getAllowCacheNullValues() {
165174
}
166175

167176
/**
168-
* @return The {@link ConversionService} used for cache key to {@link String} conversion. Never {@literal null}.
177+
* @return The {@link ConversionService} used for cache key to {@link String}
178+
* conversion. Never {@literal null}.
169179
*/
170180
public ConversionService getConversionService() {
171181
return conversionService;
172182
}
173183

174184
/**
175-
* @return {@literal true} if cache keys need to be prefixed with the {@link #getKeyPrefixFor(String)} if present or
185+
* @return {@literal true} if cache keys need to be prefixed with the
186+
* {@link #getKeyPrefixFor(String)} if present or
176187
* the default which resolves to {@link Cache#getName()}.
177188
*/
178189
public boolean usePrefix() {
@@ -197,7 +208,8 @@ public Transcoder getValueTranscoder() {
197208
}
198209

199210
/**
200-
* The name of the collection to use for this cache - if empty uses the default collection.
211+
* The name of the collection to use for this cache - if empty uses the default
212+
* collection.
201213
*/
202214
public String getCollectionName() {
203215
return collectionName;

‎src/main/java/org/springframework/data/couchbase/core/AbstractTemplateSupport.java‎

Lines changed: 98 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,8 @@
1919
import java.time.Instant;
2020
import java.util.Map;
2121
import java.util.Set;
22+
import java.util.function.BiFunction;
2223

23-
import com.couchbase.client.core.annotation.Stability;
2424
import org.slf4j.Logger;
2525
import org.slf4j.LoggerFactory;
2626
import org.springframework.context.ApplicationContext;
@@ -40,13 +40,14 @@
4040
import org.springframework.data.mapping.model.ConvertingPropertyAccessor;
4141
import org.springframework.util.ClassUtils;
4242

43+
import com.couchbase.client.core.annotation.Stability;
4344
import com.couchbase.client.core.error.CouchbaseException;
4445

45-
4646
/**
4747
* Base shared by Reactive and non-Reactive TemplateSupport
4848
*
4949
* @author Michael Reiche
50+
* @author Emilien Bevierre
5051
*/
5152
@Stability.Internal
5253
public abstract class AbstractTemplateSupport {
@@ -70,61 +71,96 @@ public AbstractTemplateSupport(ReactiveCouchbaseTemplate template, CouchbaseConv
7071

7172
public <T> T decodeEntityBase(Object id, String source, Long cas, Instant expiryTime, Class<T> entityClass,
7273
String scope, String collection, Object txResultHolder, CouchbaseResourceHolder holder) {
74+
return decodeEntityBase(id, cas, expiryTime, entityClass, scope, collection, txResultHolder, holder,
75+
(ts, converted) -> (CouchbaseDocument) ts.decode(source, converted));
76+
}
7377

74-
// this is the entity class defined for the repository. It may not be the class of the document that was read
75-
// we will reset it after reading the document
76-
//
77-
// This will fail for the case where:
78-
// 1) The version is defined in the concrete class, but not in the abstract class; and
79-
// 2) The constructor takes a "long version" argument resulting in an exception would be thrown if version in
80-
// the source is null.
81-
// We could expose from the MappingCouchbaseConverter determining the persistent entity from the source,
82-
// but that is a lot of work to do every time just for this very rare and avoidable case.
83-
// TypeInformation<? extends R> typeToUse = typeMapper.readType(source, type);
78+
public <T> T decodeEntityBase(Object id, byte[] source, Long cas, Instant expiryTime, Class<T> entityClass,
79+
String scope, String collection, Object txResultHolder, CouchbaseResourceHolder holder) {
80+
return decodeEntityBase(id, cas, expiryTime, entityClass, scope, collection, txResultHolder, holder,
81+
(ts, converted) -> (CouchbaseDocument) ts.decode(source, converted));
82+
}
8483

84+
private <T> T decodeEntityBase(Object id, Long cas, Instant expiryTime, Class<T> entityClass, String scope,
85+
String collection, Object txResultHolder, CouchbaseResourceHolder holder,
86+
BiFunction<TranslationService, CouchbaseDocument, CouchbaseDocument> translatorFn) {
8587
CouchbasePersistentEntity persistentEntity = couldBePersistentEntity(entityClass);
8688

87-
if (persistentEntity == null) { // method could return a Long, Boolean, String etc.
88-
// QueryExecutionConverters.unwrapWrapperTypes will recursively unwrap until there is nothing left
89-
// to unwrap. This results in List<String[]> being unwrapped past String[] to String, so this may also be a
90-
// Collection (or Array) of entityClass. We have no way of knowing - so just assume it is what we are told.
91-
// if this is a Collection or array, only the first element will be returned.
92-
final CouchbaseDocument converted = new CouchbaseDocument(id);
93-
Set<Map.Entry<String, Object>> set = ((CouchbaseDocument) translationService.decode(source, converted))
94-
.getContent().entrySet();
89+
if (persistentEntity == null) {
90+
CouchbaseDocument converted = new CouchbaseDocument(id);
91+
Set<Map.Entry<String, Object>> set = translatorFn.apply(translationService, converted).getContent()
92+
.entrySet();
9593
return (T) set.iterator().next().getValue();
9694
}
9795

96+
CouchbaseDocument converted = prepareConvertedDocument(id, cas, persistentEntity);
97+
T readEntity = converter.read(entityClass, translatorFn.apply(translationService, converted));
98+
return finalizeEntity(readEntity, id, cas, expiryTime, scope, collection, txResultHolder, holder);
99+
}
100+
101+
private CouchbaseDocument prepareConvertedDocument(Object id, Long cas,
102+
CouchbasePersistentEntity persistentEntity) {
103+
// persistentEntity is derived from the entityClass declared in the
104+
// repository definition. It may be an abstract class rather than the
105+
// concrete class of the document being read. The concrete type is only
106+
// known after converter.read() is called, therefore version/cas is set again
107+
// on the final entity in finalizeEntity().
108+
//
109+
// Pre-populating the CAS/version into the source document (done in getDocument)
110+
// is a best-effort step to avoid a construction failure when the concrete
111+
// class constructor takes a primitive "long version" argument (null is not a
112+
// valid value for a primitive). If the version property is only declared on the
113+
// concrete subclass and not on the abstract base, pre-population is not
114+
// possible here and the issue can be avoided by using "Long" instead of "long".
115+
//
116+
// An alternative would be to resolve the actual concrete type from the source
117+
// document's type metadata before constructing it (see the comment
118+
// below), but that adds overhead for every decode to solve a rare and avoidable
119+
// case.
120+
// TypeInformation<? extends R> typeToUse = typeMapper.readType(source, type);
121+
98122
if (id == null) {
99-
throw new CouchbaseException(TemplateUtils.SELECT_ID + " was null. Either use #{#n1ql.selectEntity} or project "
100-
+ TemplateUtils.SELECT_ID);
123+
throw new CouchbaseException("%s was null. Either use #{#n1ql.selectEntity} or project %s"
124+
.formatted(TemplateUtils.SELECT_ID, TemplateUtils.SELECT_ID));
101125
}
102126

103-
final CouchbaseDocument converted = new CouchbaseDocument(id);
104-
105-
// if possible, set the version property in the source so that if the constructor has a long version argument,
106-
// it will have a value and not fail (as null is not a valid argument for a long argument). This possible failure
107-
// can be avoid by defining the argument as Long instead of long.
108-
// persistentEntity is still the (possibly abstract) class specified in the repository definition
109-
// it's possible that the abstract class does not have a version property, and this won't be able to set the version
110-
if (persistentEntity.getVersionProperty() != null) {
111-
if (cas == null) {
112-
throw new CouchbaseException("version/cas in the entity but " + TemplateUtils.SELECT_CAS
113-
+ " was not in result. Either use #{#n1ql.selectEntity} or project " + TemplateUtils.SELECT_CAS);
114-
}
115-
if (cas != 0) {
116-
converted.put(persistentEntity.getVersionProperty().getName(), cas);
117-
}
118-
}
127+
return getDocument(id, cas, persistentEntity);
128+
}
119129

120-
// if the constructor has an argument that is long version, then construction will fail if the 'version'
121-
// is not available as 'null' is not a legal value for a long. Changing the arg to "Long version" would solve this.
122-
// (Version doesn't come from 'source', it comes from the cas argument to decodeEntity)
123-
T readEntity = converter.read(entityClass, (CouchbaseDocument) translationService.decode(source, converted));
124-
final ConvertingPropertyAccessor<T> accessor = getPropertyAccessor(readEntity);
130+
private static CouchbaseDocument getDocument(Object id, Long cas, CouchbasePersistentEntity persistentEntity) {
131+
CouchbaseDocument converted = new CouchbaseDocument(id);
132+
133+
// If possible, set the version property in the source so that if the
134+
// constructor has a long version argument, it will have a value and succeed,
135+
// as null is not a valid argument for a long. This failure can be avoided by
136+
// defining the argument as Long instead of long.
137+
// Note that persistentEntity is still the (possibly abstract) class specified
138+
// in the repository definition, so it's possible that the abstract class does
139+
// not have a version property, in which case this won't be able to set the version.
140+
if (persistentEntity.getVersionProperty() != null) {
141+
if (cas == null) {
142+
throw new CouchbaseException(
143+
"version/cas in the entity but %s was not in result. Either use #{#n1ql.selectEntity} or project %s"
144+
.formatted(TemplateUtils.SELECT_CAS, TemplateUtils.SELECT_CAS));
145+
}
146+
if (cas != 0) {
147+
converted.put(persistentEntity.getVersionProperty().getName(), cas);
148+
}
149+
}
150+
return converted;
151+
}
152+
153+
private <T> T finalizeEntity(T readEntity, Object id, Long cas, Instant expiryTime, String scope, String collection,
154+
Object txResultHolder, CouchbaseResourceHolder holder) {
155+
ConvertingPropertyAccessor<T> accessor = getPropertyAccessor(readEntity);
125156

126-
persistentEntity = couldBePersistentEntity(readEntity.getClass());
157+
CouchbasePersistentEntity persistentEntity = couldBePersistentEntity(readEntity.getClass());
127158

159+
// If the constructor has a long version argument, construction will fail if
160+
// 'version' is not available, as null is not a legal value for a long.
161+
// We therefore use the object-wrapped Long type.
162+
// (Version doesn't come from 'source', it comes from the cas argument to
163+
// decodeEntity.)
128164
if (cas != null && cas != 0 && persistentEntity.getVersionProperty() != null) {
129165
accessor.setProperty(persistentEntity.getVersionProperty(), cas);
130166
}
@@ -133,7 +169,8 @@ public <T> T decodeEntityBase(Object id, String source, Long cas, Instant expiry
133169
accessor.setProperty(persistentEntity.getExpiryProperty(), expiryTime);
134170
}
135171

136-
N1qlJoinResolver.handleProperties(persistentEntity, accessor, getReactiveTemplate(), id.toString(), scope, collection);
172+
N1qlJoinResolver.handleProperties(persistentEntity, accessor, getReactiveTemplate(), id.toString(), scope,
173+
collection);
137174

138175
if (holder != null) {
139176
holder.transactionResultHolder(txResultHolder, (T) accessor.getBean());
@@ -158,15 +195,15 @@ public <T> T applyResultBase(T entity, CouchbaseDocument converted, Object id, l
158195
Object txResultHolder, CouchbaseResourceHolder holder) {
159196
ConvertingPropertyAccessor<Object> accessor = getPropertyAccessor(entity);
160197

161-
final CouchbasePersistentEntity<?> persistentEntity = converter.getMappingContext()
198+
CouchbasePersistentEntity<?> persistentEntity = converter.getMappingContext()
162199
.getRequiredPersistentEntity(entity.getClass());
163200

164-
final CouchbasePersistentProperty idProperty = persistentEntity.getIdProperty();
201+
CouchbasePersistentProperty idProperty = persistentEntity.getIdProperty();
165202
if (idProperty != null) {
166203
accessor.setProperty(idProperty, id);
167204
}
168205

169-
final CouchbasePersistentProperty versionProperty = persistentEntity.getVersionProperty();
206+
CouchbasePersistentProperty versionProperty = persistentEntity.getVersionProperty();
170207
if (versionProperty != null) {
171208
accessor.setProperty(versionProperty, cas);
172209
}
@@ -179,10 +216,11 @@ public <T> T applyResultBase(T entity, CouchbaseDocument converted, Object id, l
179216

180217
}
181218

182-
public Long getCas(final Object entity) {
183-
final ConvertingPropertyAccessor<Object> accessor = getPropertyAccessor(entity);
184-
final CouchbasePersistentEntity<?> persistentEntity = mappingContext.getRequiredPersistentEntity(entity.getClass());
185-
final CouchbasePersistentProperty versionProperty = persistentEntity.getVersionProperty();
219+
public Long getCas(Object entity) {
220+
ConvertingPropertyAccessor<Object> accessor = getPropertyAccessor(entity);
221+
CouchbasePersistentEntity<?> persistentEntity = mappingContext
222+
.getRequiredPersistentEntity(entity.getClass());
223+
CouchbasePersistentProperty versionProperty = persistentEntity.getVersionProperty();
186224
long cas = 0;
187225
if (versionProperty != null) {
188226
Object casObject = accessor.getProperty(versionProperty);
@@ -193,24 +231,25 @@ public Long getCas(final Object entity) {
193231
return cas;
194232
}
195233

196-
public Object getId(final Object entity) {
197-
final ConvertingPropertyAccessor<Object> accessor = getPropertyAccessor(entity);
198-
final CouchbasePersistentEntity<?> persistentEntity = mappingContext.getRequiredPersistentEntity(entity.getClass());
199-
final CouchbasePersistentProperty idProperty = persistentEntity.getIdProperty();
234+
public Object getId(Object entity) {
235+
ConvertingPropertyAccessor<Object> accessor = getPropertyAccessor(entity);
236+
CouchbasePersistentEntity<?> persistentEntity = mappingContext
237+
.getRequiredPersistentEntity(entity.getClass());
238+
CouchbasePersistentProperty idProperty = persistentEntity.getIdProperty();
200239
Object id = null;
201240
if (idProperty != null) {
202241
id = accessor.getProperty(idProperty);
203242
}
204243
return id;
205244
}
206245

207-
public String getJavaNameForEntity(final Class<?> clazz) {
208-
final CouchbasePersistentEntity<?> persistentEntity = mappingContext.getRequiredPersistentEntity(clazz);
246+
public String getJavaNameForEntity(Class<?> clazz) {
247+
CouchbasePersistentEntity<?> persistentEntity = mappingContext.getRequiredPersistentEntity(clazz);
209248
MappingCouchbaseEntityInformation<?, Object> info = new MappingCouchbaseEntityInformation<>(persistentEntity);
210249
return info.getJavaType().getName();
211250
}
212251

213-
<T> ConvertingPropertyAccessor<T> getPropertyAccessor(final T source) {
252+
<T> ConvertingPropertyAccessor<T> getPropertyAccessor(T source) {
214253
CouchbasePersistentEntity<?> entity = mappingContext.getRequiredPersistentEntity(source.getClass());
215254
PersistentPropertyAccessor<T> accessor = entity.getPropertyAccessor(source);
216255
return new ConvertingPropertyAccessor<>(accessor, converter.getConversionService());

‎src/main/java/org/springframework/data/couchbase/core/CouchbaseTemplateSupport.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939
* @author Michael Reiche
4040
* @author Jorge Rodriguez Martin
4141
* @author Carlos Espinaco
42+
* @author Emilien Bevierre
4243
* @since 3.0
4344
*/
4445
class CouchbaseTemplateSupport extends AbstractTemplateSupport implements ApplicationContextAware, TemplateSupport {
@@ -69,6 +70,12 @@ public <T> T decodeEntity(Object id, String source, Long cas, Instant expiryTime
6970
return decodeEntityBase(id, source, cas, expiryTime, entityClass, scope, collection, txHolder, holder);
7071
}
7172

73+
@Override
74+
public <T> T decodeEntity(Object id, byte[] source, Long cas, Instant expiryTime, Class<T> entityClass,
75+
String scope, String collection, Object txHolder, CouchbaseResourceHolder holder) {
76+
return decodeEntityBase(id, source, cas, expiryTime, entityClass, scope, collection, txHolder, holder);
77+
}
78+
7279
@Override
7380
public <T> T applyResult(T entity, CouchbaseDocument converted, Object id, long cas,
7481
Object txResultHolder, CouchbaseResourceHolder holder) {

‎src/main/java/org/springframework/data/couchbase/core/NonReactiveSupportWrapper.java‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
*
2929
* @author Carlos Espinaco
3030
* @author Michael Reiche
31+
* @author Emilien Bevierre
3132
* @since 4.2
3233
*/
3334
public class NonReactiveSupportWrapper implements ReactiveTemplateSupport {
@@ -50,6 +51,13 @@ public <T> Mono<T> decodeEntity(Object id, String source, Long cas, Instant expi
5051
txResultHolder, holder));
5152
}
5253

54+
@Override
55+
public <T> Mono<T> decodeEntity(Object id, byte[] source, Long cas, Instant expiryTime, Class<T> entityClass,
56+
String scope, String collection, Object txResultHolder, CouchbaseResourceHolder holder) {
57+
return Mono.fromSupplier(() -> support.decodeEntity(id, source, cas, expiryTime, entityClass, scope, collection,
58+
txResultHolder, holder));
59+
}
60+
5361
@Override
5462
public <T> Mono<T> applyResult(T entity, CouchbaseDocument converted, Object id, Long cas,
5563
Object txResultHolder, CouchbaseResourceHolder holder) {

0 commit comments

Comments
 (0)