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

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

Merged

Conversation

brandonpayton
Copy link
Contributor

@brandonpayton brandonpayton commented Jun 2, 2023

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

@brandonpayton brandonpayton requested a review from a team as a code owner June 2, 2023 03:32
@todeveni
Copy link

todeveni commented Jun 2, 2023

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

@danielbachhuber danielbachhuber added this to the 2.1.1 milestone Jun 2, 2023
@danielbachhuber danielbachhuber merged commit 1d5c77f into wp-cli:main Jun 2, 2023
30 checks passed
@brandonpayton brandonpayton deleted the fix-search-replace-multi-key branch June 5, 2023 16:20
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.

search-replace can be/is broken because of incorrect implode()
3 participants