fix(configs): fill in missing values in ExperimentalConfig with defaults #10472
Conversation
the command `neard --home {home_dir} init --chain-id mainnet --download-config`
downloads the config at
https://s3-us-west-1.amazonaws.com/build.nearprotocol.com/nearcore-deploy/mainnet/config.json
and then saves a config file built from that one to
{home_dir}/config.json. This is expected to work even when particular
fields are missing by filling them in with defaults, which is why many
config fields are marked with some sort of #[serde(default)] or
doesn't fill in these defaults. The existing #[serde(default)] over
that field in `near_network::config_json::Config` will have us fill it
in if it's totally missing, but we get an error if it's present with
some fields missing, which is the case today:
Error: Failed to initialize configs
Caused by:
config.json file issue: Failed to deserialize config from /tmp/n/config.json: Error("missing field `network_config_overrides`", line: 98, column: 5)
Fix it by adding a #[serde(default)] to the ExperimentalConfig struct
itself, which is how, for example, the fact that the `archive` field
is missing in the above config url is not a problem, because
`nearcore::config::Config` is marked with this attribute.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #10472 +/- ##
==========================================
- Coverage 71.97% 71.96% -0.01%
==========================================
Files 720 720
Lines 145126 145138 +12
Branches 145126 145138 +12
==========================================
Hits 104455 104455
- Misses 35870 35884 +14
+ Partials 4801 4799 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
chain/network/src/config_json.rs
Outdated
| @@ -197,6 +197,7 @@ pub struct Config { | |||
| } | |||
|
|
|||
| #[derive(serde::Serialize, serde::Deserialize, Clone, Debug)] | |||
| #[serde(default)] | |||
There was a problem hiding this comment.
I think this is a bit risky. It is easy to add a flag where a default value (false, 0 etc.) is not a “disabled experiment”. Can we make the following change instead?
-pub experimental: ExperimentalConfig,
+pub experimental: Option<ExperimentalConfig>,There was a problem hiding this comment.
Or if not, then at least lets annotate each field individually as is done in Config above.
There was a problem hiding this comment.
ohh yea that is a good thing to think of, good point. Although, are we not protected by the explicit listing out of each struct field in the impl Default for ExperimentalConfig? Like, if I make this change:
diff --git a/chain/network/src/config_json.rs b/chain/network/src/config_json.rs
index 1225e32ae..2a327bd7f 100644
--- a/chain/network/src/config_json.rs
+++ b/chain/network/src/config_json.rs
@@ -226,6 +226,7 @@ pub struct ExperimentalConfig {
/// See `NetworkConfig`.
/// Fields set here will override the NetworkConfig fields.
pub network_config_overrides: NetworkConfigOverrides,
+ pub behave_properly: bool,
}
/// Overrides values from NetworkConfig.and then try to build it, I get this:
% cargo build -p neard
Compiling near-network v0.0.0 (/Users/marcelo/nearcore/chain/network)
error[E0063]: missing field `behave_properly` in initializer of `ExperimentalConfig`
--> chain/network/src/config_json.rs:251:9
|
251 | ExperimentalConfig {
| ^^^^^^^^^^^^^^^^^^ missing `behave_properly`
For more information about this error, try `rustc --explain E0063`.
error: could not compile `near-network` (lib) due to previous error
But maybe there is some other way this could go wrong... @nagisa wdyt?
There was a problem hiding this comment.
My concern is more that people might forget to add the field into the config file(s), which might very read
{"...": "...","experimental": {}, "...": "..." }for a large majority of users.
Picking a wrong default value by itself is still a bug in implementation of course, but when #[serde(default)] is on top of the struct it can be a little difficult for a developer to realize that such defaulting can occur at all. Meanwhile if you had to add #[serde(default)] on top of the field you’d think about what it is doing there, and if you forgot to add it tests would fail with parse errors or something.
There was a problem hiding this comment.
yeah agreed makes it more obvious if the fields are the ones marked w #[serde(default)]. updated it, PTAL. Went with that instead of the experimental: Option<ExperimentalConfig> because now at least we don't have to change stuff where it's referred to, but if you can think of a reason that would be better, we could just do it that way too.
also while we're lookin at this kind of thing, looks like near_store::config::StoreConfig uses this #[serde(default)] over the struct pattern too..
…lts (#10472) the command `neard --home {home_dir} init --chain-id mainnet --download-config` downloads the config at https://s3-us-west-1.amazonaws.com/build.nearprotocol.com/nearcore-deploy/mainnet/config.json and then saves a config file built from that one to {home_dir}/config.json. This is expected to work even when particular fields are missing by filling them in with defaults, which is why many config fields are marked with some sort of `#[serde(default)]` or `#[serde(default = default_fn)]`. But the `ExperimentalConfig` doesn't fill in these defaults. The existing `#[serde(default)]` over that field in `near_network::config_json::Config` will have us fill it in if it's totally missing, but we get an error if it's present with some fields missing, which is the case today: ``` Error: Failed to initialize configs Caused by: config.json file issue: Failed to deserialize config from /tmp/n/config.json: Error("missing field `network_config_overrides`", line: 98, column: 5) ``` Fix it by adding a `#[serde(default)]` to each of the fields in `ExperimentalConfig`
the command
neard --home {home_dir} init --chain-id mainnet --download-configdownloads the config athttps://s3-us-west-1.amazonaws.com/build.nearprotocol.com/nearcore-deploy/mainnet/config.json and then saves a config file built from that one to {home_dir}/config.json. This is expected to work even when particular fields are missing by filling them in with defaults, which is why many config fields are marked with some sort of
#[serde(default)]or#[serde(default = default_fn)]. But theExperimentalConfigdoesn't fill in these defaults. The existing#[serde(default)]over that field innear_network::config_json::Configwill have us fill it in if it's totally missing, but we get an error if it's present with some fields missing, which is the case today:Fix it by adding a
#[serde(default)]to each of the fields inExperimentalConfig