diff --git a/data/test/config_circular_dependency_test.yaml b/data/test/config_circular_dependency_test.yaml new file mode 100644 index 000000000..e1af314d3 --- /dev/null +++ b/data/test/config_circular_dependency_test.yaml @@ -0,0 +1,8 @@ +test: + __include: sometimes? + home: excited + work: + __include: /test/home + +sometimes: + work: naive diff --git a/src/rime/config/config_compiler.cc b/src/rime/config/config_compiler.cc index 33ba0754c..9a4ebcd57 100644 --- a/src/rime/config/config_compiler.cc +++ b/src/rime/config/config_compiler.cc @@ -14,6 +14,8 @@ struct ConfigDependencyGraph { vector> node_stack; vector key_stack; map>> deps; + // paths for checking circular dependencies + vector resolve_chain; void Add(an dependency); @@ -311,12 +313,6 @@ an ConfigCompiler::Compile(const string& file_name) { return resource; } -static inline an if_resolved(ConfigCompiler* compiler, - an item, - const string& path) { - return item && compiler->resolved(path) ? item : nullptr; -} - static bool ResolveBlockingDependencies(ConfigCompiler* compiler, const string& path) { if (!compiler->blocking(path)) { @@ -335,42 +331,42 @@ static an GetResolvedItem(ConfigCompiler* compiler, const string& path) { DLOG(INFO) << "GetResolvedItem(" << resource->resource_id << ":" << path << ")"; string node_path = resource->resource_id + ":"; - if (!resource || compiler->blocking(node_path)) { - return nullptr; - } - an result = *resource; + an node = resource; if (path.empty() || path == "/") { - return if_resolved(compiler, result, node_path); + return compiler->ResolveDependencies(node_path) ? **node : nullptr; } vector keys = ConfigData::SplitPath(path); for (const auto& key : keys) { - if (Is(result)) { + if (!ResolveBlockingDependencies(compiler, node_path)) { + LOG(WARNING) << "accessing blocking node with unresolved dependencies: " + << node_path; + // CAVEAT: continuing accessing subtree with this failure may result in + // referencing outdated data - sometimes an expected behavior. + // relaxing this requires checking for circular dependencies. + //return nullptr; + } + an item = **node; + if (Is(item)) { if (ConfigData::IsListItemReference(key)) { - size_t index = ConfigData::ResolveListIndex(result, key, true); + size_t index = ConfigData::ResolveListIndex(item, key, true); (node_path += "/") += ConfigData::FormatListIndex(index); - if (!ResolveBlockingDependencies(compiler, node_path)) { - return nullptr; - } - result = As(result)->GetAt(index); + node = New(nullptr, As(item), index); } else { - result.reset(); + node.reset(); } - } else if (Is(result)) { + } else if (Is(item)) { DLOG(INFO) << "advance with key: " << key; (node_path += "/") += key; - if (!ResolveBlockingDependencies(compiler, node_path)) { - return nullptr; - } - result = As(result)->Get(key); + node = New(nullptr, As(item), key); } else { - result.reset(); + node.reset(); } - if (!result) { - LOG(INFO) << "missing node: " << node_path; + if (!node) { + LOG(WARNING) << "inaccessible node: " << node_path << "/" << key; return nullptr; } } - return if_resolved(compiler, result, node_path); + return compiler->ResolveDependencies(node_path) ? **node : nullptr; } bool ConfigCompiler::blocking(const string& full_path) const { @@ -399,11 +395,6 @@ static an ResolveReference(ConfigCompiler* compiler, if (!resource) { DLOG(INFO) << "resource not loaded, compiling: " << reference.resource_id; resource = compiler->Compile(reference.resource_id); - // dependency doesn't require full resolution, this allows non conflicting - // mutual references between config files. - // this call is made even if resource is not loaded because plugins can - // edit the empty config data, adding new dependencies. - ResolveBlockingDependencies(compiler, reference.resource_id + ":"); if (!resource->loaded) { if (reference.optional) { LOG(INFO) << "optional resource not loaded: " << reference.resource_id; @@ -492,12 +483,27 @@ bool ConfigCompiler::Link(an target) { (plugin_ ? plugin_->ReviewLinkOutput(this, target) : true); } +static bool HasCircularDependencies(ConfigDependencyGraph* graph, + const string& path) { + for (const auto& x : graph->resolve_chain) { + if (boost::starts_with(x, path) && + (x.length() == path.length() || x[path.length()] == '/')) + return true; + } + return false; +} + bool ConfigCompiler::ResolveDependencies(const string& path) { DLOG(INFO) << "ResolveDependencies(" << path << ")"; auto found = graph_->deps.find(path); if (found == graph_->deps.end()) { return true; } + if (HasCircularDependencies(graph_.get(), path)) { + LOG(WARNING) << "circular dependencies detected in " << path; + return false; + } + graph_->resolve_chain.push_back(path); auto& deps = found->second; for (auto iter = deps.begin(); iter != deps.end(); ) { if (!(*iter)->Resolve(this)) { @@ -507,6 +513,7 @@ bool ConfigCompiler::ResolveDependencies(const string& path) { LOG(INFO) << "resolved: " << **iter; iter = deps.erase(iter); } + graph_->resolve_chain.pop_back(); DLOG(INFO) << "all dependencies resolved."; return true; } diff --git a/test/config_compiler_test.cc b/test/config_compiler_test.cc index 1e3f23faf..5e412ac8b 100644 --- a/test/config_compiler_test.cc +++ b/test/config_compiler_test.cc @@ -243,3 +243,19 @@ TEST_F(RimeConfigMergeTest, MergeTree) { EXPECT_EQ(6, config_->GetListSize("/starcraft/protoss/ground_units")); EXPECT_EQ(5, config_->GetListSize("/starcraft/zerg/ground_units")); } + +class RimeConfigCircularDependencyTest : public RimeConfigCompilerTestBase { + protected: + string test_config_id() const override { + return "config_circular_dependency_test"; + } +}; + +TEST_F(RimeConfigCircularDependencyTest, BestEffortResolution) { + const string& prefix = "test/"; + EXPECT_TRUE(config_->IsNull(prefix + "__include")); + EXPECT_TRUE(config_->IsNull(prefix + "work/__include")); + string work; + EXPECT_TRUE(config_->GetString(prefix + "work", &work)); + EXPECT_EQ("excited", work); +}