wp-cli / search-replace-command

Searches/replaces strings in the database.
MIT License
57 stars 45 forks source link

Fix search-replace for tables with composite primary keys #183

Closed brandonpayton closed 1 year ago

brandonpayton commented 1 year ago

Fixes #182

This PR addresses two bugs:

  1. When a table has a composite key, precise and regex search-replace fails because of a SQL syntax error.
  2. Once the cause of the SQL syntax error was addressed, there was another bug where, for a composite key (X, Y, Z) the next batch conditions were always X > lastX AND Y > lastY AND Z > lastZ. This condition can skip rows when ordering by X, Y, and Z because Y and Z may repeat values within different values of X. If we assume Y and Z always increase independently of one another, we will skip rows where previous Y and Z values repeat under subsequent values of X.

There are three commits under this PR. The first adds tests that will fail until the next two commits are applied. Please note that number of rows INSERTed for testing with composite primary keys is intentionally different than the number of rows used to test with single primary keys. The idea is to avoid symmetry to reduce the likelihood of the total count accidentally matching due to some other bug (e.g., a bug where composite key rows aren't replaced at all but single key rows are counted twice).

Perhaps it would be good to separate single key and composite key into dedicated scenarios, but so far, I have simply updated the precise and regex batch scenarios to also test with composite primary keys.

To test:

  1. Check out this branch
  2. git revert --no-commit HEAD HEAD~1 to temporarily reverse both fixes
  3. Run composer test and observe the failures due to SQL syntax errors
  4. Restore branch with git revert --abort
  5. git revert --no-commit HEAD to temporarily reverse the second fix
  6. Run composer test and observe that the total number of replacements is 3000 rather than the expected total of 4050.
  7. Restore branch with git revert --abort
  8. Run composer test and note that all tests pass
todeveni commented 1 year ago

Can confirm that this patch fixes my original problem in #182