Skip to content
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

Possible NullPointerException when assigning generated keys to objects with custom hashCode() method #1719

Closed
WeiYang1005 opened this issue Nov 7, 2019 · 8 comments · Fixed by #1728
Assignees
Labels
Milestone

Comments

@WeiYang1005
Copy link

WeiYang1005 commented Nov 7, 2019

##Summary
Hello MyBatis, here is a bug for inserting objects of List which overrides hashcode method and the key uses as hashcode identifier (like id) is null.

Person overrides hashcode by id , id is null, insert List in mybatis leads to NullPointerException because dinstinct compare with id which is null.

MyBatis version

3.5.1

Database vendor and version

5.7.1

Test case or example project


Person.java

public class Person {
    private Integer id;
    private String name;
    private Integer age;
    private String email;

    @Override
    public boolean equals(Object o) {
        if (this == o) {
            return true;
        }
        if (o == null || getClass() != o.getClass()) {
            return false;
        }

        Person baseModel = (Person) o;

        return id.equals(baseModel.id);
    }

    @Override
    public int hashCode() {
        return id.hashCode();
    }
}

insertService.java

 public void insertPersons(){
        String userName = "test";
        int age = 18;
        String mobilePhone = "18000000000@test.com";
        Person person = new Person();
        person.setAge(age);
        person.setName(userName);
        person.setEmail(mobilePhone);
        List<Person> personList = new ArrayList<>();
        personList.add(person);
        try{
           mapper.insertBatch(personList);
        }catch (Exception e){
            e.printStackTrace();
        }finally {
            System.out.println(person);
        }
    }

mapper.xml

<insert id="insertBatch" useGeneratedKeys="true"
    keyProperty="id" parameterType="test.Person">
  INSERT INTO user (`name`, age, email) VALUES
    <foreach collection="list" item="person" separator=",">
      (#{person.name}, #{person.age}, #{person.email})
    </foreach>
</insert>

Steps to reproduce

While we request insertBatch, mybatis will throw a 'java.lang.NullPointerException' exception. because in src/main/java/org/apache/ibatis/executor/keygen/Jdbc3KeyGenerator.java line 174, it uses paramMap.values().stream().distinct().count() == 1 to adjust whether the count of params is 1, but the method distinct() of stream in jdk8 uses hashcode of object to do distinct, when the object overrides hashcode method with the hashcode of id, and the insert operation doesn't have any content in field id, the exception then occrupt.

But in mybatis before version 3.4.6(I have ensured), this never happened, because Jdbc3KeyGenerator in mybatis 3.4.6 didn't use distinct() method to adjust the count of param.

Anyway, in most cases, we will use this logic to insert batch datas ,hope you can fix this bug in the future.

Expected result

true

Actual result

java.lang.NullPointerException

You can contact me by 346736752@qq.com. Thank you!

@harawata
Copy link
Member

harawata commented Nov 7, 2019

Thank you for the report, @WeiYang1005 .
Could you add a full stack trace?
An executable demo project (like these) is even better. ;)
And please use three ticks ` for code blocks. See the guide for syntax highlighting, etc..

@WeiYang1005
Copy link
Author

Here is a demo project to reproduce it, https://github.com/WeiYang1005/mybatis-issue .

@WeiYang1005
Copy link
Author

I found many issues the same reason, like #1716, and #1711, both because the method getAssignerForParamMap() in Jdbc3KeyGenerator.java uses distinct() of stream to compare input params, while the params contain a null id.
I think the duplicate inputs should be assessed mannualy by the user rather than in the process of insert operation of mybatis, it gives us too many borders to find the reason.
Hope you can fix it !!! Thanks! ! !

@harawata
Copy link
Member

harawata commented Nov 8, 2019

So, the custom hashCode() and equals() throw NPE ?
Okay... I will look into it.

@kazuki43zoo
Copy link
Member

kazuki43zoo commented Nov 9, 2019

I think your implementation of equals and hashCode method is wrong. I propose to use the java.util.Objects.equals(Object, Object) and java.util.Objects.hashCode(Object) at extending method.

harawata added a commit to harawata/mybatis-3 that referenced this issue Nov 9, 2019
To detect single parameter case, it now checks parameter name instead of checking equality of parameter objects.
This should fix mybatisgh-1719.
harawata added a commit to harawata/mybatis-3 that referenced this issue Nov 10, 2019
When detecting single parameter case, it now checks parameter name instead of equality of the parameter objects.
This should fix mybatisgh-1719.

A caveat : when there is only one parameter and its name is 'param2' (for whatever reason), `keyProperty` must include the parameter name i.e. `keyProperty="param2.xx"`.
@harawata
Copy link
Member

harawata commented Nov 10, 2019

There seem to be cases that an exception is thrown intentionally from within equals/hashCode.
https://stackoverflow.com/q/15152145/1261766
There is no guarantee that MyBatis always supports such implementation, but in this particular case, there is a workaround (gh-1728), so I am going to apply it.


@WeiYang1005
Copy link
Author

WeiYang1005 commented Nov 11, 2019

You are right, that is not a good implementation of equals and hashcode. We have already changed the implementation, but this kind of implementation mostly are old codes, we cann't prevent others' existed mistakes :D. Thus we want to avoid such cases or get more useful feedbacks. Thanks for your apply! GL!

harawata added a commit to harawata/mybatis-3 that referenced this issue Nov 11, 2019
When detecting single parameter case, it now checks parameter name instead of equality of the parameter objects.
This should fix mybatisgh-1719.

A caveat : when there is only one parameter and its name is 'param2' (for whatever reason), `keyProperty` must include the parameter name i.e. `keyProperty="param2.xx"`.
@harawata harawata changed the title NullPointerException bug when insert batch List<Obejct> overrides hashcode method Possible NullPointerException when assigning generated keys to an object with custom hashCode() method Nov 12, 2019
@harawata harawata self-assigned this Nov 12, 2019
@harawata harawata added the bug label Nov 12, 2019
@harawata harawata added this to the 3.5.4 milestone Nov 12, 2019
@harawata
Copy link
Member

Hi all,

The workaround is merged. =D
Please try the latest 3.5.4-SNAPSHOT and let us know if there is any issue.
https://github.com/mybatis/mybatis-3/wiki/Maven

@harawata harawata changed the title Possible NullPointerException when assigning generated keys to an object with custom hashCode() method Possible NullPointerException when assigning generated keys to objects with custom hashCode() method Nov 13, 2019
pulllock pushed a commit to pulllock/mybatis-3 that referenced this issue Oct 19, 2023
When detecting single parameter case, it now checks parameter name instead of equality of the parameter objects.
This should fix mybatisgh-1719.

A caveat : when there is only one parameter and its name is 'param2' (for whatever reason), `keyProperty` must include the parameter name i.e. `keyProperty="param2.xx"`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants