Fix #4978: -approver:username implicitly adds approver:any
Fix the approver:, parent:, and pixiv: metatags not working correctly when negated:
* Fix -approver:<name> not including posts that don't have an approver (the approver_id is NULL)
* Fix -parent:<id> not including posts that don't have a parent (the parent_id is NULL)
* Fix -pixiv:<id> not including posts that aren't from Pixiv (the pixiv_id is NULL)
The problem lies how the equality operator is negated when the column contains NULL values;
`approver_id != 52664` doesn't match posts where the `approver_id` is NULL.
The search `approver:evazion` boils down to:
# Post.where(approver_id: 52664).to_sql
SELECT * FROM posts WHERE approver_id = 52664;
When that is negated with `-approver:evazion`, it becomes:
# Post.where(approver_id: 52664).invert_where.to_sql
SELECT * FROM posts WHERE approver_id != 52664;
But in SQL, `approver_id != 52664` doesn't match when the approver_id IS NULL, so the search doesn't
include posts without an approver.
We could use `a IS NOT DISTINCT FROM b` instead of `a = b`:
# Post.where(Post.arel_table[:approver_id].is_not_distinct_from(52664)).to_sql
SELECT * FROM posts WHERE approver_id IS NOT DISTINCT FROM 52664;
This way when it's inverted it becomes `IS DISTINCT FROM`:
# Post.where(Post.arel_table[:approver_id].is_not_distinct_from(52664)).invert_where.to_sql
SELECT * FROM posts WHERE approver_id IS NOT DISTINCT FROM 52664;
`approver_id IS DISTINCT FROM 52664` is like `approver_id != 52664`, except it matches when
approver_id is NULL [1].
This works correctly, however the problem is that `IS NOT DISTINCT FROM` can't use indexes because
of a long-standing Postgres limitation [2]. This makes searches too slow. So instead we do this:
# Post.where(approver_id: 52664).where.not(approver_id: nil).to_sql
SELECT * FROM posts WHERE approver_id = 52664 AND approver_id IS NOT NULL;
That way when negated it becomes:
# Post.where(approver_id: 52664).where.not(approver_id: nil).invert_where.to_sql
SELECT * FROM posts WHERE approver_id != 52664 OR approver_id IS NULL;
Which is the correct behavior.
[1] https://modern-sql.com/feature/is-distinct-from
[2] https://www.postgresql.org/message-id/6FC83909-5DB1-420F-9191-DBE533A3CEDE@excoventures.com
This commit is contained in:
@@ -1061,13 +1061,6 @@ class Post < ApplicationRecord
|
||||
end
|
||||
end
|
||||
|
||||
def attribute_matches(value, field, type = :integer)
|
||||
operator, *args = PostQueryBuilder.parse_metatag_value(value, type)
|
||||
where_operator(field, operator, *args)
|
||||
rescue PostQueryBuilder::ParseError
|
||||
none
|
||||
end
|
||||
|
||||
def is_matches(value, current_user = User.anonymous)
|
||||
case value.downcase
|
||||
when "parent"
|
||||
@@ -1148,7 +1141,8 @@ class Post < ApplicationRecord
|
||||
when "pending", "flagged", "appealed", "modqueue", "deleted", "banned", "active", "unmoderated"
|
||||
where.not(parent: nil).where(parent: status_matches(parent))
|
||||
when /\A\d+\z/
|
||||
where(id: parent).or(where(parent: parent))
|
||||
# XXX must use `attribute_matches(parent, :parent_id)` instead of `where(parent_id: parent)` so that `-parent:1` works
|
||||
where(id: parent).or(attribute_matches(parent, :parent_id))
|
||||
else
|
||||
none
|
||||
end
|
||||
@@ -1348,7 +1342,9 @@ class Post < ApplicationRecord
|
||||
else
|
||||
user = User.find_by_name(username)
|
||||
return none if user.nil?
|
||||
where(approver: user)
|
||||
|
||||
# XXX must use `attribute_matches(user.id, :approver_id)` instead of `where(approver: user)` so that `-approver:evazion` works
|
||||
attribute_matches(user.id, :approver_id)
|
||||
end
|
||||
end
|
||||
|
||||
|
||||
Reference in New Issue
Block a user