mirror of
https://github.com/discourse/discourse.git
synced 2026-08-02 01:28:14 -05:00
DEV: Detect circular site setting dependencies (#38165)
### Background In #36061, dependent site settings were introduced through the `depends_on` key. This is used internally to construct a dependency graph and sort settings in order-of-dependency before bulk updates. However, no safeguards were introduced to avoid circular dependencies. This means a `TSort::Cyclic` could come along and ruin the day at arbitrary points during runtime. ### What is this change? This change does two things: 1. Rescue, format, and re-raise `TSort::Cyclic` exceptions. 2. Eagerly order settings on app reload to catch cyclic dependencies up-front.
This commit is contained in:
@@ -23,6 +23,9 @@ Rails.application.config.to_prepare do
|
||||
SiteSetting.push_api_secret_key.length == 32
|
||||
SiteSetting.push_api_secret_key = SecureRandom.hex
|
||||
end
|
||||
|
||||
# Check for circular dependencies in site settings.
|
||||
SiteSetting.type_supervisor.dependencies.order
|
||||
rescue ActiveRecord::StatementInvalid
|
||||
# This will happen when migrating a new database
|
||||
end
|
||||
|
||||
@@ -6,6 +6,8 @@ end
|
||||
class SiteSettings::DependencyGraph
|
||||
include TSort
|
||||
|
||||
CircularDependency = Class.new(StandardError)
|
||||
|
||||
attr_reader :dependencies, :behaviors
|
||||
|
||||
def initialize(dependencies = {})
|
||||
@@ -44,6 +46,15 @@ class SiteSettings::DependencyGraph
|
||||
|
||||
def order
|
||||
@order ||= tsort
|
||||
rescue TSort::Cyclic
|
||||
cycles =
|
||||
strongly_connected_components.reject(&:one?).map { |cycle| cycle.join(" <-> ") }.join("\n")
|
||||
|
||||
raise CircularDependency.new(<<~MESSAGE)
|
||||
Circular dependencies in site settings:
|
||||
|
||||
#{cycles}
|
||||
MESSAGE
|
||||
end
|
||||
|
||||
private
|
||||
|
||||
@@ -159,7 +159,7 @@ class SiteSettings::TypeSupervisor
|
||||
@validators[name] = { class: validator_type, opts: validator_opts }
|
||||
end
|
||||
|
||||
@dependencies[name] = opts[:depends_on] || []
|
||||
@dependencies[name] = (opts[:depends_on] || []).map(&:to_sym)
|
||||
@dependencies.change_behavior(name, opts[:depends_behavior]) if opts[:depends_behavior]
|
||||
end
|
||||
|
||||
|
||||
@@ -16,6 +16,21 @@ RSpec.describe SiteSettings::DependencyGraph do
|
||||
it "topologically sorts the dependencies" do
|
||||
expect(depencency_graph.order).to match_array(%i[foo qux bar baz quux])
|
||||
end
|
||||
|
||||
context "with circular dependencies" do
|
||||
let(:dependencies) { { foo: [:bar], bar: [:foo] } }
|
||||
|
||||
it "raises an exception" do
|
||||
expect { depencency_graph.order }.to raise_error(
|
||||
described_class::CircularDependency,
|
||||
<<~MESSAGE,
|
||||
Circular dependencies in site settings:
|
||||
|
||||
foo <-> bar
|
||||
MESSAGE
|
||||
)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
describe "#dependents" do
|
||||
|
||||
@@ -128,6 +128,8 @@ RSpec.describe SiteSetting::Update do
|
||||
end
|
||||
|
||||
context "when updating dependent settings" do
|
||||
before { SiteSetting.allow_user_locale = false }
|
||||
|
||||
let(:settings) do
|
||||
[
|
||||
# The first setting depends on the second one, which will fail
|
||||
|
||||
Reference in New Issue
Block a user