fix allOf of oneOfs by introducing cartesian product#982
fix allOf of oneOfs by introducing cartesian product#982PolyProgrammist wants to merge 3 commits intooxidecomputer:mainfrom
Conversation
|
@ahl could you please review? |
ahl
left a comment
There was a problem hiding this comment.
this seems reasonable, and the improvement to the github output is encouraging.
| new_variants: &[Schema], | ||
| defs: &BTreeMap<RefKey, Schema>, | ||
| ) -> Vec<Schema> { | ||
| let mut result = Vec::new(); |
There was a problem hiding this comment.
it seems possible for there to be literal or effective duplicates in this list.
There was a problem hiding this comment.
I see there may be oneOf: [A, B] and oneOf: [C, D]
And maybe something like oneOf[{}, {type: string}] and oneOf[{type:string}, {type:string}] where for all of them the result would be {type:string}.
Do you think comparing like that is enough?
There was a problem hiding this comment.
I see there may be oneOf: [A, B] and oneOf: [C, D] And maybe something like oneOf[{}, {type: string}] and oneOf[{type:string}, {type:string}] where for all of them the result would be {type:string}.
That's not quite right: oneOf[{type: string}, {type: string}] would be equivalent to the schema false... and so therefore would the outer allOf.
but perhaps something like this?
{
"allOf": [
{
"oneOf": [
{
"type": "object",
"required": ["x"],
"properties": {
"x": { "const": 1 }
}
},
{
"type": "object",
"required": ["x"],
"properties": {
"x": { "const": 2 }
}
}
]
},
{
"oneOf": [
{
"type": "object",
"required": ["y"],
"properties": {
"y": { "const": 3 }
}
},
{
"type": "object",
"required": ["x", "y"],
"properties": {
"x": { "const": 1 },
"y": { "const": 3 }
}
}
]
}
]
}I think the expansion you have in mind is more of less
{
"oneOf": [
{
"allOf": [
{ "type": "object", "required": ["x"], "properties": { "x": { "const": 1 } } },
{ "type": "object", "required": ["y"], "properties": { "y": { "const": 3 } } }
]
},
{
"allOf": [
{ "type": "object", "required": ["x"], "properties": { "x": { "const": 1 } } },
{ "type": "object", "required": ["x", "y"], "properties": { "x": { "const": 1 }, "y": { "const": 3 } } }
]
},
{
"allOf": [
{ "type": "object", "required": ["x"], "properties": { "x": { "const": 2 } } },
{ "type": "object", "required": ["y"], "properties": { "y": { "const": 3 } } }
]
},
{
"allOf": [
{ "type": "object", "required": ["x"], "properties": { "x": { "const": 2 } } },
{ "type": "object", "required": ["x", "y"], "properties": { "x": { "const": 1 }, "y": { "const": 3 } } }
]
}
]
}And if we further reduce that we get
{
"oneOf": [
{
"type": "object",
"required": ["x", "y"],
"properties": {
"x": { "const": 1 },
"y": { "const": 3 }
}
},
{
"type": "object",
"required": ["x", "y"],
"properties": {
"x": { "const": 1 },
"y": { "const": 3 }
}
},
{
"type": "object",
"required": ["x", "y"],
"properties": {
"x": { "const": 2 },
"y": { "const": 3 }
}
},
false
]
}The first 2 are identical; the last one is unsatisfiable. I think we need check for exclusivity--perhaps by seeing if the A ∪ ⌐B is empty?
There was a problem hiding this comment.
Actually that's not quite right and now I'm deeply uncertain about whether there might be duplicates.
There was a problem hiding this comment.
Now that I've had time to sleep on it:
If we have two sets of schemas, each known to consist of mutually exclusive schemas, then the cartesian product of those two sets must also be pairwise mutually exclusive. Why? A value that was valid for multiple schemas of the cartesian product would necessarily be valid for multiple schemas in (at least) one of the input schema sets (which would invalidate the initial premise).
However! I don't believe that typify currently ensures that oneOfs are mutually exclusive. Part of the (in-progress) typify rewrite is to ensure that that mutual exclusivity. For example, with an input like:
{
"oneOf": [
{
"type": "object",
"required": ["y"],
"properties": {
"y": { "const": 3 }
}
},
{
"type": "object",
"required": ["x", "y"],
"properties": {
"x": { "const": 1 },
"y": { "const": 3 }
}
}
]
}We would produce a type like this:
pub struct MyType {
y: MyTypeY, // unit type that can only have a value of the integer 3
x: MyTypeX, // basically serde_json::Value except *not* the integer 1
}Which is all to say: this PR is wrong, but it's no more wrong that what we currently have and, indeed, it's quite a bit better in all but the most esoteric cases.
| // Try to merge each pair of variants | ||
| if let Ok(merged) = try_merge_schema(base_variant, new_variant, defs) { | ||
| // Only include if the merge produced a satisfiable schema | ||
| if merged != Schema::Bool(false) |
There was a problem hiding this comment.
Does try_merge_schema return false ever? Perhaps we should use merge_schema instead?
A fix for #897