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

Add rails 7.0 support and drop rails 6.1 #150

Merged
merged 7 commits into from
Jul 31, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,19 @@ The versioning of this gem follows ActiveRecord versioning, and does not follow

## [Unreleased]

## [7.0.0] - 2024-08-01

* Use Arel.literal [#154](https://github.com/ManageIQ/activerecord-virtual_attributes/pull/154)
* drop attribute_builder [#153](https://github.com/ManageIQ/activerecord-virtual_attributes/pull/153)
* dropped virtual_aggregate [#152](https://github.com/ManageIQ/activerecord-virtual_attributes/pull/152)
* resolve rubocops (also fix bin/console) [#151](https://github.com/ManageIQ/activerecord-virtual_attributes/pull/151)
* Rails 7.0 support / dropping 6.1 [#150](https://github.com/ManageIQ/activerecord-virtual_attributes/pull/150)
* condense includes produced by replace_virtual_fields [#149](https://github.com/ManageIQ/activerecord-virtual_attributes/pull/149)
* fix bin/console [#148](https://github.com/ManageIQ/activerecord-virtual_attributes/pull/148)
* Rails 7.0 support pt1 [#146](https://github.com/ManageIQ/activerecord-virtual_attributes/pull/146)
* Fix sqlite3 v2 and rails [#140](https://github.com/ManageIQ/activerecord-virtual_attributes/pull/140)
* Use custom Arel node [#114](https://github.com/ManageIQ/activerecord-virtual_attributes/pull/114)

## [6.1.2] - 2023-10-26

* Fix bind variables for joins with static strings [#124](https://github.com/ManageIQ/activerecord-virtual_attributes/pull/124)
Expand Down Expand Up @@ -93,7 +106,8 @@ The versioning of this gem follows ActiveRecord versioning, and does not follow
* Initial Release
* Extracted from ManageIQ/manageiq

[Unreleased]: https://github.com/ManageIQ/activerecord-virtual_attributes/compare/v6.1.2...HEAD
[Unreleased]: https://github.com/ManageIQ/activerecord-virtual_attributes/compare/v7.0.0...HEAD
[7.0.0]: https://github.com/ManageIQ/activerecord-virtual_attributes/compare/v6.1.2...v7.0.0
[6.1.2]: https://github.com/ManageIQ/activerecord-virtual_attributes/compare/v6.1.1...v6.1.2
[6.1.1]: https://github.com/ManageIQ/activerecord-virtual_attributes/compare/v6.1.0...v6.1.1
[6.1.0]: https://github.com/ManageIQ/activerecord-virtual_attributes/compare/v3.0.0...v6.1.0
Expand Down
2 changes: 1 addition & 1 deletion Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

source "https://rubygems.org"

gem "activerecord", "~> 6.1.7"
gem "activerecord", "~>7.0.8"
gem "mysql2"
gem "pg"
gem "sqlite3", "< 2"
Expand Down
2 changes: 1 addition & 1 deletion activerecord-virtual_attributes.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ Gem::Specification.new do |spec|

spec.require_paths = ["lib"]

spec.add_runtime_dependency "activerecord", ">=6.1.7.8", "<7.1"
spec.add_runtime_dependency "activerecord", "~> 7.0"

spec.add_development_dependency "byebug"
spec.add_development_dependency "database_cleaner-active_record", "~> 2.1"
Expand Down
2 changes: 1 addition & 1 deletion lib/active_record/virtual_attributes/version.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
module ActiveRecord
module VirtualAttributes
VERSION = "6.1.2".freeze
VERSION = "7.0.0".freeze
end
end
153 changes: 56 additions & 97 deletions lib/active_record/virtual_attributes/virtual_fields.rb
Original file line number Diff line number Diff line change
Expand Up @@ -124,20 +124,12 @@ def assert_klass_has_instance_method(klass, instance_method)
raise NameError, msg
end

if ActiveRecord.version >= Gem::Version.new(7.0) # Rails 7.0 expected methods to patch
%w[
grouped_records
].each { |method| assert_klass_has_instance_method(ActiveRecord::Associations::Preloader::Branch, method) }
elsif ActiveRecord.version >= Gem::Version.new(6.1) # Rails 6.1 methods to patch
%w[
preloaders_for_reflection
preloaders_for_hash
preloaders_for_one
grouped_records
].each { |method| assert_klass_has_instance_method(ActiveRecord::Associations::Preloader, method) }
end
# Expect these methods to exist. (Otherwise we are patching the wrong methods)
%w[
grouped_records
preloaders_for_reflection
].each { |method| assert_klass_has_instance_method(ActiveRecord::Associations::Preloader::Branch, method) }

# Expected methods to patch on any version
%w[
build_select
arel_column
Expand All @@ -152,102 +144,70 @@ class Base
module Associations
class Preloader
prepend(Module.new {
# preloader.rb active record 6.0
# changed:
# since grouped_records can return a hash/array, we need to handle those 2 new cases
def preloaders_for_reflection(reflection, records, scope, polymorphic_parent)
case reflection
when Array
reflection.flat_map { |ref| preloaders_on(ref, records, scope, polymorphic_parent) }
when Hash
preloaders_on(reflection, records, scope, polymorphic_parent)
else
super(reflection, records, scope)
end
end

# rubocop:disable Style/BlockDelimiters, Lint/AmbiguousBlockAssociation, Style/MethodCallWithArgsParentheses
# preloader.rb active record 6.0
# changed:
# passing polymorphic around (and makes 5.2 more similar to 6.0)
def preloaders_for_hash(association, records, scope, polymorphic_parent)
association.flat_map { |parent, child|
grouped_records(parent, records, polymorphic_parent).flat_map do |reflection, reflection_records|
loaders = preloaders_for_reflection(reflection, reflection_records, scope, polymorphic_parent)
recs = loaders.flat_map(&:preloaded_records).uniq
child_polymorphic_parent = reflection && reflection.respond_to?(:options) && reflection.options[:polymorphic]
loaders.concat Array.wrap(child).flat_map { |assoc|
preloaders_on assoc, recs, scope, child_polymorphic_parent
}
loaders
# preloader is called with virtual attributes - need to resolve
def call
Copy link
Member

@jrafanie jrafanie Jul 30, 2024

Choose a reason for hiding this comment

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

EDIT: where is call being called originally? What's the entrypoint?

Copy link
Member Author

Choose a reason for hiding this comment

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

Great question.

Sure, we do explicitly call it in our specs: https://github.com/kbrock/activerecord-virtual_attributes/blob/allow_rails_7/spec/virtual_includes_spec.rb#L542

But most of the time, it is called directly by rails.

From what I can tell, this is mostly called directly from preload_associations via exec_query.

https://github.com/rails/rails/blob/7-0-stable/activerecord/lib/active_record/relation.rb#L825-L832

I did try patching in that caller, but when we ran the preloader directly, this had issues - as you know since you had to skip those 3 tests before (we were since able to unskip).

So this fixes 18 specs (probably 50+ uses) in virtual_includes_specs like:

expect(Author.includes(:nick_or_name)).to preload_values(:nick_or_name, author_name)

This includes value ends up in a Preloader. As you know, it is not valid. So we need to translate from the old includes values to the proper ones.

Now I did try and just update @associations value directly, but that only worked if there were no additional levels, so many tests were still failing.

This fix called allows us to get more complicated with our associations.
Coincidently, this recursive-ish solution is similar to the v6 changes where we call into self if the value changed. That was purely coincidental but brought me comfort.

# Possibly overkill since all records probably have the same class and associations
# use a cache so we only convert includes once per base class
assoc_cache = Hash.new { |h, klass| h[klass] = klass.replace_virtual_fields(associations) }

# convert the includes with virtual attributes to includes with proper associations
records_by_assoc = records.group_by { |rec| assoc_cache[rec.class] }
# if these are the same includes, then do the preloader work
return super if records_by_assoc.size == 1 && records_by_assoc.keys.first == associations

# for each of the associations, run a preloader
records_by_assoc.each do |klass_associations, klass_records|
next if klass_associations.blank?

Array[klass_associations].each do |klass_association| # rubocop:disable Style/RedundantArrayConstructor
# this calls back into itself, but it will take the short circuit
Preloader.new(:records => klass_records, :associations => klass_association, :scope => scope).call
end
}
end

# preloader.rb active record 6.0
# changed:
# passing polymorphic_parent to preloaders_for_reflection
def preloaders_for_one(association, records, scope, polymorphic_parent)
grouped_records(association, records, polymorphic_parent)
.flat_map do |reflection, reflection_records|
preloaders_for_reflection(reflection, reflection_records, scope, polymorphic_parent)
end
end

# preloader.rb active record 6.0, 6.1
def grouped_records(orig_association, records, polymorphic_parent)
h = {}
records.each do |record|
# The virtual_field lookup can return Symbol/Nil/Other (typically a Hash)
# so the case statement and the cases for Nil/Other are new

# each class can resolve virtual_{attributes,includes} differently
association = record.class.replace_virtual_fields(orig_association)
# 1 line optimization for single element array:
association = association.first if association.kind_of?(Array) && association.size == 1

case association
when Symbol, String
reflection = record.class._reflect_on_association(association)
next if polymorphic_parent && !reflection || !record.association(association).klass
when nil
next
else # need parent (preloaders_for_{hash,one}) to handle this Array/Hash
reflection = association
end
(h[reflection] ||= []) << record
end
h
end
# rubocop:enable Style/BlockDelimiters, Lint/AmbiguousBlockAssociation, Style/MethodCallWithArgsParentheses
})

class Branch
prepend(Module.new {
# from branched.rb 7.0
def grouped_records
h = {}
polymorphic_parent = !root? && parent.polymorphic?
source_records.each do |record|
# each class can resolve virtual_{attributes,includes} differently
@association = record.class.replace_virtual_fields(association)

# 1 line optimization for single element array:
@association = association.first if association.kind_of?(Array) # && association.size == 1

case association
when Symbol, String
reflection = record.class._reflect_on_association(association)
next if polymorphic_parent && !reflection || !record.association(association).klass
when nil
next
else # need parent (preloaders_for_{hash,one}) to handle this Array/Hash
reflection = association
end
# begin virtual_attributes changes
association = record.class.replace_virtual_fields(self.association)
# end virtual_attributes changes

reflection = record.class._reflect_on_association(association)
next if polymorphic_parent && !reflection || !record.association(association).klass
(h[reflection] ||= []) << record
end
h
end

# branched.rb 7.0
def preloaders_for_reflection(reflection, reflection_records)
reflection_records.group_by do |record|
# begin virtual_attributes changes
needed_association = record.class.replace_virtual_fields(association)
jrafanie marked this conversation as resolved.
Show resolved Hide resolved
# end virtual_attributes changes

klass = record.association(needed_association).klass

if reflection.scope && reflection.scope.arity != 0
# For instance dependent scopes, the scope is potentially
# different for each record. To allow this we'll group each
# object separately into its own preloader
reflection_scope = reflection.join_scopes(klass.arel_table, klass.predicate_builder, klass, record).inject(&:merge!)
end

[klass, reflection_scope]
end.map do |(rhs_klass, reflection_scope), rs|
preloader_for(reflection).new(rhs_klass, rs, reflection, scope, reflection_scope, associate_by_default)
end
end
})
end if ActiveRecord.version >= Gem::Version.new(7.0)
end
end
end

Expand All @@ -270,9 +230,8 @@ def build_select(arel)
end
end

# from ActiveRecord::QueryMethods (rails 5.2 - 6.0)
# TODO: remove from rails 7.0
def arel_column(field, &block)
# from ActiveRecord::QueryMethods (rails 5.2 - 7.0)
def arel_column(field)
if virtual_attribute?(field) && (arel = table[field])
arel
else
Expand Down
4 changes: 4 additions & 0 deletions spec/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,7 @@
end
end
end

require "active_record"
puts
puts "\e[93mUsing ActiveRecord #{ActiveRecord.version}\e[0m"
8 changes: 3 additions & 5 deletions spec/virtual_includes_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -243,9 +243,7 @@
expect(books.last.author).to be_nil # the book just created does not have an author

# the second time preloading throws an error
preloader = ActiveRecord::Associations::Preloader.new
preloader.preload(books, :author => :books)

preloaded(books, :author => :books)
expect(books.size).to be(4)
end
end
Expand Down Expand Up @@ -548,8 +546,8 @@
end

def preloaded(records, associations, preload_scope = nil)
preloader = ActiveRecord::Associations::Preloader.new
preloader.preload(records, associations, preload_scope)
# Rails 7+ interface, see rails commit: e3b9779cb701c63012bc1af007c71dc5a888d35a
ActiveRecord::Associations::Preloader.new(:records => records, :associations => associations, :scope => preload_scope).call
records
end
end