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

Fixes #47909 when using order or unscope #48861

Merged

Conversation

paulreece
Copy link
Contributor

@paulreece paulreece commented Aug 1, 2023

This fix corrects the logic in both the associated and missing methods. If the reflection.options hash has a key/value pair for :class_name it will use the association. This fixes 48651 which was not returning the correct value when using an enum association and querying for a record that had one key of the enum but was missing another. This also fixes 44964 where ActiveRecord was not properly aliasing self-referencing relations. If the reflection.options doesn’t contain the key/value pair then it will use the reflection.table_name. This fixes 47909 in which a user was trying to find records that were either missing a relation or were missing a relation that was ordered or unscoped or were missing a relation that was using extends in the query which resulted in an ActiveRecord Exception. It also allows for using extends in any of these capacities.

  • [ X] This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • [X ] Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • [ X] Tests are added or updated if you fix a bug or add a feature.
  • [X ] CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@zhuravel
Copy link

zhuravel commented Aug 1, 2023

This breaks the following test (due to checking @scope.values[:extending]):

# frozen_string_literal: true

require "bundler/inline"

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

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

  # Activate the gem you are reporting the issue against.
  if ARGV[0] == "main"
    gem "rails", github: "rails/rails", branch: "main"
  elsif ARGV[0] == "pull_request_48861"
    gem "rails", github: "rails/rails", ref: "refs/pull/48861/head"
  else
    gem "activerecord", "7.0.6"
  end

  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(:books, force: true) do |t|
    t.references :progress
  end

  create_table(:progresses, force: true) do |t|
    t.references :book
    t.integer :percentage, default: 0, null: false
  end
end

module Pagination
  def page(number)
    # pagination code goes here
  end

  def per_page(number)
    # pagination code goes here
  end
end

class ApplicationRecord < ActiveRecord::Base
  self.abstract_class = true

  default_scope do
    extending(Pagination) # See https://apidock.com/rails/ActiveRecord/QueryMethods/extending
  end
end

class Book < ApplicationRecord
  has_one :progress

  scope :without_progress, -> do
    where.missing(:progress)
  end

  scope :completed, -> do
    joins(:progress).merge(Progress.completed)
  end
end

class Progress < ApplicationRecord
  scope :completed, -> do
    where(percentage: 100)
  end
end

class BugTest < Minitest::Test
  def test_bug
    books = Book.order(created_at: :desc) # or `books = Book.unscope(:where)`

    assert_equal 0, books.completed.count
    assert_equal 0, books.without_progress.count
    assert_equal 0, books.without_progress.completed.count
  end
end

Run with:

ruby bug_test.rb pull_request_48861 # fails with code from this pull request
ruby bug_test.rb main # fails with code from Rails main because of https://github.com/rails/rails/pull/48738
ruby bug_test.rb # passes on ActiveRecord v7.0.6

@paulreece
Copy link
Contributor Author

paulreece commented Aug 1, 2023

Yes I noticed that locally this morning too when I was testing other use cases. I'm in the process of building a fix into this. Thanks for bringing it to my attention!

@paulreece paulreece force-pushed the correct_missing_and_associated_behavior branch from 5cde1b8 to d4b9810 Compare August 1, 2023 17:09
@paulreece
Copy link
Contributor Author

paulreece commented Aug 1, 2023

@zhuravel I built in your default_scope extend into my test app which helped narrow it down :). I think this should work for our needs now.

@paulreece paulreece force-pushed the correct_missing_and_associated_behavior branch 3 times, most recently from d793401 to e07651c Compare August 1, 2023 18:57
@rails-bot rails-bot bot added the docs label Aug 1, 2023
@paulreece paulreece force-pushed the correct_missing_and_associated_behavior branch 2 times, most recently from 398181f to f294cf5 Compare August 1, 2023 19:14
@@ -76,7 +76,7 @@ def associated(*associations)
associations.each do |association|
reflection = scope_association_reflection(association)
@scope.joins!(association)
if @scope.values.size > 1
if reflection.options[:class_name] || (reflection.options[:class_name] && @scope.values[:extending])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any case where reflection.options[:class_name] is false but (reflection.options[:class_name] && @scope.values[:extending]) is true?

I don't get why this isn't only reflection.options[:class_name] && @scope.values[:extending]

Copy link
Contributor Author

@paulreece paulreece Aug 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rafael, thanks for the feedback. I had initially thought so because of an earlier iteration of this but I just ran it without the Or clause and it works! Just pushed up a version with the Or removed.

@paulreece paulreece force-pushed the correct_missing_and_associated_behavior branch 2 times, most recently from 283b861 to 44c9728 Compare August 1, 2023 23:47
@@ -127,7 +127,20 @@ def scope_association_reflection(association)
reflection
end
end
# {:order=>[#<Arel::Nodes::Descending:0x0000000108ef8400 @expr=#<struct Arel::Attributes::Attribute relation=#<Arel::Table:0x0000000108618998 @name="books", @
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove these comments?

Copy link
Contributor Author

@paulreece paulreece Aug 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! Apologies and thanks for the feedback!

…s. If the reflection.options hash has a key/value pair for :class_name it will use the association. This fixes 48651 which was not returning the correct value when using an enum association and querying for a record that had one key of the enum but was missing another. This also fixes 44964 where ActiveRecord was not properly aliasing self-referencing relations. If the reflection.options doesn’t contain the key/value pair then it will use the reflection.table_name. This fixes 47909 in which a user was trying to find records that were either missing a relation or were missing a relation that was ordered or unscoped or were missing a relation that was using extends in the query which resulted in an ActiveRecord Exception. It also allows for using extends in any of these capacities.
@paulreece paulreece force-pushed the correct_missing_and_associated_behavior branch from 44c9728 to 4aa339c Compare August 2, 2023 15:47
@eileencodes eileencodes merged commit db6159c into rails:main Aug 3, 2023
9 checks passed
@eileencodes
Copy link
Member

Thank you!

eileencodes added a commit that referenced this pull request Aug 4, 2023
…ed_behavior

Fixes #47909 when using order or unscope
@eileencodes
Copy link
Member

Backported this and #48861 to 7-0-stable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants