FIX: TopicSummarization workaround for Postgres' discrete range types (#23105)

Our code assumed the content_range interval was inclusive, but they are open-ended due to Postgres' [discrete range types](https://www.postgresql.org/docs/current/rangetypes.html#RANGETYPES-DISCRETE), meaning [1,2] will be represented as [1,3).

It also fixes some flaky tests due to test data not being correctly setup and the registry not being resetted after each test.
This commit is contained in:
Roman Rizzi 2023-08-15 14:16:06 -03:00 committed by GitHub
parent eb4971cb06
commit 5683c90917
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 43 additions and 9 deletions

View File

@ -8,6 +8,11 @@ class TopicSummarySerializer < ApplicationSerializer
end
def new_posts_since_summary
object.target.highest_post_number.to_i - object.content_range&.end.to_i
# Postgres uses discrete range types for int4range, which means
# an inclusive [1,2] ranges is stored as [1,3). To work around this
# an provide an accurate count, we do the following:
range_end = [object.content_range&.end.to_i - 1, 0].max
object.target.highest_post_number.to_i - range_end
end
end

View File

@ -116,7 +116,10 @@ class TopicSummarization
)
end
main_summary
# Calling reload here ensures Postgres' discrete range type is applied.
# an inclusive [1,2] ranges is stored as [1,3).
# Read more about this here: https://www.postgresql.org/docs/current/rangetypes.html#RANGETYPES-DISCRETE
main_summary.reload
end
def build_sha(ids)

View File

@ -8,7 +8,7 @@ RSpec.describe Chat::Api::SummariesController do
before do
group.add(current_user)
strategy = DummyCustomSummarization.new("dummy")
strategy = DummyCustomSummarization.new({ summary: "dummy", chunks: [] })
plugin.register_summarization_strategy(strategy)
SiteSetting.summarization_strategy = strategy.model
SiteSetting.custom_summarization_allowed_groups = group.id
@ -18,6 +18,8 @@ RSpec.describe Chat::Api::SummariesController do
sign_in(current_user)
end
after { DiscoursePluginRegistry.reset_register!(:summarization_strategies) }
describe "#get_summary" do
context "when the user is not allowed to join the channel" do
fab!(:channel) { Fabricate(:private_category_channel) }

View File

@ -4,7 +4,9 @@ RSpec.describe Jobs::StreamTopicSummary do
subject(:job) { described_class.new }
describe "#execute" do
fab!(:topic) { Fabricate(:topic) }
fab!(:topic) { Fabricate(:topic, highest_post_number: 2) }
fab!(:post_1) { Fabricate(:post, topic: topic, post_number: 1) }
fab!(:post_2) { Fabricate(:post, topic: topic, post_number: 2) }
let(:plugin) { Plugin::Instance.new }
let(:strategy) { DummyCustomSummarization.new({ summary: "dummy", chunks: [] }) }
fab!(:user) { Fabricate(:leader) }
@ -16,6 +18,8 @@ RSpec.describe Jobs::StreamTopicSummary do
SiteSetting.summarization_strategy = strategy.model
end
after { DiscoursePluginRegistry.reset_register!(:summarization_strategies) }
describe "validates params" do
it "does nothing if there is no topic" do
messages =

View File

@ -10,11 +10,13 @@ describe Summarization::Base do
before do
group.add(user)
strategy = DummyCustomSummarization.new("dummy")
strategy = DummyCustomSummarization.new({ summary: "dummy", chunks: [] })
plugin.register_summarization_strategy(strategy)
SiteSetting.summarization_strategy = strategy.model
end
after { DiscoursePluginRegistry.reset_register!(:summarization_strategies) }
describe "#can_see_summary?" do
context "when the user cannot generate a summary" do
before { SiteSetting.custom_summarization_allowed_groups = "" }

View File

@ -5506,7 +5506,9 @@ RSpec.describe TopicsController do
end
describe "#summary" do
fab!(:topic) { Fabricate(:topic) }
fab!(:topic) { Fabricate(:topic, highest_post_number: 2) }
fab!(:post_1) { Fabricate(:post, topic: topic, post_number: 1) }
fab!(:post_2) { Fabricate(:post, topic: topic, post_number: 2) }
let(:plugin) { Plugin::Instance.new }
let(:strategy) { DummyCustomSummarization.new({ summary: "dummy", chunks: [] }) }
@ -5515,6 +5517,8 @@ RSpec.describe TopicsController do
SiteSetting.summarization_strategy = strategy.model
end
after { DiscoursePluginRegistry.reset_register!(:summarization_strategies) }
context "for anons" do
it "returns a 404 if there is no cached summary" do
get "/t/#{topic.id}/strategy-summary.json"
@ -5577,6 +5581,20 @@ RSpec.describe TopicsController do
expect(summary["can_regenerate"]).to eq(true)
expect(summary["new_posts_since_summary"]).to be_zero
end
it "signals the summary is outdated" do
get "/t/#{topic.id}/strategy-summary.json"
Fabricate(:post, topic: topic, post_number: 3)
topic.update!(highest_post_number: 3)
get "/t/#{topic.id}/strategy-summary.json"
expect(response.status).to eq(200)
summary = response.parsed_body["topic_summary"]
expect(summary["outdated"]).to eq(true)
expect(summary["new_posts_since_summary"]).to eq(1)
end
end
context "when the user is not a member of an allowlisted group" do

View File

@ -1,10 +1,10 @@
# frozen_string_literal: true
describe TopicSummarization do
fab!(:topic) { Fabricate(:topic) }
fab!(:user) { Fabricate(:admin) }
fab!(:post_1) { Fabricate(:post, topic: topic) }
fab!(:post_2) { Fabricate(:post, topic: topic) }
fab!(:topic) { Fabricate(:topic, highest_post_number: 2) }
fab!(:post_1) { Fabricate(:post, topic: topic, post_number: 1) }
fab!(:post_2) { Fabricate(:post, topic: topic, post_number: 2) }
shared_examples "includes only public-visible topics" do
subject { described_class.new(DummyCustomSummarization.new({})) }