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

Avoid negative values from Result::rowCount() #5915

Closed
wants to merge 12 commits into from

Conversation

phansys
Copy link
Contributor

@phansys phansys commented Feb 8, 2023

Q A
Type improvement
Fixed issues n/a

Summary

To me, returning -1 from a method that should count affected records is counter-intuitive.
IMO, the errors must be reported with a proper exception mechanism, not using arbitrary values (negative integers in this case) in methods that are intended to return something else. This way, we can have confident results.

Related to #5905 (comment).
Depends on #5917.

To-Do

@phansys phansys force-pushed the row_count_positive branch 9 times, most recently from 74983a0 to 68c85d8 Compare February 8, 2023 21:26
Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

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

FTR: I am against merging this PR in it's current form. A negative number of affected rows is treated as if the SQL statement itself was unsuccessful, which is not necessarily the case.

@phansys phansys force-pushed the row_count_positive branch 3 times, most recently from 5464946 to 28ed740 Compare February 10, 2023 03:07
@phansys phansys mentioned this pull request Feb 10, 2023
1 task
@phansys
Copy link
Contributor Author

phansys commented Feb 10, 2023

FTR: I am against merging this PR in it's current form. A negative number of affected rows is treated as if the SQL statement itself was unsuccessful, which is not necessarily the case.

I agree with your concern, that's why I opened #5917 in order to avoid unwanted effects related to the expected behavior. Any help to reproduce the -1 result is greatly appreciated 🙏

@phansys phansys force-pushed the row_count_positive branch 4 times, most recently from 8cf7c68 to 7e1a5be Compare March 20, 2023 22:32
@phansys phansys force-pushed the row_count_positive branch 4 times, most recently from 32b4a26 to 71e0a46 Compare March 20, 2023 23:34
@phansys phansys force-pushed the row_count_positive branch from 71e0a46 to d3a933d Compare March 20, 2023 23:51
derrabus pushed a commit that referenced this pull request Jun 7, 2023
|      Q       |   A
|------------- | -----------
| Type         | improvement
| Fixed issues | n/a

#### Summary

This test is required in order to confirm cases like
`Result::rowCount()` returning -1.

Related to #5915, #4880, #4870.

#### To-Do

- [x] Find a query or a command sequence capable to reproduce the -1
result.
@github-actions
Copy link

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.
If you want to continue working on it, please leave a comment.

@github-actions github-actions bot added the Stale label Jun 19, 2023
@github-actions
Copy link

This pull request was closed due to inactivity.

@github-actions github-actions bot closed this Jun 26, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants