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.

Sign up for GitHub

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

Jump to bottom

Fix for previous migrations that didn't strip whitespace for all the relevant records #2374

Merged
merged 3 commits into from
Sep 25, 2023
Merged

Fix for previous migrations that didn't strip whitespace for all the relevant records #2374

merged 3 commits into from
Sep 25, 2023

Commits on Sep 25, 2023

  1. Strip whitespace from name on existing User records 

    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
    @floehopper
    floehopper committed Sep 25, 2023
    Configuration menu
    Copy the full SHA
    90b3cfd View commit details
    Browse the repository at this point in the history

  2. Strip whitespace from name on existing BIU records 

    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
    @floehopper
    floehopper committed Sep 25, 2023
    Configuration menu
    Copy the full SHA
    d79280b View commit details
    Browse the repository at this point in the history

  3. Strip whitespace from name on existing Organisation records 

    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
    floehopper committed Sep 25, 2023
    Configuration menu
    Copy the full SHA
    4bdebf6 View commit details
    Browse the repository at this point in the history