Fix for previous migrations that didn't strip whitespace for all the relevant records #2374
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mike29736
approved these changes
Sep 25, 2023
There was a problem hiding this comment.
😳 I'm impressed that you spotted this... even just looking into it to verify the changes, there don't seem to be many clues!
FWIW, it looks like we could also use character classes to achieve the same thing (e.g. "^[[:space:]]+", found here: https://dev.mysql.com/doc/refman/8.0/en/regexp.html#regexp-syntax)
Thanks - I spotted it because I was getting some confusing results when doing counts before & after the migration. It seems as if some of the
Yes, I did try that, but I was so confused by the results I was getting in the DB console at that point, I decided to avoid MySQL altogether! In hindsight, I think it probably would've worked! |
|
Rebasing on |
floehopper
force-pushed
the
fix-for-migrations-that-stripped-whitespace
branch
from
022166b
to
5e2cc0d
Compare
In #2373 I noticed that the query we'd used to do this in #2291 didn't escape the regular expression properly. ActiveRecord seems to have generated the following SQL WHERE expression: WHERE (name REGEXP('^ +') OR name REGEXP(' +$') I _think_ this will only have found names with leading/trailing space characters and not leading/trailing newlines/carriage-returns/tabs. Thus the previous migration didn't do anything bad, it just missed _some_ records. In this new version of the migration I've constructed the query slightly differently. Firstly I've double-escaped the whitespace character in the regular expression and secondly I've used an Array Condition vs a Pure String Condition [1] to ensure argument safety. The new query generates the following SQL WHERE expression: WHERE (name REGEXP '^\\s+' OR name REGEXP '\\s+$') This has the REGEXP string argument escaped correctly. As per the migrations in #2373 I've use ActiveRecord::Persistence#update_attribute in order to skip model validation, in case some of the existing User records are invalid for other reasons. As before I did consider writing the migration as a SQL UPDATE, but generating a TRIM expression that is exactly equivalent to String#strip isn't as trivial as it sounds! [1]: https://guides.rubyonrails.org/active_record_querying.html#conditions
In #2373 I noticed that the query we'd used to do this in #2291 didn't escape the regular expression properly. ActiveRecord seems to have generated the following SQL WHERE expression: WHERE (name REGEXP('^ +') OR name REGEXP(' +$') I _think_ this will only have found names with leading/trailing space characters and not leading/trailing newlines/carriage-returns/tabs. Thus the previous migration didn't do anything bad, it just may have missed _some_ records. In this new version of the migration I've constructed the query slightly differently. Firstly I've double-escaped the whitespace character in the regular expression and secondly I've used an Array Condition vs a Pure String Condition [1] to ensure argument safety. The new query generates the following SQL WHERE expression: WHERE (name REGEXP '^\\s+' OR name REGEXP '\\s+$') This has the REGEXP string argument escaped correctly. As per the migrations in #2373 I've use ActiveRecord::Persistence#update_attribute in order to skip model validation, in case some of the existing BatchInvitationUser records are invalid for other reasons. As before I did consider writing the migration as a SQL UPDATE, but generating a TRIM expression that is exactly equivalent to String#strip isn't as trivial as it sounds! [1]: https://guides.rubyonrails.org/active_record_querying.html#conditions
In #2373 I noticed that the query we'd used to do this in #2351 didn't escape the regular expression properly. ActiveRecord seems to have generated the following SQL WHERE expression: WHERE (name REGEXP('^ +') OR name REGEXP(' +$') I _think_ this will only have found names with leading/trailing space characters and not leading/trailing newlines/carriage-returns/tabs. Thus the previous migration didn't do anything bad, it just may have missed _some_ records. In this new version of the migration I've constructed the query slightly differently. Firstly I've double-escaped the whitespace character in the regular expression and secondly I've used an Array Condition vs a Pure String Condition [1] to ensure argument safety. The new query generates the following SQL WHERE expression: WHERE (name REGEXP '^\\s+' OR name REGEXP '\\s+$') This has the REGEXP string argument escaped correctly. As per the migrations in #2373 I've use ActiveRecord::Persistence#update_attribute in order to skip model validation, in case some of the existing Organisation records are invalid for other reasons. As before I did consider writing the migration as a SQL UPDATE, but generating a TRIM expression that is exactly equivalent to String#strip isn't as trivial as it sounds! [1]: https://guides.rubyonrails.org/active_record_querying.html#conditions
floehopper
force-pushed
the
fix-for-migrations-that-stripped-whitespace
branch
from
5e2cc0d
to
4bdebf6
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This includes new migrations to strip leading/trailing whitespace from:
User#nameBatchInvitationUser#nameOrganisation#nameIn #2373 I noticed that the query we'd used to do this in #2291 & #2351 didn't escape the regular expression properly. ActiveRecord seems to have generated the following SQL
WHEREexpression:I think this will only have found names with leading/trailing space characters and not leading/trailing newlines/carriage-returns/tabs. Thus the previous migration didn't do anything bad, it just may have missed some records.
In these new versions of the migration I've constructed the query slightly differently. Firstly I've double-escaped the whitespace character in the regular expression and secondly I've used an Array Condition vs a Pure String Condition to ensure argument safety. The new query generates the following SQL
WHEREexpression:This has the
REGEXPstring argument escaped correctly.As per the migrations in #2373 I've use
ActiveRecord::Persistence#update_attributein order to skip model validation, in case some of the existing records are invalid for other reasons.As before I did consider writing the migration as a SQL
UPDATE, but generating aTRIMexpression that is exactly equivalent toString#stripisn't as trivial as it sounds!