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

Use param names for constructor auto-mapping #2192

Closed
Nayacco opened this issue Mar 7, 2021 · 14 comments
Closed

Use param names for constructor auto-mapping #2192

Nayacco opened this issue Mar 7, 2021 · 14 comments
Assignees
Labels
enhancement Improve a feature or add a new feature
Milestone

Comments

@Nayacco
Copy link

Nayacco commented Mar 7, 2021

I want MyBatis to map automatically with the constructor by argument names without defining <constructor> ... </constructor> or @ConstructorArgs.

I am using kotlin's data class, I want mybatis to be automatically mapped, just like I did through the empty constructor and getter setter in java.

@harawata
Copy link
Member

harawata commented Mar 7, 2021

There are two steps in constructor auto-mapping, basically.

  1. Choosing the constructor
  2. Mapping the result values to constructor arguments

If I understand correctly, Kotlin's data class has only one constructor, so step 1 should not be a problem [1].

Regarding step 2, MyBatis currently relies on the order of the result.
But argument-name based mapping seems to be useful for simple usages [2].
We may have to add a new global switch (e.g. enableArgNameBasedConstructorAutomapping) to avoid breaking backward compatibility, though.
Please give me some time to take a deeper look.

[1] When there are multiple constructors, you may have to use @AutomapConstructor annotation.
[2] Complex mappings involving nested results, etc. may still require explicit constructor mapping (i.e. <constructor /> or @ConstructorArg).

@harawata harawata added the enhancement Improve a feature or add a new feature label Mar 7, 2021
@Nayacco
Copy link
Author

Nayacco commented Mar 7, 2021

data class User(
    val id: Long,
    val name: String
)

The structure of table user has 3 fields: id, age, name

When I use select * from user, will age be discarded and the correct user will be constructed?

@harawata
Copy link
Member

harawata commented Mar 7, 2021

I only have a vague plan at this point, but yes, it should work.

@harawata harawata changed the title Enhance the full-parameter constructor Use param names for constructor auto-mapping Mar 11, 2021
@harawata harawata self-assigned this Mar 11, 2021
@harawata
Copy link
Member

Hi @GoldSubmarine ,

I have submitted a PR #2196 .
See if it satisfies your requirements.

@Nayacco
Copy link
Author

Nayacco commented Mar 12, 2021

// mapper
@Select("select name userName from users where id = #{id}")
User2 selectUserIdAndUserName(Integer id);

// Do
public class User2 {

  private Integer userId;
  private String name;

  public User2(@Param("userId") Integer userId, @Param("userName") String name) {
    super();
    this.userId = userId;
    this.name = name;
  }
  // ...getter setter
}

I added a unit test, should User2 be built successfully? From my point of view this should be supported and userId should be setted to null.

Have you considered primitive types yet.

// kotlin
data class User(
    val id: Long,
    val name: String
)

// User.class
public class User {

  private long userId;
  private String name;

  public User2(long userId, String name) {
    this.userId = userId;
    this.name = name;
  }
  // ...getter setter
}

If the id cannot be null, the arguments to Kotlin's compiled constructor are primitive types.

@harawata
Copy link
Member

Thank you for the reply, @GoldSubmarine !

The first case won't work.
Silently ignoring missing columns is a bad idea, I believe.
Instead, you should add userId to the column list explicitly e.g. select null userId, name userName ....

And there should be no problem with primitives.
It will fail if the column value is null, of course.

You should check out the branch and do various tests yourself.
It's easy with GitHub CLI : gh pr checkout 2196

@Nayacco
Copy link
Author

Nayacco commented Mar 12, 2021

I have tested the first case locally, it's not what I expected and I just asked, but thank you for teaching me to test through GitHub CLI.

select null userId, name userName ...

I know this is more rigorous, but I just want to build the object as much as possible based on the result, and it should be null without being selected, which I think is in line with expectations, manually specifying it every time is too cumbersome.

I think the problem is the same as whether JSON has a key or not when it's value is null. Being too strict can make it difficult to use. Is it possible to provide a configuration to adjust the strict?

@harawata
Copy link
Member

I've been thinking about it, but it seems like a clear error case to me if a constructor argument of an immutable object is missing.
And the current column-order-based constructor auto-mapping also throws an error if there are fewer columns in the result set than the constructor arguments.

Let's keep this open and see what other devs (and users) think.

We can add a new config switch to control the behavior, but only if it is absolutely necessary.

Cc: @h3adache @kazuki43zoo @eddumelendez @jeffgbutler @emacarron

@Cenibee
Copy link

Cenibee commented Nov 16, 2021

I had a same problem on Kotlin, and I solved it with No-arg compiler plugin.
It is a Gradle(or Maven) plugin.

Show the details in here.

@Nayacco
Copy link
Author

Nayacco commented Nov 16, 2021

@Cenibee Yes, but non-null check does not take effect if not use the constructor with parameter.

@Cenibee
Copy link

Cenibee commented Nov 18, 2021

@GoldSubmarine
Thank you for your answer. I missed the cases. It can only be used in certain situations.

However, on the one hand, in the DAO class (especially Kotlin's class and data class), I think that non-null attributes that does not exist in the mapper have default values or are derived from another attributes.

@Nayacco
Copy link
Author

Nayacco commented Jan 12, 2022

Is this the same feature? #2195
If they are the same feature, you can close this Issue. @harawata

@harawata
Copy link
Member

@GoldSubmarine ,
It's a different feature. This feature can be used for non-record classes as well.

@harawata harawata added this to the 3.5.10 milestone Apr 24, 2022
@harawata
Copy link
Member

#2196 is merged.
I would appreciate if you test the feature using the latest 3.5.10-SNAPSHOT.

Just to reiterate, when there is no matching column in the result set, an exception is thrown.

Thank you all for your contributions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve a feature or add a new feature
Projects
None yet
Development

No branches or pull requests

3 participants