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: fix executeBatch can not get targetSql in Statement mode #2575

Merged
merged 2 commits into from
Apr 20, 2020

Conversation

HankDevelop
Copy link
Contributor

@HankDevelop HankDevelop commented Apr 19, 2020

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

fix bug #2537

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

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

in seata 1.1.0 addBatch could invoke only once。
if use jdbcTemplate.batchUpdate(String...sql), Only supports one SQL

@zjinlei zjinlei added this to the 1.2.0 milestone Apr 19, 2020
@zjinlei zjinlei added first-time contributor first-time contributor bug labels Apr 19, 2020
@zjinlei zjinlei removed this from the 1.2.0 milestone Apr 19, 2020
@Override
public void addBatch(String sql) throws SQLException {
if (StringUtils.isNotBlank(targetSQL)) {
targetSQL += "; " + sql;
Copy link
Contributor

Choose a reason for hiding this comment

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

If the business sql has ";", the spliced sql will be wrong.
But the business side can handle it, and it can be changed here or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There may be problems using split characters here. The best strategy is to store sql in List

Copy link
Contributor

Choose a reason for hiding this comment

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

There may be problems using split characters here. The best strategy is to store sql in List

Released soon, no major changes.

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
test batchUpdate(sql1,sql2,sql3) well, unsupport batchUpdate(sql1;sql2;sql3).

@zjinlei
Copy link
Contributor

zjinlei commented Apr 19, 2020

Test case failed, because "targetSQL = null;", but I think you are right to add it.
[ERROR] StatementProxyTest.testGetTargetSQL:105 expected: not
[ERROR] Tests run: 218, Failures: 1, Errors: 0, Skipped: 0

@zjinlei zjinlei added this to the 1.2.0 milestone Apr 19, 2020
Copy link
Contributor

@l81893521 l81893521 left a comment

Choose a reason for hiding this comment

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

Please fix the unit test at io.seata.rm.datasource.StatementProxyTest#testGetTargetSQL

@HankDevelop
Copy link
Contributor Author

HankDevelop commented Apr 19, 2020 via email

@zjinlei zjinlei changed the title fix bug#2537 jdbcTemplate batchUpdate can not get targetSql bugfix: fix jdbcTemplate batchUpdate can not get targetSql #2575 Apr 19, 2020
@zjinlei zjinlei changed the title bugfix: fix jdbcTemplate batchUpdate can not get targetSql #2575 bugfix: fix jdbcTemplate batchUpdate can not get targetSql Apr 19, 2020
@zjinlei zjinlei changed the title bugfix: fix jdbcTemplate batchUpdate can not get targetSql bugfix: fix executeBatch can not get targetSql in Statement mode Apr 19, 2020
@l81893521
Copy link
Contributor

@HankDevelop Thanks. We will test it later.

@codecov-io
Copy link

Codecov Report

Merging #2575 into develop will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #2575      +/-   ##
=============================================
- Coverage      51.19%   51.18%   -0.02%     
+ Complexity      2806     2804       -2     
=============================================
  Files            554      554              
  Lines          17768    17774       +6     
  Branches        2101     2101              
=============================================
  Hits            9097     9097              
- Misses          7814     7817       +3     
- Partials         857      860       +3     
Impacted Files Coverage Δ Complexity Δ
...io/seata/rm/datasource/AbstractStatementProxy.java 85.00% <100.00%> (-3.14%) 34.00 <0.00> (-1.00)
...in/java/io/seata/rm/datasource/StatementProxy.java 100.00% <100.00%> (ø) 24.00 <2.00> (+2.00)
...torage/file/store/FileTransactionStoreManager.java 56.59% <0.00%> (-0.65%) 28.00% <0.00%> (-1.00%)
...o/seata/server/coordinator/DefaultCoordinator.java 54.63% <0.00%> (-0.52%) 28.00% <0.00%> (-1.00%)
...in/java/io/seata/server/session/GlobalSession.java 83.71% <0.00%> (-0.46%) 71.00% <0.00%> (-1.00%)

@slievrly slievrly self-requested a review April 20, 2020 06:13
@slievrly
Copy link
Member

       Object[] objParam1 = new Object[] {100, 807};
        Object[] objParam2 = new Object[] {100, 808};
        List<Object[]> params = new ArrayList<>();
        params.add(objParam1);
        params.add(objParam2);
        jdbcTemplate.batchUpdate("update storage_tbl set count=? where id=?", params);

image

Copy link
Contributor

@l81893521 l81893521 left a comment

Choose a reason for hiding this comment

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

Test passed.

jdbcTemplate.update("update seata.`account_tbl` set money = money - ? where user_id = ?;update seata.`account_tbl` set money = money - ? where user_id = ?;",
                new Object[] {money, userIds[0], money, userIds[1]});

jdbcTemplate.batchUpdate(
                "update account_tbl set money = money - " + money + ", sex = 1 where user_id = \"" + userIds[0] + "\"",
                "update account_tbl set money = money - " + money + ", sex = 1 where user_id = \"" + userIds[1] + "\"");

jdbcTemplate.batchUpdate("update account_tbl set money = money - ?, sex = 1 where user_id = ?", new BatchPreparedStatementSetter() {
            @Override
            public void setValues(PreparedStatement ps, int i) throws SQLException {
                String userId = userIds[i];
                ps.setInt(1, money);
                ps.setString(2, userId);
            }

            @Override
            public int getBatchSize() {
                return userIds.length;
            }
        });

Copy link
Contributor

@l81893521 l81893521 left a comment

Choose a reason for hiding this comment

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

LGTM. Please wait @slievrly review

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
first-time contributor first-time contributor type: bug Category issues or prs related to bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants