Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Add query methods in ActiveRecord::Base.store for convenience #6866

Closed
wants to merge 2 commits into from

8 participants

@brutuscat

Example

class User < ActiveRecord::Base
  store :settings, accessors: [ :homepage, :editor ]
end

u = User.new(homepage: '37signals.com', editor: true)
u.editor? # => true

Much like how is done in Class.class_attribute

@rafaelfranca

Related to #5969

@rafaelfranca

I prefer the #5969 implementation, since it is calling present? instead of !!. But #5969 needs to change the implementation since initialize_store_attribute was added.

@brutuscat

And why you this call present? It is implemented above the reader accessor, so it will be present anyway I think...

@brutuscat

Also I followed Class.class_attribute as guideline for this implementation see https://github.com/rails/rails/blob/master/activesupport/lib/active_support/core_ext/class/attribute.rb#L78

@rafaelfranca

@brutuscat

"".present? #=> false
!!"" #=> true

[].present? #=> false
!![] #=> true
@rafaelfranca

In this case we should follow the Active Record query method implementation I think. @josevalim @fxn thoughts?

@brutuscat

That sounds nice. I will be happy to update the PR to push this into master.

@carlosantoniodasilva

I think there's no need to do all AR dance in store, just a present? check would do. Just my 2 cents ;)

@brutuscat

@carlosantoniodasilva Although you are right ;) I also think that is very simple snippet of code and it will behave in the same expected way, very consistent, with how all AR query attributes behave. I mean Now I see that :D

@rafaelfranca

When I was talking about the Active Record implementation I was referring only about the present? check.

@brutuscat

@rafaelfranca in that case I think that the added "dance" will be consistent with current Active Record's behavior. What would be really great is to do some form of refactoring to apply the same behavior across these 3 places (AR, Class and Store)

@rafaelfranca

I want more feedback on this.

cc/ @jeremy @tenderlove @jonleighton

@hube

I'd be more than happy to update my PR to use initialize_store_attribute if that's what gets this feature into Rails.

@brutuscat

@hube I'm willing to push this too. I just want to make it more transparent, because at least for me, it wasn't very clear how it should behave. I "used" some code from rails that it turned out to be "wrong". That's weird for me. I know it is a trivial thing, but since we are all here discussing it, we can do it right and simple ;)

Next step will be to try to push some refactoring, hopefully generalizing this query_method thing.

@jonleighton
Collaborator

I'm not keen on this. But then I dislike store in general so probably a bad person to weigh in.

In general, I don't like adding query methods in all cases, when it is only applicable to some cases (and is easy to do manually with present?)

[yeah, I know we already do it in all cases for attributes]

@rafaelfranca

I agree with @jonleighton about add more query methods.

We had a discussion in the Rails core mailing list about this recently and I realized that those methods sometimes can be more confusing that use .present?.

@hube

@jonleighton I'm not much of a fan of store as a store of arbitrary key/value pairs myself. In my case, I only use it to store boolean values (user preferences), which makes predicate methods very relevant and was the motivation for my PR.

I read the discussion that @rafaelfranca linked and I agree that there are cases where using a predicate method can be misleading. I don't think it's a fault of Rails to provide these methods necessarily, it's more a fault of developers who use predicate methods in an unclear fashion. Those cases are easily avoidable with a little bit of thought (e.g. using present? instead of the predicate method as mentioned in the thread).

I think Rails should treat all attributes uniformly instead of not supporting predicate methods for store attributes simply because the policy is currently imperfect. When the Rails team can figure out a better policy for adding predicate methods to attributes, that policy can be applied to the AR store as well. As it stands now, store attributes are subtly different from other model attributes, which is probably not what developers expect (it was certainly surprising to me).

@rafaelfranca
Owner

@jeremy thoughts?

@jonleighton
Collaborator

@hube I think we disagree on the definition of an "attribute". I don't consider the methods that get added by using store to be "attributes".

activerecord/lib/active_record/store.rb
@@ -66,6 +67,17 @@ def store_accessor(store_attribute, *keys)
initialize_store_attribute(store_attribute)
send(store_attribute)[key]
end
+
+ define_method("#{key}?") do
+ value = send(key)
+ if value.blank?
+ false
+ elsif value.is_a?(Numeric)
+ !value.zero?
+ else
+ !!value
+ end
+ end
@jeremy Owner
jeremy added a note

This is implicitly type-casting the attribute value to a boolean using similar rules to the database adapters. That seems inappropriate.

I know it seems inappropriate, but it is reassembling the same behavior of others query methods for normal ActiveRecords Objects. It would be very weird for the same object to have different query methods behaviors... This should be independent of the persistence of the attribute itself (store or proper attribute-field)

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

@jonleighton I guess we do. Here are my thoughts on attributes vs. store attributes: from the perspective of objects that interact with a given model, ordinary attributes and store attributes should be indistinguishable. The fact that some attributes are persisted in their own DB columns while others are mashed into a serialized hash and stored in one column is an implementation detail that only the model should know about.

If the data in the store shouldn't be considered an attribute, I think Rails should provide a different way of accessing them to better distinguish them from "ordinary" attributes. I'm not a fan of this route though, because as I mentioned before my own preference is that Rails just treats attributes and store attributes the same. If Rails just keeps the current behavior, I suspect other people are just gonna keep creating PRs like mine and this one.

@steveklabnik
Collaborator

This needs to be rebased.

@JonRowe

@brutuscat are you still working on this? Did you get a chance to rebase it?

@brutuscat brutuscat Add query methods in ActiveRecord::Base.store for convenience
Example
    class User < ActiveRecord::Base
      store :settings, accessors: [ :homepage, :editor ]
    end

    u = User.new(homepage: '37signals.com', editor: true)
    u.editor? # => true
ce83f5d
@brutuscat brutuscat Update query method gen based on Active Record query method
This way blank? and Numeric zero? are taken as special cases and
return false.
84a775e
@brutuscat

And re-fixed ;)

@rafaelfranca
Owner

I think we didn't had so much traction for this feature so I'm closing the PR. We already rejected this feature a lot of time and I think we still don't need it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 16, 2013
  1. @brutuscat

    Add query methods in ActiveRecord::Base.store for convenience

    brutuscat authored
    Example
        class User < ActiveRecord::Base
          store :settings, accessors: [ :homepage, :editor ]
        end
    
        u = User.new(homepage: '37signals.com', editor: true)
        u.editor? # => true
  2. @brutuscat

    Update query method gen based on Active Record query method

    brutuscat authored
    This way blank? and Numeric zero? are taken as special cases and
    return false.
This page is out of date. Refresh to see the latest.
View
18 activerecord/lib/active_record/store.rb
@@ -18,11 +18,12 @@ module ActiveRecord
# Examples:
#
# class User < ActiveRecord::Base
- # store :settings, accessors: [ :color, :homepage ], coder: JSON
+ # store :settings, accessors: [ :color, :homepage, :editor ], coder: JSON
# end
#
- # u = User.new(color: 'black', homepage: '37signals.com')
+ # u = User.new(color: 'black', homepage: '37signals.com', editor: true)
# u.color # Accessor stored attribute
+ # u.editor? # Query method for convenience
# u.settings[:country] = 'Denmark' # Any attribute, even if not specified with an accessor
#
# # There is no difference between strings and symbols for accessing custom attributes
@@ -36,7 +37,7 @@ module ActiveRecord
#
# The stored attribute names can be retrieved using +stored_attributes+.
#
- # User.stored_attributes[:settings] # [:color, :homepage]
+ # User.stored_attributes[:settings] # [:color, :homepage, :editor]
#
# == Overwriting default accessors
#
@@ -83,6 +84,17 @@ def store_accessor(store_attribute, *keys)
define_method(key) do
read_store_attribute(store_attribute, key)
end
+
+ define_method("#{key}?") do
+ value = send(key)
+ if value.blank?
+ false
+ elsif value.is_a?(Numeric)
+ !value.zero?
+ else
+ !!value
+ end
+ end
end
end
View
24 activerecord/test/cases/store_test.rb
@@ -6,7 +6,14 @@ class StoreTest < ActiveRecord::TestCase
fixtures :'admin/users'
setup do
- @john = Admin::User.create!(:name => 'John Doe', :color => 'black', :remember_login => true, :height => 'tall', :is_a_good_guy => true)
+ @john = Admin::User.create!(:name => 'John Doe',
+ :color => 'black',
+ :remember_login => true,
+ :height => 'tall',
+ :is_a_good_guy => true,
+ :is_a_bad_guy => false,
+ :empty_thing => "",
+ :zero_thing => 0)
end
test "reading store attributes through accessors" do
@@ -14,6 +21,21 @@ class StoreTest < ActiveRecord::TestCase
assert_nil @john.homepage
end
+ test "query store attributes through query accessors" do
+ assert_equal true, @john.is_a_good_guy?
+ assert_equal false, @john.is_a_bad_guy?
+ assert_equal true, @john.color?
+ assert_equal false, @john.homepage?
+ end
+
+ test "quering attributes should return false in special cases" do
+ assert_equal false, @john.empty_thing?
+ assert_equal false, @john.zero_thing?
+
+ @john.zero_thing = 0.0
+ assert_equal false, @john.zero_thing?
+ end
+
test "writing store attributes through accessors" do
@john.color = 'red'
@john.homepage = '37signals.com'
View
3  activerecord/test/models/admin/user.rb
@@ -17,8 +17,9 @@ def load(s)
store :settings, :accessors => [ :color, :homepage ]
store_accessor :settings, :favorite_food
store :preferences, :accessors => [ :remember_login ]
+ store :quering, :accessors => [ :empty_thing, :zero_thing ]
store :json_data, :accessors => [ :height, :weight ], :coder => Coder.new
- store :json_data_empty, :accessors => [ :is_a_good_guy ], :coder => Coder.new
+ store :json_data_empty, :accessors => [ :is_a_good_guy, :is_a_bad_guy ], :coder => Coder.new
def phone_number
read_store_attribute(:settings, :phone_number).gsub(/(\d{3})(\d{3})(\d{4})/,'(\1) \2-\3')
View
1  activerecord/test/schema/schema.rb
@@ -38,6 +38,7 @@ def create_table(*args, &block)
create_table :admin_users, :force => true do |t|
t.string :name
t.string :settings, :null => true, :limit => 1024
+ t.text :quering, :null => true, :limit => 1024
# MySQL does not allow default values for blobs. Fake it out with a
# big varchar below.
t.string :preferences, :null => true, :default => '', :limit => 1024
Something went wrong with that request. Please try again.