Skip to content

resultMap.hasResultMapsUsingConstructorCollection should stay false if type handler is specified #3391

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

Merged
merged 3 commits into from
Jan 7, 2025

Conversation

harawata
Copy link
Member

@harawata harawata commented Jan 6, 2025

Hi @epochcoder ,

This is a follow up of #3108 .
If typeHandler is specified for a ResultMapping, it should not enable hasResultMapsUsingConstructorCollection, I think.
Could you verify if this assumption is valid?

I thought you would do this to fix CollectionInConstructorTest.testCollectionArgWithTypeHandler().
How did you fix that? 🤔

@harawata harawata requested a review from epochcoder January 6, 2025 19:52
@coveralls
Copy link

coveralls commented Jan 6, 2025

Coverage Status

coverage: 87.27% (-0.02%) from 87.289%
when pulling d1238a6 on harawata:3108-follow-up-type-handler
into 2de2506 on mybatis:master.

@epochcoder
Copy link
Contributor

Hey @harawata, in this case the mentioned test was fixed by allowing pending creations for simple result maps. (which also fixed some other cases). I cannot think of a reason (yet) why a type handler presence should disable it. If a collection is present in a constructor, we want to defer until later, whether that collection comes from a type handler or not.

What I think we should do to validate this is try and think of a (test) case where this does not hold true.

For instance if we have a mapping with a type handler that creates a list, and a nested result mapping, we still want the behavior.

Maybe we can try and come up with a constructor and mapping as a test case that could prove the doubt? :)

@harawata
Copy link
Member Author

harawata commented Jan 6, 2025

If a type handler is specified, it means that the collection is created from (a single column of) a single row, so there is no reason to defer the creation.
In the case of aforementioned test, the collection is created from a comma separated string.
This was actually the only way to use collection as a constructor argument before #3108 .

@epochcoder
Copy link
Contributor

epochcoder commented Jan 6, 2025

Ahh, I think I understand, you basically just want to ignore the pending creations if there is a type handler specified, however if any other mappings have a normal collection it will get enabled again (due to the for loop).

in this case I think it is okay, but I would still like to add another test case to this PR then, that includes such a mixed mapping :)

@harawata
Copy link
Member Author

harawata commented Jan 6, 2025

I was trying to edit my comment, but that is exactly right. :D

Regarding the test case, I noticed this because one of tests I was writing for #556 failed.
I'll push it to #3388 soon (maybe tonight).

@harawata
Copy link
Member Author

harawata commented Jan 6, 2025

I would still like to add another test case to this PR then, that includes such a mixed mapping

It is not the test I mentioned, but this is definitely a good idea.
If you can add one to this PR, please do.

@epochcoder epochcoder merged commit 9548efb into mybatis:master Jan 7, 2025
18 of 19 checks passed
@harawata harawata deleted the 3108-follow-up-type-handler branch January 7, 2025 17:05
@harawata
Copy link
Member Author

harawata commented Jan 7, 2025

Thank you, @epochcoder !

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.

3 participants