Rails 3.2.1 has_many :through does not save join-data correctly #5057

Closed
tilo opened this Issue Feb 16, 2012 · 21 comments

10 participants

@tilo

if you use :conditions in a has_many :through relationship, it does not set the data in the join-model correctly.
e.g. a join-model "Authorships" which also stores a "role" attribute:

class Author < ActiveRecord::Base
  has_many :authorships
  has_many :books,    :through => :authorships
end

class Authorship < ActiveRecord::Base
  belongs_to :author
  belongs_to :book
end

class Book < ActiveRecord::Base
  has_many :authorships
  has_many :authors, :through => :authorships

  has_many :co_authors, :through => :authorships, :source => :author, :conditions => { 'authorships.role' => :co_author }
  has_many :main_authors, :through => :authorships, :source => :author, :conditions => { 'authorships.role' => :main_author }
end

This use of :conditions produces the correct SQL for looking up co_authors and main_authors, but does not produce the correct SQL to set the data in the join table.

b = Book.create(:name => "Programming Ruby")
dave = b.main_authors.create(:name => "David Thomas")

begin transaction
  INSERT INTO "authors" ("created_at", "name", "updated_at") VALUES (?, ?, ?)  [["created_at", Thu, 16 Feb 2012 09:46:04 UTC +00:00], ["name", "David Thomas"], ["updated_at", Thu, 16 Feb 2012 09:46:04 UTC +00:00]]
  INSERT INTO "authorships" ("author_id", "book_id", "created_at", "role", "updated_at") VALUES (?, ?, ?, ?, ?)  [["author_id", 1], ["book_id", 1], ["created_at", Thu, 16 Feb 2012 09:46:04 UTC +00:00], ["role", nil], ["updated_at", Thu, 16 Feb 2012 09:46:04 UTC +00:00]]
commit transaction  

The above transaction sets ["role", nil] , instead of ["role", 'main_author"]

:conditions should set a scope for both the creation and look-up of entries -- e.g. scope the creation to set the role correctly.

The look-up however produces the correct SQL: (of course the data is never correctly saved in the join model in the first place)

b = Book.first
b.main_authors

SELECT "authors".* FROM "authors" INNER JOIN "authorships" ON "authors"."id" = "authorships"."author_id" WHERE "authorships"."book_id" = 1 AND ("authorships"."role" = 'main_author')
=> []    # empty because data was not saved correctly in the first place; but SQL for lookup is correct.
@tilo

assuming models like this:

rails g scaffold Author name:string 
rails g scaffold Book name:string edition:integer
rails g model Authorship author_id:integer book_id:integer role:string

I want to be able to set :role on the join-model "Authorship" correctly , depending if I create a co_author or a main_author

@josevalim
Ruby on Rails member
@pixeltrix
Ruby on Rails member

Doesn't the conditions hash need to be nested, e.g:

has_many :main_authors, :through => :authorships, :source => :author, 
  :conditions => { :authorships => { :role => 'main_author' } }
@tilo

@pixeltrix good suggestion.
the form you suggest looks much cleaner, thank you for suggesting that(!), but unfortunately creates the same SQL with ["role", nil] in it. I tried a couple of different styles of writing the :conditions, but all yield the same SQL.

@jonleighton
Ruby on Rails member

The two :conditions forms should both produce the same result (see PredicateBuilder).

@tilo can you provide a repro script please?

@jonleighton
Ruby on Rails member

Ok. This doesn't look like a regression - I get the same result against 3.1.3 and 3.0.11.

@tilo

but it seems to be broken, no?

@jonleighton
Ruby on Rails member

Yeah, I agree it should work.

@tilo

maybe it was introduced by the transition to AREL? e.g. in 3.0 , or it was broken for longer..

@tilo

@tenderlove @jonleighton @tenderlove has anybody looked at this issue?

@glampr

Any news on this issue please?
Does anyone have a workaround or working on a fix?

@glampr

Some thoughts on this issue:

The desired behavior can be fixed only if the :conditions options can be written as a hash.
But what if the :conditions options checks for inequalities (!=, >, <, ...) ? Then the constructor of the join attributes has no way of producing the correct values for the join model.

Can we have a more concise way of submitting attributes on creation of the join records?

@tilo

it's been more than a month since I opened this ticket..

@tenderlove @jonleighton @tenderlove has anybody looked at this issue? any comments?

@glampr

I am using this monkey-patch.
You can use it at your own risk.

module ActiveRecord
  module Associations
    module ThroughAssociation #:nodoc:

      private

        def construct_join_attributes(*records)
          if source_reflection.macro != :belongs_to
            raise HasManyThroughCantAssociateThroughHasOneOrManyReflection.new(owner, reflection)
          end

          join_attributes = {
            source_reflection.foreign_key =>
              records.map { |record|
                record.send(source_reflection.association_primary_key(reflection.klass))
              }
          }

          if options[:source_type]
            join_attributes[source_reflection.foreign_type] =
              records.map { |record| record.class.base_class.name }
          end

          join_attributes = Hash[join_attributes.map { |k, v| [k, v.first] }] if records.count == 1

          if reflection.options[:through].try(:is_a?, Symbol) && reflection.options[:conditions].try(:is_a?, Hash)
            Rails.logger.debug "-- Changing default join attributes from: #{join_attributes.inspect}"
            extra_attributes = reflection.options[:conditions][reflection.options[:through]]
            join_attributes.merge! extra_attributes
            Rails.logger.debug "-- to: #{join_attributes.inspect}"
          end

          join_attributes
        end

    end
  end
end

Just run this code in an initializer file.
As I said earlier, it only works when you specify conditions as a hash.

@HectorMalot

I submitted a very similar issue (I believe it's the same bug) some months ago. (Issue #3548)
I'm just putting this in to get this connected. I hope this'll get fixed soon.

@scottkf

This just came up in an app for me as well. It would be extremely convenient if this were fixed

@beno

I start working with ActiveRecord again for the first time in years and the very first thing I want to do stumbles on this. FML.

@tilo

re-pinging Aaron... @tenderlove Hey Aaron, any chance you could have a look at this Issue? Lookslike AREL related #5057 (comment)

@steveklabnik steveklabnik added a commit to steveklabnik/rails that referenced this issue Sep 16, 2012
@steveklabnik steveklabnik Save join data with has_many :through.
Fixes #5057.

If you have :conditions on a has_many :through, it does not
save the data. This patch fixes that by generating the
correct SQL.
8f026e1
@ernie

So, the interesting thing about this (being discussed in #7661) is that there's already a passing test showing the proper usage to get conditions on the join table:

  def test_associate_with_create_with_through_having_conditions
    impatient_people = posts(:thinking).impatient_people.count
    posts(:thinking).impatient_people.create!(:first_name => 'foo')
    assert_equal impatient_people + 1, posts(:thinking).impatient_people.count
  end

The relevant code from the post.rb model is as follows:

  has_many :skimmers, -> { where :skimmer => true }, :class_name => 'Reader'
  has_many :impatient_people, :through => :skimmers, :source => :person

This, to me, makes perfect sense. The conditions for join model creation exist on the join model's association, not on the hm:t association. Unless @tenderlove or @jonleighton have opposing thoughts, I don't think the existing code is broken, and these tickets could be closed.

/cc @steveklabnik

@steveklabnik
Ruby on Rails member

Yep!

@tenderlove @jonleighton if I'm wrong, please re-open, but I think this makes sense.

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