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

bugfix: InsertExecutor afterImage #1512

Merged
merged 37 commits into from
Sep 5, 2019
Merged

bugfix: InsertExecutor afterImage #1512

merged 37 commits into from
Sep 5, 2019

Conversation

jsbxyyx
Copy link
Member

@jsbxyyx jsbxyyx commented Aug 21, 2019

Ⅰ. Describe what this PR did

fix InsertExecutor afterImage

Ⅱ. Does this pull request fix one issue?

fixes #1500 #1430 #1516 #1470

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

mysql
create table test(
id int primary key auto_increment,
name varchar(45),
name2 varchar(45)
)

  1. containsPK (preparedstatement) pk is ?
    insert into test(id, name, name2) values (?, ?, ?)
  2. containsPK (preparedstatement) pk is null
    insert into test(id, name, name2) values(null, ?, ?)
  3. containsPK (statement) pk is null
    insert into test(id, name, name2) values(null, '11', '111')
  4. containsPK (preparedstatement) pk is null or ? is null
    insert into test(id, name, name2) values(null, ?, ?), (?, ?, ?)
    parameters: ['11', '111', null, '22', '222']
  5. not containsPK (preparedstatement) pkvalue is null
    insert into test values(null, ?, ?)
  6. not containsPK (preparedstatement) pkvalue is ?
    insert into test values(?, ?, ?)
  7. not containsPK (preparedstatement)
    insert into test(name, name2) values(?, ?)
  8. not containsPK (statement)
    insert into test(name, name3) values('11', '111')

mysql not support

  1. insert into test(id, name, name2) values(fun(), ?, ?), (fun(), ?, ?)
  2. insert into test(id, name, name2) values(null, '11', '111'), (null, '22', '222')
    excludes specify Statement.RETURN_GENERATED_KEYS case
  3. insert into test(id, name, name2) values(?, ?, ?), (1, ?, ?)

oracle
create table test(
id int primary key,
name varchar(45),
name2 varchar(45)
)
create sequence test_seq;

  1. containsPK (preparedstatement) pk is seq
    insert into test(id, name, name2) values(test_seq.nextval, ?, ?)
  2. not containsPK (preparedstatement)
    insert into test values(test_seq.nextval, ?, ?)

oracle not support

  1. insert into test(id, name, name2) values(fun(), ?, ?), (fun(), ?, ?)
  2. insert into test(id, name, name2) values(test_seq.nextval, '11', '111'), (test_seq.nextval, '22', '222')
  3. insert into test(id, name, name2) values(test_seq.nextval, ?, ?), (1, ?, ?)

Ⅴ. Special notes for reviews

@codecov-io
Copy link

codecov-io commented Aug 23, 2019

Codecov Report

Merging #1512 into develop will increase coverage by 0.16%.
The diff coverage is 50%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #1512      +/-   ##
=============================================
+ Coverage      46.28%   46.44%   +0.16%     
- Complexity      1696     1713      +17     
=============================================
  Files            348      350       +2     
  Lines          12697    12829     +132     
  Branches        1551     1616      +65     
=============================================
+ Hits            5877     5959      +82     
- Misses          6170     6224      +54     
+ Partials         650      646       -4
Impacted Files Coverage Δ Complexity Δ
.../seata/rm/datasource/sql/struct/SqlMethodExpr.java 0% <0%> (ø) 0 <0> (?)
...ource/sql/druid/oracle/OracleInsertRecognizer.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...rm/datasource/sql/druid/MySQLInsertRecognizer.java 65% <0%> (-11.48%) 7 <0> (ø)
...a/io/seata/rm/datasource/sql/struct/TableMeta.java 31.42% <100%> (-1.91%) 17 <0> (-1)
...eata/rm/datasource/sql/struct/SqlSequenceExpr.java 36.36% <36.36%> (ø) 1 <1> (?)
...va/io/seata/rm/datasource/exec/InsertExecutor.java 70.62% <60.39%> (-14.45%) 37 <12> (+17)
...in/java/io/seata/server/session/GlobalSession.java 84.61% <0%> (-0.61%) 67% <0%> (ø)
...a/io/seata/core/rpc/netty/AbstractRpcRemoting.java 9.95% <0%> (-0.21%) 3% <0%> (ø)
...n/java/io/seata/rm/datasource/ConnectionProxy.java 9.09% <0%> (-0.11%) 4% <0%> (ø)
... and 25 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f99fc51...9b63b5c. Read the comment docs.

