Giter VIP home page Giter VIP logo

Comments (57)

rzane avatar rzane commented on August 14, 2024 5

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.

from baby_squeel.

hlascelles avatar hlascelles commented on August 14, 2024 5

Thank you for looking at this, baby_squeel team. We are blocked on it too. Is there a solution in the wings or something we can help with?

from baby_squeel.

gregmolnar avatar gregmolnar commented on August 14, 2024 3

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

from baby_squeel.

zubairshams avatar zubairshams commented on August 14, 2024 3

@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

from baby_squeel.

rzane avatar rzane commented on August 14, 2024 2

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.

from baby_squeel.

rzane avatar rzane commented on August 14, 2024 2

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.

from baby_squeel.

yule avatar yule commented on August 14, 2024 2

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

from baby_squeel.

vitalinfo avatar vitalinfo commented on August 14, 2024 2

Workable solution for me with Rails 5.2.2:

  1. Gemfile:
gem 'baby_squeel'
gem 'polyamorous', '~> 1.3.4', git: 'https://github.com/vintrepid/polyamorous.git'
  1. Monkey patch for join_dependency.rb:
module BabySqueel
  module JoinDependency
    class Builder # :nodoc:
      def find_alias(associations)
        join_association = find_join_association(associations)
        table = join_association.table || OpenStruct.new(name: join_association.base_klass.table_name)
        reconstruct_with_type_caster(table, associations)
      end
    end
  end
end
  1. Change joining { association.outer } to left_joins(:association)
  2. When the same table is hit twice in the same query - use as(:alias_name)

from baby_squeel.

rzane avatar rzane commented on August 14, 2024 2

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

from baby_squeel.

chewi avatar chewi commented on August 14, 2024 1

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.

from baby_squeel.

wollistik avatar wollistik commented on August 14, 2024 1

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!

from baby_squeel.

gregmolnar avatar gregmolnar commented on August 14, 2024 1

@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

from baby_squeel.

scarroll32 avatar scarroll32 commented on August 14, 2024 1

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.

from baby_squeel.

gregmolnar avatar gregmolnar commented on August 14, 2024 1

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

gem 'polyamorous', '2.3.0'

from baby_squeel.

rzane avatar rzane commented on August 14, 2024 1

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.

from baby_squeel.

gregpardo avatar gregpardo commented on August 14, 2024 1

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

from baby_squeel.

rtweeks avatar rtweeks commented on August 14, 2024 1

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.

from baby_squeel.

rtweeks avatar rtweeks commented on August 14, 2024 1

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.

from baby_squeel.

sasurai-usagi3 avatar sasurai-usagi3 commented on August 14, 2024

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.

from baby_squeel.

kevin-at-corista avatar kevin-at-corista commented on August 14, 2024

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

from baby_squeel.

kevin-at-corista avatar kevin-at-corista commented on August 14, 2024

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

from baby_squeel.

obfuscoder avatar obfuscoder commented on August 14, 2024

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.

from baby_squeel.

rzane avatar rzane commented on August 14, 2024

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.

from baby_squeel.

rogierslag avatar rogierslag commented on August 14, 2024

Any more recent updates on this?

from baby_squeel.

atomaka avatar atomaka commented on August 14, 2024

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.

from baby_squeel.

vintrepid avatar vintrepid commented on August 14, 2024

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'

from baby_squeel.

wollistik avatar wollistik commented on August 14, 2024

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.

from baby_squeel.

rzane avatar rzane commented on August 14, 2024

@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.

from baby_squeel.

scarroll32 avatar scarroll32 commented on August 14, 2024

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 ?

from baby_squeel.

rzane avatar rzane commented on August 14, 2024

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.

from baby_squeel.

vitalinfo avatar vitalinfo commented on August 14, 2024

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

from baby_squeel.

gregmolnar avatar gregmolnar commented on August 14, 2024

@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.

from baby_squeel.

vitalinfo avatar vitalinfo commented on August 14, 2024

@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'

from baby_squeel.

gregmolnar avatar gregmolnar commented on August 14, 2024

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

from baby_squeel.

vitalinfo avatar vitalinfo commented on August 14, 2024

@gregmolnar tried hour ago with the same result

from baby_squeel.

vitalinfo avatar vitalinfo commented on August 14, 2024

@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

from baby_squeel.

vitalinfo avatar vitalinfo commented on August 14, 2024

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

from baby_squeel.

shinronin avatar shinronin commented on August 14, 2024

@yule 's post on 1/2/2019 has enabled me to upgrade to rails 5.2.2. looking good so far.

from baby_squeel.

rzane avatar rzane commented on August 14, 2024

Just an FYI: I don't think that monkey patch is actually going to work, because of this issue: activerecord-hackery/ransack#955. Please verify that your outer joins are behaving correctly. I suspect that all of your outer joins have become inner joins.

Rather than monkey-patching Baby Squeel, if you've found a solution that works, I would really appreciate a PR. Plus, that way, you can run the test suite to see what is actually working and what isn't.

from baby_squeel.

vitalinfo avatar vitalinfo commented on August 14, 2024

@rzane yes, all left joins are going to be right joins, but without this monkey patch I have a crash, because table is nil, that's why I've refactored to AR left_joins

from baby_squeel.

rzane avatar rzane commented on August 14, 2024

Great question. @gregmolnar has been working on some fixes to polyamorous over here: activerecord-hackery/ransack#1004.

I think we could really, really use some detective work. I haven't really had the time to dig in as much as is necessary. I have a branch that will at least run against @gregmolnar's work here: https://github.com/rzane/baby_squeel/tree/ar-521. Please read my comments on that polyamorous issue about the outstanding test failures.

from baby_squeel.

Arpsara avatar Arpsara commented on August 14, 2024

Hi! I got a similar issues with rails 5.2.3 and some has_many and has_and_belongs_to_many associations. The error I got was ArgumentError (wrong number of arguments (given 3, expected 2))
It took me a while before I understood that it was a baby_squeel problem.

The solution posted by vital info (#97 (comment)) seemed to work fine. I hope a proper solution could be find soon. :)

from baby_squeel.

pelletencate avatar pelletencate commented on August 14, 2024

The solution posted by vital info (#97 (comment)) seemed to work fine. I hope a proper solution could be find soon. :)

@Arpsara Have you tried ar-521?

@rzane I'll try to find the time to give ar-521 a try on my codebase somewhere next week to see if our specs are gonna hold up. I know I got a few tricky alias resolution cases in there that I'll see if I can AB test. Meanwhile thanks to both you and @gregmolnar for doing this awesome work!

from baby_squeel.

Arpsara avatar Arpsara commented on August 14, 2024

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?

from baby_squeel.

typhoon2099 avatar typhoon2099 commented on August 14, 2024

@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.

from baby_squeel.

typhoon2099 avatar typhoon2099 commented on August 14, 2024

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.

from baby_squeel.

gregmolnar avatar gregmolnar commented on August 14, 2024

@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.

from baby_squeel.

typhoon2099 avatar typhoon2099 commented on August 14, 2024

Ah, okay, I misunderstood the workaround above.

from baby_squeel.

rzane avatar rzane commented on August 14, 2024

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

from baby_squeel.

gregpardo avatar gregpardo commented on August 14, 2024

@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.

from baby_squeel.

tiagoefmoraes avatar tiagoefmoraes commented on August 14, 2024

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

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.

from baby_squeel.

tiagoefmoraes avatar tiagoefmoraes commented on August 14, 2024

The tables will be assigned to the join_association only later.

I raised errors on this line on baby_squeel

# 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

from baby_squeel.

rtweeks avatar rtweeks commented on August 14, 2024

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.

from baby_squeel.

rtweeks avatar rtweeks commented on August 14, 2024

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.

from baby_squeel.

rtweeks avatar rtweeks commented on August 14, 2024

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

from baby_squeel.

rtweeks avatar rtweeks commented on August 14, 2024

I just opened PR #109 for this.

from baby_squeel.

rocket-turtle avatar rocket-turtle commented on August 14, 2024

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

from baby_squeel.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤ī¸ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.