Add `ConfigAggregator` for merging config state across multiple files by tarek-y-ismail · Pull Request #4830 · canonical/mir

tarek-y-ismail · 2026-04-06T16:21:52Z

Related: #4799

ConfigAggregator is a new Store implementation that merges configuration from multiple files into a single consistent view. It takes a ParsingStore (e.g. an IniFile) and a list of streams, parses each in order, accumulates all values, and dispatches handlers exactly once with the final merged result.

This is the intended way to support layered configuration — a base file plus a drop-in directory of overrides — without callers having to manage accumulation themselves.

What's new

New ConfigAggregator class implementing Store, which takes a ParsingStore and a list of config streams via load_all(). Later files take precedence for scalar keys; array keys accumulate across files and can be cleared with an empty assignment.
New ParsingStore abstract interface extending Store with a load_file() method, as a common base for format-specific parsers. IniFile now implements ParsingStore.
BasicStore extracted from IniFile to provide the reusable attribute registration, typed storage, and dispatch logic that ConfigAggregator builds on.

How to test

Existing IniFile tests continue to pass unchanged.
New ConfigAggregator tests cover scalar and array attribute handling, multi-file merging and override precedence, preset values, empty-value array clearing, done handlers, and error cases.

Checklist

Tests added and pass

Saviq

There's a lot of comments saying "wins… holds invalid value… nullopt", but test expects that not to be true?

Do we have a test for the main file providing invalid values - and that baiing, as it does today?

I would also like a test with:

# base.conf
array=foo
# base.conf.d/10-override.conf
array=bar
{ "invalid": "stuff" }
array=baz

This should keep [foo].

tarek-y-ismail · 2026-04-10T09:18:38Z

Do we have a test for the main file providing invalid values - and that baiing, as it does today?

Isn't the base config being valid a core assumption? With overrides, users don't have do touch that, so it shouldn't get corrupted anyway.

I would also like a test with:
# base.conf
array=foo
# base.conf.d/10-override.conf
array=bar
{ "invalid": "stuff" }
array=baz
This should keep [foo].

We do our parsing line by line, so in this case it will go like so:

# base.conf
# `aggregator.array = []`
array=foo  # base.array = [foo]
# End of base config, aggregator notified of `array += [foo]`. `aggregator.array = [foo]`

# base.conf.d/10-override.conf
array=bar # `10-override.array=[bar]`
{ "invalid": "stuff" } # Invalid format, ignored
array=baz # `10-override.array=[bar,baz]`
# end of override file, aggregator notified of `array += [bar,baz]`, `aggregator.array = [foo,bar,baz]`

We already have a couple of test cases that test simpler cases, but I'll add this one for completeness sake.

tarek-y-ismail · 2026-04-10T09:30:42Z

+TEST_F(ConfigAggregatorTest, invalid_scalar_value_in_one_of_multiple_yields_last_valid_value_2)
+{
+    aggregator.add_int_attribute(a_key, "a scoped int", [this](auto... args) { int_handler(args...); });
+
+    std::istringstream stream1{a_key.to_string() + "=10\n" + a_key.to_string() + "=20\n"};
+    std::istringstream stream2{a_key.to_string() + "=30\n" + a_key.to_string() + "=not_a_number\n"};
+
+    EXPECT_CALL(*this, int_handler(a_key, Optional(20)));
+
+    load_all({{std::ref(stream1), "base.conf"},
+              {std::ref(stream2), "base.conf.d/10-override.conf"}});
+}


This shouldn't be the case. Instead, the final value should be 30 (we ignore just the invalid line), or the following test has the final value of 20 (we ignore the key for the whole file)

Agree, this should yield 30.

Saviq · 2026-04-10T10:35:41Z

Isn't the base config being valid a core assumption? With overrides, users don't have do touch that, so it shouldn't get corrupted anyway.

I'm just asking if we have a test for that. Not everyone will be using overrides.

This should keep [foo].

# end of override file, aggregator notified of `array += [bar,baz]`, `aggregator.array = [foo,bar,baz]`

That's wrong, then - the whole file needs to be rejected on parsing errors (parsing comes before validation).

tarek-y-ismail · 2026-04-23T16:41:19Z

Array clearing was dropped somewhere along the way. Re-implemented it again, though For the sake of keeping this PR focused on changes needed for ConfigAggregator, I'll probably split BasicStore and IniFile changes into a separate PR, as this one is already quite big.

