gensis-config: patching default RuntimeGenesisConfig fixed

[michalkucharczyk]

This PR changes the behavior of json_patch::merge function which no longer removes any keys from the base JSON object.

fixes: #6306

[michalkucharczyk]

/cmd prdoc --bump major --audience node_dev,runtime_dev

[michalkucharczyk]

/cmd prdoc --bump major --audience node_dev,runtime_dev

[bkchr]

Do we really need these two functions? IMO null should not be handled differently than any other kind of value. If someone gives such a json, just accept it.

TLDR: I would just fix the merge function and not introduce a new one.

[michalkucharczyk]

Do we really need these two functions? IMO null should not be handled differently than any other kind of value. If someone gives such a json, just accept it.

Idea of removing null comes from this document https://www.rfc-editor.org/rfc/rfc7386

But yes, we can change the behavior of merge function and introduce merge_allow_removal (which is already used here):

polkadot-sdk/substrate/bin/utils/chain-spec-builder/src/lib.rs

Lines 263 to 277 in a479161

	// We want to extract only raw genesis ("genesis::raw" key), and apply it as a patch
	// for the original json file. However, the file also contains original plain
	// genesis ("genesis::runtimeGenesis") so set it to null so the patch will erase it.
	genesis_json.as_object_mut().map(|map| {
	map.retain(|key, _| key == "genesis");
	map.get_mut("genesis").map(|genesis| {
	genesis.as_object_mut().map(|genesis_map| {
	genesis_map
	.insert("runtimeGenesis".to_string(), serde_json::Value::Null);
	});
	});
	});
	let mut org_chain_spec_json = extract_chain_spec_json(input_chain_spec.as_path())?;
	json_patch::merge(&mut org_chain_spec_json, genesis_json);

We can also change behaviour of merge and remove keys manually when needed.

I just don't want to break downstream which potentially can use json_patch::merge for actually removing fields. I think proposed approach is "safest" from this point of view..

[michalkucharczyk]

/cmd prdoc --bump major --audience node_dev

[bkchr]

Idea of removing null comes from this document https://www.rfc-editor.org/rfc/rfc7386

Yeah I saw this.

I just don't want to break downstream which potentially can use json_patch::merge for actually removing fields. I think proposed approach is "safest" from this point of view..

I mean already break downstream by removing the function export from sc_chain_spec 🙈 I would still vote for simplicity. Question is how many people downstream are using the field removal. We would also need to change the chain-spec-generator to not remove the fields, which is also a behavior change in the program. Generally, I think that merging the null values into the final json makes more sense any way.