FIX: YDB PredicateStatement incorrectly detected a string-value typ…

…e single ID field as the whole ID itself
ydb-platform · Apr 17, 2024 · 34c4018 · 34c4018
1 parent 02f551e
commit 34c4018
Show file tree

Hide file tree

Showing 11 changed files with 118 additions and 15 deletions.
diff --git a/...-inmemory/src/test/java/tech/ydb/yoj/repository/test/inmemory/TestInMemoryRepository.java b/...-inmemory/src/test/java/tech/ydb/yoj/repository/test/inmemory/TestInMemoryRepository.java
@@ -17,6 +17,7 @@
 import tech.ydb.yoj.repository.test.sample.model.EntityWithValidation;
 import tech.ydb.yoj.repository.test.sample.model.IndexedEntity;
 import tech.ydb.yoj.repository.test.sample.model.LogEntry;
+import tech.ydb.yoj.repository.test.sample.model.MultiWrappedEntity;
 import tech.ydb.yoj.repository.test.sample.model.NetworkAppliance;
 import tech.ydb.yoj.repository.test.sample.model.Primitive;
 import tech.ydb.yoj.repository.test.sample.model.Project;
@@ -130,6 +131,11 @@ public Table<VersionedAliasedEntity> versionedAliasedEntities() {
         public Table<DetachedEntity> detachedEntities() {
             return table(DetachedEntity.class);
         }
+
+        @Override
+        public Table<MultiWrappedEntity> multiWrappedIdEntities() {
+            return table(MultiWrappedEntity.class);
+        }
     }
 
     private static class Supabubble2InMemoryTable extends InMemoryTable<Supabubble2> implements TestEntityOperations.Supabubble2Table {

diff --git a/repository-test/src/main/java/tech/ydb/yoj/repository/test/entity/TestEntities.java b/repository-test/src/main/java/tech/ydb/yoj/repository/test/entity/TestEntities.java
@@ -12,6 +12,7 @@
 import tech.ydb.yoj.repository.test.sample.model.EntityWithValidation;
 import tech.ydb.yoj.repository.test.sample.model.IndexedEntity;
 import tech.ydb.yoj.repository.test.sample.model.LogEntry;
+import tech.ydb.yoj.repository.test.sample.model.MultiWrappedEntity;
 import tech.ydb.yoj.repository.test.sample.model.NetworkAppliance;
 import tech.ydb.yoj.repository.test.sample.model.NonDeserializableEntity;
 import tech.ydb.yoj.repository.test.sample.model.Primitive;
@@ -50,7 +51,8 @@ private TestEntities() {
             NetworkAppliance.class,
             VersionedEntity.class,
             VersionedAliasedEntity.class,
-            DetachedEntity.class
+            DetachedEntity.class,
+            MultiWrappedEntity.class
     );
 
     @SuppressWarnings("unchecked")

diff --git a/repository-test/src/main/java/tech/ydb/yoj/repository/test/sample/TestEntityOperations.java b/repository-test/src/main/java/tech/ydb/yoj/repository/test/sample/TestEntityOperations.java
@@ -13,6 +13,7 @@
 import tech.ydb.yoj.repository.test.sample.model.EntityWithValidation;
 import tech.ydb.yoj.repository.test.sample.model.IndexedEntity;
 import tech.ydb.yoj.repository.test.sample.model.LogEntry;
+import tech.ydb.yoj.repository.test.sample.model.MultiWrappedEntity;
 import tech.ydb.yoj.repository.test.sample.model.NetworkAppliance;
 import tech.ydb.yoj.repository.test.sample.model.Primitive;
 import tech.ydb.yoj.repository.test.sample.model.Project;
@@ -72,6 +73,8 @@ default Table<BytePkEntity> bytePkEntities() {
 
     Table<DetachedEntity> detachedEntities();
 
+    Table<MultiWrappedEntity> multiWrappedIdEntities();
+
     class ProjectTable extends AbstractDelegatingTable<Project> {
         public ProjectTable(Table<Project> target) {
             super(target);

diff --git a/...tory-test/src/main/java/tech/ydb/yoj/repository/test/sample/model/MultiWrappedEntity.java b/...tory-test/src/main/java/tech/ydb/yoj/repository/test/sample/model/MultiWrappedEntity.java
@@ -0,0 +1,31 @@
+package tech.ydb.yoj.repository.test.sample.model;
+
+import lombok.NonNull;
+import tech.ydb.yoj.databind.converter.StringColumn;
+import tech.ydb.yoj.repository.db.Entity;
+import tech.ydb.yoj.repository.db.RecordEntity;
+
+import javax.annotation.Nullable;
+
+public record MultiWrappedEntity(
+        @NonNull Id id,
+        @NonNull String payload,
+        @Nullable OptionalPayload optionalPayload
+) implements RecordEntity<MultiWrappedEntity> {
+    public record Id(
+            @StringColumn StringWrapper itIsReallyString
+    ) implements Entity.Id<MultiWrappedEntity> {
+    }
+
+    public record StringWrapper(@NonNull String value) {
+        @Override
+        public String toString() {
+            return value;
+        }
+    }
+
+    public record OptionalPayload(
+            @StringColumn @NonNull StringWrapper wrapper
+    ) {
+    }
+}
diff --git a/...sitory-ydb-v1/src/main/java/tech/ydb/yoj/repository/ydb/statement/PredicateStatement.java b/...sitory-ydb-v1/src/main/java/tech/ydb/yoj/repository/ydb/statement/PredicateStatement.java
@@ -6,7 +6,6 @@
 import com.yandex.ydb.ValueProtos;
 import lombok.Getter;
 import lombok.NonNull;
-import tech.ydb.yoj.databind.FieldValueType;
 import tech.ydb.yoj.databind.schema.Schema;
 import tech.ydb.yoj.databind.schema.Schema.JavaField;
 import tech.ydb.yoj.repository.db.Entity;
@@ -186,10 +185,15 @@ Object getParamValue(JavaField rootField, String fieldName, boolean compositeYql
         }
 
         private Object getValueForField(JavaField rootField, String fieldName, boolean compositeYqlType, Object paramValue) {
-            if (!compositeYqlType && FieldValueType.forJavaType(paramValue.getClass(), rootField.getField()).isComposite()) {
-                Map<String, Object> m = new LinkedHashMap<>();
-                rootField.collectValueTo(paramValue, m);
-                return m.get(fieldName);
+            if (!compositeYqlType) {
+                if (rootField.getValueType().isComposite() && paramValue.getClass().equals(rootField.getRawType())) {
+                    // We got ourselves a wrapper, my friends
+                    Map<String, Object> m = new LinkedHashMap<>();
+                    rootField.collectValueTo(paramValue, m);
+                    return m.get(fieldName);
+                } else {
+                    return paramValue;
+                }
             } else {
                 return fieldName.equals(rootField.getName()) ? paramValue : null;
             }

diff --git a/repository-ydb-v1/src/main/java/tech/ydb/yoj/repository/ydb/yql/YqlPredicate.java b/repository-ydb-v1/src/main/java/tech/ydb/yoj/repository/ydb/yql/YqlPredicate.java
@@ -421,7 +421,7 @@ public YqlPredicate negate() {
         public <T extends Entity<T>> String toYql(@NonNull EntitySchema<T> schema) {
             EntitySchema.JavaField field = schema.getField(fieldPath);
             if (field.isFlat()) {
-                return format("`%s` %s ?", field.getName(), rel.yql);
+                return format("`%s` %s ?", field.toFlatField().getName(), rel.yql);
             } else {
                 return format("(%s) %s ?", field.flatten()
                                 .map(f -> "`" + f.getName() + "`").collect(joining(", ")),
@@ -661,7 +661,7 @@ public <T extends Entity<T>> String toYql(@NonNull EntitySchema<T> schema) {
 
             EntitySchema.JavaField field = schema.getField(fieldPath);
             if (field.isFlat()) {
-                return format("`%s` %s ?", field.getName(), inType.yql);
+                return format("`%s` %s ?", field.toFlatField().getName(), inType.yql);
             } else {
                 return format("(%s) %s ?", field.flatten()
                                 .map(f -> "`" + f.getName() + "`").collect(joining(", ")),

diff --git a/repository-ydb-v1/src/test/java/tech/ydb/yoj/repository/ydb/TestYdbRepository.java b/repository-ydb-v1/src/test/java/tech/ydb/yoj/repository/ydb/TestYdbRepository.java
@@ -21,6 +21,7 @@
 import tech.ydb.yoj.repository.test.sample.model.EntityWithValidation;
 import tech.ydb.yoj.repository.test.sample.model.IndexedEntity;
 import tech.ydb.yoj.repository.test.sample.model.LogEntry;
+import tech.ydb.yoj.repository.test.sample.model.MultiWrappedEntity;
 import tech.ydb.yoj.repository.test.sample.model.NetworkAppliance;
 import tech.ydb.yoj.repository.test.sample.model.Primitive;
 import tech.ydb.yoj.repository.test.sample.model.Project;
@@ -149,6 +150,11 @@ public Table<VersionedAliasedEntity> versionedAliasedEntities() {
         public Table<DetachedEntity> detachedEntities() {
             return table(DetachedEntity.class);
         }
+
+        @Override
+        public Table<MultiWrappedEntity> multiWrappedIdEntities() {
+            return table(MultiWrappedEntity.class);
+        }
     }
 
     private static class YdbSupabubble2Table extends YdbTable<Supabubble2> implements Supabubble2Table {

diff --git a/...sitory-ydb-v2/src/main/java/tech/ydb/yoj/repository/ydb/statement/PredicateStatement.java b/...sitory-ydb-v2/src/main/java/tech/ydb/yoj/repository/ydb/statement/PredicateStatement.java
@@ -6,7 +6,6 @@
 import lombok.Getter;
 import lombok.NonNull;
 import tech.ydb.proto.ValueProtos;
-import tech.ydb.yoj.databind.FieldValueType;
 import tech.ydb.yoj.databind.schema.Schema;
 import tech.ydb.yoj.databind.schema.Schema.JavaField;
 import tech.ydb.yoj.repository.db.Entity;
@@ -186,10 +185,15 @@ Object getParamValue(JavaField rootField, String fieldName, boolean compositeYql
         }
 
         private Object getValueForField(JavaField rootField, String fieldName, boolean compositeYqlType, Object paramValue) {
-            if (!compositeYqlType && FieldValueType.forJavaType(paramValue.getClass(), rootField.getField()).isComposite()) {
-                Map<String, Object> m = new LinkedHashMap<>();
-                rootField.collectValueTo(paramValue, m);
-                return m.get(fieldName);
+            if (!compositeYqlType) {
+                if (rootField.getValueType().isComposite() && paramValue.getClass().equals(rootField.getRawType())) {
+                    // We got ourselves a wrapper, my friends
+                    Map<String, Object> m = new LinkedHashMap<>();
+                    rootField.collectValueTo(paramValue, m);
+                    return m.get(fieldName);
+                } else {
+                    return paramValue;
+                }
             } else {
                 return fieldName.equals(rootField.getName()) ? paramValue : null;
             }

diff --git a/repository-ydb-v2/src/main/java/tech/ydb/yoj/repository/ydb/yql/YqlPredicate.java b/repository-ydb-v2/src/main/java/tech/ydb/yoj/repository/ydb/yql/YqlPredicate.java
@@ -421,7 +421,7 @@ public YqlPredicate negate() {
         public <T extends Entity<T>> String toYql(@NonNull EntitySchema<T> schema) {
             EntitySchema.JavaField field = schema.getField(fieldPath);
             if (field.isFlat()) {
-                return format("`%s` %s ?", field.getName(), rel.yql);
+                return format("`%s` %s ?", field.toFlatField().getName(), rel.yql);
             } else {
                 return format("(%s) %s ?", field.flatten()
                                 .map(f -> "`" + f.getName() + "`").collect(joining(", ")),
@@ -661,7 +661,7 @@ public <T extends Entity<T>> String toYql(@NonNull EntitySchema<T> schema) {
 
             EntitySchema.JavaField field = schema.getField(fieldPath);
             if (field.isFlat()) {
-                return format("`%s` %s ?", field.getName(), inType.yql);
+                return format("`%s` %s ?", field.toFlatField().getName(), inType.yql);
             } else {
                 return format("(%s) %s ?", field.flatten()
                                 .map(f -> "`" + f.getName() + "`").collect(joining(", ")),

diff --git a/repository-ydb-v2/src/test/java/tech/ydb/yoj/repository/ydb/TestYdbRepository.java b/repository-ydb-v2/src/test/java/tech/ydb/yoj/repository/ydb/TestYdbRepository.java
@@ -20,6 +20,7 @@
 import tech.ydb.yoj.repository.test.sample.model.EntityWithValidation;
 import tech.ydb.yoj.repository.test.sample.model.IndexedEntity;
 import tech.ydb.yoj.repository.test.sample.model.LogEntry;
+import tech.ydb.yoj.repository.test.sample.model.MultiWrappedEntity;
 import tech.ydb.yoj.repository.test.sample.model.NetworkAppliance;
 import tech.ydb.yoj.repository.test.sample.model.Primitive;
 import tech.ydb.yoj.repository.test.sample.model.Project;
@@ -149,6 +150,11 @@ public Table<VersionedAliasedEntity> versionedAliasedEntities() {
         public Table<DetachedEntity> detachedEntities() {
             return table(DetachedEntity.class);
         }
+
+        @Override
+        public Table<MultiWrappedEntity> multiWrappedIdEntities() {
+            return table(MultiWrappedEntity.class);
+        }
     }
 
     private static class YdbSupabubble2Table extends YdbTable<Supabubble2> implements TestEntityOperations.Supabubble2Table {

diff --git a/...sitory-ydb-v2/src/test/java/tech/ydb/yoj/repository/ydb/YdbRepositoryIntegrationTest.java b/...sitory-ydb-v2/src/test/java/tech/ydb/yoj/repository/ydb/YdbRepositoryIntegrationTest.java
@@ -49,6 +49,7 @@
 import tech.ydb.yoj.repository.test.sample.model.Bubble;
 import tech.ydb.yoj.repository.test.sample.model.ChangefeedEntity;
 import tech.ydb.yoj.repository.test.sample.model.IndexedEntity;
+import tech.ydb.yoj.repository.test.sample.model.MultiWrappedEntity;
 import tech.ydb.yoj.repository.test.sample.model.Project;
 import tech.ydb.yoj.repository.test.sample.model.Supabubble2;
 import tech.ydb.yoj.repository.test.sample.model.TtlEntity;
@@ -486,6 +487,46 @@ public void predicateWithBoxedValues() {
         });
     }
 
+    @Test
+    public void predicateWithMultipleBoxedId() {
+        var m = new MultiWrappedEntity(new MultiWrappedEntity.Id(new MultiWrappedEntity.StringWrapper("string-id")), "payload", null);
+        db.tx(() -> {
+            db.multiWrappedIdEntities().save(m);
+        });
+        db.tx(() -> {
+            assertThat(db.multiWrappedIdEntities().query().where("id").eq(m.id()).findOne()).isEqualTo(m);
+            assertThat(db.multiWrappedIdEntities().query().where("id").eq(m.id().itIsReallyString()).findOne()).isEqualTo(m);
+            assertThat(db.multiWrappedIdEntities().query().where("id").eq(m.id().itIsReallyString().value()).findOne()).isEqualTo(m);
+
+            var table = (YdbTable<MultiWrappedEntity>) db.table(MultiWrappedEntity.class);
+            assertThat(table.find(YqlPredicate.where("id").eq(m.id()))).singleElement().isEqualTo(m);
+            assertThat(table.find(YqlPredicate.where("id").eq(m.id().itIsReallyString()))).singleElement().isEqualTo(m);
+            assertThat(table.find(YqlPredicate.where("id").eq(m.id().itIsReallyString().value()))).singleElement().isEqualTo(m);
+        });
+    }
+
+    @Test
+    public void predicateWithMultipleBoxedPayload() {
+        var m = new MultiWrappedEntity(
+                new MultiWrappedEntity.Id(new MultiWrappedEntity.StringWrapper("string-id")),
+                "fakefakefake",
+                new MultiWrappedEntity.OptionalPayload(new MultiWrappedEntity.StringWrapper("real-payload"))
+        );
+        db.tx(() -> {
+            db.multiWrappedIdEntities().save(m);
+        });
+        db.tx(() -> {
+            assertThat(db.multiWrappedIdEntities().query().where("optionalPayload").eq(m.optionalPayload()).findOne()).isEqualTo(m);
+            assertThat(db.multiWrappedIdEntities().query().where("optionalPayload").eq(m.optionalPayload().wrapper()).findOne()).isEqualTo(m);
+            assertThat(db.multiWrappedIdEntities().query().where("optionalPayload").eq(m.optionalPayload().wrapper().value()).findOne()).isEqualTo(m);
+
+            var table = (YdbTable<MultiWrappedEntity>) db.table(MultiWrappedEntity.class);
+            assertThat(table.find(YqlPredicate.where("optionalPayload").eq(m.optionalPayload()))).singleElement().isEqualTo(m);
+            assertThat(table.find(YqlPredicate.where("optionalPayload").eq(m.optionalPayload().wrapper()))).singleElement().isEqualTo(m);
+            assertThat(table.find(YqlPredicate.where("optionalPayload").eq(m.optionalPayload().wrapper().value()))).singleElement().isEqualTo(m);
+        });
+    }
+
     @Test
     public void testSelectDefault() {
         db.tx(() -> db.indexedTable().insert(e1, e2));