Skip to content

Commit

Permalink
filter_editions doesn't support multiple filters
Browse files Browse the repository at this point in the history
I added this "temporary" warning back in May last year, but I forgot to
make it an error until just now.

The code processing the filters is:

    filters.each do |link_type, target_content_id|
      scope = scope.where(links: { link_type:, target_content_id: })
    end

... so the .where() statements would be ANDed together. If you provided
multiple different filters, you'd never get any results.

I've checked the logs in production, and my warning has not been served
in the last 14 days (our log retention), so I'm pretty confident it's
safe to tighten this up.
  • Loading branch information
richardTowers committed Jan 10, 2025
1 parent 9f455ca commit 7d870f9
Showing 1 changed file with 1 addition and 6 deletions.
7 changes: 1 addition & 6 deletions app/models/link.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,7 @@ class Link < ApplicationRecord
validate :association_presence

def self.filter_editions(scope, filters)
if filters.size > 1
# TODO: richard.towers - temporary warning to check whether this method is ever
# called with multiple filters. The code looks wrong, so if it's not being
# called we should make this an error and only support a single filter.
logger.warn("filter_editions called with multiple filters. These will be ANDed together in a way that probably isn't what we want. Filters were: #{filters.inspect}")
end
raise "filter_editions doesn't support multiple filters" if filters.size > 1

scope = scope.joins(document: :link_set_links)

Expand Down

0 comments on commit 7d870f9

Please sign in to comment.