Breaks with Rails 5.2.1

[wollistik]

Issue

With latest Rails 5.2.1 baby_squeel breaks.

Reproduction

require 'bundler/inline'
require 'minitest/spec'
require 'minitest/autorun'

gemfile true do
  source 'https://rubygems.org'
  gem 'activerecord', '5.2.1' # which Active Record version?
  gem 'sqlite3'
  gem 'baby_squeel', github: 'rzane/baby_squeel'
end

ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')

ActiveRecord::Schema.define do
  create_table :humans, force: true do |t|
    t.string :name
  end
  create_table :dogs, force: true do |t|
    t.string :name
    t.references :user
  end
end

class Human < ActiveRecord::Base
end

class Dog < ActiveRecord::Base
belongs_to :human
end

class BabySqueelTest < Minitest::Spec
  it 'breaks' do
    Dog.joins(:human).to_sql
  end
end

[sasurai-usagi3]

In activerecord 5.2.1, ActiveRecord:: Associations:: JoinDependency#join_constraints was changed from 2 arguments to 3 arguments. And Polyamorous which BabySqueel depends on insert ActiveRecord:: Associations:: JoinDependency to 2 arguments join_constraints method. So it causes error.

Polyamorous is absorbed to Ransack, and original Polyamorous won't maintain...

In Polyamorous's README

Polyamorous is merged into Ransack since Squeel and MetaSearch is not maintained anymore.

[kevin-at-corista]

It sseems like it would be easy to work around Polyamorous not being maintained (for now) by requiring Ransack instead, right?

That's not the only problem. The join_dependency gem also doesn't work in 5.2.1, although I have a pending PR to fix that problem: rzane/join_dependency#2

[kevin-at-corista]

It seems even fixing both the Polyamorous requirements and the join_dependency gem, problems still remain. Running the tests gives me:

      Failure/Error: ::Arel::Table.new(table.name, type_caster: type_caster)

      NoMethodError:
        undefined method `name' for nil:NilClass
      # ./lib/baby_squeel/join_dependency.rb:74:in `reconstruct_with_type_caster'
      # ./lib/baby_squeel/join_dependency.rb:39:in `find_alias'
      # ./lib/baby_squeel/table.rb:89:in `find_alias'
      # ./lib/baby_squeel/association.rb:63:in `find_alias'
      # ./lib/baby_squeel/association.rb:63:in `find_alias'
      # ./lib/baby_squeel/nodes/attribute.rb:30:in `_arel'
      # ./lib/baby_squeel/nodes/proxy.rb:28:in `method_missing'
      # ./lib/baby_squeel/operators.rb:17:in `block in arel_alias'
      # ./spec/integration/where_chain_spec.rb:58:in `block (3 levels) in <top (required)>'
      # ./lib/baby_squeel/table.rb:76:in `instance_eval'
      # ./lib/baby_squeel/table.rb:76:in `evaluate'
      # ./lib/baby_squeel/dsl.rb:9:in `evaluate'
      # ./lib/baby_squeel/active_record/where_chain.rb:8:in `has'
      # ./spec/integration/where_chain_spec.rb:57:in `block (2 levels) in <top (required)>'

As well as several situations where baby_squeel expects there to be a LEFT_OUTER_JOIN but generates sql as an INNER_JOIN, a problem also reported in Ransack: activerecord-hackery/ransack#955

[obfuscoder]

I stumbled upon this issue after trying to migrate my application from Rails 4 to 5.2.1. Whew, having polyamorous not being maintained anymore (pulled into ransack), is pretty tough. I noticed that @rzane created an issue in polyamorous back in March adressing this (activerecord-hackery/polyamorous#38), but there hasn't been any response yet. That project looks dead to me.

@rzane Any plans on how to proceed? Pulling polyamorous (or the fixed code from within ransack) into baby_squeel or join_dependency? I'm currently at the point of going back to Rails std query interface with arel. I do like baby_squeel very much, but would like to have a way forward pretty soon.

[rzane]

Yeah, this is definitely a setback. I think I would most likely move the polyamorous codebase into join_dependency. Or, if it's possible and easier, just depend on ransack.

[rogierslag]

Any more recent updates on this?

[rzane]

Okay, so I started digging in here. Polyamorous becoming part of Ransack is inconvenient, but not a dealbreaker. Baby Squeel can simply depend on Ransack in order to bring in Polyamorous.

However, it really sucks when this library breaks when Active Record changes. And, digging through the Active Record source code, duplicating private Active Record functionality isn't very future proof.

So, I'm proposing a change to Baby Squeel that I hope will remove this problem. Basically, 99% of the complexity comes from predicting a table alias. It's a cool feature, but I don't think it's worth the maintenance effort or the lack of reliability.

When the same table is hit twice in the same query, Active Record will automatically create an alias for that table. In the example below, see the posts table is joined as posts_authors?

irb> Post.joins(author: :posts).where.has { author.posts.id > 0 }
SELECT "posts".* FROM "posts"
INNER JOIN "authors" ON "authors"."id" = "posts"."author_id"
INNER JOIN "posts" "posts_authors" ON "posts_authors"."author_id" = "authors"."id"
WHERE "posts_authors"."id" > 0

In the example above, Baby Squeel had to figure out that authors.posts.id references posts_authors. In future versions of Baby Squeel, you'll have to explicitly say:

>  Post.joins(author: :posts).where.has { author.posts.as('posts_authors').id > 0 }

I'm going to release a new minor version that will give you a deprecation warning in any scenario where the "automatic aliasing" kicked in. It'll describe exact steps for how you can remedy your code.

Once that's released, I'm going to release Baby Squeel 2.0. This version should be able to support 5.2.1 and won't automatically predict aliases anymore.

[chewi]

So, I'm proposing a change to Baby Squeel that I hope will remove this problem. Basically, 99% of the complexity comes from predicting a table alias. It's a cool feature, but I don't think it's worth the maintenance effort or the lack of reliability.

To me, that was of the main benefits of Baby Squeel. Preloaded associations are straightforward enough but eager loaded association aliases are hard to predict. My opinion isn't worth too much though, I've just left the company where I was using Baby Squeel.

[atomaka]

As @chewi outlines, predicting eager loaded association aliases is difficult and is the reason I pull baby_squeel into a project. I'm probably in the minority, but it's the only functionality I care about that is provided by the gem.

The complexity you outline is real though and if the primary audience of this gem doesn't rely on the feature, it makes sense to remove this functionality.

[wollistik]

We introduced baby_squeel because we have a lot of complex queries which were mostly written in plain SQL. Transforming them to plain ActiveRecord Relations was simply not possible and using Arel was far too complex for such huge queries. baby_squeel does this now for us and we are quite happy 🎉

Loosing one of the main features of baby_squeel is really a setback - but understandable 😞

@rzane i am willing to help if you have some ideas, because we rely on this awesome gem!

[rzane]

Yeah, I'm revisiting my previous statements. Now that ransack has to be a dependency, we might be able to take advantage of how they resolve the aliases. Since Ransack is pretty actively maintained, this would allow us greater reliability.

I don't have a lot of time right now, so I would love it if somebody would step up and try to achieve this. I can push up a branch of what I currently have soon.

[vintrepid]

I forked the polyamorous gem and integrated the latest updates from the ransack gem. This is currently working for me on rails 5.2.1.1, although I am not using a lot of the baby_squeel features.

https://github.com/vintrepid/polyamorous

And in my Gemfile:

gem 'polyamorous', "~> 1.3.4", git: 'https://github.com/vintrepid/polyamorous.git'

[wollistik]

Hi @vintrepid ,
problem is, that ransack/polyamorous has a bug with rails 5.2.1 and up (see activerecord-hackery/ransack#955). That breaks baby_squeels tests (for good), so it is not safe to use this solution for now.

[rzane]

@wollistik is right. I was able to get polyamorous back by bringing in Ransack as a dependency, but once I did that, all of the tests were failing for 5.2 because it would use inner join instead of left join.

I'd like to be able to isolate the problem, because I'm not 100% certain whether it's a problem with baby_squeel's usage of polyamorous or with polyamorous itself.

[scarroll32]

Polyamorous was absorbed into Ransack as we are trying to simplify the support, and the general policy of Ransack has been to drop older versions of Rails. There are 3 options here:

1. Include Ransack into Baby Squeel to gain access to Polyamorous. This works of course, but it's bringing in a lot of unneeded code.
2. Absorb Polyamorous into Baby Squeel directly
3. Re-separate and refactor Polyamorous

In my mind #3 is the best for code reuse, but it will also require the most work to maintain, esp. as Baby Squeel and Ransack have different support policies for older Rails versions. On the other hand if we join forces we could pool our programming time better.

As an extension of option 3, we could push common functionality out of both Ransack and Baby Squeel into a Polyamorous+

Thoughts @gregmolnar, @rzane ?

[rzane]

Thanks for chiming in @seanfcarroll. I think bringing polyamorous into Ransack made sense for the following reasons:

1. People know where to report Ransack issues.
2. You can version them together, so you don't end up in a situation where people are using incompatible versions.
3. Keeping related code in the same repo makes development/debugging simpler.

My opinion here is that option #1 is certainly not ideal, but doable. The situation that I'm afraid of here would be the reliance on Ransack's test suite to ensure that Polyamorous still works, which I suspect might be currently happening. As a result, I'm afraid that Polyamorous might not be doing what it's supposed to be doing, but it's really hard for me to demonstrate that.

Option #2 seems like it would be a disaster. The problem there is that if someone were to install Ransack and Baby Squeel, we'd patch Active Record twice? Plus, the obvious maintenance burden.

Option #3 sounds really great to me. But, I don't want to make Ransack harder to maintain. There were reasons that polyamorous was brought into Ransack.

Maybe Polyamorous could stay in the Ransack repo, but it would exist as a separate gem? Then, you could test/version it alongside Ransack. This is basically what Rails does.

From there, we could work on improving the tests and documentation for Polyamorous. Polyamorous previously had a few unit tests, but I don't think they were very effective in preventing regression. I think we need some higher level tests that actually assert on the resulting SQL, which has been a pretty effective strategy for Baby Squeel.

I definitely think there is a lot of overlap between Ransack and Baby Squeel, specifically in resolving aliases that never lived in Polyamorous, but probably could have. It would be really great to work together in order to make both Ransack and Baby Squeel simpler and easier to maintain.

[gregmolnar]

I think having polyamorous as a gem living in the same repo would be the easiest thing to do.

[gregmolnar]

@rzane One thing we should iron out is, we are supporting different versions of Active Record. In ransack, we don't really want to support any non-supported version of Active Record(and I mean full support, not security patches) because it is just too much extra work and hopefully it will make more people be on recent versions. Of course, as long as it doesn't mean any extra work, we will depend > 5, but if it takes too much effort to make something work for an unsupported version, we will raise the dependency probably. As far as I see baby squeel seeems to support > 4.2.
My proposal is:

• we make polyamourous it's own gem, move it under the ransack repository
• they will also have the same version and will be released together.
• we create a new version(2.2 probably) including support for old versions of Active Record
• we branch that off and later on we release 3.0 dropping support for old versions of Active Record
• we accept bugfixes to the 2.2 branch and backport PRs, but we won't actively work on that version

You could first depend on polyamorous 2.2, then later or decide whether you want to stay on that, or follow the same policy as we do and jump on 3.0.
Would you be happy with that?

/cc @seanfcarroll

[scarroll32]

Great to see all of this engagement. Keeping Polyamorous as it's own gem inside Ransack works for me.

@gregmolnar 's point about aligning versions is a good one. Rails versions are moving pretty fast these days, and I think it is a fair policy for an open source project to support the currently supported Ruby & Rails versions only.

It would be a different story if there were many PRs to these projects, but in fact there are few and additionally the Ransack codebase needs a cleanup. It is always possible to lock an older app to an older version of the gem.

I really like Baby Squeel - it's a great gem, I am going to start using it in my projects.

[rzane]

Yeah, that works for me. I'm don't expect too many new features, and as such, I have no qualms about only supporting one Active Record version at a time.

[vitalinfo]

@rzane sorry, not really following the final approach, is it possible to have baby_squeel with Rails 5.2.1+?

[gregmolnar]

@vitalinfo for now you need to add ransack 2.1.1 to your gemfile to make that work. After the festive season I will have some time to work on making this work as before.

[vitalinfo]

@gregmolnar unfortunately it doesn't work for me

NoMethodError:
  undefined method `name' for nil:NilClass
# ./gems/baby_squeel-1.3.1/lib/baby_squeel/join_dependency.rb:74:in `reconstruct_with_type_caster'
# ./gems/baby_squeel-1.3.1/lib/baby_squeel/join_dependency.rb:39:in `find_alias'
# ./gems/baby_squeel-1.3.1/lib/baby_squeel/table.rb:89:in `find_alias'
# ./gems/baby_squeel-1.3.1/lib/baby_squeel/association.rb:63:in `find_alias'
# ./gems/baby_squeel-1.3.1/lib/baby_squeel/nodes/attribute.rb:30:in `_arel'
# ./gems/baby_squeel-1.3.1/lib/baby_squeel/nodes/proxy.rb:28:in `method_missing'
# ./gems/baby_squeel-1.3.1/lib/baby_squeel/operators.rb:17:in `block in arel_alias'
# ./app/models/concerns/activity/sifters.rb:91:in `block (2 levels) in <module:Sifters>'
# ./gems/baby_squeel-1.3.1/lib/baby_squeel/dsl.rb:14:in `instance_exec'
# ./gems/baby_squeel-1.3.1/lib/baby_squeel/dsl.rb:14:in `block in evaluate_sifter'
# ./gems/baby_squeel-1.3.1/lib/baby_squeel/table.rb:78:in `evaluate'
# ./gems/baby_squeel-1.3.1/lib/baby_squeel/dsl.rb:9:in `evaluate'
# ./gems/baby_squeel-1.3.1/lib/baby_squeel/dsl.rb:13:in `evaluate_sifter'
# ./gems/baby_squeel-1.3.1/lib/baby_squeel/active_record/base.rb:33:in `block in sifter'
# ./gems/activerecord-5.2.2/lib/active_record/relation/delegation.rb:114:in `public_send'
# ./gems/activerecord-5.2.2/lib/active_record/relation/delegation.rb:114:in `block in method_missing'
# ./gems/activerecord-5.2.2/lib/active_record/relation.rb:281:in `scoping'
# ./gems/activerecord-5.2.2/lib/active_record/relation/delegation.rb:114:in `method_missing'
# ./gems/baby_squeel-1.3.1/lib/baby_squeel/relation.rb:28:in `public_send'
# ./gems/baby_squeel-1.3.1/lib/baby_squeel/relation.rb:28:in `sift'

[gregmolnar]

Bummer. You can try this #97 (comment) or wait till we make it properly work, but I can't promise a date for that.

[vitalinfo]

@gregmolnar tried hour ago with the same result

[vitalinfo]

@gregmolnar just an experiment have changed find_alias method to

      def find_alias(associations)
        join_association = find_join_association(associations)
        reconstruct_with_type_caster(join_association.table || OpenStruct.new(name: join_association.base_klass.table_name), associations)
      end

and it works, but why doesn't work original implementation will investigate in the nearest days

[yule]

FWIW, I got this working with a mixture of these suggestions.

# Gemfile
 gem 'polyamorous', "~> 1.3.4", git: 'https://github.com/vintrepid/polyamorous.git'
 gem 'ransack', '2.2.1'

# initializer - monkey patch find_alias
module BabySqueel  
  module JoinDependency
    class Builder
      def find_alias(associations)
        join_association = find_join_association(associations)
        reconstruct_with_type_caster(join_association.table || OpenStruct.new(name: join_association.base_klass.table_name), associations)
      end
    end
  end
end

[vitalinfo]

As I understand find_alias monkey patch is that what we should live with?
and @vintrepid's fork for polyamorous

[Arpsara]

Hmm, I just tried the ar-521, I can not manage to install the polyamorous gem...

It tells me:

Bundler could not find compatible versions for gem "polyamorous":
  In Gemfile:
    baby_squeel was resolved to 1.3.1, which depends on
      polyamorous (~> 2.1.1)

Could not find gem 'polyamorous (~> 2.1.1)'

And I could indeed not find polyamorous version with tag 2.1.1
https://github.com/activerecord-hackery/polyamorous/tree/master

Did I do something wrong?

[zubairshams]

@Arpsara You can get latest polyamorous from ransack and then use ar-521

git 'https://github.com/activerecord-hackery/ransack', branch: 'master' do
  gem 'polyamorous'
end
gem 'baby_squeel', github: 'rzane/baby_squeel', branch: 'ar-521

[typhoon2099]

@Arpsara You can get latest polyamorous from ransack and then use ar-521

git 'https://github.com/activerecord-hackery/ransack', branch: 'master' do
  gem 'polyamorous'
end
gem 'baby_squeel', github: 'rzane/baby_squeel', branch: 'ar-521

This doesn't seem to be working for me. I get the same error that @Arpsara was getting. I've tried using tag v2.1.1 but then polyamorous isn't found at all.

[gregmolnar]

@typhoon2099 polyamorous is released separately now, so you can just try:

gem 'polyamorous', '2.3.0'

[typhoon2099]

Using the following:

gem "polyamorous"
gem "baby_squeel", git: "https://github.com/rzane/baby_squeel.git", branch: "ar-521"

gives me the following:

Bundler could not find compatible versions for gem
"polyamorous":
  In Gemfile:
    polyamorous

    baby_squeel was resolved to 1.3.1, which depends on
      polyamorous (~> 2.1.1)

Could not find gem 'polyamorous (~> 2.1.1)', which is required
by gem 'baby_squeel', in any of the sources.

[gregmolnar]

@typhoon2099 you need to bump the dependency version in baby squeel in that branch. Maybe @rzane has time to sort that, but I am sure he would be happy to accept a PR handling it.

[typhoon2099]

Ah, okay, I misunderstood the workaround above.

[rzane]

Disclaimer: Active Record >= 5.2.1 is currently broken. While the ar-521 branch might appear to work, I promise you, there are some very real problems on that branch. I do not recommend using it.

If this was simply a matter of updating dependencies, I would have released a new version. There are numerous test failures on that branch, all stemming from the inability to locate the correct alias for a table.

Unfortunately, I don't have the time right now to dig in and figure out what is causing those issues. This is the line that is causing all of the problems. I would really appreciate help figuring out what the issue is.

[gregpardo]

This may not be a solution for everyone but we found that we only used baby_squeel for a few things and I found some ways to have a decent syntax without it...

Most of our queries that used it were simple greater than or less then. I created an extension that could help like this.

module QueryExtensions
  extend ActiveSupport::Concern

  included do
    scope :where_gt, ->(column, value) { where("#{column} > ?", value) }
    scope :where_gte, ->(column, value) { where("#{column} >= ?", value) }
    scope :where_lt, ->(column, value) { where("#{column} < ?", value) }
    scope :where_lte, ->(column, value) { where("#{column} <= ?", value) }
  end
end

Then I included in my ApplicationRecord class that all my models inherit from.

class ApplicationRecord < ActiveRecord::Base
  include QueryExtensions
end

This allowed us to convert queries that were like this

User.where.has { created_at > 10.days.ago }

to

User.where_gt(:created_at, 10.days.ago)

And for joining we just used standard joins or left_joins depending on if it was outer or not.

c = customer # Needed for name conflict 
GiftCard
        .joining { customer }
        .where.has {
          (customer.id == c.id) | (phone == c.phone)
        }

To something like

query = GiftCard.joins(:customer)
query.where(gift_cards: { customers: { id: customer.id } })
  .or(query.where(gift_cards: { phone: customer.phone }))

Depending on how deeply embedded baby_squeel is in your application this may not be an option. The downside is that the queries are a little harder to read in my opinion but with active record '.or' and '.merge' you can get most things working without use of Arel tables. Also, the column names are not escaped but since this is not dynamic data passed in from user in our case should not be much of a problem

[rzane]

Hey @gregpardo, I'm glad you found a simple solution.

If BabySqueel didn't have to worry about joins, it would actually be incredibly simple. Here's what that would look like in just 45 lines of code:

https://gist.github.com/rzane/8104af09369fbce3d2776c4a194e1de6

[gregpardo]

@rzane Thanks thats actually pretty handy and looks more flexible than what I came up with. Will keep this in mind and hopefully it can help others.

[tiagoefmoraes]

I'm having a look at this issue and found that baby_squeel started breaking after this rails commit: rails/rails@50036e6
After that commit the join_association at line

baby_squeel/lib/baby_squeel/join_dependency.rb

Line 43 in a6a9f43

if join_association.table

doesn't have values at the @tables and @alias_tracker instance variables, resulting in a nil table as said by @rzane.

I have no idea how to solve this yet, if anyone can look that commit and give some help.

[tiagoefmoraes]

The tables will be assigned to the join_association only later.

I raised errors on this line on baby_squeel

baby_squeel/lib/baby_squeel/join_dependency.rb

Line 46 in a6a9f43

# This literally does not work. This will often

And this line on activerecord https://github.com/rails/rails/blob/7b5cc5a5dfcf38522be0a4b5daa97c5b2ba26c20/activerecord/lib/active_record/associations/join_dependency/join_association.rb#L58

And this is the diff in the backtrace https://gist.github.com/tiagoefmoraes/e2319a75f82bdc136e2d95cf1cf01143/revisions?diff=split#diff-f80222de64b3079c261e058703f4e07a

[rtweeks]

My team is just running into this issue now, too. I'm wondering if BabySqueel might just force aliases on associations (and track the ones it creates) rather than trying to discover the aliases assigned dynamically by ActiveRecord? I'd be willing to help with the implementation if this seems like a viable solution.

[rzane]

@rtweeks, that would be a viable solution and would massively decrease the complexity of this library. Happy to accept pull requests!

[rtweeks]

I found a quick way to fix a lot of the broken tests (about 10 of them) by directing Active Record's JoinDependency to construct Arel tables (including the aliases) and attach them the the JoinAssociations contained in the JoinDependency. This still leaves several issues with polymorphic joins and inner- vs. outer-joins. TL;DR I'm still working on it.

[rtweeks]

I think the polymorphism issues are because Polyamorous is not injecting the polymorph's join condition when a Polyamorous::Join is used as demonstrated here:

Picture.joins(Polyamorous::Join.new(:imageable, Arel::Nodes::InnerJoin, Post) => {}).to_sql
# => "SELECT \"pictures\".* FROM \"pictures\" INNER JOIN \"posts\" ON \"posts\".\"id\" = \"pictures\".\"imageable_id\""

The output string here should include a "pictures"."imageable_type" = 'Post' as an additional criterion in the ON of the INNER JOIN ☹️

ActiveRecord 5.2.0 also shows this behavior (I happen to have a VM with that version lying around), so this is not new to AR >= 5.2.1.

[rtweeks]

By the same token, inner- and outer-joins seem to be getting confused by the Polyamorous code:

Post.joins(
  Polyamorous::Join.new(:author, Arel::Nodes::OuterJoin) => {
    Polyamorous::Join.new(:comments, Arel::Nodes::InnerJoin) => {}
  }
).to_sql
# => "SELECT \"posts\".* FROM \"posts\" LEFT OUTER JOIN \"authors\" ON \"authors\".\"id\" = \"posts\".\"author_id\" LEFT OUTER JOIN \"comments\" ON \"comments\".\"author_id\" = \"authors\".\"id\""

@gregmolnar and team, this is not intended in any to bash Polyamorous, I'm just trying to document where the tests and actual results diverge. It could also be I'm misunderstanding how this is supposed to work.

[rtweeks]

If I use the master branch (commit c9cc20de9e) of Ransack for Polyamorous, I've got it down to 2 errors from the unit tests -- the ones about INNER JOINs coming after OUTER JOINs:

• rspec ./spec/integration/joining_spec.rb:152 # #joining when joining implicitly nested joins outer joins
• rspec ./spec/integration/joining_spec.rb:197 # #joining when joining implicitly nested joins joins a through association and then back again

[rtweeks]

I just opened PR #109 for this.

[rocket-turtle]

I think this issue is fixed in 1.4.0.beta1 - April 21, 2021 (20 KB) and should be closed.