Skip to content
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

Add ability to obtain null value with SingleValueConverter #369

Open
ArtDu opened this issue Mar 20, 2023 · 2 comments
Open

Add ability to obtain null value with SingleValueConverter #369

ArtDu opened this issue Mar 20, 2023 · 2 comments

Comments

@ArtDu
Copy link
Contributor

ArtDu commented Mar 20, 2023

Now you can't obtain null if you use this converter

if (result == null) {
throw new TarantoolFunctionCallException("Function call result is null");
}

What if I want to collect the first value and null it's acceptable

@ArtDu
Copy link
Contributor Author

ArtDu commented Mar 20, 2023

Proposal how it could be

     public TarantoolResultMapper<TarantoolTuple> withArrayValueToTarantoolTupleResultConverter(
         ArrayValueToTarantoolTupleConverter tupleConverter) {
-        return withConverterWithoutTargetClass(
-            messagePackMapper.copy(),
-            ValueType.ARRAY,
-            new ArrayValueToTarantoolTupleResultConverter(tupleConverter)
-        );
+        return withConvertersWithoutTargetClass(messagePackMapper.copy(), Arrays.asList(
+            new ValueConverterWithInputTypeWrapper<>(
+                ValueType.ARRAY,
+                new ArrayValueToTarantoolTupleResultConverter(tupleConverter)
+            ),
+            new ValueConverterWithInputTypeWrapper<>(
+                ValueType.NIL,
+                new DefaultNilValueToNullConverter()
+            )
+        ));
     }
 

possible-solution.patch

diff --git a/src/main/java/io/tarantool/driver/core/SingleValueCallResultImpl.java b/src/main/java/io/tarantool/driver/core/SingleValueCallResultImpl.java
index d44ea092..74b19f45 100644
--- a/src/main/java/io/tarantool/driver/core/SingleValueCallResultImpl.java
+++ b/src/main/java/io/tarantool/driver/core/SingleValueCallResultImpl.java
@@ -39,7 +39,7 @@ public class SingleValueCallResultImpl<T> implements SingleValueCallResult<T> {
         Value errorsValue = result.getOrNilValue(1);
         if (callResultSize == 0 || callResultSize == 1 && resultValue.isNilValue()) {
             // [nil] or []
-            return null;
+            return valueGetter.apply(resultValue);
         } else if (callResultSize == 2 && (resultValue.isNilValue() && !errorsValue.isNilValue())) {
             // [nil, "Error msg..."] or [nil, {str="Error msg...", stack="..."}]
             throw TarantoolErrorsParser.parse(errorsValue);
diff --git a/src/main/java/io/tarantool/driver/core/metadata/DDLTarantoolSpaceMetadataConverter.java b/src/main/java/io/tarantool/driver/core/metadata/DDLTarantoolSpaceMetadataConverter.java
index 7a504f0f..6bf1c11a 100644
--- a/src/main/java/io/tarantool/driver/core/metadata/DDLTarantoolSpaceMetadataConverter.java
+++ b/src/main/java/io/tarantool/driver/core/metadata/DDLTarantoolSpaceMetadataConverter.java
@@ -57,6 +57,11 @@ public final class DDLTarantoolSpaceMetadataConverter implements ValueConverter<
 
     @Override
     public TarantoolMetadataContainer fromValue(Value value) {
+        ProxyTarantoolMetadataContainer proxyMetadata = new ProxyTarantoolMetadataContainer();
+
+        if (value.isNilValue()) {
+            return proxyMetadata;
+        }
 
         if (!value.isMapValue()) {
             throw new TarantoolClientException("Unsupported space metadata format: expected map");
@@ -71,8 +76,6 @@ public final class DDLTarantoolSpaceMetadataConverter implements ValueConverter<
         }
 
         spacesMap = spacesMap.get(SPACES_KEY).asMapValue().map();
-
-        ProxyTarantoolMetadataContainer proxyMetadata = new ProxyTarantoolMetadataContainer();
         for (Value nameValue : spacesMap.keySet()) {
             if (!nameValue.isStringValue()) {
                 throw new TarantoolClientException(
diff --git a/src/main/java/io/tarantool/driver/mappers/factories/AbstractResultMapperFactory.java b/src/main/java/io/tarantool/driver/mappers/factories/AbstractResultMapperFactory.java
index dad58775..4e0ee5aa 100644
--- a/src/main/java/io/tarantool/driver/mappers/factories/AbstractResultMapperFactory.java
+++ b/src/main/java/io/tarantool/driver/mappers/factories/AbstractResultMapperFactory.java
@@ -56,6 +56,10 @@ public abstract class AbstractResultMapperFactory<O, T extends AbstractResultMap
         MessagePackValueMapper valueMapper,
         List<ValueConverterWithInputTypeWrapper<O>> converters);
 
+    protected abstract T createMapperWithoutTargetClass(
+        MessagePackValueMapper valueMapper,
+        List<ValueConverterWithInputTypeWrapper<Object>> converters);
+
     /**
      * Create {@link AbstractResultMapper} instance with the passed converter.
      *
@@ -131,10 +135,10 @@ public abstract class AbstractResultMapperFactory<O, T extends AbstractResultMap
         return createMapper(valueMapper, converters, resultClass);
     }
 
-    public T withConverterWithoutTargetClass(
+    public T withConvertersWithoutTargetClass(
         MessagePackValueMapper valueMapper,
-        List<ValueConverterWithInputTypeWrapper<O>> converters) {
-        return createMapper(valueMapper, converters);
+        List<ValueConverterWithInputTypeWrapper<Object>> converters) {
+        return createMapperWithoutTargetClass(valueMapper, converters);
     }
 
 }
diff --git a/src/main/java/io/tarantool/driver/mappers/factories/ArrayValueToTarantoolResultMapperFactory.java b/src/main/java/io/tarantool/driver/mappers/factories/ArrayValueToTarantoolResultMapperFactory.java
index dd1338fc..58e2ba21 100644
--- a/src/main/java/io/tarantool/driver/mappers/factories/ArrayValueToTarantoolResultMapperFactory.java
+++ b/src/main/java/io/tarantool/driver/mappers/factories/ArrayValueToTarantoolResultMapperFactory.java
@@ -48,7 +48,7 @@ public class ArrayValueToTarantoolResultMapperFactory<T> extends TarantoolResult
             messagePackMapper.copy(),
             ValueType.ARRAY,
             new ArrayValueToTarantoolResultConverter<>(valueConverter)
-        );
+                                              );
     }
 
     /**
diff --git a/src/main/java/io/tarantool/driver/mappers/factories/ArrayValueToTarantoolTupleResultMapperFactory.java b/src/main/java/io/tarantool/driver/mappers/factories/ArrayValueToTarantoolTupleResultMapperFactory.java
index 4202f683..73f53bd7 100644
--- a/src/main/java/io/tarantool/driver/mappers/factories/ArrayValueToTarantoolTupleResultMapperFactory.java
+++ b/src/main/java/io/tarantool/driver/mappers/factories/ArrayValueToTarantoolTupleResultMapperFactory.java
@@ -1,11 +1,14 @@
 package io.tarantool.driver.mappers.factories;
 
+import java.util.Arrays;
+
 import io.tarantool.driver.api.metadata.TarantoolSpaceMetadata;
 import io.tarantool.driver.api.tuple.TarantoolTuple;
 import io.tarantool.driver.mappers.MessagePackMapper;
 import io.tarantool.driver.mappers.MessagePackValueMapper;
 import io.tarantool.driver.mappers.TarantoolResultMapper;
 import io.tarantool.driver.mappers.converters.ValueConverterWithInputTypeWrapper;
+import io.tarantool.driver.mappers.converters.object.DefaultNilValueToNullConverter;
 import io.tarantool.driver.mappers.converters.value.ArrayValueToTarantoolTupleConverter;
 import io.tarantool.driver.mappers.converters.value.ArrayValueToTarantoolTupleResultConverter;
 import org.msgpack.value.ValueType;
@@ -58,11 +61,16 @@ public class ArrayValueToTarantoolTupleResultMapperFactory
 
     public TarantoolResultMapper<TarantoolTuple> withArrayValueToTarantoolTupleResultConverter(
         ArrayValueToTarantoolTupleConverter tupleConverter) {
-        return withConverterWithoutTargetClass(
-            messagePackMapper.copy(),
-            ValueType.ARRAY,
-            new ArrayValueToTarantoolTupleResultConverter(tupleConverter)
-        );
+        return withConvertersWithoutTargetClass(messagePackMapper.copy(), Arrays.asList(
+            new ValueConverterWithInputTypeWrapper<>(
+                ValueType.ARRAY,
+                new ArrayValueToTarantoolTupleResultConverter(tupleConverter)
+            ),
+            new ValueConverterWithInputTypeWrapper<>(
+                ValueType.NIL,
+                new DefaultNilValueToNullConverter()
+            )
+        ));
     }
 
     public ValueConverterWithInputTypeWrapper<Object> getArrayValueToTarantoolTupleResultConverter(
diff --git a/src/main/java/io/tarantool/driver/mappers/factories/RowsMetadataToTarantoolTupleResultMapperFactory.java b/src/main/java/io/tarantool/driver/mappers/factories/RowsMetadataToTarantoolTupleResultMapperFactory.java
index 18f2f481..b4f1df16 100644
--- a/src/main/java/io/tarantool/driver/mappers/factories/RowsMetadataToTarantoolTupleResultMapperFactory.java
+++ b/src/main/java/io/tarantool/driver/mappers/factories/RowsMetadataToTarantoolTupleResultMapperFactory.java
@@ -62,7 +62,7 @@ public class RowsMetadataToTarantoolTupleResultMapperFactory
             messagePackMapper.copy(),
             ValueType.MAP,
             new RowsMetadataToTarantoolTupleResultConverter(tupleConverter)
-        );
+                                              );
     }
 
     public ValueConverterWithInputTypeWrapper<Object> getRowsMetadataToTarantoolTupleResultConverter(
@@ -80,6 +80,6 @@ public class RowsMetadataToTarantoolTupleResultMapperFactory
             valueMapper,
             ValueType.MAP,
             new RowsMetadataToTarantoolTupleResultConverter(tupleConverter)
-        );
+                                              );
     }
 }
diff --git a/src/main/java/io/tarantool/driver/mappers/factories/TarantoolCallResultMapperFactory.java b/src/main/java/io/tarantool/driver/mappers/factories/TarantoolCallResultMapperFactory.java
index b46bbd88..909e897b 100644
--- a/src/main/java/io/tarantool/driver/mappers/factories/TarantoolCallResultMapperFactory.java
+++ b/src/main/java/io/tarantool/driver/mappers/factories/TarantoolCallResultMapperFactory.java
@@ -54,4 +54,10 @@ public class TarantoolCallResultMapperFactory<T, R extends CallResult<T>> extend
         MessagePackValueMapper valueMapper, List<ValueConverterWithInputTypeWrapper<R>> converters) {
         return new CallResultMapper<>(valueMapper, converters);
     }
+
+    @Override
+    protected CallResultMapper<T, R> createMapperWithoutTargetClass(
+        MessagePackValueMapper valueMapper, List<ValueConverterWithInputTypeWrapper<Object>> converters) {
+        return new CallResultMapper(valueMapper, converters);
+    }
 }
diff --git a/src/main/java/io/tarantool/driver/mappers/factories/TarantoolResultMapperFactory.java b/src/main/java/io/tarantool/driver/mappers/factories/TarantoolResultMapperFactory.java
index f517affe..cd564f62 100644
--- a/src/main/java/io/tarantool/driver/mappers/factories/TarantoolResultMapperFactory.java
+++ b/src/main/java/io/tarantool/driver/mappers/factories/TarantoolResultMapperFactory.java
@@ -57,4 +57,11 @@ public class TarantoolResultMapperFactory<T> extends
         List<ValueConverterWithInputTypeWrapper<TarantoolResult<T>>> converters) {
         return new TarantoolResultMapper<>(valueMapper, converters);
     }
+
+    @Override
+    protected TarantoolResultMapper<T> createMapperWithoutTargetClass(
+        MessagePackValueMapper valueMapper,
+        List<ValueConverterWithInputTypeWrapper<Object>> converters) {
+        return new TarantoolResultMapper(valueMapper, converters);
+    }
 }
diff --git a/src/test/java/io/tarantool/driver/integration/TarantoolErrorsIT.java b/src/test/java/io/tarantool/driver/integration/TarantoolErrorsIT.java
index 63e61ecd..f144f5b2 100644
--- a/src/test/java/io/tarantool/driver/integration/TarantoolErrorsIT.java
+++ b/src/test/java/io/tarantool/driver/integration/TarantoolErrorsIT.java
@@ -10,6 +10,8 @@ import io.tarantool.driver.exceptions.TarantoolClientException;
 import io.tarantool.driver.exceptions.TarantoolInternalException;
 import io.tarantool.driver.exceptions.TarantoolInternalNetworkException;
 import io.tarantool.driver.exceptions.TarantoolNoSuchProcedureException;
+import io.tarantool.driver.exceptions.TarantoolSpaceNotFoundException;
+
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
 
@@ -213,4 +215,18 @@ public class TarantoolErrorsIT extends SharedCartridgeContainer {
         assertTrue(cause.getMessage()
             .contains("Execute access to function 'ddl.get_schema' is denied for user 'restricted_user'"));
     }
+
+    @Test
+    void test_failedToRefreshMetadata_shouldHasReasonOfException() {
+        ProxyTarantoolTupleClient adminClient = setupAdminClient();
+
+        adminClient.eval("true_ddl = ddl; " +
+                             "ddl = {get_schema = function() return nil end}").join();
+
+        TarantoolSpaceNotFoundException exception = assertThrows(TarantoolSpaceNotFoundException.class,
+            () -> adminClient.space("test_space").select(Conditions.any()));
+        assertEquals("Space with name 'test_space' not found", exception.getMessage());
+
+        adminClient.eval("ddl = true_ddl").join();
+    }
 }

@akudiyar
Copy link
Collaborator

Although I don't like the idea of passing a list of converters simultaneously (that gives some ambiguity about the order of these converters in the stack), I agree with this bugfix in general.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants