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

Add support to serialized fields with AR 5 #43

Merged
merged 1 commit into from
Nov 18, 2016
Merged

Conversation

nettofarah
Copy link
Contributor

This commit enables full support to ActiveRecord 5.
Our biggest change here is the use of the type_caster provided by the arel table.
This is done like this so we can support custom field serializers.

cc @rf-

@dv
Copy link
Contributor

dv commented Oct 17, 2016

This branch gives me a ton of new deprecation warnings:

Arel performing automatic type casting is deprecated, and will be removed in Arel 8.0. If you are seeing this, it is because you are manually passing a value to an Arel predicate, and the `Arel::Table` object was constructed manually. The easiest way to remove this warning is to use an `Arel::Table` object returned from calling `arel_table` on an ActiveRecord::Base subclass.

If you're certain the value is already of the right type, change `attribute.eq(value)` to `attribute.eq(Arel::Nodes::Quoted.new(value))` (you will be able to remove that in Arel 8.0, it is only required to silence this deprecation warning).

You can also silence this warning globally by setting `$arel_silence_type_casting_deprecation` to `true`. (Do NOT do this if you are a library author)

If you are passing user input to a predicate, you must either give an appropriate type caster object to the `Arel::Table`, or manually cast the value before passing it to Arel.
DEPRECATION WARNING: Passing a column to `quote` has been deprecated. It is only used for type casting, which should be handled elsewhere. See https://github.com/rails/arel/commit/6160bfbda1d1781c3b08a33ec4955f170e95be11 for more information. (called from raw_sql at /Users/david/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/bundler/gems/polo-2f6f3b3de264/lib/polo/sql_translator.rb:53)

Is it a WIP still?

@nettofarah
Copy link
Contributor Author

Yeah, I'm still working on it.

I have considered releasing it as is though, so I can start unblocking people on Rails 5.

The funny thing about this is that I don't think we're actually making calls that trigger these warnings directly.

My guess is that these warnings are being generated by some internal active record calls.

@nettofarah
Copy link
Contributor Author

@dv did you run into any problems using this branch? (other than the warnings)

@dv
Copy link
Contributor

dv commented Oct 19, 2016

@nettofarah yeah maybe release it as a beta-named gem, so people who want to upgrade to Rails 5 have a way to do so without pointing to github/master, but also to signify it's not yet 100% complete.

I did not run into any problems, but we haven't put it in production yet so it's not been thoroughly worked through either. If we do run into anything I'll make sure to let you know and help find a solution.

Investigated it a bit but can't figure out how to make it go away. Created an arel issue about it, maybe there's something obvious I'm missing.

@dv
Copy link
Contributor

dv commented Oct 19, 2016

Seem to have found a reasonably dirty solution which I submitted in #44

Feel free to merge or copy/paste and refactor, it probably takes some roundabout way to get there, but it seems to do the trick and without deprecation notices. It was built after analyzing the rails5 codepath for generating SQL inserts.

@nettofarah nettofarah merged commit 433ab9e into master Nov 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants