Skip to content

Add ability to use different converters via factory#359

Merged
akudiyar merged 3 commits intomasterfrom
autoconverter
Mar 1, 2023
Merged

Add ability to use different converters via factory#359
akudiyar merged 3 commits intomasterfrom
autoconverter

Conversation

@ArtDu
Copy link
Copy Markdown
Contributor

@ArtDu ArtDu commented Feb 17, 2023

To use mappers for custom behavior

MessagePackMapper valueMapper = client.getConfig().getMessagePackMapper();

ResultMapperFactoryFactory factory =
    client.getResultMapperFactoryFactory();
CallResultMapper callReturnMapper = factory.createMapper()
            .withSingleValueConverter(
                factory.createMapper()
                    .withArrayValueToTarantoolTupleResultConverter(valueMapper, null)
                    .withRowsMetadataToTarantoolTupleResultMapper(valueMapper, null)
                    .buildMessagePackMapper(valueMapper.copy())
            )
            .buildCallResultMapper(valueMapper.copy());

Object crudResult =
    client.call("crud.select", Collections.singletonList("test_space"), returnMapper).join();
assertTrue(crudResult instanceof TarantoolTupleResultImpl);
Object boxResult =
    client.call("select_request_counters", new ArrayList<>(), returnMapper).join();
assertTrue(boxResult instanceof TarantoolTupleResultImpl);
Object primitiveObject = client.call("returning_number", new ArrayList<>(), callReturnMapper).join();
assertEquals(2, primitiveObject);

I haven't forgotten about:

  • Tests
  • Changelog
  • Documentation
  • Commit messages comply with the guideline
  • Cleanup the code for review. See checklist

Related issues:
Closes #320

@ArtDu ArtDu changed the title WIP Use different converters besides via factory Add ability to use different converters via factory Feb 20, 2023
@ArtDu ArtDu force-pushed the autoconverter branch 2 times, most recently from 7c8c7cf to 372f921 Compare February 20, 2023 14:33
@ArtDu ArtDu marked this pull request as ready for review February 20, 2023 14:43
@ArtDu ArtDu requested review from Elishtar and akudiyar February 20, 2023 14:43
Copy link
Copy Markdown
Collaborator

@akudiyar akudiyar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea in general. Let's correct the API a bit and add some short examples in the Javadocs for the people to be able to use it and read this code.

We will need to explain what that hierarchy of mappers represents actually, how it is mapped to a real result from the server.

*/
<T> ArrayValueToTarantoolResultMapperFactory<T> rowsMetadataStructureResultMapperFactory();

Builder createMapper();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Javadoc is missing. I suggest putting short builder usage example in the Javadoc

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added something

}

public MessagePackValueMapper buildMessagePackMapper(MessagePackValueMapper valueMapper) {
return new CallResultMapper(valueMapper, mappers);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a copy-paste, should it be a DefaultMessagePackMapper?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed something

client.getResultMapperFactoryFactory();
CallResultMapper callReturnMapper = factory.createMapper()
.withSingleValueConverter(
factory.createMapper()
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this example, we have valueMapper passed 3 times and one time with a necessary copy() call. We will never have a chance to explain how to use this to the customers, and we will make mistakes ourselves.

But let's pass the valueMapper once to the builder:

CallResultMapper callReturnMapper = factory.createMapper(valueMapper)
            .withSingleValueConverter(
                factory.createMapper(valueMapper)
                    .withArrayValueToTarantoolTupleResultConverter()
                    .withRowsMetadataToTarantoolTupleResultMapper()
                    .buildMessagePackMapper()
            )
            .buildCallResultMapper();

The copy() method will be called internally where it is needed.

We can make a second createMapper() method accepting also space metadata. Passing null several times is also not comfortable.

What do you think?

Copy link
Copy Markdown
Contributor Author

@ArtDu ArtDu Feb 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, your're right. It should be more easier. I changed

ArtDu and others added 2 commits February 28, 2023 14:51
Co-authored-by: Alexey Kuzin <akudiyar@gmail.com>
@ArtDu ArtDu requested a review from akudiyar February 28, 2023 14:08
@akudiyar akudiyar added this pull request to the merge queue Mar 1, 2023
Merged via the queue into master with commit ef61296 Mar 1, 2023
@akudiyar akudiyar deleted the autoconverter branch March 1, 2023 22:52
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

Successfully merging this pull request may close these issues.

Add ability to use auto converter from cartridge-java

2 participants