ConfigAggregator merges configuration state across multiple files by combining a set of Sources (for format-specific parsing) with a BasicStore (for config value accumulation). Attribute registrations are forwarded to both: the parser accumulates raw string values into the aggregated store, and the store holds the user handlers and presets. load_all() clears prior state, loads each file in order through the parser, then dispatches all handlers once with the merged result and calls done handlers with the list of loaded paths.

Copilot

Pull request overview

This PR introduces a new MirAL live-config utility (ConfigAggregator) intended to provide a single merged configuration view from multiple layered config sources (base + drop-ins), plus supporting API changes to enable array “initial values” propagation during parsing.

Changes:

Added miral::live_config::ConfigAggregator (new Store implementation) and exported it in the ABI map/symbols.
Extended the live_config::Store interface with add_strings_attribute(..., initial_values) overloads and implemented them in IniFile/BasicStore.
Added a new miral-test unit test suite covering multi-file merge behavior for scalar/array keys.

Reviewed changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
`include/miral/miral/config_aggregator.h`	Public API for the new `ConfigAggregator` and `Source` model.
`src/miral/config_aggregator.cpp`	Implementation of config merging across multiple sources using `BasicStore` transactions.
`include/miral/miral/live_config.h`	Adds new `Store` overloads for array attributes with `initial_values`.
`include/miral/miral/live_config_ini_file.h` / `src/miral/live_config_ini_file.cpp`	Implements the new `Store` overloads in `IniFile`.
`src/miral/basic_store.h` / `src/miral/basic_store.cpp`	Adds array-setting and attribute enumeration helpers used by the aggregator.
`src/miral/symbols.map` / `debian/libmiral7.symbols`	Exports the new API under `MIRAL_5.8`.
`tests/miral/config_aggregator.cpp`	New gtest/gmock coverage for layered merging semantics.
`tests/miral/CMakeLists.txt` / `src/miral/CMakeLists.txt`	Builds/links the new implementation and tests.

absent from override

AlanGriffiths

I think this is trying to do too many things and loses sight of the end goal. Let's talk Monday

AlanGriffiths · 2026-04-30T15:25:43Z

+    virtual void add_strings_attribute(
+        Key const& key,
+        std::string_view description,
+        HandleStrings handler,
+        std::span<std::string const> initial_values) = 0;
+    virtual void add_strings_attribute(
+        Key const& key,
+        std::string_view description,
+        std::span<std::string const> preset,
+        HandleStrings handler,
+        std::span<std::string const> initial_values) = 0;
+


These functions do not belong in the Store (or IniFile) APIs.

They are not part of the Store abstraction used to subscribe to configuration

They are an implementation detail of ConfigAggregator

Adding to the vtab breaks ABI

AlanGriffiths · 2026-04-30T15:28:06Z

+    struct Source
+    {
+        std::shared_ptr<Store> store;
+        std::move_only_function<void()> load;
+        std::filesystem::path file_path;
+    };


Having structs like this in the API is a recipe for unstable APIs.

AlanGriffiths · 2026-04-30T15:39:17Z

+
+    void add_source(Source&& source);
+    void update_source(Source&& source);
+    void remove_source(std::filesystem::path const& path);


This might make sense in an internal API, but is not what we want as a public API for this.

The end user shouldn't need to deal with sources, they should simply specify the type the sources and the files to monitor/aggregate. Any adding or deleting of sources should be done internally in response to filesystem changes.

AlanGriffiths · 2026-04-30T15:47:27Z

+    using ForeachAttributeCallback =
+        std::function<void(Key const&, std::string_view, std::optional<std::string> const& preset)>;
+    using ForeachArrayCallback = std::function<void(
+        Key const&,
+        std::string_view,
+        std::span<std::string const> current_value,
+        std::optional<std::span<std::string const>> preset)>;
+
+    void foreach_attribute(ForeachAttributeCallback const& callback) const;
+    void foreach_array_attribute(ForeachArrayCallback const& callback) const;


These functions appear to poking holes in the BasicStore abstraction - which suggests the abstraction is wrong. Or that we need a different abstraction.

Specifically, there appears to be a missing AttributesCollection that both BasicStore and ConfigAggregator can use to maintain a list of attributes. (Instead of poking a hole in BasicStore so that ConfigAggregator can get at the attributes.)

AlanGriffiths · 2026-04-30T15:47:51Z

+    void foreach_array_attribute(ForeachArrayCallback const& callback) const;
+
    void update_key(Key const& key, std::string_view value, std::filesystem::path const& modification_path);
+    void set_array(Key const& key, std::optional<std::span<std::string const>> value, std::filesystem::path const& modification_path);


These functions appear to poking holes in the BasicStore abstraction - which suggests the abstraction is wrong. Or that we need a different abstraction.

Specifically, BasicStore is not basic. It has IniFile specific logic for accumulating arrays. This set_array() function is an alternative implementation, not one that belongs in the same class.

Maybe we can "push down" the IniFile specifics into BasicStore::Self and leave something truly basic?

Copilot

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.

Copilot · 2026-04-30T16:06:57Z

+        std::span<std::string const> initial_values) = 0;
+    virtual void add_strings_attribute(
+        Key const& key,
+        std::string_view description,
+        std::span<std::string const> preset,
+        HandleStrings handler,
+        std::span<std::string const> initial_values) = 0;


Adding new pure-virtual overloads to miral::live_config::Store changes the required interface for all Store implementations and (because Store is a public virtual type with exported vtable/typeinfo) is an ABI-breaking change for out-of-tree implementers. If this is intended to remain ABI-stable within libmiral.so.7, consider introducing a separate derived interface for the new array-initial-values behavior (or providing a non-pure default implementation) rather than extending Store directly.

Suggested change

std::span<std::string const> initial_values) = 0;

virtual void add_strings_attribute(

Key const& key,

std::string_view description,

std::span<std::string const> preset,

HandleStrings handler,

std::span<std::string const> initial_values) = 0;

std::span<std::string const> initial_values)

{

static_cast<void>(initial_values);

add_strings_attribute(key, description, std::move(handler));

}

virtual void add_strings_attribute(

Key const& key,

std::string_view description,

std::span<std::string const> preset,

HandleStrings handler,

std::span<std::string const> initial_values)

{

static_cast<void>(initial_values);

add_strings_attribute(key, description, preset, std::move(handler));

}

Copilot · 2026-04-30T16:06:57Z

+    struct Source
+    {
+        std::shared_ptr<Store> store;
+        std::move_only_function<void()> load;
+        std::filesystem::path file_path;
+    };


PR description mentions a new ParsingStore interface, but there is no ParsingStore type in the tree and ConfigAggregator::Source accepts a std::shared_ptr<Store> instead. Either the description needs updating, or the intended ParsingStore abstraction should be added (especially if the goal was to avoid extending Store’s virtual interface).

tarek-y-ismail mentioned this pull request Apr 6, 2026

Live config ini file changes #4799

Closed

1 task

tarek-y-ismail force-pushed the add-config-aggregator branch from 5ad80ad to 57cb0ff Compare April 6, 2026 16:47

tarek-y-ismail changed the base branch from main to extract-basic-store April 6, 2026 16:50

tarek-y-ismail force-pushed the extract-basic-store branch from d72b128 to 1b9e5af Compare April 6, 2026 16:53

tarek-y-ismail force-pushed the add-config-aggregator branch from 6665b12 to 617b76d Compare April 6, 2026 16:56

tarek-y-ismail self-assigned this Apr 6, 2026

tarek-y-ismail force-pushed the extract-basic-store branch from ae0e7ca to b64279e Compare April 6, 2026 17:11

tarek-y-ismail force-pushed the add-config-aggregator branch from 617b76d to 1c9d2dc Compare April 7, 2026 10:45

tarek-y-ismail mentioned this pull request Apr 7, 2026

Extract BasicStore from IniFile #4829

Merged

1 task

tarek-y-ismail force-pushed the extract-basic-store branch from b5a7472 to cda1c1f Compare April 8, 2026 15:30

tarek-y-ismail force-pushed the add-config-aggregator branch from 1c9d2dc to 0b35184 Compare April 8, 2026 15:45

Saviq requested changes Apr 9, 2026

View reviewed changes

Comment thread tests/miral/config_aggregator.cpp Outdated

Comment thread tests/miral/config_aggregator.cpp Outdated

tarek-y-ismail force-pushed the add-config-aggregator branch from ffa3585 to 2f5f488 Compare April 10, 2026 09:28

tarek-y-ismail commented Apr 10, 2026

View reviewed changes

tarek-y-ismail force-pushed the extract-basic-store branch 6 times, most recently from adec2d2 to ae2261b Compare April 14, 2026 15:28

Base automatically changed from extract-basic-store to main April 15, 2026 08:24

tarek-y-ismail force-pushed the add-config-aggregator branch from 2f5f488 to 9296cf2 Compare April 15, 2026 11:32

tarek-y-ismail changed the base branch from main to split-parsing-from-basic-store April 15, 2026 11:33

tarek-y-ismail force-pushed the add-config-aggregator branch from 9296cf2 to 019c166 Compare April 16, 2026 15:06

tarek-y-ismail mentioned this pull request Apr 16, 2026

Add ConfigFileStoreAdapter to bridge ConfigFile to ConfigAggregator #4858

Draft

3 tasks

tarek-y-ismail force-pushed the split-parsing-from-basic-store branch from 325426d to bd05301 Compare April 16, 2026 15:18

tarek-y-ismail force-pushed the add-config-aggregator branch 2 times, most recently from 7563575 to 9e50ae8 Compare April 16, 2026 15:53

tarek-y-ismail force-pushed the split-parsing-from-basic-store branch 3 times, most recently from 1c0210c to 2f6d28d Compare April 22, 2026 14:59

tarek-y-ismail mentioned this pull request Apr 23, 2026

Split parsing out from BasicStore #4850

Closed

tarek-y-ismail force-pushed the add-config-aggregator branch from 8a9431f to 9d9d66e Compare April 23, 2026 15:13

tarek-y-ismail commented Apr 23, 2026

View reviewed changes

Comment thread src/miral/symbols.map

tarek-y-ismail force-pushed the split-parsing-from-basic-store branch from c72ce7f to 1a4c0a1 Compare April 23, 2026 17:41

tarek-y-ismail force-pushed the add-config-aggregator branch from 04b180d to 7b331b4 Compare April 23, 2026 18:18

tarek-y-ismail commented Apr 24, 2026

View reviewed changes

Comment thread src/miral/symbols.map

Add BasicStore foreach_string_attribute methods

b9f889f

tarek-y-ismail force-pushed the add-config-aggregator branch from 7b331b4 to cf9c1d3 Compare April 30, 2026 14:31

tarek-y-ismail changed the base branch from split-parsing-from-basic-store to main April 30, 2026 14:31

tarek-y-ismail force-pushed the add-config-aggregator branch from cf9c1d3 to 5f707f5 Compare April 30, 2026 14:57

tarek-y-ismail marked this pull request as ready for review April 30, 2026 14:57

Copilot AI review requested due to automatic review settings April 30, 2026 14:57

Copilot started reviewing on behalf of tarek-y-ismail April 30, 2026 14:58 View session

tarek-y-ismail added 2 commits April 30, 2026 18:01

Add tests for ConfigAggregator

0093d49

tarek-y-ismail force-pushed the add-config-aggregator branch from 5f707f5 to 0093d49 Compare April 30, 2026 15:01

Copilot AI reviewed Apr 30, 2026

View reviewed changes

Comment thread src/miral/config_aggregator.cpp Outdated

Comment thread src/miral/config_aggregator.cpp

Comment thread src/miral/basic_store.h Outdated

Comment thread src/miral/basic_store.cpp Outdated

Comment thread src/miral/basic_store.cpp

tarek-y-ismail added 4 commits April 30, 2026 18:27

Update symbols for miral::live_config changes in MIRAL_5.8 section

d57acb8

Fix ConfigAggregator scalar preset overwriting base value when key is

50e3ec4

absent from override

Format set_array declaration/definition to follow line length limit

2d5900a

Avoid duplicate entries in modification_paths

7377660

tarek-y-ismail requested review from AlanGriffiths and Copilot April 30, 2026 16:00

Copilot started reviewing on behalf of tarek-y-ismail April 30, 2026 16:00 View session

AlanGriffiths requested changes Apr 30, 2026

View reviewed changes

Copilot AI reviewed Apr 30, 2026

View reviewed changes

Conversation

tarek-y-ismail commented Apr 6, 2026

What's new

How to test

Checklist

Uh oh!

Saviq left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tarek-y-ismail commented Apr 10, 2026

Uh oh!

tarek-y-ismail Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Saviq Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Saviq commented Apr 10, 2026

Uh oh!

Uh oh!

tarek-y-ismail commented Apr 23, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlanGriffiths left a comment

Choose a reason for hiding this comment

Uh oh!

AlanGriffiths Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

AlanGriffiths Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

AlanGriffiths Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

AlanGriffiths Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

AlanGriffiths Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants