Use mappers for parsing TarantoolResult instead of single converter

[ArtDu]

Changes

Mappers Factories

before:
We didn't reuse tarantool mapper factory for single/multivalue structures. In fact, TarantoolResultMapper factory is used only for parsing structures where tuples are placed. We had TarantoolResultConverter for both structures for tarantool/crud response and for box responses and Mappers were designed for the converter use. But we want to use mappers with two converters(for crud aka Map structure, for box aka List structure) instead of using a universal "if else" converter.

https://drive.google.com/file/d/1PH7kzS8vrbMGPbdUOjJnP35tcLSMX-f3/view?usp=sharing

after:
We've rewritten Mappers, and now we can pass Mapper to another Mapper. It may be confusing. But we build a stack of converters - it's called Mapper, and we wanted to have the ability to create a stack of converters by not only passing converters but also passing mappers. And here Single/Multi*MapperFactory uses ResultMapper factory for parsing inner structures.

https://drive.google.com/file/d/1AKSzfUE0ebj8pRM6wnDUC7GvoCotDR6E/view?usp=sharing

Mappers were only for ArrayValue

We had mappers only for MP ArrayValue and we solved problems with MAP in inner class like we have TarantoolResultImpl in TarantoolResultConverter. Also we can pass list of converters now, it is needed especially for parse Map/List tuple structures.

before:

after:

ValueConverters without targetType

• We can't create Mapper with ValueConverter with a specific target object type, for example, we can't create converter for TarantoolResult target object type. It's happening because we use the result class to match ResultConverter for cases when we have a lot of converters for one type(for IntegerValue we have 5 converters). We can get around because for structure converters we don't need to choose. Here we have an ugly hack of casting to ValueConverter without generics

ValueConverter valueConverter =
            new DefaultArrayValueToTarantoolTuplesResultConverter(tupleConverter);
        return withConverter(
            messagePackMapper.copy(),
            ValueType.ARRAY,
            valueConverter,
            resultClass
        );

And the main reason to create methods that aren't using targetType is that we don't use targetType. We use it only for getting values when we pass valueClass besides.

Need to think

• Prefix TuplesResult instead of TupleResult - because we working only with list of tuples

I haven't forgotten about:

• Tests
• Changelog - TODO: add breaking changes lines
• Documentation
• Commit messages comply with the guideline
• Cleanup the code for review. See checklist
• Javadocs

Related issues:
Closes #301

[ArtDu]

@akudiyar I need you to see concepts. I haven't finished with javadocs and refactoring, but you can give your opinion about structure and hierarchy

[akudiyar]

@ArtDu Thanks for such detailed changes description, very much appreciated!
The PR is in a good shape, so let's remove some roughness and it's good to go.

General comments/questions:

• Please avoid mixing meaningful changes with re-indentation and renaming files next time. It makes things hard to review. Such changes need to be applied in atomic commits and better in separate PRs.
• I didn't like much the idea of renaming the container mappers and adding "s" to "tuple". See one of the comments below for more details.
• Would you mind elaborating a bit more about "converter without target type"? I understand the raw idea, but using an untyped generic interface tricks the compiler, so we need at least to use some workarounds or unspecified Object type generics like ValueConverter<?, ?> if we don't care about the result type.
• We need to put more documentation about the converters in the DefaultMapper and other classes. Especially we need to describe the workflow where each result mapper class is used in. This will ease understanding of this code if we will have to fix bugs here.

[akudiyar]

The logic behind this name was the following: SingleResult<T> + T := TarantoolTuple gives us "single result of Tarantool tuple type", and with English language rules the adjective nouns are placed in front of the object being described, so that we get TarantoolTuple single result.
What's the new naming logic, would you mind explaining that?

[ArtDu]

My logic was that we have TarantoolTupleResult stuff and we add on this stack SingleConverter. And we add some words, so we should have this SingleTarantoolTupleResult like we have a stack from the top to bottom, but from the lef to the right. I don't know why I added Tarantool to begin of name

In general, I propose names SingleValueTarantoolTupleResult or SingleValueWithTarantoolTupleResult

[akudiyar]

The latter variants look more meaningful but still are too long. The nasty part about Java long names is that they greatly reduce readability when being re-idented with newlines.

[akudiyar]

Do we really need the existing method once we have this new one? We need either to remove the former one or to refactor them in a way that eliminates the copy-paste.

[ArtDu]

We need it when we pass only one converter to parsing. We parse callForMultiResult with one converter on the top of stack

[akudiyar]

nit: We used previously the file creation date as a version UID. Although it doesn't matter much, it will be easier to compare the updated value if we by any chance change it after changing the class structure in the future.

[akudiyar]

Looks like this ValueConverter type spec is not flexible. I believe we need ? extends Value or ArrayValue here.

[ArtDu]

We can't use ? extends Value

And we shouldn't use ArrayValue because we can also parse not ArrayValue values

[akudiyar]

