Bump cartridge-java version #124

ArtDu · 2023-04-03T11:35:14Z

I haven't forgotten about:

Related issues:
Closes #123

ArtDu · 2023-04-13T12:50:35Z

src/test/resources/cartridge/app/roles/api_router.lua

+        metadata = {
+            {name='testId', type=''},
+            {name='testBoolean', type=''},
+            {name='testString', type=''},
+            {name='testInteger', type=''},
+            {name='testDouble', type=''}
+        },
+        rows = { { nil, true, "abc", 123, 1.23 } }


We expect that crud response either contains metadata or it's not a crud response

Closes #123

...main/java/org/springframework/data/tarantool/core/convert/MappingTarantoolReadConverter.java

akudiyar · 2023-04-14T13:07:27Z

src/main/java/org/springframework/data/tarantool/core/TarantoolTemplate.java

-
    private <T> CallResultMapper<T, SingleValueCallResult<T>>
-    withDefaultSingleValueMapper(MessagePackMapper customMapper,
-                                 Class<T> entityClass) {


Seems that entityClass is now missing. Aren't there any compiler warnings visible?

It wasn't used properly in the cartridge-java part, we never get real entityClass. It was used as a stub for the target class which isn't used anywhere except tuple value mapping. This is what I was talking about implying that the mapping functionality is overloaded

No, its purpose is broader. It allows the Java compiler to have a certain assumption about the expression type at compile time. That's the limitation of Java, but it helps t avoid some runtime errors earlier, or you will get a clearer error message. Also it saves you from further spurious unchecked casts at different places that just make the readability worse.

You're right. It's w/a to get all types by one Mapper

<T> CompletableFuture<T> callForSingleResult( String functionName, List<?> arguments, CallResultMapper<T, SingleValueCallResult<T>> resultMapper) throws TarantoolClientException;

We have a generic T in all methods with callForSingleResult in the result mapper. It forces us to make a specific class that we should obtain, only this one. That obliges us to have a container for results like TarantoolResult. And I just used Object as a generic type for further values except for the first layer which is SingleValueCallResult

I agree that it's not good way. Maybe we can use MessagePackValueMapper as a mapper for generic calls, but with MessagePackValueMapper you should parse singleValue result every time

I created PR about changing this clunky problem tarantool/cartridge-java#391

With this one you can use methods with CallResultMapper<T, SingleValueCallResult<T>> resultMapper in a method signature and so you'll get the result, not SingleValueResult class

CallResultMapper<Object, SingleValueCallResult<Object>> callReturnMapper = factory.createMapper(valueMapper) .buildSingleValueResultMapper( factory.createMapper(valueMapper) .withArrayValueToTarantoolTupleResultConverter() .withRowsMetadataToTarantoolTupleResultConverter() .buildCallResultMapper() );

With this one you'll get a SingleValueResult as return of methods

MessagePackValueMapper returnMapper = factory.createMapper(valueMapper) .withSingleValueConverter( factory.createMapper(valueMapper) .withArrayValueToTarantoolTupleResultConverter() .withRowsMetadataToTarantoolTupleResultConverter() .buildCallResultMapper() ).buildCallResultMapper();

I don't like that we have raw Objects here. One of the targets of our driver API is to avoid any casts and raw Object usage whenever possible. And we shouldn't try to minimize the conversion code here in the SpringData module, if that worsens the user-facing API.

...main/java/org/springframework/data/tarantool/core/convert/MappingTarantoolReadConverter.java

akudiyar · 2023-04-28T15:35:10Z

src/main/java/org/springframework/data/tarantool/core/TarantoolTemplate.java

-
    private <T> CallResultMapper<T, SingleValueCallResult<T>>
-    withDefaultSingleValueMapper(MessagePackMapper customMapper,
-                                 Class<T> entityClass) {


No, its purpose is broader. It allows the Java compiler to have a certain assumption about the expression type at compile time. That's the limitation of Java, but it helps t avoid some runtime errors earlier, or you will get a clearer error message. Also it saves you from further spurious unchecked casts at different places that just make the readability worse.

pom.xml

akudiyar · 2023-04-28T15:40:18Z

src/main/java/org/springframework/data/tarantool/core/BaseTarantoolTemplate.java

+                                       .withRowsMetadataToTarantoolTupleResultConverter()
+                                       .buildCallResultMapper(
+                                               DefaultMessagePackMapperFactory.getInstance().emptyMapper()))
+                       .buildCallResultMapper(DefaultMessagePackMapperFactory.getInstance().emptyMapper());


That empty mapper here looks a bit confusing. Seems that it should be either a default mapper, or no mapper parameter at all (the empty mapper as a default value in the method).

It is confusing logic. I suggest fixing it after #124 (comment)

Let's add a ticket to the driver at least?

akudiyar · 2023-04-28T15:42:10Z

src/main/java/org/springframework/data/tarantool/core/TarantoolTemplate.java

-        MessagePackMapper customMapper = getDefaultComplexTypesMapper();
-        registerTupleResultMapper(customMapper, spaceMetadata);
-        return withDefaultSingleValueMapper(customMapper, entityClass);
+    getAutoResultMapper(Optional<TarantoolSpaceMetadata> spaceMetadata) {


Looks like that we could combine this builder sequence with the same one above and make a factory method that accepts a single mapper (that's the only difference it seems).

...main/java/org/springframework/data/tarantool/core/convert/MappingTarantoolReadConverter.java

…MappingTarantoolReadConverter.java Co-authored-by: Alexey Kuzin <[email protected]>

akudiyar

I have a question about the used concrete type

src/main/java/org/springframework/data/tarantool/core/BaseTarantoolTemplate.java

akudiyar

The blocker PR tarantool/cartridge-java#391 is merged now, let's make a new release and update this PR

akudiyar

LGTM

ArtDu force-pushed the sync branch from 54a802f to ef38858 Compare April 13, 2023 12:29

ArtDu commented Apr 13, 2023

View reviewed changes

Bump cartridge-java version

ff30533

Closes #123

ArtDu force-pushed the sync branch from ef38858 to ff30533 Compare April 13, 2023 12:52

ArtDu marked this pull request as ready for review April 13, 2023 12:55

ArtDu requested review from akudiyar, bitgorbovsky and vasilevichra April 13, 2023 12:57

vasilevichra previously approved these changes Apr 14, 2023

View reviewed changes

...main/java/org/springframework/data/tarantool/core/convert/MappingTarantoolReadConverter.java Outdated Show resolved Hide resolved

Remove redundant call

31d8997

ArtDu dismissed vasilevichra’s stale review via 31d8997 April 14, 2023 12:03

akudiyar requested changes Apr 14, 2023

View reviewed changes

Fix hasMetadata logic

8556968

ArtDu requested a review from akudiyar April 24, 2023 20:54

dkasimovskiy requested a review from vasilevichra April 26, 2023 07:12

akudiyar requested changes Apr 28, 2023

View reviewed changes

Update src/main/java/org/springframework/data/tarantool/core/convert/…

f9f61b4

…MappingTarantoolReadConverter.java Co-authored-by: Alexey Kuzin <[email protected]>

ArtDu force-pushed the sync branch from 21d8120 to f9f61b4 Compare May 2, 2023 09:40

ArtDu requested a review from akudiyar May 3, 2023 07:03

ArtDu added 2 commits May 29, 2023 10:37

WIP

3700755

Update according to cartridge-java#391

4525640

ArtDu mentioned this pull request May 31, 2023

Make mapper builder more universal tarantool/cartridge-java#391

Merged

5 tasks

akudiyar requested changes Jun 1, 2023

View reviewed changes

src/main/java/org/springframework/data/tarantool/core/BaseTarantoolTemplate.java Outdated Show resolved Hide resolved

ArtDu requested a review from akudiyar June 1, 2023 22:48

Remove Impl classes in mappers

98d100d

akudiyar requested changes Jun 2, 2023

View reviewed changes

Bump cartridge-java to 0.12.0

a90ad8e

ArtDu force-pushed the sync branch from 95d42e1 to a90ad8e Compare June 13, 2023 09:02

ArtDu requested a review from akudiyar June 13, 2023 09:06

akudiyar approved these changes Jun 13, 2023

View reviewed changes

akudiyar merged commit b462c98 into master Jun 13, 2023

akudiyar deleted the sync branch June 13, 2023 21:30

Bump cartridge-java version #124

Bump cartridge-java version #124

Uh oh!

Conversation

ArtDu commented Apr 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akudiyar Apr 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArtDu May 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

akudiyar Apr 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

akudiyar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

akudiyar left a comment

Choose a reason for hiding this comment

Uh oh!

akudiyar left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ArtDu commented Apr 3, 2023 •

edited

Loading

akudiyar Apr 28, 2023 •

edited

Loading

ArtDu May 2, 2023 •

edited

Loading

akudiyar Apr 28, 2023 •

edited

Loading