Skip to content

Add Affected Rows Count for queries that can modify data in wp db query #277

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

Merged
merged 16 commits into from
Mar 10, 2025

Conversation

karthick-murugan
Copy link
Contributor

@karthick-murugan karthick-murugan commented Feb 25, 2025

Fixes #120

Description

This PR enhances the wp db query command to return the number of rows affected for UPDATE and DELETE queries. Currently, the command only returns a success or failure message without specifying how many rows were impacted. This update aligns WP-CLI's behavior with MySQL, providing more transparency when executing data modifications.
Changes

  • Appends SELECT ROW_COUNT(); to UPDATE and DELETE queries to retrieve affected row count.
  • Extracts and displays the affected rows in the success message.
  • Ensures compatibility with MySQL and MariaDB by executing both commands within the same session.

Testing Instructions

  1. Run an UPDATE or DELETE query using wp db query, e.g.:

wp db query "DELETE FROM wp_posts WHERE ID < 100;"

  1. The output should now include the affected row count:

Success: Query succeeded. Rows affected: 15

Additional Notes

  • A video demonstration of the feature has been provided for review.
REC-20250225224011.mp4
  • This improvement helps users running large-scale operations (e.g., batch deletions) track their query results more effectively.

@karthick-murugan karthick-murugan requested a review from a team as a code owner February 25, 2025 17:04
@swissspidy
Copy link
Member

Thanks for your pull request!

A few tests now need to be updated because the expected output changed. Would you be up for taking a look at those and updating the tests?

@karthick-murugan
Copy link
Contributor Author

@swissspidy - Could you please provide any insights or recommendations on resolving the "Testing / test / Functional" issues? Any guidance would be greatly appreciated!

Thanks in advance for your help.

@swissspidy
Copy link
Member

What’s the issue specifically? Are you able to run tests locally?

composer behat or WP_CLI_TEST_DBTYPE=sqlite composer behat. You can also run only a specific file or test, e.g. composer behat features/db-check.feature:3

With the new output, some of the expected output in tests has changed and needs updating. Unfortunately we don’t have a good diff if the expected output changed, but you could also try running the commands from the test locally against your database.

On that note, I don‘t get the recent is_test_environment change in this PR… That‘s not something we‘d want to do. We should update/add tests, not disable code from running in tests. Otherwise we can‘t actually test it

@karthick-murugan
Copy link
Contributor Author

@swissspidy - I have updated your feedbacks and I think only this test ( composer behat features/db-query.feature ) is failing in local and can you provide some insights to fix those. Thanks.

db_command

What’s the issue specifically? Are you able to run tests locally?

composer behat or WP_CLI_TEST_DBTYPE=sqlite composer behat. You can also run only a specific file or test, e.g. composer behat features/db-check.feature:3

With the new output, some of the expected output in tests has changed and needs updating. Unfortunately we don’t have a good diff if the expected output changed, but you could also try running the commands from the test locally against your database.

On that note, I don‘t get the recent is_test_environment change in this PR… That‘s not something we‘d want to do. We should update/add tests, not disable code from running in tests. Otherwise we can‘t actually test it

@swissspidy
Copy link
Member

Look at the actual test and what it does:

When I run `wp db query "SELECT COUNT(ID) FROM wp_users;"`
Then STDOUT should be:
"""
COUNT(ID)
1
"""

With your PR, if you now run wp db query "SELECT COUNT(ID) FROM wp_users;" you don't get any output at all.

That's because it prevents default output and only calls WP_CLI::success() if it's an UPDATE or DELETE query. But anything else is ignored and the output isn't forwarded. You'd need a else { WP_CLI::line( $stdout ); } there or so.

I just pushed such a change to demonstrate it.

@karthick-murugan
Copy link
Contributor Author

@swissspidy – Most of the tests are now running successfully in the local environment. However, there is one remaining issue with composer behat features/db.feature, where the test fails due to a "Table 'custom_table' already exists" error. Interestingly, if I manually remove the table and re-run the command, it executes without any issues.

Would you have any recommendations on how to best resolve this? Perhaps handling table cleanup within the test setup or ensuring the table is dropped before execution? Looking forward to your insights!

db_command1

Look at the actual test and what it does:

When I run `wp db query "SELECT COUNT(ID) FROM wp_users;"`
Then STDOUT should be:
"""
COUNT(ID)
1
"""

With your PR, if you now run wp db query "SELECT COUNT(ID) FROM wp_users;" you don't get any output at all.

That's because it prevents default output and only calls WP_CLI::success() if it's an UPDATE or DELETE query. But anything else is ignored and the output isn't forwarded. You'd need a else { WP_CLI::line( $stdout ); } there or so.

I just pushed such a change to demonstrate it.

@swissspidy
Copy link
Member

@karthick-murugan That's a fallacy. It isn't because of the table existing.

Tests already clean up the database, but if you of course run the commands manually then you'll get an error about the table existing

What happened is that the test expected STDOUT to be empty, but my previous change printed an empty line. This is now fixed.

All tests (with the exception of the SQLIte ones) now pass :)

@swissspidy swissspidy requested a review from a team March 6, 2025 15:27
@swissspidy
Copy link
Member

Now what's missing is adding new tests to verify that this change works as expected.

@swissspidy
Copy link
Member

@karthick-murugan Note that the expected output needs to be on its own line. Look at other tests to see how the formatting is supposed to be.

@karthick-murugan
Copy link
Contributor Author

Thanks @swissspidy I was able to fix those errors. Now only the errors from SQLite remains. Can you provide me some insights?

@karthick-murugan Note that the expected output needs to be on its own line. Look at other tests to see how the formatting is supposed to be.

@swissspidy
Copy link
Member

Those are expected, see #234

@karthick-murugan
Copy link
Contributor Author

Those are expected, see #234

Thanks @swissspidy for the information. Hope we can move forward with this PR.

Copy link

codecov bot commented Mar 7, 2025

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@karthick-murugan
Copy link
Contributor Author

@swissspidy - Do I need to update anything else in this PR?

@swissspidy
Copy link
Member

No :)

This just needs a review now, ideally from someone else than me.

Copy link
Member

@mrsdizzie mrsdizzie left a comment

Choose a reason for hiding this comment

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

Great, thanks for the quick response! Everything looks good to me

@mrsdizzie mrsdizzie changed the title Add Affected Rows Count for UPDATE and DELETE Queries in wp db query Add Affected Rows Count for queries that can modify data in wp db query Mar 10, 2025
@mrsdizzie mrsdizzie merged commit 19b2555 into wp-cli:main Mar 10, 2025
29 of 39 checks passed
@mrsdizzie mrsdizzie added this to the 2.1.3 milestone Mar 10, 2025
@karthick-murugan karthick-murugan deleted the wpcli/delet-update-changes branch March 11, 2025 04:38
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.

Feature Request: Return number of rows affected by update/delete queries.
3 participants