take-five / activerecord-hierarchical_query

Simple DSL for creating recursive queries with ActiveRecord
MIT License
119 stars 24 forks source link

ActiveRecord::StatementInvalid errors with v1.0.0 and Rails 5 #21

Closed xtagon closed 5 years ago

xtagon commented 7 years ago

Hi,

I'm trying to use v1.0.0 with Rails 5, and my queries don't quite work. I could really use some help.

This does what is expected (gets the ancestors as a relation):

relation = self.class.join_recursive do |query|
  query.start_with(id: id).connect_by(parent_organization_id: :id)
end

and if I call #to_a on that, I get the correct results. However, if I do any of these things:

relation.first, relation.where(...), relation.limit(...), relation.order(..) I get an error like this:

ActiveRecord::StatementInvalid: PG::ProtocolViolation: ERROR:  bind message supplies 1 parameters, but prepared statement "a3" requires 2

Here's an example of the generated SQL when it fails to on relation.first:

SELECT "organizations".* FROM "organizations"
INNER JOIN
  (WITH RECURSIVE "organizations__recursive" AS
     (SELECT "organizations"."id", "organizations"."parent_organization_id"
      FROM "organizations"
      WHERE "organizations"."deleted_at" IS NULL
        AND "organizations"."id" = $1
        UNION ALL
        SELECT "organizations"."id",
               "organizations"."parent_organization_id"
        FROM "organizations"
        INNER JOIN "organizations__recursive" ON "organizations__recursive"."parent_organization_id" = "organizations"."id" WHERE "organizations"."deleted_at" IS NULL ) SELECT "organizations__recursive".*
   FROM "organizations__recursive") AS "organizations__recursive" ON "organizations"."id" = "organizations__recursive"."id"
WHERE "organizations"."deleted_at" IS NULL
ORDER BY "organizations"."id" ASC
LIMIT $2

Any ideas? Thanks in advance!

thooams commented 7 years ago

It's start_with method which generates an error. I don't know why. Only variable $2 (LIMIT) is taken into account.

thooams commented 7 years ago

Try to replace start_with(id: id) by start_with(YourModel.find(id)) or start_with(self)

mkamensky commented 6 years ago

I see the same problem. If I try to give the object as an argument to start_with, it ignores the condition

mkamensky commented 6 years ago

It appears that when chaining with other queries, the argument list gets replaced

mkamensky commented 6 years ago

A workaround similar to the one mention in https://github.com/rails/rails/issues/20077#issuecomment-266427441 worked for me

zachaysan commented 6 years ago

Is this still an issue in Rails 5.1? I'm going to help keep this project maintained and I want to fix what's broken and make sure we can support Rails 5.2 now that it is out.

xtagon commented 6 years ago

@zachaysan I would suspect so. One of the reasons I'm still stuck on Rails 4.2 is because I couldn't upgrade activerecord-hierarchical_query without issues like this when attempting a Rails 5 upgrade, so if you're looking to help fix and maintain, I would be happy to help you and do testing and try a Rails 5 upgrade again. Not sure on the time frame though due to work responsibilities.

zachaysan commented 6 years ago

Great @xtagon I don't want you to waste time so if you could just give me a detailed, but austere way of replicating the bug I'll try to handle as soon as possible.

xtagon commented 6 years ago

@zachaysan I'll have to pull my Rails 5 upgrade branch and see if I can reproduce. There have been a lot of changes to my own app since I last tried the upgrade. I'll try to do it this weekend. Thanks so much for the help!

zachaysan commented 5 years ago

Hey @xtagon I'm going to close this issue. If you can re-produce in 1.1.0 let me know, but I'm currently unable to.

xtagon commented 5 years ago

Sounds fair. It is going to take me a little longer to reproduce again as I'm fixing some conflicts in the branch I was working in. I'll re-open the issue when it gets to that point.

xtagon commented 5 years ago

@zachaysan Just to let you know, I reproduced this on 1.1.0 and Rails 5.0, but this time when I changed start_with to use self instead of the model ID, it fixed it. Awesome!

Thanks again for everyone's help.

zachaysan commented 5 years ago

@xtagon Dang really? What happens if you do self.id?

xtagon commented 5 years ago

If I pass the ID it has a prepared statement error like the one I originally shared. If I pass the model object it works. I think if I pass an array of multiple IDs it works too.

zachaysan commented 5 years ago

I'm trying to make sure it's not related to some id weirdness. Sometimes ids are strings in rails and sometimes they're integers.

What happens if you do this:

relation = self.class.join_recursive do |query|
  query.start_with(id: self.id).connect_by(parent_organization_id: :id)
end
xtagon commented 5 years ago

Here is part of the code that I got the error from:

relation = self.class.join_recursive do |query|
      query.
        start_with(id: self.id) { select("0 depth") }.
        select(query.prior[:depth] + 1, start_with: false).
        connect_by(id: :parent_organization_id)
    end

It only causes an error if you do a where condition or similar later on this relation as described above. If you do a to_a directly on it it's fine. Replacing self.id with self fixes it too.

I didn't think of this before but I am using Postgres GUID strings as IDs if that helps.

zachaysan commented 5 years ago

Hmm. I'm going to bet that GUID is why I can't seem to replicate because I'm not currently using them and they're another tricky case of sometimes integer, sometimes string tomfoolery.

Are you able to clone the repo and run the tests? Could you create a failing tests with a GUID column? If you can do that I'll try to figure out the root cause.

xtagon commented 5 years ago

I'll give it a shot.

xtagon commented 5 years ago

All tests pass when I switch your test schema to use UUIDs. I even added a test case that looks as close as possible to the one in my app (I am not able to give access to my app's source unfortunately).

Very strange, I'm not sure why it won't reproduce.

https://github.com/xtagon/activerecord-hierarchical_query/commit/4ea5b6e48091898aa2d2afa29dcad22cbc93cb8f

I made sure to use the Rails 5.0 gemfile too.

zachaysan commented 5 years ago

Hmmm. Are you using any weird default scopes or redefining any core methods?

xtagon commented 5 years ago

I am using this soft deletion gem on the model, which adds a default scope for deleted_at IS NULL: https://rubygems.org/gems/soft_deletion

zachaysan commented 5 years ago

Sorry for the delay. Does disabling the gem do anything? I dislike uncertainty and spookiness in libraries so I'd like to figure out what is wrong.

xtagon commented 5 years ago

Just tried that, it didn't seem to have an effect.

This is on Rails 5.0.0 - I'm going to be upgrading to 5.1 and then 5.2 soon-ish, maybe it was something that changed in Arel

xtagon commented 5 years ago

I'd like to figure out what is wrong too, but just to clarify I'm perfectly fine with the workaround of using the record instance -- it doesn't solve the mystery but it solves my use case

zachaysan commented 5 years ago

Well if it is still an issue in 5.2 then I'm going to try to figure it out. Partially for my own interests. I want to be able to rely on this library in production and I generate some pretty abstract queries during a request cycle so this needs to be dependable.