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

Fix builds for Rails 7.1.0 #1439

Merged
merged 2 commits into from
Oct 20, 2023

Conversation

yuki24
Copy link
Contributor

@yuki24 yuki24 commented Sep 25, 2023

Right now, Ransack fails to execute on Rails 7.1.0beta1 with the error below:

  1) Ransack::Search#method_missing allows chaining to access nested conditions
     Failure/Error: table        = attr.arel_attribute.relation.table_name

     NoMethodError:
       undefined method `table_name' for #<Arel::Table:...>

This is because the table_name method has been dropped as part of this commit rails/rails@1d98bc5, which has made Rails imcompatible with Ransack.

This PR adds a check for the existence of the table_name method to restore the compatibility.

@yuki24 yuki24 force-pushed the rails-7-1 branch 4 times, most recently from f8da5da to 777a747 Compare September 25, 2023 12:54
@jlupox
Copy link

jlupox commented Sep 26, 2023

Thanks @yuki24.
It doesn't work for me. I have to change to:

table = relation.respond_to?(:table_name) ? relation.try(:table_name) : relation.name

@yuki24
Copy link
Contributor Author

yuki24 commented Sep 26, 2023

@jlupox Thanks for pointing that out! It was embarrassingly broken 😅 But it should be fixed now.

@engineersmnky
Copy link

I would recommend just changing this to

table        = attr.arel_attribute.relation.name

Checking for the existence of table_name is unnecessary overhead as Arel::Table#name has always existed.

@yuki24
Copy link
Contributor Author

yuki24 commented Sep 30, 2023

@engineersmnky That didn't work, as it's not always an Arel::Table object but could be an Arel::TableAlias object, in which case there are 19 failing tests with the error below:

  18) Ransack::Search#method_missing allows chaining to access nested conditions
      Failure/Error: raise "No table named #{table} exists."

      RuntimeError:
        No table named children_people exists.
      # ./lib/ransack/adapters/active_record/context.rb:20:in `type_for'
      # ./lib/ransack/nodes/attribute.rb:35:in `type'
      # ./lib/ransack/nodes/condition.rb:266:in `default_type'
      # ./lib/ransack/nodes/condition.rb:25:in `extract'
      # ./lib/ransack/nodes/grouping.rb:175:in `write_attribute'
      # ./lib/ransack/nodes/grouping.rb:155:in `block in build'
      # ./lib/ransack/nodes/grouping.rb:150:in `each'
      # ./lib/ransack/nodes/grouping.rb:150:in `build'
      # ./lib/ransack/nodes/grouping.rb:146:in `new_grouping'
      # ./lib/ransack/nodes/grouping.rb:95:in `block in groupings='
      # ./lib/ransack/nodes/grouping.rb:94:in `each'
      # ./lib/ransack/nodes/grouping.rb:94:in `groupings='
      # ./lib/ransack/search.rb:106:in `method_missing'
      # ./spec/ransack/search_spec.rb:687:in `block (3 levels) in <module:Ransack>'

@yuki24
Copy link
Contributor Author

yuki24 commented Sep 30, 2023

@engineersmnky The try call is not there any more. It would have been greatly appreciated if you had read the actual code in the PR.

@yuki24
Copy link
Contributor Author

yuki24 commented Oct 5, 2023

Rails 7.1 release is right around the corner, and it would be great if we could get this merged. The tests are all green and the Code Climate check seems trivial (only 1 point of complexity check).

@jayshepherd
Copy link

jayshepherd commented Oct 5, 2023

Rails 7.1 release is right around the corner, and it would be great if we could get this merged. The tests are all green and the Code Climate check seems trivial (only 1 point of complexity check).

Looks like it was this morning.. Mind bumping to 7.1?

@yuki24
Copy link
Contributor Author

yuki24 commented Oct 6, 2023

@jayshepherd The world moves too fast! Just updated the CI config and this should be good now.

@davidwessman
Copy link

Is there something we can help with to move this along? Testing?

@n-rodriguez
Copy link

Hi there! Any news?

@deivid-rodriguez
Copy link
Contributor

Hello! I'll try to merge and get a release out this week, thanks for the patience!

Copy link
Contributor

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@deivid-rodriguez deivid-rodriguez merged commit 301fe48 into activerecord-hackery:main Oct 20, 2023
32 checks passed
@deivid-rodriguez
Copy link
Contributor

Hei, I was expecting to release this week as I said, but I realized I'd like to give @scarroll32 a chance to validate what's going in, and leave whatever feedback he has. So I'd let this PR sit for a couple of days in main before releasing.

Apologies if I created some false expectations there! New ETA around the middle of next week unless Sean gets to it earlier.

@scarroll32 I left a release draft ready in case you want to have a look. There's not much to be released, really.

@scarroll32
Copy link
Member

Looks good to me, thank you @deivid-rodriguez and @yuki24

@deivid-rodriguez
Copy link
Contributor

Awesome, I will release now then. By the way, I sneaked #1447 into the release in the last minute and will include it too!

@iainbeeston
Copy link

Could this be back ported to ransack 3? I'm stuck with ransack 3 until #1403 is resolved

@yuki24 yuki24 deleted the rails-7-1 branch October 23, 2023 23:27
@Augustin-Grenne
Copy link

I'd like to second @iainbeeston request to backport this to ransack 3 as sadly patching 4 proofed insufficient (see my comment) and I'm also waiting on #1403

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

Successfully merging this pull request may close these issues.

10 participants