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

Remove parameter name restriction when assigning keys generated by multi-row insert. #1249

Merged
merged 4 commits into from
Aug 16, 2018

Conversation

harawata
Copy link
Member

@harawata harawata commented Apr 7, 2018

Currently, to assign keys generated by multi-row insert, the target parameter must have a special name (i.e. 'list', 'collection' or 'array').
Many users found this restriction unintuitive and inflexible (I agree).

This PR removes the restriction so that the target parameter can have any name.
It comes with two backward incompatible change, though.

  1. When there are multiple parameters, keyProperty must include the parameter name.
  2. When calling SqlSession methods directly, the target list, collection or array must be the only parameter.

See below for the details

1. When there are multiple parameters, keyProperty must include the parameter name.

Let's consider the following insert statement which takes two parameters.
Assuming that you want to assign generated keys to id property of each Country inserted.

int insert(
  @Param("list") List<Country> countries,
  @Param("someBean") SomeBean someBean);

In version ≤3.4.6, the correct value of keyProperty was id.

<insert id="insertListAndSomeId" useGeneratedKeys="true"
    keyProperty="id">
  insert into country (countryname,countrycode) values
  <foreach collection="list" separator="," item="country">
    (#{country.countryname},#{country.countrycode})
  </foreach>
</insert>

Once this PR gets merged, keyProperty must be specified as list.id.

<insert id="insertListAndSomeId" useGeneratedKeys="true"
    keyProperty="list.id">
...

2. When calling SqlSession methods directly, the target list, collection or array must be the only parameter.

With the following code, the generated keys were assigned to the 'list' parameter in version ≤3.4.6.
This PR does not support this usage.

List<Country> countries = ...;
SomeBean someBean = ...;

Map<String, Object> params = new HashMap<>();
params.put("list", countries);
params.put("someBean", someBean);

sqlSession.update("...", params);

@harawata harawata changed the title Remove the parameter name restriction when assigning keys generated by multi-row insert. Remove parameter name restriction when assigning keys generated by multi-row insert. Apr 7, 2018
}

private Object getSoleParameter(Object parameter) {
if (!(parameter instanceof ParamMap || parameter instanceof StrictMap)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it support following code ?

List<Country> countries = ...;
SomeBean someBean = ...;

Map<String, Object> params = new HashMap<>();
params.put("list", countries);
params.put("someBean", someBean);

sqlSession.update("...", params);

In this case, is need to use the DefaultSqlSession$StrictMap as follows ?

List<Country> countries = ...;
SomeBean someBean = ...;

Map<String, Object> params = new StrictMap<Object>();
params.put("list", countries);
params.put("someBean", someBean);

sqlSession.update("...", params);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @kazuki43zoo .
This PR does not support it (I intentionally dropped it, but forgot about it).
With this PR, when calling SqlSession methods directly (i.e. using XML mapper only), the target list, collection or array must be the only parameter.

Do you think that usage is important?
If so, I would take a different approach i.e. 1) introducing a new syntax list[].id for keyProperty (this was proposed in the original issue #324) or 2) introducing a new option keyCollection to specify the target list, collection or array.

@jeffgbutler , You reviewed the original PR. Any thoughts on the spec?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think that usage is important?

No. However, I prefer to announce dropping backward compatibility and introduce workaround in a release note(or migration guide).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it should be mentioned in the release note.
By workaround, you mean the code using StrictMap? I thought that was an internal class.
The workaround I had in mind was to use Java mapper. Could this be a big limitation?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By workaround, you mean the code using StrictMap? I thought that was an internal class.
The workaround I had in mind was to use Java mapper.

I agree.

# Conflicts:
#	src/main/java/org/apache/ibatis/executor/keygen/Jdbc3KeyGenerator.java
#	src/test/java/org/apache/ibatis/submitted/keygen/Jdbc3KeyGeneratorTest.java
@lovepoem
Copy link

@harawata

Let's consider the following insert statement which takes two parameters.
Assuming that you want to assign generated keys to id property of each Country inserted.

int insert(
  @Param("list") List<Country> countries,
  @Param("someBean") SomeBean someBean);

If this pr is merged, can param name be named arbitrarily instead of the specified "list"?

@harawata
Copy link
Member Author

Thanks for the comment, @lovepoem !

If this pr is merged, can param name be named arbitrarily instead of the specified "list"?

Yes. For example, you can name the parameter 'countries'.

int insert(
  @Param("countries") List<Country> countries,
  @Param("someBean") SomeBean someBean);

The keyProperty, in this case, needs to be 'countries.id'.

<insert id="insertListAndSomeId" useGeneratedKeys="true"
    keyProperty="countries.id">

@lovepoem
Copy link

If so , It look good to me

It's a good pr

@harawata harawata added enhancement Improve a feature or add a new feature no backward compatibility Includes change no backward compatibility for previous version labels Aug 16, 2018
@harawata harawata self-assigned this Aug 16, 2018
@harawata harawata added this to the 3.5.0 milestone Aug 16, 2018
@harawata harawata merged commit f44dcef into mybatis:master Aug 16, 2018
pulllock pushed a commit to pulllock/mybatis-3 that referenced this pull request Oct 19, 2023
…ement-2

Remove parameter name restriction when assigning keys generated by multi-row insert.
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 no backward compatibility Includes change no backward compatibility for previous version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants