@tlconnor this indeed is a pretty well known bug. The inconsistencies around it are described in #53 and #45 and are one of the reasons I started #141

I don't mind merging this, but I will have to ask you to provide a test-case as well. Would you be kind?! 🙇

@tlconnor Thanks, agreed that something should happen here long-term. I wrote #45 to address it but couldn't get everyone behind that particular implementation.

My concern is that this will be a change in behavior (and therefore a breaking change) for people using id_method_name. If we are intentionally making a breaking change, we should follow proper semantic versioning with a major version bump and deprecation warnings in the meantime.

I'm thinking the right approach would be to remove the part in ObjectSerializer where id_method_name is being computed, so that we can differentiate between when the developer actually specifies it vs. when it's up to the serialization process to automatically determine it. In other words, the Relationship object's id_method_name should be nil if not specified, and then we can safely move the computation into the Relationship object where it belongs. This way, we can focus on improving the way it is automatically computed in a backwards compatible way, without breaking the use case where the developer specifies id_method_name directly.

Last thing. When a relationship is being included for serialization, it's no problem to get the ID from the object and serializer. When it's not being included, it gets more complicated from a performance standpoint, because we don't necessarily want to fetch all the records / models just to get the IDs if it can be avoided. ActiveRecord has a shortcut where, if a Book has_many :authors then we can get the IDs as an array by simply calling book.author_ids. The id_method_name currently leverages this here.

There are many things to consider here if we are going to try to maintain performance and backward compatibility. Happy to assist you and @stas . Some basic pseudo-code might look something like this:

def fetch_ids(record)
  if object_block.nil?
    # if an object block is specified, avoid these quick attempts in favor of fetching the objects below

    # try the id_method_name that was specified by the developer in the relationship's properties
    return record.send(id_method_name) if id_method_name.present? && record.respond_to?(id_method_name)

    # try the conventional relationship_ids method name, e.g. author_ids or publisher_id
    return record.send(relationship_id_method_name) if record.respond_to?(relationship_id_method_name)
  end

  if has_many?
    # iterate over the objects and get the IDs
    rel = fetch_associated_object(record)
    if rel.respond_to?(:map)
      return rel.map { |o| relationship_object_id_for(o) }
    end

    # not sure what else should happen if a has_many relationship does not respond to map ... error?
  end

  if belongs_to? || has_one?
    # iterate over the objects and get the IDs
    rel = fetch_associated_object(record)
    return relationship_object_id_for(rel)
  end
end

def relationship_object_id_for(rel)
   if id_method_name.present?
    return rel.send(id_method_name)
  else
    return serializer_for(rel).id_for(rel)
  end
end

def relationship_id_method_name
  @relationship_id_method_name ||= has_many? "#{name}_ids" : "#{name}_id"
end

The main point of uncertainty is, if the developer specifies id_method_name, whether he means for it to apply to the parent or the child. In my opinion, moving forward, it should only apply to the parent, because the object's serializer should be the single source of truth for determining how to determine the ID of a single object. That's why I advocated for deprecating id_method_name as is and implementing a separate foreign_key field for this purpose moving forward. I don't care what it's named, as long as what it does is clear...

Thanks @stas / @christophersansone for the feedback.

I have updated this PR to make the following changes:

• Backwards compatibility is now maintained. If a user provides an id_method_name, it will continue to be used. This applies to both the foreign key and primary key use cases.
• Foreign key computation is moved into the Relationship class
• If the foreign key can not be found, the serializer is used to look up the ID.
• Explicit test cases are added for the cases where the serializer is now used to look up related IDs.

What this means is the existing behavior is now maintained, except that instead of

undefined method `actor_ids' for #<Movie>

we now use the serializer to look up the IDs.

@tlconnor 👏 Thanks for the updates! I added a couple comments, but in general I think it's as good as it can be if we go down this path. You definitely opened a can of worms though!

My concern is that there still will be edge cases where it causes a breaking change: the order in which the process flows, the removal of the hardcoded :id in some cases, possibly now requiring a serializer that was not required prior to this, and of course the uncertainty of whether id_method_name is intended to be called on the parent or the child.

@stas, how important is it to maintain perfect backward compatibility here, at least until a major version bump? Is there a new major release on the horizon soon?

Ultimately, my opinion is to deprecate id_method_name entirely, in favor of a new parameter that applies only to the parent. We can simply call it id_method. Its job is to simply customize the method that returns the foreign key(s), e.g. book_id, author_ids, etc. If the developer wants to customize the ID method of the child, then they would do that through the child object serializer only. To me, that gives us a nice path forward, notifies the developer of the changes, and ultimately puts things where they belong.

@stas, how important is it to maintain perfect backward compatibility here, at least until a major version bump? Is there a new major release on the horizon soon?

Good question. #141 is pretty much ready for a beta. I'm stuck finding some time to finish up the inline-docs and general documentation. And this issue is already handled fully (:crossed_fingers:) in the that branch. Saying that, I really don't know if I have time and energy to introduce an intermediary significant release and deal with the requests for that.

Honestly, I'd push for #141 as a v.3-beta release, even the way it is now, close a bunch of issues it handles and keep the good work on top of it. But I also don't want to sound like 💩 and not consider the contributions from the community like what @tlconnor is doing (where I apologise for not taking time to look into yet 🙈)...

@stas Sweet, I just did a quick review of #141 and see that id_method_name was removed in favor of ids_method_name. Love it! Looks like a lot of simplification all around. Great work! 👏

@tlconnor Thanks again for tackling this... twice! Because of the potential for breaking changes with certain use cases, I'm thinking #141 with the major version bump is the better option. Are you able to solve your specific issue today using the current version of this library, either with id_method_name or a Proc? If not, can you provide a specific example? Let's definitely make sure this isn't blocking you from getting your work done.

@stas Sweet, I just did a quick review of #141 and see that id_method_name was removed in favor of ids_method_name. Love it! Looks like a lot of simplification all around. Great work! 👏

@tlconnor Thanks again for tackling this... twice! Because of the potential for breaking changes with certain use cases, I'm thinking #141 with the major version bump is the better option. Are you able to solve your specific issue today using the current version of this library, either with id_method_name or a Proc? If not, can you provide a specific example? Let's definitely make sure this isn't blocking you from getting your work done.

I am not blocked, I've added my 2 fixes to my app as monkey patches for now. The main area of concern for me is the non-rails use cases where relationships aren't backed by ActiveRecord.

I'm happy to drop this change in favor of #141, as long as some sort of forward progress is being made there. If you release it as a beta I would be happy to switch to the beta version.

@stas @christophersansone would one of you also be able to look at #154? It is a simpler bug fix that fixes another blocking bug for me.