-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Conversation
Build failed, I cancelled it. Apparently travis don't support json which is a native type since 9.2. |
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. |
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 |
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. |
just rebase it, that bug in unicode was already resolved. |
Rebased and squashed. It might be a good idea to add postgresql 9.3 to the travis config if that's possible. |
Any plan on when this will be merged? I confirm this also.. PR #461 introduced the bug because
|
@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
travis ci fix? need help? |
Looks like travis CI failed because of a ruby gems timeout. I think it will pass otherwise. |
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. |
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 aSELECT 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?
.uniq
for any to use a group by the primary key of the table. This avoids the wildcard distinct.tagged_with