madeintandem / hstore_accessor

Adds typed hstore-backed field support to ActiveRecord models.
MIT License
242 stars 47 forks source link

@hstore_accessible_attributes #71

Open Adamantish opened 8 years ago

Adamantish commented 8 years ago

TLDR; A little more metadata please to ease the migration to HStorified data.

When writing generalised code (i.e. a model concern) it's sometimes necessary to apply some kind of check across all attributes in a model. This often requires that the hstore_accessors get some special treatment. Without knowing in advance what our hstore_accessor attributes names are this can only be inferred by searching all 900 or so methods for start_with?('hstore_metadata_for'). Not so efficient when unnecessarily re-called once for every instance. This calls for a pre-load!

Could this be collected into a class attribute reader on the model named something like hstore_accessible_attributes ? Just a flat array of attribute names would be enough. e.g.

class MyModel
  hstore_accessor :my_data,
                              dongle_time: :datetime,
                              dongle_what: :string

  hstore_accessor :separate_things,
                              spangle_quantity: :decimal,
                              spangle_how: :string
end

MyModel.hstore_accessible_attributes # --> [:my_data, :separate_things]

Possible objections I could imagine

  1. But you can get nearly the same thing by looking for DB columns of the hstore type. This doesn't keep out hstore columns you haven't made accessors for.
  2. But isn't this overreaching what the gem is for? It's only for generating accessors. You can get those the same way as any other accessor: by listing all setter methods. Yes, but in practice it is used for augmenting the flat list of persisted attributes. One of the best reasons for using this gem is it keeps this just working even with hstore columns: person.update(flat_params).

Example At work we have a concern which uses attributes.each. We then refactored one of the models which includes that concern to migrate many of those attributes into an hstore column. attributes.each no longer does the job. attributes_hstore_flattened.each would have done the job. Perhaps a method like that would be good, perhaps it's going too far. In any case, we could have neatly done that flattening ourselves if armed with the class instance var requested here.

So to come back to what's wrong with a list of setters? It's not the same thing as a list of persisted flattened attributes and we need a little more metadata to build that.

3 . Adding class instance vars is risky risky business It's good enough for @columns_hash, @default_attributes, @table_name, etc.. they serve a very similar type of role.

Cheers for reading. Now here's where you point out some stupid oversight I've made.