Extend `ActiveRecord::FinderMethods#find` with support for composite pk values #47664

nvasilevski · 2023-03-13T21:58:19Z

ActiveRecord::FinderMethods#find now supports passing sets of composite primary key values like:

Cpk::Book.find([1, 1])
Cpk::Book.find([[1, 1]])
Cpk::Book.find([1, 1], [1, 2])
Cpk::Book.find([[1, 1], [1, 2]])

and treats values as values of the composite primary key columns but only for models with the primary_key being an Array.

The find method has always had a signature that accepts an "identifier" like:

Model.find(ID)
Model.find([ID])
Model.find(ID, ID)
Model.find([ID, ID])

where ID is what identifies a model. It wasn't particularly clear as long as Rails only supported single-column identifies. Though with introduction of the composite primary keys support the meaning of ID as an identifier has extended - now a model can be identified by a set of values and we need to extend find's signature to support that.

Implementation details

The behavior changes only for models with primary_key being an Array which led to having several is_a?(Array) branches and a few duplications like primary_key.zip(values_set) being a repetitive pattern or the way how we build batch-query relation using .inject(&:or) is also repetitive. However at this point I would prefer to introduce duplication to gather as many use-cases as possible and make a refactoring by abstracting these examples into a single concept and having verbose use-cases will only help with finding a proper name and place for the concept.

nvasilevski · 2023-03-13T21:59:52Z

activerecord/test/cases/finder_test.rb

We don't really care about the order of the resulting value, we only care about order being used to hit a different code branch

Maybe so, but multi-find does have a guaranteed return order... seems misleading/confusing to include a definitely-needless sort.

eileencodes · 2023-03-14T13:30:35Z

activerecord/lib/active_record/relation/finder_methods.rb

When we have access to the class we never need to do an is_a?(Array) check.

Suggested change

expects_array = if primary_key.is_a?(Array)

expects_array = if @klass.query_constraints_list

I have mixed feelings about encoding query_constraints_list == multi-keyed. If we can think of a better name, perhaps a primary_key_is_an_array? helper could defer that question?

... or perhaps even @klass.is_this_a_list_of_ids_rather_than_just_one?(ids.first)?

How about @klass.composite_primary_key?

@klass.composite_primary_key? sounds reasonable to me. Do you think it would be more reasonable to have a separate PR to introduce @klass.composite_primary_key? abstraction? We have more places that still do primary_key.is_a?(Array) and I would prefer changing those all at once while also having a separate PR so we can discuss implementation details like whether we should memoize the value or whether the value should be always derived from primary_key.is_a?(Array) vs explicitly setting @composite_primary_key = true once when we derive it from schema

Extract it as a method here, in a separate commit, replacing existing usage as appropriate, but without introducing new logic / caching -- we can consider that part later, but it's fine to do a straight "extract method" here.

Good point 👍

eileencodes · 2023-03-14T13:30:49Z

activerecord/lib/active_record/relation/finder_methods.rb

Suggested change

ids = if primary_key.is_a?(Array)

ids = if @klass.query_constraints_list

eileencodes · 2023-03-14T13:50:12Z

activerecord/lib/active_record/relation/finder_methods.rb

Suggested change

relation = if primary_key.is_a?(Array)

relation = if @klass.query_constraints_list

eileencodes · 2023-03-14T13:50:29Z

activerecord/lib/active_record/relation/finder_methods.rb

Suggested change

relation = if primary_key.is_a?(Array)

relation = if @klass.query_constraints_list

eileencodes · 2023-03-14T13:55:42Z

activerecord/lib/active_record/relation/finder_methods.rb

I don't think this ids.one? is needed, tests pass without it.

Yep, wasn't necessary, thanks!

nvasilevski · 2023-03-14T15:40:44Z

Addressed comments and extracted composite_primary_key? introduction into a separate commit 3acf54e

matthewd · 2023-03-14T16:39:31Z

activerecord/lib/active_record/relation/finder_methods.rb

This feels hard to follow. The flatten made sense before as an easy way of ignoring expects_array... but now I think we can just always respect that:

Suggested change

ids = if klass.composite_primary_key?

expects_array ? ids.first : ids

else

ids.flatten

end

ids = expects_array ? ids.first : ids

Totally agree, the reason I decided to keep flatten for a regular model as it was handling cases like MyNonCpkModel.find([1,[1,2],[3,4,5],[[[6]]]]) which doesn't look like a supported API though it was working and I decided to keep it to avoid breaking some use-case which I'm not aware of.

But if we all agree that find does not support passing non-flattened arrays then ids.first will work for all models. I'll make the change

matthewd · 2023-03-14T16:42:10Z

activerecord/lib/active_record/relation/finder_methods.rb

This, on the other hand, feels avoidably-allocatey for something that's worth keeping tight. Pending a where([] => ..) option, my instinct is that this is probably worth branching on primary_key.is_a?(Array).

Actually that's fair, I totally agree with the argument that having a branch for this context clearly reveals an option to get rid of the branch if we ever support where([] => ..) syntax. I'll add a branch

matthewd · 2023-03-14T16:45:27Z

activerecord/lib/active_record/relation/finder_methods.rb

merge is scary... is ids.map { relation.where(..) }.inject(:or) not a good fit here?

Actually, I think I should have used relation.and(or_scope)

And I also just realized that self.where and relation.where have the same concern I was trying to avoid. If self relation is not an empty relation and has some additional clause (for example a default scope) we will append the clause to every set of statements. For example if default scope is user_type='admin' the resulting query may be something like:

select * from users where (user_type='admin' and id=1 and tenant_id=1) or (user_type='admin' and id=2 and tenant_id=1)

Which won't change the resulting dataset but unnecessarily grows the sql statement

So we should actually use klass.where to get a clean relation to build our OR statements.

Let me double check that

I clearly missed something in my reasoning as user_type='admin' doesn't get duplicated if we just use relation.where(..)

So I simplified it as per your suggestion

I will revisit
https://github.com/rails/rails/blob/4bd3a108cc245383e8b59ea6ae7937ecb3853587/activerecord/lib/active_record/associations/preloader/association.rb#L40-L41

At least to consider using scope.and instead of scope.merge but I do remember having some issues with using scope.where instead of scope.klass.where (clean state)

Eventually I'm hoping we can define an internal helper to build these cross-tenant OR statements and reuse it everywhere. But I've been postponing this until we have more use-cases

eileencodes · 2023-03-14T19:50:56Z

activerecord/lib/active_record/relation/finder_methods.rb

This one is not using the klass.composite_primary_key?

Ah, lost it in a rebase. Fixed, thanks!

eileencodes

This is looking good, I have one last question.

eileencodes · 2023-03-15T13:24:05Z

activerecord/lib/active_record/relation/finder_methods.rb

There doesn't seem to be any tests where these aren't arrays. Is it actually possible for these to not be an array? If not we should remove the Array around them to end up with where(primary_key.zip(id).to_h)

You are right, this wasn't needed. It was added to handle both single-column and composite pks but then we decided to branch

…pk values `ActiveRecord::FinderMethods#find` now supports passing sets of composite primary key values like: ```ruby Cpk::Book.find([1, 1]) Cpk::Book.find([[1, 1]]) Cpk::Book.find([1, 1], [1, 2]) Cpk::Book.find([[1, 1], [1, 2]]) ``` and treats values as values of the composite primary key columns but only for models with the `primary_key` being an `Array`.

Initial implementation falls back to `primary_key.is_a?(Array)` so it can be immediately used in place of direct `is_a?` checks. Though implementation may be changed to rely on a pre-initialized `@composite_primary_key` ivar in the future.

eileencodes · 2023-03-15T14:57:18Z

As a note: for anyone looking at this and wondering why we're doing checks everywhere instead of making a strategy object or using meta programming we're getting the feature in place and then will look at existing patterns to improve internal code.

nickborromeo · 2023-03-15T18:42:38Z

👋 we have a use case that passes in ([[ID, ID]]) to find which already seems to work prior to this change. Does this mean that for this format to work going forward it needs to be on a table that has a composite primary key?

our use case is when we batch queries when running transitions to modify data.

I think the regression is coming from this added logic
b05321d#diff-f35ba011d4738058fa067289f7585a0224ef8ab22efe7a7ecdb07c2fed6940eaR453-R455

since we were flattening the array before but now we aren't

nvasilevski · 2023-03-15T19:03:32Z

since we were flattening the array before but now we aren't

That's right, this is something we discussed in #47664 (comment)

Basically the question boils down to whether .flatten was unintentionally allowing find to accept multi-nested arrays or it was only supposed to unwrap the outer array once like turning [[1,2,3]] into [1,2,3] If it was done unintentionally then [[ID, ID]] format was a non-documented and not supported way of using find and it's a caller's responsibility to flatten the argument value before passing it to the method.

But if multi-nested arrays were intentionally supported I'll be more than happy to bring the flattening back. Perhaps such question has never been raised and now it is the time for us to decide what is the public api for find when it comes to passing an array of values.

nickborromeo · 2023-03-15T19:08:23Z

it was done unintentionally then [[ID, ID]] format was a non-documented and not supported way of using find and it's a caller's responsibility to flatten the argument value before passing it to the method.

But if multi-nested arrays were intentionally supported I'll be more than happy to bring the flattening back. Perhaps such question has never been raised and now it is the time for us to decide what is the public api for find when it comes to passing an array of values

sounds good. we will handle the flattening on our end for now since there is no decision yet on if this should be supported or not.

Active Record uses this method since rails/rails#47664 in some places. Without implementing this method, a spec fails with the following error. ``` NoMethodError: undefined method `composite_primary_key?' for Country:Class # ./lib/active_hash/base.rb:227:in `method_missing' # lib/ruby/gems/3.2.0/gems/activerecord-7.1.1/lib/active_record/reflection.rb:863:in `association_primary_key' # lib/ruby/gems/3.2.0/gems/activerecord-7.1.1/lib/active_record/associations/belongs_to_association.rb:138:in `primary_key' # lib/ruby/gems/3.2.0/gems/activerecord-7.1.1/lib/active_record/associations/belongs_to_association.rb:127:in `replace_keys' # lib/ruby/gems/3.2.0/gems/activerecord-7.1.1/lib/active_record/associations/belongs_to_polymorphic_association.rb:26:in `replace_keys' # lib/ruby/gems/3.2.0/gems/activerecord-7.1.1/lib/active_record/associations/belongs_to_association.rb:98:in `replace' # lib/ruby/gems/3.2.0/gems/activerecord-7.1.1/lib/active_record/associations/singular_association.rb:19:in `writer' # lib/ruby/gems/3.2.0/gems/activerecord-7.1.1/lib/active_record/associations/builder/association.rb:112:in `subject=' # ./spec/active_hash/base_spec.rb:1476:in `block (3 levels) in <top (required)>' ```

rails-bot bot added the activerecord label Mar 13, 2023

nvasilevski force-pushed the support-composite-identifier-in-find branch from d9ef618 to bfdb997 Compare March 13, 2023 21:59

nvasilevski commented Mar 13, 2023

View reviewed changes

eileencodes requested changes Mar 14, 2023

View reviewed changes

nvasilevski force-pushed the support-composite-identifier-in-find branch from bfdb997 to 3acf54e Compare March 14, 2023 15:39

matthewd reviewed Mar 14, 2023

View reviewed changes

nvasilevski force-pushed the support-composite-identifier-in-find branch from 3acf54e to 4bd3a10 Compare March 14, 2023 17:39

eileencodes reviewed Mar 14, 2023

View reviewed changes

nvasilevski force-pushed the support-composite-identifier-in-find branch from 4bd3a10 to 582e78c Compare March 14, 2023 20:04

eileencodes reviewed Mar 15, 2023

View reviewed changes

nvasilevski added 2 commits March 15, 2023 13:44

nvasilevski force-pushed the support-composite-identifier-in-find branch from 582e78c to 7375ff2 Compare March 15, 2023 13:45

eileencodes approved these changes Mar 15, 2023

View reviewed changes

eileencodes merged commit 3b7e413 into rails:main Mar 15, 2023

eileencodes deleted the support-composite-identifier-in-find branch March 15, 2023 14:57

nvasilevski mentioned this pull request Sep 1, 2023

[Docs] Document using FinderMethods.find on composite primary key models #49109

Merged

y-yagi mentioned this pull request Oct 17, 2023

Support Rails 7.1 active-hash/active_hash#291

Merged

Seth-SentrexWind mentioned this pull request Jun 20, 2025

CPK ID is not an usable ID. #55218

Closed

	expects_array = if primary_key.is_a?(Array)
	expects_array = if @klass.query_constraints_list

	ids = if primary_key.is_a?(Array)
	ids = if @klass.query_constraints_list

	relation = if primary_key.is_a?(Array)
	relation = if @klass.query_constraints_list

-        ids = if klass.composite_primary_key?
-          expects_array ? ids.first : ids
-        else
-          ids.flatten
-        end
+        ids = expects_array ? ids.first : ids

Extend ActiveRecord::FinderMethods#find with support for composite pk values #47664

Extend ActiveRecord::FinderMethods#find with support for composite pk values #47664

Uh oh!

Conversation

nvasilevski commented Mar 13, 2023

Implementation details

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nvasilevski commented Mar 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eileencodes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eileencodes commented Mar 15, 2023

Uh oh!

nickborromeo commented Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nvasilevski commented Mar 15, 2023

Uh oh!

nickborromeo commented Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Extend `ActiveRecord::FinderMethods#find` with support for composite pk values #47664

Extend `ActiveRecord::FinderMethods#find` with support for composite pk values #47664

nickborromeo commented Mar 15, 2023 •

edited

Loading

nickborromeo commented Mar 15, 2023 •

edited

Loading