Skip to content

Removing composed_of #6743

Merged
merged 1 commit into from Jun 18, 2012
@steveklabnik
Ruby on Rails member

composed_of is a complicated feature that's rarely used. So let's deprecate it in 3-2 and remove it in master.

Related: #6742

/cc @jonleighton @tenderlove

@josevalim
Ruby on Rails member
@jonleighton
Ruby on Rails member

@josevalim we were thinking that it might be okay to simply remove as it's a seldom used feature and this is a major release. also I think moving to a plugin might not be straightforward because there is interaction between aggregates and various other parts of rails, e.g. finders:

Fare.find(price: Money.new(5))

How strongly do you feel that it should be extracted?

@jonleighton
Ruby on Rails member

@josevalim also note that we were thinking to add deprecations to 3.2

@jonleighton
Ruby on Rails member

an alternative would be to deprecate in 4.0 and remove in 4.1. extracting a plugin seems hard though.

@steveklabnik
Ruby on Rails member

As a note, this is a pretty naive removal, so please, give it a good code review. I'd like to give it a much better commit message before it gets merged too.

I've updated it to remove the AggregateReflection too. I'm pretty sure that this means that the AssociationReflection stuff and MacroReflection stuff can be simplified.

@josevalim
Ruby on Rails member
@iain
iain commented Jun 15, 2012

I use this feature in some projects. I would have no problem with rolling my own implementation, like this:

class Fare
  include ActiveRecord::Model
  def price
    Money.new(super)
  end
end

About the finder case: maybe add a convention for usage in finders? Like "if Arel doesn't know the object, it will try to call the to_query method on it".

@jonleighton
Ruby on Rails member

I don't really think we need to find some alternative implementation for the finder. If they want people can do e.g.

class Fare
  def self.costing(money)
    where(price: money.value)
  end
end
@tomstuart

I use composed_of often — because it's there! — but I agree it has a complex implementation with too much generality and an API which is hard to understand. In any specific case it's usually easy to roll your own specific implementation (per @iain's comment) which would almost always produce clearer code than the "magic" composed_of equivalent.

I believe it would send a useful message to simply remove it rather than extracting it. Namely: this particular abstraction rarely makes things simpler, and you are better off writing plain old Ruby code (which you can understand and maintain) to achieve what you want.

@josevalim
Ruby on Rails member

I like @iain proposal to duck type and call to to_rel (to_arel, to_sql or whatever) in the objects given to where.

@tomstuart agreed. we just need to make the process clear. maybe write a section in the updating guide or release notes that people can refer to.

@tomstuart

@josevalim I think a small dose of before-and-after example code can go a long way. For example, :class_name, :constructor and :mapping are just a contrived and indirect way of arranging a particular call to a constructor method, so why not show how to write the explicit getter that makes that call?

@oriolgual

I'm also OK with removing it. We've used it some times and the result normally it's enough complex to move it to another class/object.

@josevalim
Ruby on Rails member

@tomstuart exactly.

@homakov
homakov commented Jun 15, 2012

composed_of is a complicated feature that's rarely used

@steveklabnik it is true, I barely seen it used in projects. But seems composed_of is awesome, very nice sugar.
Thank you so much for opening the issue I will use it since now!

p.s. sure, I'm against of removing it. It is rarely used but should be popular

@iain
iain commented Jun 15, 2012

@homakov I agree that extracting stuff into composed value objects should be popular. However, passing procs in hashes is a pain, and is for me a big reason not to use it. At least not how it's currently implemented.

@jonleighton
Ruby on Rails member

Note also that for single-attribute composed_of usage (e.g. Money), this can be achieved via a customer serializer:

class MoneySerializer
  def dump(money)
    money.value
  end

  def load(value)
    Money.new(value)
  end
end

class Fare
  serialize :price, MoneySerializer.new
end
@homakov
homakov commented Jun 15, 2012

@iain hmm it is really dirty. composed_of can be easily replaced with:

def full_name
  "#{first_name} #{second_name}"
end

also for 1 attribute @jonleighton 's example is not bad. the only profit of composed_of for me - readers. writers(or 'load') are not so useful.

def price
  Money.new read_attribute(:price)
end

is nice too
seems it can be painlessly removed...

@iain
iain commented Jun 15, 2012

There is a problem with a to_arel or to_sql duck typing method. This is when the object wraps multiple fields.

class Fare
  def price
    Money.new(price_value, price_unit)
  end
end

Now what would need to happen if we wanted to call:

Fare.where(price: Money.new(4, "USD"))

Somehow, the key would need to change too. It would need to be converted to:

Fare.where(price_value: 4, price_unit: "USD")

This complexity is a good argument for the solution @jonleighton suggested a couple of comments back. Just create the finder yourself. Another pro of that solution is that the database logic remains inside the AR class.

If you do decide to implement this, then you could also do this for existing objects, like relations.

Now you would have to do:

Employee.where(manager_id: Manager.find(params[:manager_id]))

But when you allow changing the key as well, you could write:

Employee.where(manager: Manager.find(params[:manager_id]))

Still, this sounds complex and magical to me.

@josevalim
Ruby on Rails member
@carlosantoniodasilva
Ruby on Rails member

Although I've already used composed_of in some projects, it's just because it was there. I'm ok with the removal :+1:.

@steveklabnik awesome, thanks for your work on that :)

@iain agreed, seems better to go with @jonleighton's suggestion, create the finder yourself in such cases.

Just want to point that this conversion should happen with the current sanitization code - from the docs:

# Given:
#     class Person < ActiveRecord::Base
#       composed_of :address, :class_name => "Address",
#         :mapping => [%w(address_street street), %w(address_city city)]
#     end
# Then:
#     { :address => Address.new("813 abc st.", "chicago") }
#       # => { :address_street => "813 abc st.", :address_city => "chicago" }
@steveklabnik
Ruby on Rails member

Removed some extra stuff from some extra places.

expand_hash_conditions_for_aggregates seems like it should be removed, but it's used in sanitize_sql_hash_for_conditions, and I'm not sure of the best way to remove it.

@steveklabnik
Ruby on Rails member

Okay, I think I've got it now. That should be removed properly.

@carlosantoniodasilva carlosantoniodasilva commented on the diff Jun 15, 2012
activerecord/lib/active_record/sanitization.rb
@@ -88,8 +88,6 @@ def expand_hash_conditions_for_aggregates(attrs)
# { :address => Address.new("123 abc st.", "chicago") }
# # => "address_street='123 abc st.' and address_city='chicago'"
def sanitize_sql_hash_for_conditions(attrs, default_table_name = self.table_name)
- attrs = expand_hash_conditions_for_aggregates(attrs)
-
@carlosantoniodasilva
Ruby on Rails member

I think you can remove the expand_hash_conditions_for_aggregates method now.

@steveklabnik
Ruby on Rails member
steveklabnik added a note Jun 15, 2012

Ah! Yes. An awkward git checkout. Pushing that now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@steveklabnik
Ruby on Rails member

Some tests used Customer stuff. So I killed all the tests relating to Aggregates, and left in a Customer without aggregate stuff and killed some classes that just had aggregate stuff in it.

@verto
verto commented Jun 15, 2012

I think that plain objects can be an alternative to define complex domain model without exists an AR and composed_of method works for it.

I really love composed_of because i've working with hibernate components for my java projects. what is alternative to use something like this: http://docs.jboss.org/hibernate/orm/3.3/reference/en/html/components.html ?

@steveklabnik
Ruby on Rails member

Rebaed so that it merges cleanly again.

@pnegri
pnegri commented Jun 16, 2012

I still use this feature a lot in multi country ecommerce carts where we use composed_of to handle custom currencies of items. Its a complicated feature, but its a very good one when you learn how to use it.

@supaspoida

I use this feature quite a bit. I agree that it's under documented and overly complex, but would rather it remain in as is than rip it out without establishing some alternate conventions for composition within ActiveModel.

@pnegri
pnegri commented Jun 16, 2012

Also, i dont think these users at #1436, #2084 and #3807 are seeing this as a solution for their issues. Removing a thing doesnt fix bugs.

@rafaelfranca
Ruby on Rails member

@pnegri you don't need to use it. You can use plain ruby methods to archive the same behavior. composed_of is only a poor wrapper to give you:

def price
  @price ||= Money.new(value, currency)
end

def price=(price)
  @price = price

  self[:value] = price.value
  self[:currency] = price.currency
end

One of the solutions for these issues is stop to use composed_of.

Right now this feature is not often used. We have to take into account the trade-off between value and maintainability cost, and the value of this feature is dubious since you can reproduce it easily.

@franckverrot

@pnegri @rafaelfranca The "plain ruby method" way of achieving a replacement for composed_of is still
verbose and thus error-prone.

IMHO it's just a matter of whether we want to make Value Objects first-class citizens in Rails or not, and if
it means removing composed_of I'd love to see this feature either maintained as a plugin as per @josevalim comment or improved (with a "multiple-field" serializer maybe?)

@rafaelfranca
Ruby on Rails member

@cesario yes, I agree that is verbose and error-prone, but It is easier to understand and customize, without use options like :converter and :constructor that you never understand what they means, or how to use.

And, yes, I agree that is a metter of we want to make Value Object's first-class in the Rails, and given the actual user base of this feature, I would say that it is not adding any real value to the framework.

I'll really appreciate if someone could extract this to a plugin, but I don't think that the Rails team should maintain that plugin.

@steveklabnik could you move this forward improving the commit message? You could add some examples how the users can archive this behavior without use the composed_of method.

@steveklabnik
Ruby on Rails member

@rafaelfranca Give me a few hours, I'll get that updated.

@steveklabnik
Ruby on Rails member

@rafaelfranca updated

@steveklabnik steveklabnik Removing composed_of from ActiveRecord.
This feature adds a lot of complication to ActiveRecord for dubious
value. Let's talk about what it does currently:

class Customer < ActiveRecord::Base
  composed_of :balance, :class_name => "Money", :mapping => %w(balance amount)
end

Instead, you can do something like this:

    def balance
      @balance ||= Money.new(value, currency)
    end

    def balance=(balance)
      self[:value] = balance.value
      self[:currency] = balance.currency
      @balance = balance
    end

Since that's fairly easy code to write, and doesn't need anything
extra from the framework, if you use composed_of today, you'll
have to add accessors/mutators like that.

Closes #1436
Closes #2084
Closes #3807
14fc8b3
@rafaelfranca rafaelfranca merged commit 14fc8b3 into rails:master Jun 18, 2012
@WojtekKruszewski

with 9 additions and 768 deletions. -- thanks man. Keeping a framework from getting bloated is really hard work.

@hakanensari hakanensari added a commit to hakanensari/money that referenced this pull request Jun 24, 2012
@hakanensari hakanensari Remove composed_of example in README a66ddfa
@joneslee85

@steveklabnik it is about time to remove it in master

@saurabhnanda

I just discovered composed_of in the Rails docs to subsequently stumble upon this issue. I'm not sure whether this is a deprecated feature or will composed_of live on. Any help?

@rafaelfranca
Ruby on Rails member

In my opinion we should remove it. But right now it will live on.

@steveklabnik
Ruby on Rails member

yup, we want to get rid of it, but not yet.

@hazah
hazah commented Oct 11, 2013

Seems like this is a feature that does not lend itself to many use cases, but for the cases that it's meant for, it's a perfect fit. I do not believe that its prudent to remove an entire interface (which can always be improved, at least in theory) just because the benefits are not immediately obvious. As far as general purpose frameworks go, the competition is in the offered tools. This is a tool. Its only crime here is that the current implementation is still lacking according to this thread.

Furthermore, value objects are an accepted, well defined, concept in of itself. It seems a bit odd to support the entity but not the value as a concept.

@sirfilip

Let's remove everything that is not popular in rails.

@hazah
hazah commented Oct 31, 2013

Popular for whom?

@sirfilip

Honestly removing a feature from rails just because it is not popular it is a bad thing to do. I found about this feature today and i was really happy till a colleague of mine told me that they plan to deprecate this feature. A feature that allows an object to act as a decorator for a property and offers a feature that cant be replaced by anything in rails atm. If you want to remove something well offer a better alternative dont just cripple the framework cause that feature is not popular enough.

@jeremy
Ruby on Rails member
jeremy commented Oct 31, 2013

No worries: composed_of is sticking around. It nails certain cases like money and time zones perfectly.

If we're saying this is deprecated anywhere, please point it out and we'll correct it.

@sirfilip

Thanks a lot guys you just made my day. @pftg thanks for the tip will check it out.

@rafaelfranca
Ruby on Rails member

:+1: It will be sticking around. (and I think I updated the blog post to point this)

@hazah
hazah commented Nov 2, 2013

Good to hear. What I think this discussion brings to light is that it's completely unclear what the actual API should be for the feature. Just about every comment seems to boil down to the fact that it's clunky in it's current state. So then the question is really, what's the expectation for this feature? How far can the integration go? How far should configuration go? What should it all look like? What kind of use cases are encountered regularly (beyond the famous example for money)? Essentially, what are we really missing if we assume this will stay?

@KurtRMueller

Hey all, my apologies for bringing this thread back from the dead. Could I trouble you individuals into pointing me to some recent resources (e.g. blog posts, tuts) regarding composed_of use cases that go beyond money and timezones? If there any learning materials that point out the ugly parts as well, I'd appreciate that too. Thank you!

I can also move comment somewhere else if this isn't the appropriate place.

@hazah
hazah commented Jan 11, 2016

I don't know any materials, and this should probably be asked at stack exchange. A case I can think of is proper names. Maybe international addresses. That sort of thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.