Add data type support to store_accessor by palkan · Pull Request #18942 · rails/rails

palkan · 2015-02-14T15:49:30Z

Add type casting for store_accessors. First, example:

class TypedUser < User
  store_accessor :settings, login_at: :date_time
end

u = TypedUser.new(active: false, login_at: '2015-01-01 00:01')   
u.login_at.is_a?(DateTime) #=> true 
u.login_at = DateTime.new(2015,1,1,11,0,0)
u.reload

# after loading record from db store contains raw data
u.settings['login_at'] #=> '2015-01-01 11:00:00'
# but accessor returns type casted value
u.login_at == DateTime.new(2015,1,1,11,0,0) #=> true

This feature is the most efficient when working with Postresql hstore, because hstore stores all values as strings. Example:

# without typed accessors
class User < ActiveRecord::User
  # suppose we store some counter within hstore 'settings'
  store_accessor :settings, :counter

  # we have to override getter to get value as integer
  def counter
    super.to_i  
  end
end

# with typed accessor
class User < ActiveRecord::User
  store_accessor :settings, counter: :integer
end

This makes code depend less on store type.

Another feature is data normalization:

class User < ActiveRecord::User
  store_accessor :settings, birthday: :date
end

User.new(birthday: '01/01/2010') #=> stores 'birthday' as '2010-01-01' 
User.new(birthday: '2010-01-01') #=> stores 'birthday' as '2010-01-01'  too

Custom types are also supported (they must inherits from ActiveRecord::Type::Value):

class User < ActiveRecord::User
  # provide class or instance itself
  store_accessor :settings, birthday: BirthdayValue, money: MoneyValue.new
end

prathamesh-sonpatki · 2015-02-14T16:40:14Z

keys + typed_keys.keys

prathamesh-sonpatki · 2015-02-14T16:47:27Z

@palkan Looks good. I just added some minor comments about formatting :)

simi · 2015-02-14T16:57:21Z

Isn't there chance to use https://github.com/rails/rails/blob/master/activerecord/lib/active_record/attributes.rb for similar purpose?

palkan · 2015-02-14T18:29:38Z

@simi What do you mean? To override store type with custom type?

egilburg · 2015-02-15T06:14:33Z

Perhaps next phase, but would be cool to allow users api to manage these mappings, similar to how you can use the attribute :some_attribute, :some_user_defined_type.

Also, perhaps deprecate store_accessor method name in favor of store_attribute? It'd be more in-line with new attribute API, and also make more semantic sense because accessor implies a basic in-memory ruby getter/setter, while attribute macro does casting and interacts with db.

Alternatively this could be attribute :something, store: true or attribute :something, store: { some_attr: :some_type }.

CC @sgrif

Yeah. It's late, and I'll look at this more in the morning, but I'd be against this feature overall and definitely this should lean in that direction implementation wise.

@egilburg
Store accessor (or store attribute) is very different from attribute, because it doesn't deal with raw db data (which varies for different store types). Kind of another layer of abstraction.

I think that solution should be as simple as possible. Store accessor is just a getter/setter, right. The idea is to make these getters/setters a little bit smarter.

Kind of another layer of abstraction.

Yes. Agree, but this doesn't mean we can't use the same concept of type casting applied on attribute for this feature too.

@rafaelfranca
Oh, there is a new API method ActiveRecord::Type.lookup, it's just what we need here. Great. I gonna patch the code!

palkan · 2015-02-19T21:05:55Z

Updated using ActiveRecord::Type.lookup.
store_attribute method added (just a stored version of attribute):

class Account < ActiveRecord::Base
 # this method allows type options
  store_attribute :data, :progress, :float, precision: 2 
end

But I think it's still useful to have shorthand syntax within store_accessor(store_name, *keys, **typed_keys) for simple types.

sgrif · 2015-02-20T02:29:11Z

I'll take a closer look at this tomorrow.

egilburg · 2015-02-20T03:04:57Z

store_attribute should be documented and explained how it's different from store_accessor

palkan · 2015-02-20T06:27:30Z

Updated docs

sgrif · 2015-02-20T15:18:52Z

Why did we need to move this out of _prepare_local_stored_attributes?

You mean smth like _prepare_local_stored_attributes(store_name, keys)? Reasonable.

Oh I see, the diff didn't make it clear that is a new method.

sgrif · 2015-02-20T15:45:39Z

So there's a couple of problems here. First is that the serialize method from the type gets completely ignored, which severely limits the ability to pass custom types (even some of the built in types do important work there. Like integer.). I also don't like that the type is only accessible via the closure for the method definition, which severely limits the ability to build on top of it later.

My personal opinion is that if you get to this point, you should just have separate columns for the values rather than putting it in a store accessor. However, if we do go this route, I think we should take it a step further and promote the accessors to full-blown attributes. (At this point I think anything that defines attr_name and "#{attr_name}=" as methods is a smell that it should be a full-blown attribute)

This gets into a feature that I'm actively working on, which is attribute composers. I'd prefer if we held off on this feature until I've got that API a little bit more fleshed out, but this should be able to be built on top of that a bit more easily. I'm still wrapping up the standard attributes API, so it's just a bit too soon for me to hash out too many specifics on composers, and at this point introducing new code with its own semantics could potentially make implementing that more difficult.

I hope my concerns make sense. Let me know what you think.

sgrif · 2015-02-20T15:49:55Z

Please use doulbe quoted strings consistently.

palkan · 2015-02-20T16:01:09Z

Totally agree.
I was thinking of the way to implement this feature using Attributes API (namely, attribute decoration).
But I could not find any simple solution, which doesn't require Attributes API refactoring. The main problem was that AttrAPI provides only the way to replace default type with another one, but in case of stores it means that the new type should know about underlying store realization.
So we need kind of attribute wrapping, not substitution. Is it what your composers about?

sgrif · 2015-02-20T16:02:09Z

Yes, but my previous comment is invalid given #18942 (comment)

palkan · 2015-03-06T19:35:44Z

I came up with the following:

TypedStore type which handles store keys type casting. We use it to decorate store type, when adding store attributes.
TypedStore also wraps underlying store accessor and provide type casting on write.

So, type casting occurs:

when deserializing data from db;
when updating store itself from user input;
when writing values through accessors.

/cc @sgrif

palkan · 2015-03-27T15:45:56Z

ping @sgrif @rafaelfranca

claudiob · 2015-12-29T12:52:05Z

This gets into a feature that I'm actively working on, which is attribute composers.

@sgrif is this something you are still considering? I know many things changed since February, and I was wondering what you think now about this ticket. Thanks and happy holidays! 🎄

palkan · 2016-07-02T17:27:12Z

Extracted to separate gem https://github.com/palkan/store_attribute

prathamesh-sonpatki reviewed Feb 14, 2015
View reviewed changes

Comment thread activerecord/lib/active_record/store.rb Outdated

prathamesh-sonpatki Feb 14, 2015

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

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

keys + typed_keys.keys

palkan force-pushed the feature-store-type-cast branch from be754ee to 9fd3a39 Compare February 14, 2015 17:35

egilburg reviewed Feb 15, 2015
View reviewed changes

rafaelfranca added the activerecord label Feb 18, 2015

rafaelfranca assigned sgrif Feb 18, 2015

palkan force-pushed the feature-store-type-cast branch 2 times, most recently from 5af3b71 to 2892472 Compare February 19, 2015 20:51

palkan force-pushed the feature-store-type-cast branch from 2892472 to 11df2e1 Compare February 20, 2015 06:27

sgrif reviewed Feb 20, 2015
View reviewed changes

Comment thread activerecord/test/cases/store_test.rb Outdated

sgrif Feb 20, 2015

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

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

Please use doulbe quoted strings consistently.

palkan force-pushed the feature-store-type-cast branch from 11df2e1 to a92447f Compare March 6, 2015 13:02

palkan force-pushed the feature-store-type-cast branch from 87542ff to ea7094b Compare March 6, 2015 20:20

palkan force-pushed the feature-store-type-cast branch 3 times, most recently from df60c9b to 1744e39 Compare March 10, 2015 21:44

rafaelfranca added this to the 5.0.0 milestone Mar 27, 2015

palkan force-pushed the feature-store-type-cast branch from 1744e39 to 4917117 Compare June 18, 2015 08:51

palkan added 2 commits December 1, 2015 14:12

Add TypedStore type; add store_attribute method

874590e

Fix delegate type

394ea9d

palkan force-pushed the feature-store-type-cast branch from 4917117 to 394ea9d Compare December 1, 2015 11:42

rafaelfranca removed this from the 5.0.0 [temp] milestone Apr 5, 2016

palkan closed this Jul 2, 2016

palkan mentioned this pull request Oct 27, 2016

Add types to ActiveRecord::Store accessors #26882

Closed

Uh oh!

Conversation

palkan commented Feb 14, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prathamesh-sonpatki commented Feb 14, 2015

Uh oh!

simi commented Feb 14, 2015

Uh oh!

palkan commented Feb 14, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

palkan commented Feb 19, 2015

Uh oh!

sgrif commented Feb 20, 2015

Uh oh!

egilburg commented Feb 20, 2015

Uh oh!

palkan commented Feb 20, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sgrif commented Feb 20, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

palkan commented Feb 20, 2015

Uh oh!

sgrif commented Feb 20, 2015

Uh oh!

palkan commented Mar 6, 2015

Uh oh!

palkan commented Mar 27, 2015

Uh oh!

claudiob commented Dec 29, 2015

Uh oh!

palkan commented Jul 2, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants