Skip to content

Commit

Permalink
Address PR review comments.
Browse files Browse the repository at this point in the history
  • Loading branch information
jshearer committed Aug 7, 2023
1 parent 79fd07b commit dcca058
Show file tree
Hide file tree
Showing 4 changed files with 135 additions and 133 deletions.
3 changes: 1 addition & 2 deletions crates/doc/src/inference.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ pub enum Reduction {

Append,
FirstWriteWins,
JsonSchemaMerge,
LastWriteWins,
Maximize,
Merge,
Expand All @@ -107,8 +108,6 @@ pub enum Reduction {

// Multiple concrete strategies may apply at the location.
Multiple,

JsonSchemaMerge,
}

impl Reduction {
Expand Down
1 change: 1 addition & 0 deletions crates/doc/src/reduce/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use std::cmp::Ordering;
pub mod strategy;
pub use strategy::Strategy;

mod schema;
mod set;

pub static DEFAULT_STRATEGY: &Strategy = &Strategy::LastWriteWins;
Expand Down
131 changes: 131 additions & 0 deletions crates/doc/src/reduce/schema.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
use super::{count_nodes_heap, Cursor, Error, Result};
use crate::{inference::Shape, schema::SchemaBuilder, AsNode, HeapNode};
use json::schema::index::IndexBuilder;

pub fn json_schema_merge<'alloc, L: AsNode, R: AsNode>(
cur: Cursor<'alloc, '_, '_, '_, '_, L, R>,
) -> Result<HeapNode<'alloc>> {
let Cursor {
tape,
loc,
full: _,
lhs,
rhs,
alloc,
} = cur;

let (lhs, rhs) = (lhs.into_heap_node(alloc), rhs.into_heap_node(alloc));

*tape = &tape[count_nodes_heap(&rhs)..];

// Ensure that we're working with objects on both sides
// Question: Should we actually relax this to support
// reducing valid schemas like "true" and "false"?
let (
lhs @ HeapNode::Object(_),
rhs @ HeapNode::Object(_)
) = (lhs, rhs) else {
return Err(Error::with_location(Error::JsonSchemaMergeWrongType { detail: None }, loc) )
};

let left = shape_from_node(lhs).map_err(|e| Error::with_location(e, loc))?;
let right = shape_from_node(rhs).map_err(|e| Error::with_location(e, loc))?;

// Union together the LHS and RHS, and convert back from `Shape` into `HeapNode`.
let merged_doc =
serde_json::to_value(&SchemaBuilder::new(Shape::union(left, right)).root_schema())
.and_then(|value| HeapNode::from_serde(value, alloc))
.map_err(|e| {
Error::with_location(
Error::JsonSchemaMergeWrongType {
detail: Some(e.to_string()),
},
loc,
)
})?;

Ok(merged_doc)
}

fn shape_from_node<'a, N: AsNode>(node: N) -> Result<Shape> {
// Should this be something more specific/useful?
let url = url::Url::parse("json-schema-reduction:///").unwrap();

let serialized =
serde_json::to_value(node.as_node()).map_err(|e| Error::JsonSchemaMergeWrongType {
detail: Some(e.to_string()),
})?;

let schema = json::schema::build::build_schema::<crate::Annotation>(url.clone(), &serialized)
.map_err(|e| Error::JsonSchemaMergeWrongType {
detail: Some(e.to_string()),
})?;

let mut index = IndexBuilder::new();
index.add(&schema).unwrap();
index.verify_references().unwrap();
let index = index.into_index();

Ok(Shape::infer(
index
.must_fetch(&url)
.map_err(|e| Error::JsonSchemaMergeWrongType {
detail: Some(e.to_string()),
})?,
&index,
))
}

#[cfg(test)]
mod test {
use super::super::test::*;
use super::*;

#[test]
fn test_merge_json_schemas() {
run_reduce_cases(
json!({ "reduce": { "strategy": "jsonSchemaMerge" } }),
vec![
Partial {
rhs: json!({
"type": "string",
"maxLength": 5,
"minLength": 5
}),
expect: Ok(json!({
"type": "string",
"maxLength": 5,
"minLength": 5
})),
},
Partial {
rhs: json!("oops!"),
expect: Err(Error::JsonSchemaMergeWrongType { detail: None }),
},
Partial {
rhs: json!({
"type": "foo"
}),
expect: Err(Error::JsonSchemaMergeWrongType {
detail: Some(
r#"at keyword 'type' of schema 'json-schema-reduction:///': expected a type or array of types: invalid type name: 'foo'"#.to_owned(),
),
}),
},
Partial {
rhs: json!({
"type": "string",
"minLength": 8,
"maxLength": 10
}),
expect: Ok(json!({
"$schema": "http://json-schema.org/draft-07/schema#",
"type": "string",
"minLength": 5,
"maxLength": 10,
})),
},
],
)
}
}
133 changes: 2 additions & 131 deletions crates/doc/src/reduce/strategy.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,9 @@
use json::schema::index::IndexBuilder;

use super::{
compare_key_lazy, count_nodes, count_nodes_generic, count_nodes_heap, reduce_item, reduce_prop,
Cursor, Error, Result,
schema::json_schema_merge, Cursor, Error, Result,
};
use crate::{
inference::Shape,
lazy::{LazyArray, LazyDestructured, LazyObject},
schema::SchemaBuilder,
AsNode, BumpVec, HeapNode, Node, Pointer,
};

Expand Down Expand Up @@ -127,7 +123,7 @@ impl Strategy {
Strategy::Minimize(min) => Self::minimize(cur, min),
Strategy::Set(set) => set.apply(cur),
Strategy::Sum => Self::sum(cur),
Strategy::JsonSchemaMerge => Self::json_schema_merge(cur),
Strategy::JsonSchemaMerge => json_schema_merge(cur),
}
}

Expand Down Expand Up @@ -309,54 +305,6 @@ impl Strategy {
}
}

fn json_schema_merge<'alloc, L: AsNode, R: AsNode>(
cur: Cursor<'alloc, '_, '_, '_, '_, L, R>,
) -> Result<HeapNode<'alloc>> {
let Cursor {
tape,
loc,
full: _,
lhs,
rhs,
alloc,
} = cur;

let (lhs, rhs) = (lhs.into_heap_node(alloc), rhs.into_heap_node(alloc));

// Precompute this up here because we're going to consume `rhs` later on
let rhs_tape_len = count_nodes_heap(&rhs);

// Ensure that we're working with objects on both sides
// Question: Should we actually relax this to support
// reducing valid schemas like "true" and "false"?
let (
lhs @ HeapNode::Object(_),
rhs @ HeapNode::Object(_)
) = (lhs, rhs) else {
return Err(Error::with_location(Error::JsonSchemaMergeWrongType { detail: None }, loc) )
};

let left = shape_from_node(lhs).map_err(|e| Error::with_location(e, loc))?;
let right = shape_from_node(rhs).map_err(|e| Error::with_location(e, loc))?;

// Union together the LHS and RHS, and convert back from `Shape` into `HeapNode`.
let merged_doc =
serde_json::to_value(&SchemaBuilder::new(Shape::union(left, right)).root_schema())
.and_then(|value| HeapNode::from_serde(value, alloc))
.map_err(|e| {
Error::with_location(
Error::JsonSchemaMergeWrongType {
detail: Some(e.to_string()),
},
loc,
)
})?;

*tape = &tape[rhs_tape_len..];

Ok(merged_doc)
}

fn merge<'alloc, L: AsNode, R: AsNode>(
cur: Cursor<'alloc, '_, '_, '_, '_, L, R>,
merge: &Merge,
Expand Down Expand Up @@ -455,35 +403,6 @@ impl Strategy {
}
}

fn shape_from_node<'a, N: AsNode>(node: N) -> Result<Shape> {
// Should this be something more specific/useful?
let url = url::Url::parse("json-schema-reduction:///").unwrap();

let serialized =
serde_json::to_value(node.as_node()).map_err(|e| Error::JsonSchemaMergeWrongType {
detail: Some(e.to_string()),
})?;

let schema = json::schema::build::build_schema::<crate::Annotation>(url.clone(), &serialized)
.map_err(|e| Error::JsonSchemaMergeWrongType {
detail: Some(e.to_string()),
})?;

let mut index = IndexBuilder::new();
index.add(&schema).unwrap();
index.verify_references().unwrap();
let index = index.into_index();

Ok(Shape::infer(
index
.must_fetch(&url)
.map_err(|e| Error::JsonSchemaMergeWrongType {
detail: Some(e.to_string()),
})?,
&index,
))
}

#[cfg(test)]
mod test {
use super::super::test::*;
Expand Down Expand Up @@ -1029,52 +948,4 @@ mod test {
],
)
}

#[test]
fn test_merge_json_schemas() {
run_reduce_cases(
json!({ "reduce": { "strategy": "jsonSchemaMerge" } }),
vec![
Partial {
rhs: json!({
"type": "string",
"maxLength": 5,
"minLength": 5
}),
expect: Ok(json!({
"type": "string",
"maxLength": 5,
"minLength": 5
})),
},
Partial {
rhs: json!("oops!"),
expect: Err(Error::JsonSchemaMergeWrongType { detail: None }),
},
Partial {
rhs: json!({
"type": "foo"
}),
expect: Err(Error::JsonSchemaMergeWrongType {
detail: Some(
r#"at keyword 'type' of schema 'json-schema-reduction:///': expected a type or array of types: invalid type name: 'foo'"#.to_owned(),
),
}),
},
Partial {
rhs: json!({
"type": "string",
"minLength": 8,
"maxLength": 10
}),
expect: Ok(json!({
"$schema": "http://json-schema.org/draft-07/schema#",
"type": "string",
"minLength": 5,
"maxLength": 10,
})),
},
],
)
}
}

0 comments on commit dcca058

Please sign in to comment.