Skip to content

Conversation

ArtDu
Copy link
Contributor

@ArtDu ArtDu commented Mar 22, 2023

It's needed when we expected crud and box responses by one mapper(AUTO mapper in cartridge-springdata). In case when we obtain box response and we didn't have prepared metadata from ddl/_vspace, we can't map these values to Entity without metadata and we can't check whether we have it or not. And we can't raise an exception that we don't have metadata before because we can take it from crud response. Springdata doesn't see a difference between crud and box responses, it only got TarantoolTupleResult

I haven't forgotten about:

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

Related issues:
Needed for tarantool/cartridge-springdata#123

@ArtDu ArtDu requested a review from akudiyar March 22, 2023 12:07
@ArtDu ArtDu self-assigned this Mar 22, 2023
Copy link
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.

This change is OK by itself, just some wording and naming needs to be fixed.

But there is a greater question, do we need this change at all. It seems it is not comfortable to work with a tuple that represents in fact two different entities.

Could it be better if we derive a TarantoolTupleWithMetadata interface from the original TarantoolTuple one, move to the second one all methods that access fields by names, add the corresponding *Impl class, and then use that interface distinction in SpringData via instanceof? That will allow us to finally avoid all the checks for metadata presence that we have to use in current implementation, and the user confusion when they try to get a field by name and see a strange "metadata does not exist" error.
With that implementation, we can change the *Proxy* API classes to return TarantoolTupleWithMetadata tuples instead of TarantoolTuple ones. That may also probably require changing the TarantoolTuple interface somewhere either to Packable or to ? extends TarantoolTuple.

*
* @return true if we obtain fields by names
*/
boolean metadataFormatIsEmpty();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest using a simpler naming like hasMetadata or isMetadataEmpty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to hasMetadata

@ArtDu
Copy link
Contributor Author

ArtDu commented Mar 28, 2023

Added ticket about splitting TarantoolTuple class to use for input and output(with/without metadata)

#375

@ArtDu ArtDu requested a review from akudiyar March 28, 2023 16:36
Copy link
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 still have the same question, see below

*
* @return true if we can obtain the tuple fields by names
*/
boolean hasMetadata();
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we are going to use different types (TarantoolTuple and TarantoolTupleWithMetadata). do we really need this method then? It is a public API, we need to carefully think before changing it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll break compatibility anyway. So I don't see a big problem to have hasMetadata now and then remove it. I want to sync springdata and cartridge-java as soon as possible, because we've already been being late

CHANGELOG.md Outdated
## [Unreleased]
- Remove config request timeout as default parameter in crud client
- Add details for the case of space metadata fetching failure ([#200](https://github.com/tarantool/cartridge-java/issues/200))
- Add metadataFormatIsEmpty in TarantoolTuple
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to change this entry too

@akudiyar
Copy link
Collaborator

OK. Let's amend this change in the nearest updates then.

@akudiyar akudiyar added this pull request to the merge queue Mar 31, 2023
Merged via the queue into master with commit 7af0378 Mar 31, 2023
@akudiyar akudiyar deleted the metadata_empty branch March 31, 2023 10:57
@ArtDu ArtDu added the 0.5sp Weight 0.5 SP label Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

0.5sp Weight 0.5 SP

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants