feat!: Support default field for array and map#467
Open
Kriskras99 wants to merge 1 commit intomainfrom
Open
Conversation
Kriskras99
force-pushed
the
feat/default_array_map
branch
from
ac975d8 to
f4a809a
Compare
martin-g
reviewed
Feb 16, 2026
| #[error("Default value for a map must be a object! Got: {0}")] | ||
| MapDefaultWrongType(serde_json::Value), | ||
|
|
||
| #[error("Default value for a map must be a object with (String, {0})! Found: (String, {1:?})")] |
Member
There was a problem hiding this comment.
Suggested change
| #[error("Default value for a map must be a object with (String, {0})! Found: (String, {1:?})")] | |
| #[error("Default value for a map must be an object with (String, {0})! Found: (String, {1:?})")] |
| #[error("Default value for an array must be an array of {0}! Found: {1:?}")] | ||
| ArrayDefaultWrongInnerType(Schema, Value), | ||
|
|
||
| #[error("Default value for a map must be a object! Got: {0}")] |
Member
There was a problem hiding this comment.
Suggested change
| #[error("Default value for a map must be a object! Got: {0}")] | |
| #[error("Default value for a map must be an object! Got: {0}")] |
| .unwrap_or_else(|| "unexpected".to_string()); | ||
| assert_eq!(expected, err); | ||
| assert_eq!( | ||
| r#"Default value for a map must be a object! Got: "invalid""#, |
Member
There was a problem hiding this comment.
Suggested change
| r#"Default value for a map must be a object! Got: "invalid""#, | |
| r#"Default value for a map must be an object! Got: "invalid""#, |
|
|
||
| assert_eq!( | ||
| err.to_string(), | ||
| "Default value for a map must be a object with (String, String)! Found: (String, Boolean(true))" |
Member
There was a problem hiding this comment.
Suggested change
| "Default value for a map must be a object with (String, String)! Found: (String, Boolean(true))" | |
| "Default value for a map must be an object with (String, String)! Found: (String, Boolean(true))" |
|
|
||
| assert_eq!( | ||
| err.to_string(), | ||
| "Default value for a map must be a object with (String, String)! Found: (String, Boolean(true))" |
Member
There was a problem hiding this comment.
Suggested change
| "Default value for a map must be a object with (String, String)! Found: (String, Boolean(true))" | |
| "Default value for a map must be an object with (String, String)! Found: (String, Boolean(true))" |
| unreachable!("JsonValue::Object can only become a Value::Map") | ||
| }; | ||
| // Check that the default type matches the schema type | ||
| if let Some(value) = map.values().find(|v| !v.validate(&types)) { |
Member
There was a problem hiding this comment.
If the values contain Schema::Ref then you will need to pass the schemata, i.e. use Value::validate_internal()
|
|
||
| let default = if let Some(default) = complex.get("default").cloned() { | ||
| if let Value::Object(_) = default { | ||
| let crate::types::Value::Map(map) = crate::types::Value::from(default) else { |
Member
There was a problem hiding this comment.
Not caused by this PR:
This could be exploited to trigger the panic at
Line 286 in af56163
I think we should get rid of this panic!() and log an error/warning + return
JsonValue::Double(f64::MAX) (a best effort).
| ); | ||
|
|
||
| let json = serde_json::to_string(&Schema::Array(array))?; | ||
| json.contains(r#""default":["foo","bar"]"#); |
Member
There was a problem hiding this comment.
Suggested change
| json.contains(r#""default":["foo","bar"]"#); | |
| assert!(json.contains(r#""default":["foo","bar"]"#)); |
| assert_eq!(map.default, Some(HashMap::new())); | ||
|
|
||
| let json = serde_json::to_string(&Schema::Map(map))?; | ||
| json.contains(r#""default":{}"#); |
Member
There was a problem hiding this comment.
Suggested change
| json.contains(r#""default":{}"#); | |
| assert!(json.contains(r#""default":{}"#)); |
| assert_eq!(map.default, Some(hashmap)); | ||
|
|
||
| let json = serde_json::to_string(&Schema::Map(map))?; | ||
| json.contains(r#""default":{"foo":"bar"}"#); |
Member
There was a problem hiding this comment.
Suggested change
| json.contains(r#""default":{"foo":"bar"}"#); | |
| assert!(json.contains(r#""default":{"foo":"bar"}"#)); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
One thing I did notice in the spec is that the text says
support a single attributewhile the example does show adefaultfield, so the text of the spec should be updated as well.