Copy link
Contributor

@zjinlei zjinlei left a comment

Choose a reason for hiding this comment

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

LGTM

@zjinlei zjinlei requested a review from slievrly August 26, 2019 07:31
@zjinlei zjinlei changed the title fix InsertExecutor afterImage bugfix: InsertExecutor afterImage Aug 26, 2019
// pk auto generated while column exists and value is null
if (pkValues.size() > 0 && pkValues.get(0) instanceof Null) {
else if (pkValues.size() > 0 && pkValues.get(0) instanceof Null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

pkValues.size() > 0 && pkValues.stream().allMatch(pkValue -> pkValue instanceof Null)
easy to understand

@zjinlei zjinlei added the status: waiting-for-feedback Waiting for feedback label Sep 1, 2019
@jsbxyyx
Copy link
Member Author

jsbxyyx commented Sep 2, 2019

mysql
create table test(
id int primary key auto_increment, 
name varchar(45), 
name2 varchar(45)
)

1. containsPK (preparedstatement)  pk is ?
insert into test(id, name, name2) values (?, ?, ?)
2. containsPK (preparedstatement) pk is null
insert into test(id, name, name2) values(null, ?, ?)
3. containsPK (statement) pk is null
insert into test(id, name, name2) values(null, '11', '111')
4. containsPK (preparedstatement) pk is null or ? is null
insert into test(id, name, name2) values(null, ?, ?), (?, ?, ?)
parameters: ['11', '111', null, '22', '222']
5. not containsPK (preparedstatement) pkvalue is null
insert into test values(null, ?, ?)
6. not containsPK (preparedstatement) pkvalue is ?
insert into test values(?, ?, ?)
7. not containsPK (preparedstatement)
insert into test(name, name2) values(?, ?)
8. not containsPK (statement)
insert into test(name, name3) values('11', '111')

mysql not support
1. insert into test(id, name, name2) values(fun(), ?, ?), (fun(), ?, ?)
2. insert into test(id, name, name2) values(null, '11', '111'), (null, '22', '222')
excludes specify Statement.RETURN_GENERATED_KEYS case
3. insert into test(id, name, name2) values(?, ?, ?), (1, ?, ?)

oracle
create table test(
id int primary key, 
name varchar(45), 
name2 varchar(45)
)
create sequence test_seq;

1. containsPK (preparedstatement) pk is seq
insert into test(id, name, name2) values(test_seq.nextval, ?, ?)
2. not containsPK (preparedstatement)
insert into test values(test_seq.nextval, ?, ?)

oracle not support
1. insert into test(id, name, name2) values(fun(), ?, ?), (fun(), ?, ?)
2. insert into test(id, name, name2) values(test_seq.nextval, '11', '111'), (test_seq.nextval, '22', '222')
3. insert into test(id, name, name2) values(test_seq.nextval, ?, ?), (1, ?, ?)

@jsbxyyx jsbxyyx mentioned this pull request Sep 3, 2019
1 task
Copy link
Member

@slievrly slievrly left a comment

Choose a reason for hiding this comment

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

mysql 4
image

Copy link
Member

@slievrly slievrly left a comment

Choose a reason for hiding this comment

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

check all case's result OK

@slievrly slievrly removed the status: waiting-for-feedback Waiting for feedback label Sep 5, 2019
@slievrly slievrly merged commit 0cd1d98 into apache:develop Sep 5, 2019
@jsbxyyx jsbxyyx deleted the no_parameter branch September 5, 2019 13:11
@cobraOps cobraOps mentioned this pull request Sep 23, 2019
1 task
@wangliang181230 wangliang181230 added this to the 0.8.1 milestone Aug 9, 2021
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.

discuss InsertExecutor#afterImage
6 participants