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

Preload no longer associates record with owner when custom scope is passed #36638

Open
NoNonsense126 opened this issue Jul 9, 2019 · 13 comments
Open

Comments

@NoNonsense126
Copy link

@NoNonsense126 NoNonsense126 commented Jul 9, 2019

Steps to reproduce

ActiveRecord::Associations::Preloader.new.preload(records, :association, Association.scope)

Expected behavior

Preloaded records when using custom scope should be associated with owner

Actual behavior

Custom scope is loaded into hash #record_by_owner and not associated with owner.

This defeats the purpose of preloading the association with a custom scope.

System configuration

Rails version:
6.0.0rc1

Ruby version:
2.5.1

@NoNonsenseSolutions

This comment has been minimized.

Copy link

@NoNonsenseSolutions NoNonsenseSolutions commented Jul 10, 2019

Functionality broke from PR #35496.

@bogdan could you advise how I would be able to access the custom scope records through the main records?

@bogdan

This comment has been minimized.

Copy link
Contributor

@bogdan bogdan commented Jul 10, 2019

@NoNonsenseSolutions that will be impossible now. This functionality is not officially documented, so I am pretty sure we can not consider this a bug.

What is your use case on having a custom scope for association in particular context?
This functionality breaks OOD because it makes association reader method to behave differently in different context, so I would prefer to find a different workaround for this type of functionality like defining additional association with custom scope or modify association at the instance level.

@NoNonsenseSolutions

This comment has been minimized.

Copy link

@NoNonsenseSolutions NoNonsenseSolutions commented Jul 10, 2019

While I agree that it's undocumented, there's precedent of other users using it. For example, in this blog post. http://aserafin.pl/2017/09/12/preloading-associations-with-dynamic-condition-in-rails/

Another common use case would be to check whether a user has 'liked' a collection of resources.
To load all the resource
Resource.all
To check whether a current user a liked said resource, it's inefficient to do
Resource.all.includes(:likes), as it will load all likes into memory.
Previously, we could use the custom scope to load just the current user's likes.

The current change will cause all applications that were previously using this functionality to break silently. If a custom scope will never be useful, perhaps the change should remove that option from the method and throw an explicit error.

@bogdan

This comment has been minimized.

Copy link
Contributor

@bogdan bogdan commented Jul 10, 2019

I didn't quite get the use case. Can you show me the code that uses Preloader for the purpose you have described as well as the usage of the loaded association?

The current change will cause all applications that were previously using this functionality to break silently. If a custom scope will never be useful, perhaps the change should remove that option from the method and throw an explicit error.

The option is used internally, that is why it is there, not because someone uses it explicitly. Rails will always have breaking changes for undocumented things because they are not part of the test suite and there is no other way to ensure a consistent behavior without it. If you want to relay on that, consider convincing the core team to add the related test case, otherwise write that test on the application side to ensure it never breaks silently.

There are many other ways to fix that:

  • include additional option like force_loading_of_the_association_even_with_custom_scope: true
  • put a warning that the behavior did change

But I just want to confirm that your use case makes sense and that the core team considers this an issue to be addressed.

@NoNonsenseSolutions

This comment has been minimized.

Copy link

@NoNonsenseSolutions NoNonsenseSolutions commented Jul 10, 2019

The blog post i linked has an example

products = Product.all.to_a
ActiveRecord::Associations::Preloader.new.preload(products, :prices, Price.where(currency_code: 'USD'))

products.each do |product|
  product.prices.first.cents
end

This would allow you to preload only the necessary prices

Another example would be

class Post < ApplicationRecord
  has_many :likes

  def liked_by?(user)
    likes.any? {|like| like.user == user}
  end
end

posts = Post.all
ActiveRecord::Associations::Preloader.new.preload(posts, :likes, Likes.where(user_id: current_user))

posts.all.each do |post|
  post.liked_by?(current_user)
end
@bogdan

This comment has been minimized.

Copy link
Contributor

@bogdan bogdan commented Jul 10, 2019

Ok, so if core team would confirm that activerecord needs this feature, I can add a special option like in_memory that would be false by default. We can even avoid the breaking change this way.

@ngouy

This comment has been minimized.

Copy link

@ngouy ngouy commented Sep 26, 2019

have some serious issue here too because of that (especially using / trying to port graphql-preload for rails 6), a gem which is pretty useful and still used by some people

@coorasse

This comment has been minimized.

Copy link
Contributor

@coorasse coorasse commented Oct 31, 2019

We also stumbled upon this issue. Before finding this issue already open I prepared the following gist to reproduce.
I also wrote a blog post to explain the case and how we were using it. This is currently blocking us from migrating to Rails 6.0 at least six applications where we make use of this feature.
I see from @bogdan that this is already considered by the core team, I hope my comment adds some useful details. I'd be also glad to help by opening a PR and add a parameter.

@clausti

This comment has been minimized.

Copy link

@clausti clausti commented Nov 17, 2019

To add another vote and use case for passing a custom scope on the preloader, I ended up here trying to chase down why I can't get the trick to work from the blog post @NoNonsenseSolutions already mentioned.

My use case is to preload, on a collection of Post records, a replies association, but only replies which come from users who aren't blocked by the current_user. I'm writing it as a scope on the Post model, to which the current_user is passed.

@Envek

This comment has been minimized.

Copy link
Contributor

@Envek Envek commented Dec 19, 2019

This also breaks functionality of gems like graphql-preload that uses this feature to allow to customize preloading of associations.

For example I'm using custom scopes to ensure custom ordering of associated records.

class GraphQL::Types::User < GraphQL::Types::BaseObject
  field :accounts, [Graphql::Types::Account], null: false do
    preload :accounts
    preload_scope ::Account.where(deleted: false).order(id: :asc)
    description "Linked accounts"
  end
end

Which will execute something like that if and only if a client will ask for user's accounts in GraphQL query:

ActiveRecord::Associations::Preloader.new.preload([u1, u2], :accounts, Account.where(deleted: false).order(id: :asc))

See ConsultingMD/graphql-preload#27

@palkan

This comment has been minimized.

Copy link
Contributor

@palkan palkan commented Dec 19, 2019

Here is a possible workaround:

::ActiveRecord::Associations::Preloader.new.preload(records, :association, :scope).then(&:first).then do |preloader|
  preloader.send(:owners).each do |owner|
    preloader.send(:associate_records_to_owner, owner, preloader.records_by_owner[owner] || [])
  end
end

Or for those struggling with graphql-batch here is the updated association_loader.rb.

This functionality is not officially documented, so I am pretty sure we can not consider this a bug.

Yeah, Preloader is not a part of the public API. So, I wouldn't expect Core Team consider it a bug as well.
Should we made Preloader public? That's a different question (and I think, the answer should be "Nope").

@kaspth

This comment has been minimized.

Copy link
Member

@kaspth kaspth commented Dec 22, 2019

@palkan exactly 🙌

@Envek

This comment has been minimized.

Copy link
Contributor

@Envek Envek commented Dec 23, 2019

Should we made Preloader public? That's a different question (and I think, the answer should be "Nope").

Well, I can’t agree here. It seems that the association preloader became way too widely adopted to ignore its importance for various gems and projects.

It is hard to think about it as “private” when it was actually “documented” in various blog posts throughout the last years and is being used in many gems (including quite popular ones).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.