Skip to content

Conversation

@jrafanie
Copy link
Member

@jrafanie jrafanie commented Oct 7, 2024

@jrafanie jrafanie added wip Work in progress rails7.1 enhancement New feature or request labels Oct 7, 2024
@jrafanie jrafanie mentioned this pull request Oct 7, 2024
56 tasks
def virtual_attribute_alias?(name)
return false unless respond_to?(:attribute_alias?)

attribute_alias?(name) && virtual_attributes_to_define.keys.include?(name)
Copy link
Member Author

Choose a reason for hiding this comment

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

this whole method could be simply:

def virtual_attribute_alias?(name)
  virtual_attribute_alias_names.include?(name)
end

attribute_names
end
names = attribute_names | virtual_attribute_alias_names
names -= column_names if respond_to?(:column_names)
Copy link
Member Author

Choose a reason for hiding this comment

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

@kbrock I believe this is the biggest question for this problem. What exactly is an attribute and what is not. It feels like it should be:

virtual_attributes_to_define.keys

Do you know why we're pulling from attribute_names instead of the list of virtual attributes to define?

Copy link
Member

Choose a reason for hiding this comment

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

I think the end desire was to not have a virtual attributes list but rather just have it as a rails attribute with the added arel attribute

Rails 7.1 removed the table_name alias to name so just use name:
See: https://www.github.com/rails/rails/pull/46864

Rails 7.1 removed the writer for the table_alias so set the instance
variable. We will need to see if there is a way to do this that's less brittle.
See: https://www.github.com/rails/rails/pull/48927
@jrafanie jrafanie closed this Feb 13, 2025
@jrafanie jrafanie reopened this Feb 13, 2025
@jrafanie jrafanie changed the title [WIP] Rails 7.1 support Rails 7.1 support Feb 14, 2025
@jrafanie
Copy link
Member Author

@kbrock please take a look...any other changes needed here?

@kbrock
Copy link
Member

kbrock commented Feb 15, 2025

I like this but my issue is we version virtual attributes by rails version. Master will only be for rails 7.1, so I don't want the version matrix logic.

If you could put in only the field name logic I would say to merge

@jrafanie
Copy link
Member Author

@kbrock ok, pushed... dropped rails 7.0 for master. Dropped ruby 2.7-3.0 and added 3.2-3.4.

@jrafanie jrafanie removed the wip Work in progress label Feb 17, 2025
Comment on lines 276 to 278
to_table = to_table.dup
# use a table alias to not conflict with table name in the primary query
to_table.table_alias = "#{to_table.table_name}_sub"
to_table.instance_variable_set(:@table_alias, "#{to_table.name}_sub")
Copy link
Member

Choose a reason for hiding this comment

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

can we try `to_table = to_table.alias("#{to_table.table_name}_sub")
?

also, maybe comment out the to_table bit to figure out if we even have a test for this

Copy link
Member Author

Choose a reason for hiding this comment

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

also, maybe comment out the to_table bit to figure out if we even have a test for this

Without setting it, it looks like we have tests that depend on it. The last one looks to be testing it.

fails with this:

  1) ActiveRecord::VirtualAttributes::VirtualDelegates delegates to parent (sql)
     Failure/Error: expect(tcs.map(&:x)).to match_array([nil, 4])

       expected collection contained:  [nil, 4]
       actual collection contained:    [nil, nil]
       the missing elements were:      [4]
       the extra elements were:        [nil]
     # ./spec/virtual_delegates_spec.rb:26:in `block (2 levels) in <top (required)>'
     # ./spec/spec_helper.rb:40:in `block (3 levels) in <top (required)>'
     # /Users/joerafaniello/.gem/ruby/3.3.6/gems/database_cleaner-core-2.0.1/lib/database_cleaner/strategy.rb:30:in `cleaning'
     # /Users/joerafaniello/.gem/ruby/3.3.6/gems/database_cleaner-core-2.0.1/lib/database_cleaner/cleaners.rb:34:in `block (2 levels) in cleaning'
     # /Users/joerafaniello/.gem/ruby/3.3.6/gems/database_cleaner-core-2.0.1/lib/database_cleaner/cleaners.rb:35:in `cleaning'
     # ./spec/spec_helper.rb:39:in `block (2 levels) in <top (required)>'

  2) ActiveRecord::VirtualAttributes::VirtualDelegates with has_one :parent delegates to child (sql)
     Failure/Error: expect { expect(tcs.map(&:child_col1)).to match_array([nil, tc.id]) }.to_not make_database_queries

       expected collection contained:  [nil, 2]
       actual collection contained:    [nil, nil]
       the missing elements were:      [2]
       the extra elements were:        [nil]
     # ./spec/virtual_delegates_spec.rb:84:in `block (4 levels) in <top (required)>'
     # /Users/joerafaniello/.gem/ruby/3.3.6/gems/activesupport-7.1.5.1/lib/active_support/notifications.rb:259:in `subscribed'
     # /Users/joerafaniello/.gem/ruby/3.3.6/gems/db-query-matchers-0.14.0/lib/db_query_matchers/make_database_queries.rb:75:in `block (2 levels) in <top (required)>'
     # ./spec/virtual_delegates_spec.rb:84:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:40:in `block (3 levels) in <top (required)>'
     # /Users/joerafaniello/.gem/ruby/3.3.6/gems/database_cleaner-core-2.0.1/lib/database_cleaner/strategy.rb:30:in `cleaning'
     # /Users/joerafaniello/.gem/ruby/3.3.6/gems/database_cleaner-core-2.0.1/lib/database_cleaner/cleaners.rb:34:in `block (2 levels) in cleaning'
     # /Users/joerafaniello/.gem/ruby/3.3.6/gems/database_cleaner-core-2.0.1/lib/database_cleaner/cleaners.rb:35:in `cleaning'
     # ./spec/spec_helper.rb:39:in `block (2 levels) in <top (required)>'

  3) ActiveRecord::VirtualAttributes::VirtualDelegates with has_one :parent uses table alias for subquery
     Failure/Error: expect(sql).to match(/["`]test_classes_[^"`]*["`][.]["`]col1["`]/i)

       expected "SELECT \"test_classes\".\"id\", \"test_classes\".\"col1\", ((SELECT \"test_classes\".\"col1\" FROM \...test_classes\".\"col1\" = \"test_classes\".\"id\" LIMIT 1)) AS \"child_col1\" FROM \"test_classes\"" to match /["`]test_classes_[^"`]*["`][.]["`]col1["`]/i
       Diff:
       @@ -1 +1 @@
       -/["`]test_classes_[^"`]*["`][.]["`]col1["`]/i
       +"SELECT \"test_classes\".\"id\", \"test_classes\".\"col1\", ((SELECT \"test_classes\".\"col1\" FROM \"test_classes\" WHERE \"test_classes\".\"col1\" = \"test_classes\".\"id\" LIMIT 1)) AS \"child_col1\" FROM \"test_classes\""

     # ./spec/virtual_delegates_spec.rb:92:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:40:in `block (3 levels) in <top (required)>'
     # /Users/joerafaniello/.gem/ruby/3.3.6/gems/database_cleaner-core-2.0.1/lib/database_cleaner/strategy.rb:30:in `cleaning'
     # /Users/joerafaniello/.gem/ruby/3.3.6/gems/database_cleaner-core-2.0.1/lib/database_cleaner/cleaners.rb:34:in `block (2 levels) in cleaning'
     # /Users/joerafaniello/.gem/ruby/3.3.6/gems/database_cleaner-core-2.0.1/lib/database_cleaner/cleaners.rb:35:in `cleaning'
     # ./spec/spec_helper.rb:39:in `block (2 levels) in <top (required)>'

Finished in 0.74258 seconds (files took 0.43978 seconds to load)
328 examples, 3 failures, 1 pending

Failed examples:

rspec ./spec/virtual_delegates_spec.rb:22 # ActiveRecord::VirtualAttributes::VirtualDelegates delegates to parent (sql)
rspec ./spec/virtual_delegates_spec.rb:80 # ActiveRecord::VirtualAttributes::VirtualDelegates with has_one :parent delegates to child (sql)
rspec ./spec/virtual_delegates_spec.rb:89 # ActiveRecord::VirtualAttributes::VirtualDelegates with has_one :parent uses table alias for subquery

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, pushed changes to use the public table alias API.

Since they dropped the table_alias writer on the Arel::Table, we're
no longer modifying it so we can drop the dup.
@jrafanie
Copy link
Member Author

@jrafanie
Copy link
Member Author

I think this is ready to go... https://github.com/ManageIQ/activerecord-virtual_attributes/tree/7-0-stable has been cut.

@kbrock kbrock merged commit 7ca0e13 into ManageIQ:master Feb 19, 2025
6 checks passed
@kbrock kbrock self-assigned this Feb 19, 2025
@kbrock kbrock mentioned this pull request Feb 19, 2025
@jrafanie jrafanie deleted the rails71 branch July 18, 2025 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request rails7.1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants