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

Update comment in MyBatisBatchItemWriter.java #399

Merged
merged 2 commits into from
Aug 21, 2019

Conversation

evans810
Copy link
Contributor

@evans810 evans810 commented Aug 19, 2019

Modify comment in Line 67. As in code line 150, if assertUpdates is true, only results.size()==1 can pass the validation

#390

Modify comment in Line 67. As in code line 150, if assertUpdates is true, only results.size()==1 can pass the validation
@kazuki43zoo
Copy link
Member

@evans810 Thanks for your contributing!

I've confirmed your suggestion and current implementation. As a result, I thought the current comment was correct because assertUpdates property is flag that whether assert results.get(0).getUpdateCounts()[i] != 0(L.159) rather that results.size() == 1.
results.size() != 1 never occur if used the MyBatisBatchItemWriter correctly.

int[] updateCounts = results.get(0).getUpdateCounts();
for (int i = 0; i < updateCounts.length; i++) {
int value = updateCounts[i];
if (value == 0) {
throw new EmptyResultDataAccessException(
"Item " + i + " of " + updateCounts.length + " did not update any rows: [" + items.get(i) + "]", 1);
}
}

If you agree my opinion, I will close this PR. WDYT?

Thanks.

@evans810
Copy link
Contributor Author

evans810 commented Aug 21, 2019

@kazuki43zoo Thank you for your reply.

As the code, in the code block of below if statements(line 149 ~ line 166) ,

if (assertUpdate) {...}

two conditions will be checked:
1.

if (results.size() != 1) {
          throw new InvalidDataAccessResourceUsageException("Batch execution returned invalid results. "
              + "Expected 1 but number of BatchResult objects returned was " + results.size());
        }
if (value == 0) {
            throw new EmptyResultDataAccessException(
                "Item " + i + " of " + updateCounts.length + " did not update any rows: [" + items.get:i + "]", 1);
          }

In our practice, results.size() == 1 has appeared and we were confused by the comment of assertUpdates. That's why we thought the comment should be more clear or change the code to reflect the logic more clearly.

I have updated PR. Could you help to review?

Thanks.

Modify comment in Line 67. As if assertUpdates is true, only results.size() == 1 and results.get(0).getUpdateCounts()[i] != 0 can pass the validation
@kazuki43zoo kazuki43zoo self-assigned this Aug 21, 2019
@kazuki43zoo kazuki43zoo added this to the 2.0.3 milestone Aug 21, 2019
@kazuki43zoo kazuki43zoo merged commit 9600ed8 into mybatis:master Aug 21, 2019
@kazuki43zoo
Copy link
Member

@evans810 thx!

pulllock pushed a commit to pulllock/mybatis-spring that referenced this pull request Oct 19, 2023
Update comment in MyBatisBatchItemWriter.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants