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

Quote binary strings in Arel #47493

Merged
merged 3 commits into from Mar 9, 2023
Merged

Conversation

olefriis
Copy link
Contributor

Motivation / Background

When binding binary strings in Arel, we want the data to be properly quoted, otherwise we will generate e.g. MySQL warnings.

Detail

When quoting strings, we now check if they are binary-encoded, and if so, we quote them using the native binary representation.

Co-authored with @matthewd.

Additional information

There is a bit of duplication in the quotation logic between adapters and Type::Binary::Data. Ideas for reducing this without causing additional coupling will be appreciated!

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@zzak
Copy link
Member

zzak commented Mar 3, 2023

Tests are failing, can you take a look? 🙇

Error:
--
  | BinaryTest#test_raises_warnings_when_behaviour_raise:
  | Encoding::UndefinedConversionError: "\xEE" from ASCII-8BIT to UTF-8
  | /rails/activerecord/lib/active_record/connection_adapters/sqlite3/quoting.rb:71:in `encode'
  | /rails/activerecord/lib/active_record/connection_adapters/sqlite3/quoting.rb:71:in `type_cast'
  | /rails/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb:244:in `block in type_casted_binds'
  | /rails/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb:240:in `map'
  | /rails/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb:240:in `type_casted_binds'
  | /rails/activerecord/lib/active_record/connection_adapters/sqlite3/database_statements.rb:43:in `exec_query'
  | /rails/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:142:in `exec_insert'
  | /rails/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:177:in `insert'
  | /rails/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb:22:in `insert'
  | /rails/activerecord/test/cases/binary_test.rb:47:in `test_raises_warnings_when_behaviour_raise'

@olefriis olefriis marked this pull request as draft March 3, 2023 06:54
@olefriis
Copy link
Contributor Author

olefriis commented Mar 3, 2023

@zzak Yes, sorry, I should have posted earlier on this PR that we found out about that after creating the PR. Sorry about that. We'll definitely look into fixing this!

I've changed the status of the PR to draft while we're working out the kinks so it doesn't get merged by error.

@zzak
Copy link
Member

zzak commented Mar 3, 2023

@olefriis You also don't need to use Draft PR, we will know when the PR is ready for review when the build is green and you've squashed your commits. Someone may review it at any time though! [ref]

We should properly hex-encode binary strings for SQLite3 and MySQL.
For e.g. MySQL, _not_ doing that _may_ work, but it will also
produce database warnings.
In Ruby 2.7, `BigDecimal#to_s` generates ASCII-8BIT-encoded strings.
We don't want those to get hex-encoded in the generated SQL.
@olefriis olefriis marked this pull request as ready for review March 7, 2023 13:08
@olefriis
Copy link
Contributor Author

olefriis commented Mar 7, 2023

@zzak the PR is now in a proper shape for review!

Copy link
Member

@byroot byroot left a comment

Choose a reason for hiding this comment

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

Just a minor nitpick, but LGTM.

activemodel/test/cases/type/immutable_string_test.rb Outdated Show resolved Hide resolved
@matthewd matthewd merged commit 7048f30 into rails:main Mar 9, 2023
@olefriis olefriis deleted the quote_binary_strings branch March 9, 2023 11: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.

None yet

4 participants