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

Add affectData attribute to @Select, @SelectProvider and <select /> #2741

Merged
merged 4 commits into from
Nov 28, 2022

Conversation

harawata
Copy link
Member

Some DBs support returning result set from INSERT/UPDATE/DELETE statements. e.g.

  • PostgreSQL has RETURNING
  • MS SQL Server has OUTPUT

By using @Select, @SelectProvider or <select />, it is possible to retrieve data from those statements.
The problem is that calling SqlSession#commit() or SqlSession#rollback()does not work as expected because MyBatis assumes SELECT does not affect DB data [1].

By setting affectData=true, users can indicate that the SELECT statement affects DB data.

Related to:

Notes:

  • The use of @Select, @SelectProvider, <select /> might look unintuitive, but changing the INSERT/UPDATE/DELETE behavior requires bigger change. The characteristic of these statement is closer to SELECT from MyBatis' perspective.
  • Enabling affectData does not automatically activate flushCache.

TODO: doc

@emacarron @jeffgbutler @mnesarco @hazendaz @kazuki43zoo @eddumelendez
Any input would be appreciated!

[1] An obvious (but not ideal) workaround is to execute another INSERT/UPDATE/DELETE in the same session.

Some DBs support INSERT, UPDATE or DELETE statement that returns result set.
PostgreSQL has RETURNING
MS SQL Server has OUTPUT

MyBatis can return results by using @select, @SelectProvider or <select />, however, rollback does not work as expected because SELECT does not mark the session 'dirty'.
@coveralls
Copy link

coveralls commented Nov 19, 2022

Coverage Status

Coverage increased (+0.01%) to 87.322% when pulling 9e786dc on harawata:dirty-select into aad75eb on mybatis:master.

To indicate the SELECT affects DB data.
e.g. PostgreSQL's RETURNING, MS SQL Server's OUTPUT
@jeffgbutler
Copy link
Member

@harawata I'm happy you are working on this issue! Your solution makes sense to me, but I wonder if it will cause too much confusion as statements will be written like this:

<select ...>
  UPDATE...
</select>

I think users are sometimes confused about this thinking that <select> means more than it actually does. For us it means "don't dirty the session and do look for a result set", but users sometimes think it forces an actual SELECT statement.

I wonder if it would ultimately be less confusing to add resultSet support to <insert>, <update>, and <delete>? I know you said it is a bigger change, but those three tags are essentially the same under the covers - so maybe it isn't so bad. The main thing you have to reason about is the correct order of processing generated keys and resultSets.

@harawata
Copy link
Member Author

Thank you, @jeffgbutler for the comment!
It seems that this syntax is getting popular for some reason.

I understand the look of <select ..>UPDATE could be confusing, but the alternative also is confusing in a way.

INSERT / UPDATE / DELETE annotations / tags will have all sorts of attributes that @Select or <select> has (e.g. resultType, resultMap, resultSets, fetchSize, resultOrdered, ...).
And INSERT / UPDATE / DELETE methods can have return type other than void, int or long.
Basically, there will be no point in having independent annotation / tag for SELECT / INSERT / UPDATE / DELETE anymore [1].
It's not hard to imagine people misunderstand/misuse these attributes/return type when they write plain INSERT / UPDATE / DELETE.

[1] This is off-topic, but I might be OK with the idea of having only one annotation like @Statement in a future major version. MyBatis internally calls java.sql.Statement#execute() for all operations anyway.


I also explored possible implementation to add result set support to INSERT / UPDATE / DELETE a little bit.
And I could think of one tricky usage.
There may be no way for MyBatis to differentiate the following two INSERTs in MapperMethod.

// This one expects update count
@Insert("insert into users (name) values (#{name})")
int insert(String name);

// This one expects result set
@Insert("insert into users (name) values (#{name}) returning id")
int insert(String name);

This, I think, means that we need to introduce a new attribute or something (e.g. expectResultSet="true|false") that lets users indicate which method to call (i.e. classic insert / update / delete or newly added insertReturning / updateReturning / deleteReturning).
Is this OK with you?
Or, is there a better idea?


Regarding simultaneous use with generated keys, I did quick JDBC tests.

  • pgjdbc behaves as if there is no result set when RETURN_GENERATED_KEYS is specified.
  • mariadb behaves as if there is no generated keys when there is RETURNING clause.
  • mssql-jdbc returns both results, but there is an oddity and it might require special treatment.

So, it's a mess, LOL.
I would not worry about this usage at this point.


Batch: I didn't test it, but these statement clearly are incompatible with JDBC batch API.

@jeffgbutler
Copy link
Member

I thought about just having a single @Statement too! But you are correct about the tradeoffs and difficulties. The problem of returning rows affected and/or a mapped result from an update is difficult to overcome in Java.

Spring JDBC template basically has four methods: query, update, batchUpdate, and execute. I believe normal usage there would be to use query for an insert ... returning. So they have the same issue we have.

Your idea is the least intrusive for sure. I'm +1 on your implementation.

@harawata
Copy link
Member Author

Thank you for the comment, @jeffgbutler !
I'm now somewhat more confident about the change. :D
I'll wait for a few more days in case others have some thoughts.

@hazendaz
Copy link
Member

hazendaz commented Nov 22, 2022 via email

@kazuki43zoo
Copy link
Member

@harawata LGTM 👍

@epochcoder
Copy link
Contributor

For anyone that has a large codebase, here is an IntelliJ inspection that can detect and fix these issues for you, after the 3.5.13 upgrade though:

.idea/inspectionProfiles/your-profile.xml:

      <replaceConfiguration name="SELECT statements affecting data" uuid="b73cae76-4e66-330c-8a61-c7c45c7cb78e" description="This select statement is affecting data, it needs to be annotated with affectData=&quot;true&quot;" suppressId="mybatis-affect-data" problemDescriptor="Add affectData=&quot;true&quot; to #ref" text="&lt;$select$ $other_attrs$ $affectData$&gt;$text$&lt;/$select$&gt;" recursive="false" caseInsensitive="false" type="XML" reformatAccordingToStyle="false" shortenFQN="false" replacement="&lt;$select$ $other_attrs$ affectData=&quot;true&quot;&gt;$text$&lt;/$select$&gt;">
        <constraint name="__context__" within="" contains="" />
        <constraint name="text" regexp=".*((?&lt;!for )\bupdate\b|\binsert\b|\bdelete\b).*" within="" contains="" />
        <constraint name="affectData" regexp="affectData" minCount="0" maxCount="0" within="" contains="" />
        <constraint name="other_attrs" regexp="affectData" minCount="0" maxCount="2147483647" negateName="true" within="" contains="" />
        <constraint name="select" regexp="select" target="true" within="" contains="" />
      </replaceConfiguration>

.idea/scopes/MyBatis_XML_Mappers.xml:

<component name="DependencyValidationManager">
  <scope name="MyBatis XML Mappers" pattern="file:resources/mappers//*.xml" />
</component>

@kazuki43zoo kazuki43zoo modified the milestone: 3.5.12 May 14, 2023
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

Successfully merging this pull request may close these issues.

6 participants