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

textacular not compatible w/ rails 4.1 #47

Closed
wants to merge 3 commits into from
Closed

Conversation

benhamill
Copy link
Contributor

No description provided.

@benhamill
Copy link
Contributor

Can you go into more detail about the incompatibility? Like a stack trace or something?

@adasfan
Copy link
Author

adasfan commented Apr 17, 2014

sorry about that. I am getting this sql syntax error from postgres:

PG::SyntaxError: ERROR:  syntax error at or near "AS"
LINE 1: ... plainto_tsquery('english', 'berosin'::text)), 0) AS "rank32...
                                                             ^
: SELECT COUNT("blogs".*, COALESCE(ts_rank(to_tsvector('english', "blogs"."title"::text), plainto_tsquery('english', 'berosin'::text)), 0) + COALESCE(ts_rank(to_tsvector('english', "blogs"."content"::text), plainto_tsquery('english', 'berosin'::text)), 0) AS "rank32614878636684109") FROM "blogs"  WHERE (to_tsvector('english', "blogs"."title"::text) @@ plainto_tsquery('english', 'berosin'::text) OR to_tsvector('english', "blogs"."content"::text) @@ plainto_tsquery('english', 'berosin'::text)) 

I am simply calling basic_search(query) where query parameter is passed from the index.

@benhamill
Copy link
Contributor

@mcetin71 What version of Postgres are you running this against?

Other maintainers: Can y'all spot the syntax error in that line of SQL? I'm not seeing it. I'll have another look later on and pull up the docs, but don't have time to dig just now.

@adasfan
Copy link
Author

adasfan commented Apr 18, 2014

PostgreSQL 9.3.2. It was working fine with Rails 4.0.2

@benhamill
Copy link
Contributor

Can you paste in the .to_sql from 4.0.2 so we can compare the outputted SQL? Must be something about the way newer AR is realizing it.

@benhamill
Copy link
Contributor

Other maintainers: Should we consider doing the thing where you tell Travis to build against several versions of dependencies? Specifically, maybe we should build ActiveRecord 3.2.17, 4.0.4 and 4.1.0 (the latest patch on the most recent 3 minor versions)?

@gregmolnar
Copy link
Member

I am up for that. Which method should we use? The appraisal(https://github.com/thoughtbot/appraisal) gem?

@benhamill
Copy link
Contributor

Oh. Travis has a built-in method for specifying that kind of thing.

@gregmolnar
Copy link
Member

Everyday is a schoolday :). Are you gonna set it up or shall I do it when I got some free time?

@benhamill
Copy link
Contributor

Whoever gets the freetime first.​

@adasfan
Copy link
Author

adasfan commented Apr 18, 2014

ok, so with rails 4.0.2 (and w/ textacular 3.2.0), two part sql get issued:

first:

SELECT COUNT(*) FROM "blogs" WHERE (to_tsvector('english', "blogs"."title"::text) @@ plainto_tsquery('english', 'berosin'::text) OR to_tsvector('english', "blogs"."content"::text) @@ plainto_tsquery('english', 'berosin'::text))

and then:

SELECT "blogs".*, COALESCE(ts_rank(to_tsvector('english', "blogs"."title"::text), plainto_tsquery('english', 'berosin'::text)), 0) + COALESCE(ts_rank(to_tsvector('english', "blogs"."content"::text), plainto_tsquery('english', 'berosin'::text)), 0) AS "rank55416461070260279" FROM "blogs" WHERE (to_tsvector('english', "blogs"."title"::text) @@ plainto_tsquery('english', 'berosin'::text) OR to_tsvector('english', "blogs"."content"::text) @@ plainto_tsquery('english', 'berosin'::text)) ORDER BY "rank55416461070260279" DESC LIMIT 5 OFFSET 0

@benhamill
Copy link
Contributor

@mcetin71: FWIW, I've been editing your posts to add tripple-backticks to make the code more readable. I hope you don't mine. 😓

@adasfan
Copy link
Author

adasfan commented Apr 18, 2014

not at all. i appreciate it. thank you.

@adasfan
Copy link
Author

adasfan commented Jul 13, 2014

I just upgraded to rails 4.1.4 and the issue is gone. I found this in the release documentation: "The security patches introduced a regression on the PostgreSQL Range feature. This regression was only introduced to Rails 4.x. Rails ..." I suspect it might have been related but am not sure.

@adasfan adasfan closed this Jul 13, 2014
@pixelate
Copy link

I'm still having issues in Rails 4.1.4:

ActiveRecord::StatementInvalid: PG::SyntaxError: ERROR:  syntax error at or near "AS"
LINE 1: ...), plainto_tsquery('simple', 'Shadow'::text)), 0) AS "rank58...
                                                             ^
: SELECT COUNT("posts".*, COALESCE(ts_rank(to_tsvector('simple', "posts"."title"::text), plainto_tsquery('simple', 'Shadow'::text)), 0) AS "rank58127663186812986") FROM "posts"  WHERE (to_tsvector('simple', "posts"."title"::text) @@ plainto_tsquery('simple', 'Shadow'::text))

@sowenjub
Copy link

sowenjub commented Sep 3, 2014

Hello,

I encountered this issue with rails 4.1.5 and textacular 3.2.0

User.fuzzy_search(first_name: 'john').count

SELECT COUNT("users".*, similarity("users"."first_name", 'john') AS "rank14664220501247485") 
FROM "users"  WHERE (("users"."first_name" % 'john'))

PG::SyntaxError: ERROR:  syntax error at or near "AS"
LINE 1: ...sers".*, similarity("users"."first_name", 'john') AS "rank14...
                                                         ^
: SELECT COUNT("users".*, similarity("users"."first_name", 'john') AS "rank14664220501247485")
FROM "users"  WHERE (("users"."first_name" % 'john'))
ActiveRecord::StatementInvalid: PG::SyntaxError: ERROR:  syntax error at or near "AS"
LINE 1: ...sers".*, similarity("users"."first_name", 'john') AS "rank14...
                                                         ^

@neektza
Copy link

neektza commented Sep 26, 2014

Having the same problem. Snippet:

[46] pry(main)> Entity.basic_search('something').count

ActiveRecord::StatementInvalid: PG::SyntaxError: ERROR:  syntax error at or near "AS"
LINE 1: ...lainto_tsquery('english', 'something'::text)), 0) AS "rank32...

The full snippet

@benhamill
Copy link
Contributor

Well, this is fun: Our development dependencies are locked to an ealier version of something that's demanding an earlier version of AcriveRecord. So to fix this, I'm going to need to spend some time loving on the tests. While I'm at it, I might just rewrite them using RSpec because... I like it better.

@benhamill
Copy link
Contributor

Should add: This means we can't run the existing tests against modern versions of ActiveRecord. So good.

@ecin
Copy link
Member

ecin commented Oct 1, 2014

@benhamill more details on what the misbehaving dependency is?

@benhamill
Copy link
Contributor

It's either Shoulda or MiniTest. Taking our version locks off those let newer ActiveRecord install, but had other problems, which I didn't chase down.

@ecin
Copy link
Member

ecin commented Oct 2, 2014

Really weird, testing versions shouldn't prevent ActiveRecord. Bummer.

@benhamill
Copy link
Contributor

I assume it's a shared dependency that's locking them. Maybe ActiveSupport or... something. 🤷

@benhamill benhamill mentioned this pull request Oct 2, 2014
@neektza
Copy link

neektza commented Oct 24, 2014

Are the tests close to being done? I'd like to take a shot at solving this.

@benhamill
Copy link
Contributor

I'm wanting a few of the other committers to look at #61 first. Once/if that gets merged, we'll change the Travis set up to run against a few different versions of ActiveRecord, which should illustrate the problem (I hope). Then we can fix it. That's my plan, at least.

@benhamill
Copy link
Contributor

I'm going to start a new branch that'll test the gem against a few different versions of ActiveRecord which should hopefully uncover any bugs.

@benhamill benhamill added the bug label Oct 30, 2014
@benhamill
Copy link
Contributor

I believe #64 should be showing off this bug, now. The new tests are merged and that PR expands the versions of stuff we run tests against. I may merge that PR without fixing that test and keep discussion of the bug here, assuming everything else in that PR looks good. Also want to have a what-versions-do-we-support discussion. Maybe that's yet another PR.

@benhamill
Copy link
Contributor

If we look closely at this query, we can see what's wrong:

SELECT
COUNT(
  "games".*,
  COALESCE(
    ts_rank(
      to_tsvector('english', "games"."system"::text),
      to_tsquery('english', 'NES'::text)
    ),
    0
  ) AS "rank50717309137277712"
)
FROM "games" 
WHERE (to_tsvector('english', "games"."system"::text) @@ to_tsquery('english', 'NES'::text))

It looks like the AS clause is inside the COUNT call, which is obviously meaningless, if you think about it. So this has to do with how that bit is interpolated in in assemble_query and how count messes with the SELECT clause when called.

I'll need to have a deeper look at this to figure out how to modify it, but I think it's doable. Just too tired for it now. If anyone else has an idea, speak up. 😄

@benhamill
Copy link
Contributor

Have a look at this:

[1] pry(main)> WebComic.create name: 'foo'
D, [2014-10-31T15:09:55.877602 #30055] DEBUG -- :    (0.3ms)  BEGIN
D, [2014-10-31T15:09:55.882728 #30055] DEBUG -- :   SQL (0.9ms)  INSERT INTO "web_comics" ("name") VALUES ($1) RETURNING "id"  [["name", "foo"]]
D, [2014-10-31T15:09:55.912226 #30055] DEBUG -- :    (28.8ms)  COMMIT
=> #<WebComic id: 1, name: "foo", author: nil, review: nil>
[2] pry(main)> WebComic.where(name: 'foo')
D, [2014-10-31T15:10:09.549103 #30055] DEBUG -- :   WebComic Load (1.2ms)  SELECT "web_comics".* FROM "web_comics"  WHERE "web_comics"."name" = 'foo'
=> [#<WebComic id: 1, name: "foo", author: nil, review: nil>]
[3] pry(main)> WebComic.where(name: 'foo').count
D, [2014-10-31T15:10:13.901858 #30055] DEBUG -- :    (0.7ms)  SELECT COUNT(*) FROM "web_comics"  WHERE "web_comics"."name" = 'foo'
=> 1
[4] pry(main)> WebComic.where(name: 'foo').select('name')
D, [2014-10-31T15:10:42.554063 #30055] DEBUG -- :   WebComic Load (0.7ms)  SELECT "web_comics"."name" FROM "web_comics"  WHERE "web_comics"."name" = 'foo'
=> [#<WebComic id: nil, name: "foo">]
[5] pry(main)> WebComic.where(name: 'foo').select('name').count
D, [2014-10-31T15:10:49.523231 #30055] DEBUG -- :    (1.3ms)  SELECT COUNT("web_comics"."name") FROM "web_comics"  WHERE "web_comics"."name" = 'foo'
=> 1
[6] pry(main)> WebComic.where(name: 'foo').select('name AS n')
D, [2014-10-31T15:11:08.922824 #30055] DEBUG -- :   WebComic Load (0.8ms)  SELECT name AS n FROM "web_comics"  WHERE "web_comics"."name" = 'foo'
=> [#<WebComic id: nil>]
[7] pry(main)> _.first.attributes
=> {"n"=>"foo", "id"=>nil}
[8] pry(main)> WebComic.where(name: 'foo').select('name AS n').count
D, [2014-10-31T15:11:32.279225 #30055] DEBUG -- :    (0.8ms)  SELECT COUNT(name AS n) FROM "web_comics"  WHERE "web_comics"."name" = 'foo'
ActiveRecord::StatementInvalid: PG::SyntaxError: ERROR:  syntax error at or near "AS"
LINE 1: SELECT COUNT(name AS n) FROM "web_comics"  WHERE "web_comics...
                          ^
: SELECT COUNT(name AS n) FROM "web_comics"  WHERE "web_comics"."name" = 'foo'
from /home/ben/.rbenv/versions/2.1.4/lib/ruby/gems/2.1.0/gems/activerecord-4.1.7/lib/active_record/connection_adapters/postgresql_adapter.rb:822:in `async_exec'

Is this a Rails bug, maybe, and not a bug with us?

@benhamill
Copy link
Contributor

Look what I found: rails/rails#15138

It looks like a work-around would be to use count(:all) for the time being. Which is kind of gross.

[9] pry(main)> WebComic.where(name: 'foo').select('name AS n').count(:all)
D, [2014-10-31T15:25:03.502323 #30055] DEBUG -- :    (0.8ms)  SELECT COUNT(*) FROM "web_comics"  WHERE "web_comics"."name" = 'foo'
=> 1

@ecin
Copy link
Member

ecin commented Dec 22, 2014

@benhamill @mcetin71 I'm closing this pull request unless there's still something here for us to fix. Please reopen if necessary.

@ecin ecin closed this Dec 22, 2014
@benhamill
Copy link
Contributor

Yeah, that's fine. I should cherry pick the changes to .pryrc into master, since they're globally relevant. And I wanna keep this branch for a while, since it has a test that shows off the bug. But we don't need the PR hanging out, I think.

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

Successfully merging this pull request may close these issues.

None yet

7 participants