If we leave Value here, it will prevent us from passing a ValueConverter<ArrayValue, List<String>> for example. That can cause some trouble in the derived libraries because that's how the compiler works. The users will have to create a converter with a generic "Value" generic type, which means an "in-line" converter from a lambda function may not work sometimes or will require some dirty casts and excessive code.

Let's look together at the code if you change this to a supertype generic. Maybe I will see some hints there.

[ArtDu]

This converter is used only in one place in SingleValueCallResultImpl. Because we expect any value in there we should use Value like it was before https://github.com/tarantool/cartridge-java/blob/master/src/main/java/io/tarantool/driver/mappers/converters/value/custom/SingleValueCallResultConverter.java .

cartridge-java/src/main/java/io/tarantool/driver/core/SingleValueCallResultImpl.java

Line 32 in 339b5de

value = valueConverter.fromValue(result.get(0));

cartridge-java/src/main/java/io/tarantool/driver/core/SingleValueCallResultImpl.java

Lines 16 to 34 in 339b5de

	public class SingleValueCallResultImpl<T> implements SingleValueCallResult<T> {
	private final T value;
	public SingleValueCallResultImpl(ArrayValue result, ValueConverter<Value, T> valueConverter) {
	if (result == null) {
	throw new TarantoolFunctionCallException("Function call result is null");
	}
	if (result.size() == 0 || result.size() == 1 && result.get(0).isNilValue()) {
	// [nil] or []
	value = null;
	} else if (result.size() == 2 && (result.get(0).isNilValue() && !result.get(1).isNilValue())) {
	// [nil, "Error msg..."] or [nil, {str="Error msg...", stack="..."}]
	throw TarantoolErrorsParser.parse(result.get(1));
	} else {
	// [result]
	value = valueConverter.fromValue(result.get(0));
	}
	}

It'd work if we had generic method. But we have determined type and method require only this captured ? extends Value type

[akudiyar]

nit: the random UID values are not too readable when you change them 🙂

[ArtDu]

@akudiyar

General comments/questions:

• Please avoid mixing meaningful changes with re-indentation and renaming files next time. It makes things hard to review. Such changes need to be applied in atomic commits and better in separate PRs.

Yeah, you're right, we should have done it early. I created PR #304 with codestyle and refactoring

• I didn't like much the idea of renaming the container mappers and adding "s" to "tuple". See one of the comments below for more details.

It's my fault, I'm always forgetting the rule that nouns can be adjectives. I returned TupleResult

• Would you mind elaborating a bit more about "converter without target type"? I understand the raw idea, but using an untyped generic interface tricks the compiler, so we need at least to use some workarounds or unspecified Object type generics like ValueConverter<?, ?> if we don't care about the result type.

We still can use generics for main purpose to determine return type

public interface ValueConverter<V extends Value, O> extends Converter {
    /**
     * Convert MessagePack entity to a Java object
     *
     * @param value entity
     * @return object
     */
    O fromValue(V value);
    ...

Also driver used the generic target type from converter for second purpose. It was used to determine converter by target Type when we call tuple.fromValue(..., targetType) and it works only in TarantoolTuple. We have only one structure that stores converters - DefaultMessagePackMapper. It requires to store target Type to determine converter. But when we want to parse structures before tuples we don't need the determining targetType.

[ArtDu]

@akudiyar Maybe we should add new interface with simple use of mappers above the factories in another PR. Cause it's so hard to use them now

List<?> result = client.call("f",
            client.getResultMapperFactoryFactory().customStructureResultMapperFactory().withArrayValueToTarantoolResultConverter(
                (list) -> {
                    return list.get(0).asStringValue();
                }
            )).join();

For example:

public interface ResultMapperFactory {
    TarantoolResultMapper<TarantoolTuple> withTarantoolTupleMapper(
        MessagePackMapper messagePackMapper, TarantoolSpaceMetadata spaceMetadata);

    TarantoolResultMapper<TarantoolTuple> withUnflattenTupleMapper(
        MessagePackMapper messagePackMapper, TarantoolSpaceMetadata spaceMetadata);

    TarantoolResultMapper<TarantoolTuple> withFlattenTupleMapper(
        MessagePackMapper messagePackMapper, TarantoolSpaceMetadata spaceMetadata);

    CallResultMapper<TarantoolResult<TarantoolTuple>, SingleValueCallResult<TarantoolResult<TarantoolTuple>>>
    withSingleValueTarantoolTupleResult(
        MessagePackMapper messagePackMapper, TarantoolSpaceMetadata spaceMetadata);

    <T> CallResultMapper<T, SingleValueCallResult<T>>
    withSingleValueResultConverter(
        ValueConverter<Value, T> structureConverter);
}

before:

MessagePackValueMapper valueMapper = client.getConfig().getMessagePackMapper();
CallResultMapper<TestComposite, SingleValueCallResult<TestComposite>> mapper =
    client.getResultMapperFactoryFactory().<TestComposite>singleValueResultMapperFactory()
        .withSingleValueResultConverter(v -> {
            Map<String, Object> valueMap = valueMapper.fromValue(v);
            TestComposite composite = new TestComposite();
            composite.field1 = (String) valueMap.get("field1");
            composite.field2 = (Integer) valueMap.get("field2");
            composite.field3 = (Boolean) valueMap.get("field3");
            composite.field4 = (Double) valueMap.get("field4");
            return composite;
        }, TestCompositeCallResult.class);
TestComposite actual =
    client.callForSingleResult("get_composite_data", Collections.singletonList(123000), mapper).get();

after:

MessagePackValueMapper valueMapper = client.getConfig().getMessagePackMapper();
TestComposite actual = client.callForSingleResult(
    "get_composite_data",
    Collections.singletonList(123000),
    client.getResultMapperFactory().withSingleValueResultConverter(
        v -> {
            Map<String, Object> valueMap = valueMapper.fromValue(v);
            TestComposite composite = new TestComposite();
            composite.field1 = (String) valueMap.get("field1");
            composite.field2 = (Integer) valueMap.get("field2");
            composite.field3 = (Boolean) valueMap.get("field3");
            composite.field4 = (Double) valueMap.get("field4");
            return composite;
        }
    )).get();

[akudiyar]

I generally still don't have any objections against this approach. Let's rebase to eliminate the visual noise and not forget to add the documentation. Also the code duplication in the mapper constructors is something that needs to be addresssed.

I've checked the changes with Spark connector and they seem to work, but there is no extensive usage of the mappers in there. The SpringData library has some tricky usage and must be checked before merging.

[akudiyar]

Maybe we should add new interface with simple use of mappers above the factories in another PR. Cause it's so hard to use them now

I like this idea, but that will not be as simple as you imagined it. I think a PoC in a new PR is worth trying to find a better interface form. I think it may be a kind of builder or a builder factory.

[akudiyar]

I fixed a bit the javadocs for ResultMapperFactoryFactory methods. Some other new files are
still lacking javadoc headers, so we need to check this carefully again.

I have two concerns at the moment:

1. I'm strongly against using "custom" words in the names of the classes. Especially if such a class is fundamental for a subtree of classes actually used in the driver code. "Custom" should only be left for the examples and users' code, otherwise it will embarrass the readers as it implies that there are some "core" classes and these classes are "supplemental" to them.
2. We need to check every place where we are passing a mapper instance as an inner mapper so that it is an isolated shallow copy and we are not affecting the outer mapper (and more importantly, the default mapper from the settings) when building a mapper stack. I strongly advise writing some tests on this matter, since it is hard enough to check these relationships just with the eyes.

[akudiyar]

This method description is identical to one of the methods above. Is this method really needed? If yes, we need to change the description accordingly.

[ArtDu]

It has similar functionality with multiValueTarantoolResultMapperFactory. But MultiValueCallResultConverter checks that the result shouldn't be null and it also checks second item return error. Besides multiValueTarantoolResultMapperFactory uses customStructureResultMapperFactory for further parsing

Also we have example how to use this mapper

cartridge-java/src/test/java/io/tarantool/driver/mappers/TarantoolResultMapperTest.java

Lines 53 to 61 in 60d30c3

	CustomStructureToTarantoolResultMapperFactory<CustomTuple> mapperFactory
	= mapperFactoryFactory.customStructureResultMapperFactory();
	TarantoolResultMapper<CustomTuple> mapper = mapperFactory.withArrayValueToTarantoolResultConverter(v -> {
	CustomTuple tuple = new CustomTuple();
	List<Value> values = v.list();
	tuple.setId(values.get(0).asIntegerValue().asInt());
	tuple.setName(values.get(1).asStringValue().asString());
	return tuple;
	});

[akudiyar]

We need to describe the difference clearly. It is not obvious even for me

[ArtDu]

Check comments below

[ArtDu]

The SpringData library has some tricky usage and must be checked before merging.

We have AutoConverter there. The converter has same functionality like we have here except canConvertValue. I tried to add this method and it's working. I suggest to add this method in next PR

[akudiyar]

We need to rename "Custom" to something else. I suggest anything neutral like "default" (in fact we are instantiating "Default*" and "Simple*" classes there). That also applies to the directory name.

Also look at my replies in the comments.

[ArtDu]

We need to rename "Custom" to something else. I suggest anything neutral like "default" (in fact we are instantiating "Default*" and "Simple*" classes there). That also applies to the directory name.

Also look at my replies in the comments.

I did it. We can discuss if you don't like it.

7e64c00

7711634

d58ac92

[akudiyar]

Almost there, we need to just give a better name to one class and a method.

[akudiyar]

I suggest renaming this also to "RowsMetadataStructureToTupleResultConverter"

[ArtDu]

Now we have:

[akudiyar]

LGTM

Thanks for the work!