composite-primary-keys / composite_primary_keys

Composite Primary Keys support for Active Record
1.03k stars 350 forks source link

Saving CPK in a normal PK attribute stopped working after upgrading to Rails 6.1 #602

Closed dmke closed 1 year ago

dmke commented 1 year ago

I'm in the process of updating an older Rails app from 5.2 to 7.0, and I stumbled upon a small(?) issue.

I'm currently on Rails 6.1, CPK 13.0.7, with a PostgreSQL database.

For context, this is a simplified view the current database layout (the interesting bits are highlighted):

image

These are the associations:

class HouseInquiry
  self.primary_keys = %i[inquiry_id house_id]
  belongs_to :inquiry
  belongs_to :house

  has_one :house_owner_billing,
    primary_key: :inquiry_id,
    foreign_key: :inquiry_id,
    inverse_of:  :house_inquiry
end

class HouseOwnerBilling
  belongs_to :inquiry
  belongs_to :owner

  belongs_to :house_inquiry,
    primary_key: :inquiry_id,
    foreign_key: :inquiry_id,
    inverse_of:  :house_owner_billing
end

As you can see, the Ruby code tries to establish a strong connection between HouseOwnerBilling and HouseInquiry by (ab)using the fact that they share the same inquiry_id. In the past, this has worked well:

inq = HouseInquiry.last
inq.build_house_owner_billing.inquiry_id #=> 42
inq.build_house_owner_billing.inquiry_id == inq.id #=> true

The upgrade to Rails 6.1 has now revealed a change:

inq = HouseInquiry.last
inq.build_house_owner_billing.inquiry_id #=> nil

I've found ActiveRecord::Associations::HasOneAssociation#replace to be the culprit (called by build_house_owner_association(options)association("house_owner_billing").build(options)set_new_record(record)replace(record, false)), which does reset the owner:

def replace(record, save = true)
  # ...
  if assigning_another_record || record.has_changes_to_save?
    # ...
    transaction_if(save) do
      # ...
      if record
        # record.inquiry_id.nil? == false
        set_owner_attributes(record)
        # record.inquiry_id.nil? == true

and in ActiveRecord::Associations::ForeignAssociation#set_owner_attributes:

def set_owner_attributes(record)
  # ...
  key = owner._read_attribute(reflection.join_foreign_key)
  record._write_attribute(reflection.join_primary_key, key)

My current workaround is to "enhance" build_house_owner_billing, but that feels wrong:

def build_house_owner_billing
  super.tap { _1.inquiry_id ||= inquiry_id }
end

Do you have any suggestions as how to proceed from here?

dmke commented 1 year ago

Turns out, there must be something else borked... I've build a reproduction, which does what it should on all Rails versions so far (5.2, 6.0, 6.1).

Sorry for the noise.

cfis commented 1 year ago

No worries - glad you figured it out!

dmke commented 1 year ago

In the end, there was a misconfiguration: We have an Inquirable module mixed which (a) got mixed in the *Inquiry classes, and (b) dynamically creates the association based on the name of the class it was mixed in (e.g. HouseInquiry → belongs_to :house, has_one :house_owner_billing etc.).

The actual problem was a double declaration of the house_owner_billing (for whatever reason; once in the mixin, once in the class itself). This didn't bother Rails < 6.1, but since 6.1 seems to be an error.