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

PR showing resultMap Bug when SQL using "as" aliases for column names #1100

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@dankirkd

dankirkd commented Sep 19, 2017

This PR is intended to show a bug that was introduced with mybatis 3.4.3.

I've added a couple of tests in org.apache.ibatis.submitted.automapping.AutomappingTest (shouldGetAUserWithoutAsAliases and shouldGetAUserWithAsAliases) that show the issue. The tests are the same, but the second fails using a SQL statement that include an "as" alias.

This test would have worked with mybatis 3.4.2 and earlier.

I'm not 100% sure, but I think the issue was introduced with #895.

Note: this PR is expected to fail the build because it includes tests that expose what is believed to be a bug (see #1101).

@dankirkd dankirkd changed the title from resultMap Bug when SQL using "as" aliases for column names to PR showing resultMap Bug when SQL using "as" aliases for column names Sep 19, 2017

@harawata

This comment has been minimized.

Show comment
Hide comment
@harawata

harawata Sep 21, 2017

Member

Hi @dankirkd ,

Thank you for the test case.
In the test shouldGetAUserWithAsAliases, the result of the query is:

ID NAME PHONE
1 User1 12345678901

And the result map is declared as follows.

<resultMap type="issue1101.User" id="result2">
  <id property="id" column="id"/>
  <result property="name" column="name"/>
  <result property="phone" column="phone_number"/>
</resultMap>

There is no phone_number in the result, so user.getPhone() being null is the expected behavior.
Just to be sure, I ran the test with 3.4.2 and it still failed.
Can we close this or is there any mistake in the test case?

Member

harawata commented Sep 21, 2017

Hi @dankirkd ,

Thank you for the test case.
In the test shouldGetAUserWithAsAliases, the result of the query is:

ID NAME PHONE
1 User1 12345678901

And the result map is declared as follows.

<resultMap type="issue1101.User" id="result2">
  <id property="id" column="id"/>
  <result property="name" column="name"/>
  <result property="phone" column="phone_number"/>
</resultMap>

There is no phone_number in the result, so user.getPhone() being null is the expected behavior.
Just to be sure, I ran the test with 3.4.2 and it still failed.
Can we close this or is there any mistake in the test case?

@dankirkd

This comment has been minimized.

Show comment
Hide comment
@dankirkd

dankirkd Sep 21, 2017

I am very surprised to hear that with 3.4.2 this test failed.

We have a similar setup with the problem:

    <select id="getEEEsForCodes" resultMap="EEERowResult">
        SELECT pe.pppEEEID AS id,
               pe.pppEEESourceID,
               pe.pppEEEName AS name,
               pe.pppEEECode AS code,
               pe.pppEEETypeCd AS type,
               pe.sssEEEID AS sssEEEID,
               se.sssEEESourceID AS sssEEESourceID,
               tm.nodeID AS tttNodeID,
               COALESCE(f.subAAAName, f.aaaName, f.bbbName, f.sssName) AS buName,
               f.sssName AS sssName,
               f.tttLevel AS tttLevel
        FROM pppEEE pe
        INNER JOIN pppEEEFFFTTTMapping tm ON pe.pppEEEID = tm.pppEEEID
        INNER JOIN FFFTTT f ON tm.nodeID = f.nodeID
        LEFT JOIN sssEEE se ON se.sssEEEID = pe.sssEEEID
        WHERE (pe.endDate IS NULL
               OR pe.endDate > CURDATE())
            AND pe.pppEEECode IN (
            <foreach collection="list" item="pppEEECode" index="index" separator=",">
                #{pppEEECode}
            </foreach>
            )
    </select>

With the following resultMap definition:

    <resultMap type="EEERow" id="EEERowResult">
        <id property="id" column="pppEEEID"/>
        <result property="sourceID" column="pppEEESourceID"/>
        <result property="sssEEEID" column="sssEEEID"/>
        <result property="sssEEESourceID" column="sssEEESourceID"/>
        <result property="sssEEEName" column="sssEEEName"/>
        <result property="name" column="pppEEEName"/>
        <result property="type" column="pppEEETypeCd"/>
        <result property="code" column="pppEEECode"/>
        <result property="tttNodeID" column="nodeID"/>
        <result property="buName" column="buName"/>
        <result property="startDate" column="startDate"/>
        <result property="endDate" column="endDate"/>
    </resultMap>

The code is null in 3.4.3. But if the SQL for that column reads pe.pppEEECode, instead of pe.pppEEECode AS code, it works.

In 3.4.2 the as code is not an issue and the object field for code is not null.

dankirkd commented Sep 21, 2017

I am very surprised to hear that with 3.4.2 this test failed.

We have a similar setup with the problem:

    <select id="getEEEsForCodes" resultMap="EEERowResult">
        SELECT pe.pppEEEID AS id,
               pe.pppEEESourceID,
               pe.pppEEEName AS name,
               pe.pppEEECode AS code,
               pe.pppEEETypeCd AS type,
               pe.sssEEEID AS sssEEEID,
               se.sssEEESourceID AS sssEEESourceID,
               tm.nodeID AS tttNodeID,
               COALESCE(f.subAAAName, f.aaaName, f.bbbName, f.sssName) AS buName,
               f.sssName AS sssName,
               f.tttLevel AS tttLevel
        FROM pppEEE pe
        INNER JOIN pppEEEFFFTTTMapping tm ON pe.pppEEEID = tm.pppEEEID
        INNER JOIN FFFTTT f ON tm.nodeID = f.nodeID
        LEFT JOIN sssEEE se ON se.sssEEEID = pe.sssEEEID
        WHERE (pe.endDate IS NULL
               OR pe.endDate > CURDATE())
            AND pe.pppEEECode IN (
            <foreach collection="list" item="pppEEECode" index="index" separator=",">
                #{pppEEECode}
            </foreach>
            )
    </select>

With the following resultMap definition:

    <resultMap type="EEERow" id="EEERowResult">
        <id property="id" column="pppEEEID"/>
        <result property="sourceID" column="pppEEESourceID"/>
        <result property="sssEEEID" column="sssEEEID"/>
        <result property="sssEEESourceID" column="sssEEESourceID"/>
        <result property="sssEEEName" column="sssEEEName"/>
        <result property="name" column="pppEEEName"/>
        <result property="type" column="pppEEETypeCd"/>
        <result property="code" column="pppEEECode"/>
        <result property="tttNodeID" column="nodeID"/>
        <result property="buName" column="buName"/>
        <result property="startDate" column="startDate"/>
        <result property="endDate" column="endDate"/>
    </resultMap>

The code is null in 3.4.3. But if the SQL for that column reads pe.pppEEECode, instead of pe.pppEEECode AS code, it works.

In 3.4.2 the as code is not an issue and the object field for code is not null.

@dankirkd

This comment has been minimized.

Show comment
Hide comment
@dankirkd

dankirkd Sep 21, 2017

It seems that in 3.4.2 and earlier <result property="code" column="pppEEECode"/> would have looked for a column called pppEEECode and if it didn't find it fall back on the as alias name of code. In 3.4.3 it seems that fallback no longer happens.

dankirkd commented Sep 21, 2017

It seems that in 3.4.2 and earlier <result property="code" column="pppEEECode"/> would have looked for a column called pppEEECode and if it didn't find it fall back on the as alias name of code. In 3.4.3 it seems that fallback no longer happens.

@harawata

This comment has been minimized.

Show comment
Hide comment
@harawata

harawata Sep 21, 2017

Member

Yes, 3.4.3+ is stricter in that sense and your code worked with 3.4.2 because of the bug fixed in #895 .

  • Your result map explicitly maps the column pppEEECode to the property code.
  • The result set returned from the query does not contain pppEEECode, but it contains code.

With ≤ 3.4.2 , auto-mapping is applied to the property code even though the mapping is explicitly defined in the result map.
With ≥ 3.4.3 , auto-mapping is not applied to the properties that are explicitly mapped in a result map.


The 3.4.2 behavior can be a problem when both columns are in the result set and pppEEECode is NULL and code has some non-null value. For example...

pppEEECode code
NULL 123

With the above example result set, the expected value of the property code is null, but the actual result is 123.

Hope my explanation makes sense to you.

Member

harawata commented Sep 21, 2017

Yes, 3.4.3+ is stricter in that sense and your code worked with 3.4.2 because of the bug fixed in #895 .

  • Your result map explicitly maps the column pppEEECode to the property code.
  • The result set returned from the query does not contain pppEEECode, but it contains code.

With ≤ 3.4.2 , auto-mapping is applied to the property code even though the mapping is explicitly defined in the result map.
With ≥ 3.4.3 , auto-mapping is not applied to the properties that are explicitly mapped in a result map.


The 3.4.2 behavior can be a problem when both columns are in the result set and pppEEECode is NULL and code has some non-null value. For example...

pppEEECode code
NULL 123

With the above example result set, the expected value of the property code is null, but the actual result is 123.

Hope my explanation makes sense to you.

@dankirkd

This comment has been minimized.

Show comment
Hide comment
@dankirkd

dankirkd Sep 21, 2017

Are you sure this is related to #859 and not to #895?

I think 859 was released with 3.4.2 not 3.4.3.

dankirkd commented Sep 21, 2017

Are you sure this is related to #859 and not to #895?

I think 859 was released with 3.4.2 not 3.4.3.

@dankirkd

This comment has been minimized.

Show comment
Hide comment
@dankirkd

dankirkd Sep 21, 2017

Also, what is my workaround? I don't think I should have to make code changes for a revision. If what you are saying is so that should have been at least a minor release change (i.e. 3.5).

dankirkd commented Sep 21, 2017

Also, what is my workaround? I don't think I should have to make code changes for a revision. If what you are saying is so that should have been at least a minor release change (i.e. 3.5).

@harawata

This comment has been minimized.

Show comment
Hide comment
@harawata

harawata Sep 21, 2017

Member

Sorry, I meant #895 (corrected the comment).

I am saying that your code is invalid and it was not supposed to work in the first place.
You understand that the following mapping is wrong because there is no pppEEECode in the result set, right?

<result property="code" column="pppEEECode"/>

What you need to do is to correct the mistake (i.e. rewrite the query or the result map).

Member

harawata commented Sep 21, 2017

Sorry, I meant #895 (corrected the comment).

I am saying that your code is invalid and it was not supposed to work in the first place.
You understand that the following mapping is wrong because there is no pppEEECode in the result set, right?

<result property="code" column="pppEEECode"/>

What you need to do is to correct the mistake (i.e. rewrite the query or the result map).

@dankirkd

This comment has been minimized.

Show comment
Hide comment
@dankirkd

dankirkd Sep 21, 2017

@harawata - I understand what you are saying, but I think that the previous behavior resulted in fallback behavior that could be construed as acceptable, making this not so much a bug fix, but a feature behavior change, and one that is not called out or made clear in any of the release notes or even that PR.

dankirkd commented Sep 21, 2017

@harawata - I understand what you are saying, but I think that the previous behavior resulted in fallback behavior that could be construed as acceptable, making this not so much a bug fix, but a feature behavior change, and one that is not called out or made clear in any of the release notes or even that PR.

@harawata

This comment has been minimized.

Show comment
Hide comment
@harawata

harawata Sep 22, 2017

Member

OK, I'll ask other devs opinion. Please give us some time!

Member

harawata commented Sep 22, 2017

OK, I'll ask other devs opinion. Please give us some time!

@dankirkd

This comment has been minimized.

Show comment
Hide comment
@dankirkd

dankirkd Sep 22, 2017

Meanwhile, I am adjusting our mapper to work around the changes. But I wouldn't be surprised if others discover a similar problem.

dankirkd commented Sep 22, 2017

Meanwhile, I am adjusting our mapper to work around the changes. But I wouldn't be surprised if others discover a similar problem.

@harawata

This comment has been minimized.

Show comment
Hide comment
@harawata

harawata Sep 22, 2017

Member

one that is not called out or made clear in any of the release notes or even that PR.

We just didn't realize that. I have added a note on the release note.
https://github.com/mybatis/mybatis-3/releases/tag/mybatis-3.4.3
http://blog.mybatis.org/2017/04/mybatis-344-released.html

Member

harawata commented Sep 22, 2017

one that is not called out or made clear in any of the release notes or even that PR.

We just didn't realize that. I have added a note on the release note.
https://github.com/mybatis/mybatis-3/releases/tag/mybatis-3.4.3
http://blog.mybatis.org/2017/04/mybatis-344-released.html

@dankirkd

This comment has been minimized.

Show comment
Hide comment
@dankirkd

dankirkd commented Sep 22, 2017

Thanks.

@dankirkd dankirkd closed this Sep 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment