Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

The SQLite3 Adapter mutates arguments #7867

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants

apeiros commented Oct 7, 2012

I'd like to add a test, but I'm a bit lost on where it'd belong within all those tests :-/
So if somebody can tell me where to add it, I'll gladly amend this pull request with a matching test.

For now, an example to reproduce the problem:

ActiveRecord::Migration.create_table :foos do |t| t.string :demo end
class Foo < ActiveRecord::Base; attr_accessible :demo; end
demo = "foo".force_encoding('binary')
p before: demo.encoding # => binary
Foo.create(demo: demo)
p after: demo.encoding # => utf-8

# worse, it crashes on frozen strings:
Foo.create(demo: "demo".force_encoding('binary').freeze)
#!> ActiveRecord::StatementInvalid: RuntimeError: can't modify frozen String: INSERT INTO "foos" ("demo") VALUES (?)
#!>   from /…/gems/ruby-1.9.3-p194/gems/activerecord-3.2.8/lib/active_record/connection_adapters/sqlite_adapter.rb:208:in `encode!'

A method should never mutate its arguments.
In this specific case, after reading the code, I also think it'd be even better to raise an exception right away, stating the two options (provide a utf-8 string or use a binary type column). A warning in the log like this is IMO mostly useless.

Member

robin850 commented Oct 10, 2012

Hi,

Could you add a changelog entry please ? :)

Contributor

frodsan commented Oct 27, 2012

@rafaelfranca Does he need to add a test for this kind of change?

Owner

rafaelfranca commented Oct 28, 2012

Yes, we need to add tests for it. It will avoid regressions in the future

Contributor

frodsan commented Oct 29, 2012

Merged in b5133d0. Thanks! 👍

@frodsan frodsan closed this Oct 29, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment