Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Benchmark command fail due to json deserialize error #6306

Closed
2 tasks done
NingLin-P opened this issue Oct 30, 2024 · 4 comments · Fixed by #6382
Closed
2 tasks done

Benchmark command fail due to json deserialize error #6306

NingLin-P opened this issue Oct 30, 2024 · 4 comments · Fixed by #6382
Assignees
Labels
I2-bug The node fails to follow expected behavior. I10-unconfirmed Issue might be valid, but it's not yet known.

Comments

@NingLin-P
Copy link
Contributor

NingLin-P commented Oct 30, 2024

Is there an existing issue?

  • I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

  • This is not a support question.

Description of bug

The benchmark command now requires the genesis-builder-preset arg to get JSON-encoded RuntimeGenesisConfig from the runtime but this value is not used directly, instead it is used to patch the default value of RuntimeGenesisConfig and the result is used:

let mut config = self.get_default_config()?;
crate::json_patch::merge(&mut config, patch);
self.get_storage_for_config(config)

within json_patch::merge if the value is null the whole kv pair will be removed from the JSON:

for (k, v) in b {
if v.is_null() {
a.remove(&k);

this causes issue for enum that contains Option value:

enum Test {
    V(Option<u32>),
}

Test::V(None) will be serialized to {v: null}, after json_patch::merge it became { } and is impossible to deserialize back to the same value

Steps to reproduce

Add this unit test to substrate/client/chain-spec/src/json_patch.rs and it will fail due to deserialize

#[test]
	fn test_null_value() {
		use serde::{Deserialize, Serialize};

		#[derive(Debug, Serialize, Deserialize)]
		enum Test {
			V(Option<u32>),
		}

		let v1 = serde_json::to_value(Test::V(None)).unwrap();
		let mut v2 = serde_json::to_value(Test::V(None)).unwrap();

		merge(&mut v2, v1);

		let j = serde_json::to_string(&v2).expect("serialize should succeed");

		serde_json::from_slice::<Test>(&j.into_bytes()).expect("deserialize should succeed");
	}
@NingLin-P NingLin-P added I10-unconfirmed Issue might be valid, but it's not yet known. I2-bug The node fails to follow expected behavior. labels Oct 30, 2024
@bkchr
Copy link
Member

bkchr commented Nov 5, 2024

CC @michalkucharczyk

@michalkucharczyk
Copy link
Contributor

Thanks for reporting and in-depth analysis of the problem. It all sounds good, removal of keys in the base JSON object which is default is indeed not necessary. The fix proposal is here: #6382

github-merge-queue bot pushed a commit that referenced this issue Nov 7, 2024
This PR changes the behavior of `json_patch::merge` function which no
longer removes any keys from the base JSON object.

fixes: #6306

---------

Co-authored-by: GitHub Action <[email protected]>
@michalkucharczyk
Copy link
Contributor

would you confirm fix does work for you?

@NingLin-P
Copy link
Contributor Author

The fix does works, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I2-bug The node fails to follow expected behavior. I10-unconfirmed Issue might be valid, but it's not yet known.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants