Skip to content

Commit 2e52d54

Browse files
committed
feat(config): best effort resolution for circurlar dependencies
1 parent fa7b5a5 commit 2e52d54

File tree

3 files changed

+63
-32
lines changed

3 files changed

+63
-32
lines changed
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
test:
2+
__include: sometimes?
3+
home: excited
4+
work:
5+
__include: /test/home
6+
7+
sometimes:
8+
work: naive

src/rime/config/config_compiler.cc

+39-32
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ struct ConfigDependencyGraph {
1414
vector<of<ConfigItemRef>> node_stack;
1515
vector<string> key_stack;
1616
map<string, vector<of<Dependency>>> deps;
17+
// paths for checking circular dependencies
18+
vector<string> resolve_chain;
1719

1820
void Add(an<Dependency> dependency);
1921

@@ -311,12 +313,6 @@ an<ConfigResource> ConfigCompiler::Compile(const string& file_name) {
311313
return resource;
312314
}
313315

314-
static inline an<ConfigItem> if_resolved(ConfigCompiler* compiler,
315-
an<ConfigItem> item,
316-
const string& path) {
317-
return item && compiler->resolved(path) ? item : nullptr;
318-
}
319-
320316
static bool ResolveBlockingDependencies(ConfigCompiler* compiler,
321317
const string& path) {
322318
if (!compiler->blocking(path)) {
@@ -335,42 +331,42 @@ static an<ConfigItem> GetResolvedItem(ConfigCompiler* compiler,
335331
const string& path) {
336332
DLOG(INFO) << "GetResolvedItem(" << resource->resource_id << ":" << path << ")";
337333
string node_path = resource->resource_id + ":";
338-
if (!resource || compiler->blocking(node_path)) {
339-
return nullptr;
340-
}
341-
an<ConfigItem> result = *resource;
334+
an<ConfigItemRef> node = resource;
342335
if (path.empty() || path == "/") {
343-
return if_resolved(compiler, result, node_path);
336+
return compiler->ResolveDependencies(node_path) ? **node : nullptr;
344337
}
345338
vector<string> keys = ConfigData::SplitPath(path);
346339
for (const auto& key : keys) {
347-
if (Is<ConfigList>(result)) {
340+
if (!ResolveBlockingDependencies(compiler, node_path)) {
341+
LOG(WARNING) << "accessing blocking node with unresolved dependencies: "
342+
<< node_path;
343+
// CAVEAT: continuing accessing subtree with this failure may result in
344+
// referencing outdated data - sometimes an expected behavior.
345+
// relaxing this requires checking for circular dependencies.
346+
//return nullptr;
347+
}
348+
an<ConfigItem> item = **node;
349+
if (Is<ConfigList>(item)) {
348350
if (ConfigData::IsListItemReference(key)) {
349-
size_t index = ConfigData::ResolveListIndex(result, key, true);
351+
size_t index = ConfigData::ResolveListIndex(item, key, true);
350352
(node_path += "/") += ConfigData::FormatListIndex(index);
351-
if (!ResolveBlockingDependencies(compiler, node_path)) {
352-
return nullptr;
353-
}
354-
result = As<ConfigList>(result)->GetAt(index);
353+
node = New<ConfigListEntryRef>(nullptr, As<ConfigList>(item), index);
355354
} else {
356-
result.reset();
355+
node.reset();
357356
}
358-
} else if (Is<ConfigMap>(result)) {
357+
} else if (Is<ConfigMap>(item)) {
359358
DLOG(INFO) << "advance with key: " << key;
360359
(node_path += "/") += key;
361-
if (!ResolveBlockingDependencies(compiler, node_path)) {
362-
return nullptr;
363-
}
364-
result = As<ConfigMap>(result)->Get(key);
360+
node = New<ConfigMapEntryRef>(nullptr, As<ConfigMap>(item), key);
365361
} else {
366-
result.reset();
362+
node.reset();
367363
}
368-
if (!result) {
369-
LOG(INFO) << "missing node: " << node_path;
364+
if (!node) {
365+
LOG(WARNING) << "inaccessible node: " << node_path << "/" << key;
370366
return nullptr;
371367
}
372368
}
373-
return if_resolved(compiler, result, node_path);
369+
return compiler->ResolveDependencies(node_path) ? **node : nullptr;
374370
}
375371

376372
bool ConfigCompiler::blocking(const string& full_path) const {
@@ -399,11 +395,6 @@ static an<ConfigItem> ResolveReference(ConfigCompiler* compiler,
399395
if (!resource) {
400396
DLOG(INFO) << "resource not loaded, compiling: " << reference.resource_id;
401397
resource = compiler->Compile(reference.resource_id);
402-
// dependency doesn't require full resolution, this allows non conflicting
403-
// mutual references between config files.
404-
// this call is made even if resource is not loaded because plugins can
405-
// edit the empty config data, adding new dependencies.
406-
ResolveBlockingDependencies(compiler, reference.resource_id + ":");
407398
if (!resource->loaded) {
408399
if (reference.optional) {
409400
LOG(INFO) << "optional resource not loaded: " << reference.resource_id;
@@ -492,12 +483,27 @@ bool ConfigCompiler::Link(an<ConfigResource> target) {
492483
(plugin_ ? plugin_->ReviewLinkOutput(this, target) : true);
493484
}
494485

486+
static bool HasCircularDependencies(ConfigDependencyGraph* graph,
487+
const string& path) {
488+
for (const auto& x : graph->resolve_chain) {
489+
if (boost::starts_with(x, path) &&
490+
(x.length() == path.length() || x[path.length()] == '/'))
491+
return true;
492+
}
493+
return false;
494+
}
495+
495496
bool ConfigCompiler::ResolveDependencies(const string& path) {
496497
DLOG(INFO) << "ResolveDependencies(" << path << ")";
497498
auto found = graph_->deps.find(path);
498499
if (found == graph_->deps.end()) {
499500
return true;
500501
}
502+
if (HasCircularDependencies(graph_.get(), path)) {
503+
LOG(WARNING) << "circular dependencies detected in " << path;
504+
return false;
505+
}
506+
graph_->resolve_chain.push_back(path);
501507
auto& deps = found->second;
502508
for (auto iter = deps.begin(); iter != deps.end(); ) {
503509
if (!(*iter)->Resolve(this)) {
@@ -507,6 +513,7 @@ bool ConfigCompiler::ResolveDependencies(const string& path) {
507513
LOG(INFO) << "resolved: " << **iter;
508514
iter = deps.erase(iter);
509515
}
516+
graph_->resolve_chain.pop_back();
510517
DLOG(INFO) << "all dependencies resolved.";
511518
return true;
512519
}

test/config_compiler_test.cc

+16
Original file line numberDiff line numberDiff line change
@@ -243,3 +243,19 @@ TEST_F(RimeConfigMergeTest, MergeTree) {
243243
EXPECT_EQ(6, config_->GetListSize("/starcraft/protoss/ground_units"));
244244
EXPECT_EQ(5, config_->GetListSize("/starcraft/zerg/ground_units"));
245245
}
246+
247+
class RimeConfigCircularDependencyTest : public RimeConfigCompilerTestBase {
248+
protected:
249+
string test_config_id() const override {
250+
return "config_circular_dependency_test";
251+
}
252+
};
253+
254+
TEST_F(RimeConfigCircularDependencyTest, BestEffortResolution) {
255+
const string& prefix = "test/";
256+
EXPECT_TRUE(config_->IsNull(prefix + "__include"));
257+
EXPECT_TRUE(config_->IsNull(prefix + "work/__include"));
258+
string work;
259+
EXPECT_TRUE(config_->GetString(prefix + "work", &work));
260+
EXPECT_EQ("excited", work);
261+
}

0 commit comments

Comments
 (0)