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

Incorrect work with immutable objects #2207

Open
OverDrone opened this issue Mar 25, 2021 · 1 comment
Open

Incorrect work with immutable objects #2207

OverDrone opened this issue Mar 25, 2021 · 1 comment

Comments

@OverDrone
Copy link

OverDrone commented Mar 25, 2021

Mybatis has immutable object support but not ideal. Suppose we have class and mapper

public enum TestEnum {
  ONE,
  TWO;
  public static final Map<Integer, TestEnum> BY_CODE = ...
}

public class TestRecord {
  private final TestEnum field;
  public TestRecord(final Integer field) {
    super();
    this.field = TestEnum.BY_CODE.get(field);
  }

  public TestEnum getField() {
    return field;
  }
}

public interface TestMapper {
  @Select("SELECT 1 AS field")
  TestRecord get();
}

If we set AutoMappingBehavior to NONE, then MyBatis does not use constructor with parameters and throws exception ExecutionException: Do not know how to create an instance of class.
If we set AutoMappingBehavior to PARTIAL/FULL, then MyBatis finds constructor, calls it BUT also sets final fields with the same name because JDK allows that via reflection. I can't see any reason why would MyBatis ignores all conventions, the fact that there is no setter, ignores the fact that field is final (i.e. set once in constructor) and still tries to set final field. In my example another exception is thrown, I don't remember exact wording, but the problem was trying to set value 1 to field of type TestEnum.

I used very awkward workaround changing field names in select so that they won't match java bean field names. It makes my code stink. I need to write comment on every select/java bean to explain why I do this. New developer implementing new functionality might forget/never know this and receive this exception again in new code and I (as a lead dev) has no way to prevent this except reviewing every single line of new code and remembering this problem by heart.

  1. This is a very bad idea to set final fields via reflection. If they are final, then constructor must initialize them, MyBatis may only call correct constructor in this case. Again I can think of no single reason why MyBatis may need to set final fields.
  2. Even if constructor arguments has the same type as fields then no exception is thrown, but under the hood MyBatis initializes all fields TWICE. First time passing value to constructor which sets those fields using developer logic (which is not always set as is), second time setting value directly to each field overriding any custom constructor initialization logic.
  3. If you want to keep this strange behavior for some reason at least give us opportunity to tell MyBatis: Do constructor auto mapping, don't do field auto mapping. In other case pls give me correct configuration/annotation settings so that my example above works. Yes I can use mapping with explicit list of arguments, but the whole idea of auto mapping is get rid of explicit mappings, otherwise what's the advantage of MyBatis against good old JDBC queries?
@harawata
Copy link
Member

Hello @OverDrone ,

Regarding 1, setting final fields is a proper 'feature' of MyBatis.
I understand your point, but I know some users find it useful and we cannot just change it.
I have an idea that allows users to customize the behavior, but it needs to wait another patch I am working on and it won't happen any time soon.

2 is a known corner case.
Fixing this is difficult because there probably are many solutions that works because of this behavior.

Now, we plan to release 'argument name based constructor auto-mapping' as an alternative to the current 'column order based constructor auto-mapping'. See #2196 #2192
And this new constructor auto-mapping method does not have the issue described in 2, so I would suggest you to give it a try.
It's currently available in the 3.5.10-SNAPSHOT.

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

No branches or pull requests

2 participants