Search readable property when resolving constructor arg type by name

[harawata]

Should fix #2803

Also added more info to the error message. Related to #2787

[coveralls]

Coverage: 87.183%. Remained the same when pulling 43fcd30 on harawata:record-name-only-constructor into 4c8153c on mybatis:master.

[epochcoder]

Hey @harawata, this introduced a regression for us, we use to have a construct like this:

public void setDeviceType(String deviceTypeCode) {
    if (!Text.isEmptyOrNull(deviceTypeCode)) {
        this.deviceType = DeviceTypeCache.getDeviceType(deviceTypeCode);
    }
}

public DeviceType getDeviceType() {
    return deviceType;
}

Mapper XML:

<result property="deviceType" column="deviceTypeCode"/>

We relied on the fact that the javaType got determined by the setter, now the javaType is determined by the getter, and we get thousands of errors stating that a type handler could not be found.

In the above example, the javaType in 3.5.11 was java.lang.String, in 3.5.12 it is acme.company.DeviceType.

It seems to relate to this line:

-        javaType = metaResultType.getSetterType(property);
+       javaType = metaResultType.getGetterType(property);

I'm not sure what would be the best way to resolve this, if we have to change all our entities or if we can provide some configuration option that will tell mybatis which method to use for a resultMap to determine the javaType

Also, relevant XKCD 🤣

[harawata]

Yeah...I kind of expected that (and hoped no one does that, but you know).

When fixing #2192 , I stupidly thought it would be helpful if users can omit javaType and that is the reason of this mess.
At this point, only fair solution I can think of is to make the javaType required as it should have been in the first place.
What do you say?

Cc: @sp00m @tzie0062

Apologies for the double post. Forgot the @s.

[harawata]

By the way, I really would like to avoid adding a new option for this.
If I made javaType required, everyone will complain and I can take it.
What I don't tolerate is to add a confusing option because of my stupidity...

[epochcoder]

Haha, makes sense, I agree that adding an option for something that should be fixed in a different way is silly. However if we make it required I think it should be a smart 'required', i.e: if the getter and setter type are ambiguous... Else it would force a lot of people to update a crazy amount of code. What do you think?

…

On Wed, Mar 1, 2023, 18:19 Iwao AVE! ***@***.***> wrote: By the way, I really would like to avoid adding a new option for this. If I made javaType required, everyone will complain and I can take it. What I don't tolerate is to add a stupid option... — Reply to this email directly, view it on GitHub <#2804 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHEYG4HBSJJG5VXMGC6SFDWZ6AJVANCNFSM6AAAAAAUYACS5E> . You are receiving this because you commented.Message ID: ***@***.***>

[harawata]

@epochcoder ,

It sounds reasonable, but for your usage, javaType still will be required, am I right?
I mean, as the read/write types of deviceType don't match, it is considered to be 'ambiguous'.
Please let me know if I misunderstand.

[epochcoder]

@epochcoder ,

It sounds reasonable, but for your usage, javaType still will be required, am I right?
I mean, as the read/write types of deviceType don't match, it is considered to be 'ambiguous'.
Please let me know if I misunderstand.

Exactly yes, there will for sure still be some work to do on the mapper xml's, given that these are still ambiguous ;-)

[harawata]

Got it.
I will be away for a week or two, but I'll look into it when I am back.
Thank you very much for the report and the idea!

[tzie0062]

My 2 cents: I could live with a mandatory javaType in the XML however I'd consider this a breaking change and that's not supposed to happen in a minor version update.

[hazendaz]

The yes that is correct, it should not change such behavior in patch release, change is going to be reverted tonight.

[MaximeCheramy]

Also impacted by this change, I like the suggested idea to make it required only when it's ambiguous. Alternative could be to even forbid ambiguous cases: the getter and the setter should have the correct type and it's up to the developer to either use a different type or create an extra method.

[harawata]

Hello,

After re-evaluating #2803 , I am going to simply revert this change.
Simply put, if your result map worked in 3.5.11, it will work in the next release (3.5.13).
I'll write some tests to avoid making the same mistake again.

@tzie0062 ,
I am really sorry, but your result map will break in 3.5.13.
I should not have fixed #2803 in the first place. I'll comment on #2803 later.