Skip to content

Commit d2f8ed5

Browse files
nvamelichevnvamelichev
committed
Introduce strict enum deserialization: case-sensitive, and not substituting null for unknown enum constants.
To use strict deserialization, call `CommonConverters#defineEnumDeserializer(EnumDeserializer)` and use the `EnumDeserializer#STRICT` deserialization strategy. Drive-by change: Much better `InMemoryTable#update(Entity.Id, Changeset)` implementation (it was needed for tests). We hope to phase out current lenient enum deserialization behavior eventually, making it non-default in YOJ 2.7.0 (where the default will become the STRICT deserializer), and planning to remove it entirely in YOJ 3.0.0, where you probably won't be able to customize enum deserialization strategy any more.
1 parent 7c28079 commit d2f8ed5

File tree

20 files changed

+536
-94
lines changed

20 files changed

+536
-94
lines changed

‎json-jackson-v2/src/main/java/tech/ydb/yoj/repository/db/json/JacksonJsonConverter.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@ public static JsonConverter getDefault() {
7474
return instance;
7575
}
7676

77+
@NonNull
7778
@Override
7879
public String toJson(@NonNull Type type, @Nullable Object o) throws ConversionException {
7980
try {
@@ -83,6 +84,7 @@ public String toJson(@NonNull Type type, @Nullable Object o) throws ConversionEx
8384
}
8485
}
8586

87+
@Nullable
8688
@Override
8789
public <T> T fromJson(@NonNull Type type, @NonNull String content) throws ConversionException {
8890
try {
@@ -92,6 +94,7 @@ public <T> T fromJson(@NonNull Type type, @NonNull String content) throws Conver
9294
}
9395
}
9496

97+
@Nullable
9598
@Override
9699
@SuppressWarnings("unchecked")
97100
public <T> T fromObject(@NonNull Type type, @Nullable Object content) throws ConversionException {
@@ -105,13 +108,15 @@ public <T> T fromObject(@NonNull Type type, @Nullable Object content) throws Con
105108
}
106109
}
107110

111+
@NonNull
108112
public String toString() {
109113
return "JacksonJsonConverter";
110114
}
111115

112116
/**
113117
* @return {@code ObjectMapper} with reasonable defaults for mapping between Java objects and JSON
114118
*/
119+
@NonNull
115120
public static ObjectMapper createDefaultObjectMapper() {
116121
ObjectMapper mapper = new ObjectMapper();
117122
mapper.setTimeZone(TimeZone.getDefault());

‎repository-inmemory/src/main/java/tech/ydb/yoj/repository/test/inmemory/Columns.java

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,12 @@
1717

1818
import java.lang.reflect.Type;
1919
import java.util.ArrayList;
20+
import java.util.HashSet;
2021
import java.util.LinkedHashMap;
2122
import java.util.List;
2223
import java.util.Map;
2324
import java.util.Objects;
25+
import java.util.Set;
2426

2527
import static java.lang.String.format;
2628
import static java.util.Collections.unmodifiableMap;
@@ -43,6 +45,30 @@ public static <T extends Entity<T>> Columns fromEntity(EntitySchema<T> schema, T
4345
return new Columns(Maps.immutable.<String, Object>empty().newWithAllKeyValues(newValues));
4446
}
4547

48+
public Columns patch(EntitySchema<?> schema, Map<String, Object> patch) {
49+
Set<String> columnNames = new HashSet<>(schema.flattenFieldNames());
50+
for (String column : patch.keySet()) {
51+
if (!columnNames.contains(column)) {
52+
String message = format("Invalid patch: unknown column name '%s'; does not belong to entity <%s>", column, schema.getTypeName());
53+
throw new ConversionException(message);
54+
}
55+
}
56+
57+
List<Pair<String, Object>> newValues = new ArrayList<>();
58+
for (Schema.JavaField field : schema.flattenFields()) {
59+
String column = field.getName();
60+
if (patch.containsKey(column)) {
61+
Object value = serialize(field, patch.get(column));
62+
newValues.add(pair(column, value));
63+
}
64+
}
65+
return new Columns(map.newWithAllKeyValues(newValues));
66+
}
67+
68+
public Map<String, Object> toMutableMap() {
69+
return this.map.toMap();
70+
}
71+
4672
public <T> T toSchema(Schema<T> schema) {
4773
try {
4874
return toSchemaUnchecked(schema);
@@ -120,7 +146,7 @@ private static Object deserialize(Schema.JavaField field, Object value) {
120146
default -> throw new IllegalStateException("Don't know how to deserialize field: " + field);
121147
};
122148

123-
return CustomValueTypes.postconvert(field, deserialized);
149+
return deserialized == null ? null : CustomValueTypes.postconvert(field, deserialized);
124150
} catch (Exception e) {
125151
throw new ConversionException("Could not deserialize value of type <" + type + ">", e);
126152
}

‎repository-inmemory/src/main/java/tech/ydb/yoj/repository/test/inmemory/InMemoryDataShard.java

Lines changed: 35 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ public synchronized void checkLocks(long version, InMemoryTxLockWatcher watcher)
9292
continue;
9393
}
9494

95-
for (Range<Entity.Id<T>> lockedRange: lockedRanges) {
95+
for (Range<Entity.Id<T>> lockedRange : lockedRanges) {
9696
if (lockedRange.contains(entry.getKey())) {
9797
throw new OptimisticLockException("Table lock failed " + tableDescriptor.toDebugString());
9898
}
@@ -112,28 +112,28 @@ public synchronized void rollback(long txId) {
112112

113113
@Nullable
114114
public synchronized T find(long txId, long version, Entity.Id<T> id, InMemoryTxLockWatcher watcher) {
115-
checkLocks(version, watcher);
116-
117-
InMemoryEntityLine entityLine = entityLines.get(id);
118-
if (entityLine == null) {
119-
return null;
120-
}
121-
Columns columns = entityLine.get(txId, version);
115+
Columns columns = findColumns(txId, version, id, watcher);
122116
return columns != null ? columns.toSchema(schema) : null;
123117
}
124118

125119
@Nullable
126120
public synchronized <V extends Table.View> V find(
127121
long txId, long version, Entity.Id<T> id, Class<V> viewType, InMemoryTxLockWatcher watcher
128122
) {
123+
Columns columns = findColumns(txId, version, id, watcher);
124+
return columns != null ? columns.toSchema(schema.getViewSchema(viewType)) : null;
125+
}
126+
127+
@Nullable
128+
public synchronized Columns findColumns(long txId, long version, Entity.Id<T> id, InMemoryTxLockWatcher watcher) {
129129
checkLocks(version, watcher);
130130

131131
InMemoryEntityLine entityLine = entityLines.get(id);
132132
if (entityLine == null) {
133133
return null;
134134
}
135-
Columns columns = entityLine.get(txId, version);
136-
return columns != null ? columns.toSchema(schema.getViewSchema(viewType)) : null;
135+
136+
return entityLine.get(txId, version);
137137
}
138138

139139
public synchronized List<T> findAll(long txId, long version, InMemoryTxLockWatcher watcher) {
@@ -162,32 +162,44 @@ public synchronized void insert(long txId, long version, T entity) {
162162
}
163163

164164
public synchronized void save(long txId, long version, T entity) {
165-
InMemoryEntityLine entityLine = entityLines.computeIfAbsent(entity.getId(), __ -> new InMemoryEntityLine());
165+
save(txId, version, entity.getId(), Columns.fromEntity(schema, entity));
166+
}
167+
168+
public synchronized void update(long txId, long version, Entity.Id<T> entityId, InMemoryTxLockWatcher watcher, Map<String, Object> patch) {
169+
Columns columns = findColumns(txId, version, entityId, watcher);
170+
if (columns == null) {
171+
return;
172+
}
173+
save(txId, version, entityId, columns.patch(schema, patch));
174+
}
175+
176+
private synchronized void save(long txId, long version, Entity.Id<T> entityId, Columns columns) {
177+
InMemoryEntityLine entityLine = entityLines.computeIfAbsent(entityId, __ -> new InMemoryEntityLine());
166178

167-
validateUniqueness(txId, version, entity);
168-
uncommited.computeIfAbsent(txId, __ -> new HashSet<>()).add(entity.getId());
179+
validateUniqueness(txId, version, entityId, columns);
180+
uncommited.computeIfAbsent(txId, __ -> new HashSet<>()).add(entityId);
169181

170-
entityLine.put(txId, Columns.fromEntity(schema, entity));
182+
entityLine.put(txId, columns);
171183
}
172184

173-
private void validateUniqueness(long txId, long version, T entity) {
174-
List<Schema.Index> indexes = schema.getGlobalIndexes().stream()
185+
private void validateUniqueness(long txId, long version, Entity.Id<T> entityId, Columns entityColumns) {
186+
List<Schema.Index> uniqueIndexes = schema.getGlobalIndexes().stream()
175187
.filter(Schema.Index::isUnique)
176188
.toList();
177-
for (Schema.Index index : indexes) {
178-
Map<String, Object> entityIndexValues = buildIndexValues(index, entity);
189+
for (Schema.Index uniqueIndex : uniqueIndexes) {
190+
Map<String, Object> entityIndexValues = buildIndexValues(uniqueIndex, entityColumns);
179191
entityLines.forEach((id, line) -> {
180192
Columns columns = line.get(txId, version);
181-
if (columns != null && !id.equals(entity.getId())
182-
&& entityIndexValues.equals(buildIndexValues(index, columns.toSchema(schema)))) {
183-
throw new EntityAlreadyExistsException("Entity " + entity.getId() + " already exists");
193+
if (columns != null && !id.equals(entityId)
194+
&& entityIndexValues.equals(buildIndexValues(uniqueIndex, columns))) {
195+
throw new EntityAlreadyExistsException("Entity " + entityId + " already exists");
184196
}
185197
});
186198
}
187199
}
188200

189-
private Map<String, Object> buildIndexValues(Schema.Index index, T entity) {
190-
Map<String, Object> cells = new HashMap<>(schema.flatten(entity));
201+
private Map<String, Object> buildIndexValues(Schema.Index index, Columns entityColumns) {
202+
Map<String, Object> cells = entityColumns.toMutableMap();
191203
cells.keySet().retainAll(index.getFieldNames());
192204
return cells;
193205
}

‎repository-inmemory/src/main/java/tech/ydb/yoj/repository/test/inmemory/InMemoryTable.java

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@
2424
import tech.ydb.yoj.repository.db.statement.Changeset;
2525

2626
import javax.annotation.Nullable;
27-
import java.util.HashMap;
27+
import java.util.LinkedHashMap;
2828
import java.util.List;
2929
import java.util.Map;
3030
import java.util.Set;
@@ -88,17 +88,12 @@ public long count(String indexName, FilterExpression<T> filter) {
8888
@Override
8989
@Deprecated
9090
public void update(Entity.Id<T> id, Changeset changeset) {
91-
T found = find(id);
92-
if (found == null) {
93-
return;
94-
}
95-
Map<String, Object> cells = new HashMap<>(schema.flatten(found));
96-
97-
changeset.toMap().forEach((k, v) -> cells.putAll(schema.flattenOneField(k, v)));
98-
99-
T newInstance = schema.newInstance(cells);
91+
Map<String, Object> patch = new LinkedHashMap<>();
92+
changeset.toMap().forEach((k, v) -> patch.putAll(schema.flattenOneField(k, v)));
10093

101-
save(newInstance);
94+
transaction.getWatcher().markRowRead(tableDescriptor, id);
95+
transaction.doInWriteTransaction("update(" + id + ", " + changeset + ")", tableDescriptor, shard -> shard.update(id, patch));
96+
transaction.getTransactionLocal().firstLevelCache(tableDescriptor).remove(id);
10297
}
10398

10499
@Override

‎repository-inmemory/src/main/java/tech/ydb/yoj/repository/test/inmemory/TxDataShardImpl.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
import javax.annotation.Nullable;
88
import java.util.List;
9+
import java.util.Map;
910

1011
@RequiredArgsConstructor
1112
/*package*/ final class TxDataShardImpl<T extends Entity<T>> implements ReadOnlyTxDataShard<T>, WriteTxDataShard<T> {
@@ -41,6 +42,11 @@ public void save(T entity) {
4142
shard.save(txId, version, entity);
4243
}
4344

45+
@Override
46+
public void update(Entity.Id<T> id, Map<String, Object> patch) {
47+
shard.update(txId, version, id, watcher, patch);
48+
}
49+
4450
@Override
4551
public void delete(Entity.Id<T> id) {
4652
shard.delete(txId, id);

‎repository-inmemory/src/main/java/tech/ydb/yoj/repository/test/inmemory/WriteTxDataShard.java

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,15 @@
22

33
import tech.ydb.yoj.repository.db.Entity;
44

5+
import java.util.Map;
6+
57
/*package*/ interface WriteTxDataShard<T extends Entity<T>> {
68
void insert(T entity);
79

810
void save(T entity);
911

12+
void update(Entity.Id<T> id, Map<String, Object> patch);
13+
1014
void delete(Entity.Id<T> id);
1115

1216
void deleteAll();

‎repository-test/src/main/java/tech/ydb/yoj/repository/test/RepositoryTest.java

Lines changed: 87 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@
2222
import tech.ydb.yoj.repository.db.Tx;
2323
import tech.ydb.yoj.repository.db.TxManager;
2424
import tech.ydb.yoj.repository.db.TxOptions;
25+
import tech.ydb.yoj.repository.db.common.CommonConverters;
26+
import tech.ydb.yoj.repository.db.common.CommonConverters.EnumDeserializer;
2527
import tech.ydb.yoj.repository.db.exception.ConversionException;
2628
import tech.ydb.yoj.repository.db.exception.DropTableException;
2729
import tech.ydb.yoj.repository.db.exception.EntityAlreadyExistsException;
@@ -32,6 +34,7 @@
3234
import tech.ydb.yoj.repository.db.list.ListRequest;
3335
import tech.ydb.yoj.repository.db.list.ListResult;
3436
import tech.ydb.yoj.repository.db.readtable.ReadTableParams;
37+
import tech.ydb.yoj.repository.db.statement.Changeset;
3538
import tech.ydb.yoj.repository.test.entity.TestEntities;
3639
import tech.ydb.yoj.repository.test.sample.TestDb;
3740
import tech.ydb.yoj.repository.test.sample.TestDbImpl;
@@ -44,6 +47,7 @@
4447
import tech.ydb.yoj.repository.test.sample.model.DetachedEntity;
4548
import tech.ydb.yoj.repository.test.sample.model.DetachedEntityId;
4649
import tech.ydb.yoj.repository.test.sample.model.EntityWithValidation;
50+
import tech.ydb.yoj.repository.test.sample.model.EnumEntity;
4751
import tech.ydb.yoj.repository.test.sample.model.IndexedEntity;
4852
import tech.ydb.yoj.repository.test.sample.model.MultiLevelDirectory;
4953
import tech.ydb.yoj.repository.test.sample.model.MultiWrappedEntity2;
@@ -595,7 +599,8 @@ public void testSpliteratorDoubleTryAdvance() {
595599

596600
// this loop calls tryAdvance() on spliterator one time after tryAdvance() says false
597601
while (true) {
598-
if (!spliterator.tryAdvance(__ -> {})) {
602+
if (!spliterator.tryAdvance(__ -> {
603+
})) {
599604
return;
600605
}
601606
}
@@ -2953,6 +2958,87 @@ public void multiWrapperEntity2WithCustomConverterFullyWrapped() {
29532958
)).isEmpty();
29542959
}
29552960

2961+
@Test
2962+
public void lenientEnumBehaviorExplicit() {
2963+
try {
2964+
CommonConverters.defineEnumDeserializer(EnumDeserializer.LENIENT);
2965+
lenientEnumBehaviorTestImpl();
2966+
} finally {
2967+
CommonConverters.useDefaultEnumDeserializer();
2968+
}
2969+
}
2970+
2971+
@Test
2972+
public void lenientEnumBehaviorImplicit() {
2973+
// NB: Currently the implicit system property uses LENIENT deserializer, but this will change in YOJ 2.7.0!
2974+
CommonConverters.useDefaultEnumDeserializer();
2975+
lenientEnumBehaviorTestImpl();
2976+
}
2977+
2978+
@Test
2979+
public void strictEnumBehaviorExplicit() {
2980+
try {
2981+
CommonConverters.defineEnumDeserializer(EnumDeserializer.STRICT);
2982+
strictEnumBehaviorTestImpl();
2983+
} finally {
2984+
CommonConverters.useDefaultEnumDeserializer();
2985+
}
2986+
}
2987+
2988+
private void lenientEnumBehaviorTestImpl() {
2989+
var entity1 = new EnumEntity(new EnumEntity.Id("qee1"), EnumEntity.IpVersion.IPV6, EnumEntity.NetworkType.OVERLAY);
2990+
var entity2 = new EnumEntity(new EnumEntity.Id("qee2"), EnumEntity.IpVersion.IPV4, EnumEntity.NetworkType.UNDERLAY_V4);
2991+
var entity3 = new EnumEntity(new EnumEntity.Id("qee3"), EnumEntity.IpVersion.IPV6, EnumEntity.NetworkType.UNDERLAY_V6);
2992+
db.tx(() -> db.table(EnumEntity.class).insert(entity1, entity2, entity3));
2993+
db.tx(() -> db.table(EnumEntity.class).update(entity2.id(), new Changeset()
2994+
.set("ipVersion", "ipv4") // the lowercase of IPV4.name(), which IS allowed by the LENIENT enum deserializer
2995+
.set("networkType", "underlay-v4") // this uses toString() to convert enums, and "underlay-v4" corresponds to a UNDERLAY_V4 constant
2996+
));
2997+
db.tx(() -> db.table(EnumEntity.class).update(entity3.id(), new Changeset()
2998+
.set("ipVersion", "IPV5") // mwahahahahah! This is an unknown constant which the LENIENT enum deserializer will turn into null (!!)
2999+
.set("networkType", "zz") // same here!!!
3000+
));
3001+
3002+
db.tx(() -> {
3003+
assertThat(db.table(EnumEntity.class).find(entity1.id())).isEqualTo(entity1);
3004+
3005+
// lowercase 'ipv4' treated as 'IPV4' constant
3006+
assertThat(db.table(EnumEntity.class).find(entity2.id())).isEqualTo(entity2);
3007+
// unknown constant 'IPV5' treated as null (!!)
3008+
assertThat(db.table(EnumEntity.class).find(entity3.id())).isEqualTo(entity3.withIpVersion(null).withNetworkType(null));
3009+
});
3010+
}
3011+
3012+
private void strictEnumBehaviorTestImpl() {
3013+
var entity1 = new EnumEntity(new EnumEntity.Id("qee1"), EnumEntity.IpVersion.IPV6, EnumEntity.NetworkType.OVERLAY);
3014+
var entity2 = new EnumEntity(new EnumEntity.Id("qee2"), EnumEntity.IpVersion.IPV4, EnumEntity.NetworkType.UNDERLAY_V4);
3015+
var entity3 = new EnumEntity(new EnumEntity.Id("qee3"), EnumEntity.IpVersion.IPV4, EnumEntity.NetworkType.UNDERLAY_V4);
3016+
var entity4 = new EnumEntity(new EnumEntity.Id("qee4"), EnumEntity.IpVersion.IPV6, EnumEntity.NetworkType.UNDERLAY_V6);
3017+
db.tx(() -> db.table(EnumEntity.class).insert(entity1, entity2, entity3, entity4));
3018+
db.tx(() -> db.table(EnumEntity.class).update(entity2.id(), new Changeset()
3019+
.set("ipVersion", "IPV4") // IPV4.name(), verbatim
3020+
.set("networkType", "underlay-v4") // this uses toString() to convert enums, and "underlay-v4" corresponds to a UNDERLAY_V4 constant
3021+
));
3022+
db.tx(() -> db.table(EnumEntity.class).update(entity3.id(), new Changeset()
3023+
.set("ipVersion", "ipv4") // the lowercase of IPV4.name(), which IS NOT allowed by the STRICT enum deserializer
3024+
.set("networkType", "underlay-v4") // this uses toString() to convert enums, and "underlay-v4" corresponds to a UNDERLAY_V4 constant
3025+
));
3026+
db.tx(() -> db.table(EnumEntity.class).update(entity4.id(), new Changeset()
3027+
.set("ipVersion", "IPV5") // mwahahahahah! This is an unknown constant which the STRICT enum deserializer disallows (!!)
3028+
.set("networkType", "zz") // same here!!!
3029+
));
3030+
3031+
db.tx(() -> {
3032+
assertThat(db.table(EnumEntity.class).find(entity1.id())).isEqualTo(entity1);
3033+
assertThat(db.table(EnumEntity.class).find(entity2.id())).isEqualTo(entity2);
3034+
3035+
// lowercase 'ipv4' leads to ConversionError
3036+
assertThatExceptionOfType(ConversionException.class).isThrownBy(() -> db.table(EnumEntity.class).find(entity3.id()));
3037+
// unknown constants 'IPV5' and 'zz' lead to ConversionError
3038+
assertThatExceptionOfType(ConversionException.class).isThrownBy(() -> db.table(EnumEntity.class).find(entity4.id()));
3039+
});
3040+
}
3041+
29563042
protected void runInTx(Consumer<RepositoryTransaction> action) {
29573043
// We do not retry transactions, because we do not expect conflicts in our test scenarios.
29583044
RepositoryTransaction transaction = startTransaction();

0 commit comments

Comments
 (0)