Navigation Menu

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

Use group rather than distinct or .uniq when using tagged_with(any:true). Fixes PostgreSQL errors #496

Merged
merged 1 commit into from May 1, 2014

Conversation

leklund
Copy link
Contributor

@leklund leklund commented Mar 19, 2014

Summary: SELECT DISTINCT foo.* FROM foo breaks in PostgreSQL when foo has a column with json datatype. acts-as-taggable-on uses distinct to get unique results when using any: true. However, this pull request: #461, introduced a bug so that every call to Model.tagged_with('foo') would use the .uniq method which adds a SELECT DISTINCT the the query. #461 was attempting to fix #357 but moving the explicit distinct to a .uniq only fixed certain collisions with other gems.

The 'distinct' bug in action: https://gist.github.com/leklund/9643920

The error raised is PG::UndefinedFunction: ERROR: could not identify an equality operator for type json because the distinct is unnecessarily comparing every column on the table using the equality operator.

What does this pull request add?

  • changes the .uniq for any to use a group by the primary key of the table. This avoids the wildcard distinct.
  • test to expose the bug introduce by Move 'Distinct' out of select string and use .uniq instead #461 that calls 'distinct' on every query generated from tagged_with
  • tests for postgresql specific tables with json columns
  • fixes an intermittently failing test in postgresql (one of the exclude tests was expecting a certain order back from the database and wasn't always getting that order).

@seuros
Copy link
Collaborator

seuros commented Mar 19, 2014

Build failed, I cancelled it. Apparently travis don't support json which is a native type since 9.2.

@leklund
Copy link
Contributor Author

leklund commented Mar 19, 2014

I pushed a commit that should fix the build issue since travis by default uses 9.1.

After some more thought and testing with an existing app I'm thinking this might be better as an option passed with tagged_with. The default would be to use distinct (or uniq). For example, complex queries that have a join and then use a joined column to order by will break without adding that ordering column to the group by. I'd rather not introduce a breaking change so I'm looking at using an option instead.

@leklund
Copy link
Contributor Author

leklund commented Mar 19, 2014

In reference to my previous comment: this pull request isn't a breaking change for the complex query described. They don't work with the uniq method either. So I don't believe this would be a breaking change.

@leklund
Copy link
Contributor Author

leklund commented Apr 4, 2014

Looks like the build is failing for sqlite3 with 1.9.3 and rails 4. I'll take a look at this and try to get a fix pushed.

@seuros
Copy link
Collaborator

seuros commented Apr 4, 2014

just rebase it, that bug in unicode was already resolved.

@leklund
Copy link
Contributor Author

leklund commented Apr 4, 2014

Rebased and squashed. It might be a good idea to add postgresql 9.3 to the travis config if that's possible.

@ignatiusreza
Copy link

Any plan on when this will be merged? I confirm this also..

PR #461 introduced the bug because options.delete(:any) got called twice.. so when the following line is called:

((context and tag_types.one?) && options.delete(:any)) ? request : request.uniq

options.delete(:any) will always return nil..

@seuros seuros added this to the 3.2.0 milestone Apr 19, 2014
@seuros seuros self-assigned this Apr 19, 2014
@seuros
Copy link
Collaborator

seuros commented Apr 19, 2014

@ignatiusreza , once it get rebased and tests are green.

fix bug mbleigh#506 - .uniq breaks with pg and json columns

updated specs for 'select distinct' and pg issues

* new model/table with a json column for postgresql specific testing
* add tests for explicit distinct in a select and for all queries to
  test if tagged_with adds a distinct clause on the query.
  http://github.com/mbleigh/acts-as-taggable-on/issues/357
* add tests for taggable_model_with_json
* exclude test should sort the array result and expected array.
  PostgreSQL won't always return the two expected results in
  the expected order so the test failed intermittently.

use group vs distinct when options[:any] is true

update CHANGELOG

check pg version and only use json if >=9.2

move change to master section

move table with json to spec/internal/db/schema.rb
@bf4
Copy link
Collaborator

bf4 commented Apr 27, 2014

travis ci fix? need help?

@leklund
Copy link
Contributor Author

leklund commented Apr 28, 2014

Looks like travis CI failed because of a ruby gems timeout. I think it will pass otherwise.

@leklund
Copy link
Contributor Author

leklund commented Apr 29, 2014

While working on the app that caused me to discover this bug, I've found that it can return interesting results if you have things tagged with the same 'tag' under two different contexts. I'm slammed at work but should able to get a new test and fix in by early next week. It's an edge case but it bit me pretty hard.

@seuros
Copy link
Collaborator

seuros commented Apr 29, 2014

@leklund, @bf4 should merge your current PR in #517

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

Successfully merging this pull request may close these issues.

using "tagged_with" with .uniq or .select(distinct(...) throws DISTINCT error
4 participants