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

has_many with condition is not scoped on create, only for update #12464

Closed
derveloper opened this issue Oct 8, 2013 · 11 comments
Closed

has_many with condition is not scoped on create, only for update #12464

derveloper opened this issue Oct 8, 2013 · 11 comments

Comments

@derveloper
Copy link

Hi,

using Rails 4.0.0

the guide say this:

If you use a hash-style where option, then record creation
via this association will be automatically scoped using the has.

for code like this

class Customer < ActiveRecord::Base
  has_many :confirmed_orders, -> { where confirmed: true },
                              class_name: "Order"
end

but in my app this is only when updating the model, if i create it, it is not scoped so the confirmed attribute is nil.

My code looks like this:

has_many :group_users, -> { where manager: false }, class_name: 'GroupUser'
has_many :group_managers, -> { where manager: true }, class_name: 'GroupUser'
has_many :users, through: :group_users
has_many :managers, through: :group_managers, source: :user

When i add a User to users or managers via update, the manager attribute is filled correctly. On create, it is not.
Maybe there is bug or I'm doing it wrong.

@pftg
Copy link
Contributor

pftg commented Oct 8, 2013

May you provide test case for this issue by using https://github.com/rails/rails/blob/master/guides/bug_report_templates/action_controller_gem.rb or https://github.com/rails/rails/blob/master/guides/bug_report_templates/action_controller_master.rb

I think there are some duplicated issues, one of them is #11535, but will be great to see your test case to confirm that.

@jyao6
Copy link
Contributor

jyao6 commented Feb 8, 2014

I was not able to replicate this issue (i.e. the scope from the has-many seems to work fine under both create and update). However, I noticed that as mentioned in #11535, the default scope of an object will overwrite the has-many scope, which may or may not be desirable...

i.e. where

class Customer < ActiveRecord::Base
  has_many :confirmed_orders, -> { where confirmed: true },
                              class_name: "Order"
end

if you have a Customer object "buyer",

buyer.confirmed_orders.create

will generate an Order object where "confirmed" is true.

However, if within the Order class we have

class Order < ActiveRecord::Base
  default_scope { where confirmed: false }
end

creating an order through the buyer will have "confirmed" be false rather than true.

@rafaelfranca
Copy link
Member

@jyao6 can you check the behavior of 3.2.x about the default scope?

@jyao6
Copy link
Contributor

jyao6 commented Feb 8, 2014

@rafaelfranca so I checked with 3.2.x, and it seems that the default scope still overrides the has-many scope

So the behavior seems to be consistent, and it's just a matter of deciding what the correct behavior should be. Another mentor raised the issue of such a change affecting the merge operation. What do you think?

@rafaelfranca
Copy link
Member

@jyao6 I think the correct behavior would be doing a merge.

So for example if we have:

class Customer < ActiveRecord::Base
  has_many :confirmed_orders, -> { where confirmed: true },
                              class_name: "Order"
end

class Order < ActiveRecord::Base
  default_scope { where active: true }
end

So buyer.confirmed_orders.create would create an order with confirmed = true and active = true.

But, in this case:

class Customer < ActiveRecord::Base
  has_many :confirmed_orders, -> { where confirmed: true },
                              class_name: "Order"
end

class Order < ActiveRecord::Base
  default_scope { where confirmed: false }
end

buyer.confirmed_orders.create would give precedence to the association scope and create an order with confirmed = true.

But this goes against the ideas discussed at #13875. Specifically at #13875 (comment).

That discussion is about what is the intended behavior of Developer.jamis, where:

class Developer < ActiveRecord::Base
  default_scope { where(name: 'David') }
  scope :jamis, -> { where(name: 'Jamis') }
end

Seems to have a consensus that this call should generate the following query:

SELECT * FROM developers WHERE name = 'David' AND name = 'Jamis'

And it is the same as Developer.where(name: 'David').where(name: 'Jamis').

In our case, if we think that buyer.confirmed_orders.create means Order.where(confirmed: false).where(confirmed: true).create, what should be the value of the confirmed attribute? Since the query would create a WHERE with AND using both values.

@jonleighton thoughts?

@jyao6
Copy link
Contributor

jyao6 commented Feb 9, 2014

@rafaelfranca okay, let me know if there is anything I should look into changing (i.e. do you want me to try to figure out how to create the behavior you described above?)

@rafaelfranca
Copy link
Member

@jyao6 this is a good start. We could use the same behavior of Order.where(confirmed: false).where(confirmed: true).create to our problem here.

@pftg
Copy link
Contributor

pftg commented Feb 9, 2014

@rafaelfranca #11535 resolves this issue, with the same idea, may you recheck it again?

@jonleighton
Copy link
Member

@rafaelfranca I don't have anything particular to add, except that yes I think we should AND both conditions for consistency. the proper way to write this would then be:

class Customer < ActiveRecord::Base
  has_many :confirmed_orders, -> { unscope(where: :confirmed).where confirmed: true },
                              class_name: "Order"
end

(I don't think that will work til 4.1 though.)

@rails-bot
Copy link

This issue has been automatically marked as stale because it has not been commented on for at least
three months.

The resources of the Rails team are limited, and so we are asking for your help.

If you can still reproduce this error on the 4-1-stable, 4-0-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

@rails-bot
Copy link

rails-bot bot commented Jan 18, 2018

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 5-1-stable branch or on master, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants