Don't you have to call Books.strict_loading? Or are you expecting that to be the default behavior?

Don't you have to call Books.strict_loading? Or are you expecting that to be the default behavior?

@zzak I forgot to add it in the test script. In the app I had string_loading n_plus_one_only enabled by default at environment config. I have updated to test script to include it here too

@uxxman The changelog entry for :n_plus_one_only says, in part:

Previously, enabling strict loading would cause any lazily loaded association to raise an error. Using n_plus_one_only mode allows us to lazily load belongs_to, has_many, and other associations that are fetched through a single query.

So I think this is behaving as designed. Take a look at 5ec1cac and tell me if you disagree?

@flavorjones from the commit, It looks like it only handles deeply nested associations and not the N+1 query problem in general. Which is misleading from the name of the option.

Reading the original PR #41704, I think your assessment is correct, @uxxman.

I believe the intention is to more fully document this feature before it ships in a release. Would that be sufficient to address your concerns here?

@flavorjones Actually my concern was having an application wide N+1 query problems detection, so developers can catch them during development.

strict_loading with n_plus_one_only mode seemed like a good solution for it but it doesn't hold up to it names, since it only caters 2nd level nested has_many association. May be we can change the option name or add a disclaimer that it doesn't handle all N+1 query problems.

@uxxman 🤔 Can you help me understand why you wouldn't choose to use the default strict mode (:all) to do this?

@flavorjones the issue with mode(:all) is that it raises error for cases that will execute the same set of queries even if they are eager loaded or not. e.g.

With eager loading.

Book.includes(:author).first

# Book Load (0.0ms)  SELECT "books".* FROM "books" ORDER BY "books"."id" ASC LIMIT ?  [["LIMIT", 1]]
# Author Load (0.0ms)  SELECT "authors".* FROM "authors" WHERE "authors"."id" = ?  [["id", 1]]

Without eager loading.

b = Book.first
b.author

# Book Load (0.0ms)  SELECT "books".* FROM "books" ORDER BY "books"."id" ASC LIMIT ?  [["LIMIT", 1]]
# Author Load (0.0ms)  SELECT "authors".* FROM "authors" WHERE "authors"."id" = ?  [["id", 1]]

So, the issue is raised without any difference in the amount of queries being fired.

In the same example eager loading can cause unnecessary queries when there is conditional logic in the view. e.g. only show author if the current_user is logged in.

<% if current_user.present? %>
  <%= book.author %>
<% end %>

Here, eager loading would run 2 queries even if the user is not logged in, but without eager loading only a single query is fired. Correct me if I am wrong 🙈 but the real performance gain comes from strict loading if it is able to detect N+1 issues

Reading the original PR it seems you need to set the strict_loading on the instance of a record.
This works as expected:

# frozen_string_literal: true

require 'bundler/inline'

gemfile(true) do
  source 'https://rubygems.org'

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem 'rails', github: 'rails/rails', branch: 'main'
  gem 'sqlite3'
end

require 'active_record'
require 'minitest/autorun'
require 'logger'

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
ActiveRecord::Base.logger = Logger.new($stdout)

ActiveRecord::Schema.define do
  create_table :authors, force: true do |t|
  end

  create_table :books, force: true do |t|
    t.integer :author_id
    t.integer :library_id
  end

  create_table :libraries, force: true do |t|
  end

end

class ApplicationRecord < ActiveRecord::Base
  self.abstract_class = true
  self.strict_loading_by_default = true
  self.strict_loading_mode = :n_plus_one_only
end

class Author < ApplicationRecord
  has_many :books
end

class Book < ApplicationRecord
  belongs_to :author
  belongs_to :library
end

class Library < ApplicationRecord
  has_many :books
end


class BugTest < Minitest::Test
  def test_association_stuff
    library = Library.create!

    author1 = Author.create!
    author2 = Author.create!

    author1.books << Book.create!(library: library)
    author1.books << Book.create!(library: library)

    author2.books << Book.create!(library: library)
    author2.books << Book.create!(library: library)

    library = Library.first
    library.strict_loading!(mode: :n_plus_one_only)

    assert_raises ActiveRecord::StrictLoadingViolationError do
      library.books.each do |book|
        puts book.author.inspect
      end
    end
  end
end

Thanks for the report and test script!

The :n_plus_one_only mode was only introduced for use on a single record, not at application- or model-level. In other words, it is only supported for the following use (better example in @p8's comment above!):

book.strict_loading!(mode: :n_plus_one_only)
book.author.inspect

The reason it can only operate on a single record is because it uses this assumption to determine when to raise errors. When we are loading associations on a single record, we know that only has_many associations can lead to an N+1. Therefore we turn on strict_loading for has_many associations here. Meanwhile, other association types are allowed to be fetched without raising errors.

If we try to move this to the model or application level, we can no longer make this assumption. All association types are fair game. In your example, a belongs_to association leads to an N+1. So the :n_plus_one_only mode breaks down when we are not using it on a single record.

The bug here is that we should not allow developers to set this instance var at the class level. Your test script should pass because strict_loading_by_default = true and default mode should be :all. (I will work on this patch!)

Correct me if I am wrong 🙈 but the real performance gain comes from strict loading if it is able to detect N+1 issues

This is a good thing to call out. The original intention of adding strict_loading was not necessarily to catch N+1s, but rather to explicitly disallow lazy loading. The mode was added to try to build on top of this functionality to catch N+1s without raising on harmless lazy loads like the case you described above.

Reading the original PR it seems you need to set the strict_loading on the instance of a record.
This works as expected:

@p8 your example involves deeply nested associations just like mentioned in the issue. Library -> Books -> Author. In the issue's original example, Books -> Author leads to an N+1 query problem which never gets detected even if you set it on each book.

assert_raises ActiveRecord::StrictLoadingViolationError do
  Book.all.each do |book|
    book.strict_loading!(mode: :n_plus_one_only)
    puts book.author.inspect
  end
end

This is a good thing to call out. The original intention of adding strict_loading was not necessarily to catch N+1s, but rather to explicitly disallow lazy loading. The mode was added to try to build on top of this functionality to catch N+1s without raising on harmless lazy loads like the case you described above.

@dinahshi understood. That's exactly the discussion which was going on with @flavorjones that the option name :n_plus_one misleads that it solves all N+1 problems, so a different option name or a disclaimer that it only handles one particular case would avoid the confusion.

@uxxman The following will raise an ActiveRecord::StrictLoadingViolationError or am I missing something?

Book.strict_loading.all.each do |book|
  puts book.author.inspect
end

Edit: Ah, sorry.. It's that the :n_plus_one_only doesn't work that's the issue...