`as_object` on Schema? - Githubissues

I am not sure this is a good idea though.Perhaps we can impl JsonSchemaExt on Schema instead of SchemaObject, with the newly introduced method as_object:

trait JsonSchemaExt {
    fn is_true(&self) -> bool;
    fn effective_type(&mut self) -> InternalJsonSchemaType;
    fn as_object(&mut self) -> Option<&mut SchemaObject>;
}

impl JsonSchemaExt for Schema {
    fn is_true(&self) -> bool {
        matches!(self, Schema::Bool(true))
    }

    fn effective_type(&mut self) -> InternalJsonSchemaType {
        match self {
            Schema::Object(obj) => {
                if let Some(ref ty) = obj.instance_type {
                    match ty {
                        SingleOrVec::Single(ty) => JsonSchemaType::from(**ty).into(),
                        SingleOrVec::Vec(tys) => InternalJsonSchemaType::Multiple(
                            tys.iter().copied().map(JsonSchemaType::from).collect(),
                        ),
                    }
                } else if let Some(ref constant) = obj.const_value {
                    serde_value_to_own(constant).into()
                } else if !obj.object().properties.is_empty() {
                    JsonSchemaType::Object.into()
                } else if let Some(ref mut any_of) = obj.subschemas().any_of {
                    InternalJsonSchemaType::Multiple(
                        any_of
                            .iter_mut()
                            .flat_map(|a| Self::effective_type(a).explode())
                            .collect::<BTreeSet<_>>()
                            .into_iter()
                            .collect(),
                    )
                } else if obj.subschemas().not.as_ref().map_or(false, |x| x.is_true()) {
                    InternalJsonSchemaType::Never
                } else {
                    InternalJsonSchemaType::Any
                }
            }
            Schema::Bool(true) => InternalJsonSchemaType::Any,
            Schema::Bool(false) => InternalJsonSchemaType::Never,
        }
    }

    fn as_object(&mut self) -> Option<&mut SchemaObject> {
        match self {
            Schema::Object(obj) => Some(obj),
            _ => None,
        }
    }
}

A main advantage here is that we can get rid of cloning Schema just for obtaining &mut SchemaObject, as done in some places. e.g.

https://github.com/getsentry/json-schema-diff/blob/19d0fd5fec55904a7a0370a503f5cb71656b9797/src/diff_walker.rs#L146

A drawback would be increased lines of code. We would rewrite diff_properties as below:

fn diff_properties(
        &mut self,
        json_path: &str,
        lhs: &mut Schema,
        rhs: &mut Schema,
    ) -> Result<(), Error> {
        let lhs_props: BTreeSet<_> = lhs
            .as_object()
            .map(|obj| obj.object().properties.keys().cloned().collect())
            .unwrap_or_default();
        let rhs_props: BTreeSet<_> = rhs
            .as_object()
            .map(|obj| obj.object().properties.keys().cloned().collect())
            .unwrap_or_default();

        let lhs_additional_properties = lhs
            .as_object()
            .and_then(|obj| obj.object().additional_properties.as_ref())
            .map_or(true, |x| x.is_true());

        for removed in lhs_props.difference(&rhs_props) {
            (self.cb)(Change {
                path: json_path.to_owned(),
                change: ChangeKind::PropertyRemove {
                    lhs_additional_properties,
                    removed: removed.clone(),
                },
            });
        }

I believe this discussion is worth having upstream in schemars. If they reject, go for it here

On Sat, 20 May 2023, 20:21 Yudai Kiyofuji, @.***> wrote:

I am not sure this is a good idea though.Perhaps we can impl JsonSchemaExt on Schema instead of SchemaObject, with the newly introduced method as_object:

trait JsonSchemaExt { fn is_true(&self) -> bool; fn effective_type(&mut self) -> InternalJsonSchemaType; fn as_object(&mut self) -> Option<&mut SchemaObject>; }

impl JsonSchemaExt for Schema { fn is_true(&self) -> bool { matches!(self, Schema::Bool(true)) }
fn effective_type(&mut self) -> InternalJsonSchemaType {
    match self {
        Schema::Object(obj) => {
            if let Some(ref ty) = obj.instance_type {
                match ty {
                    SingleOrVec::Single(ty) => JsonSchemaType::from(**ty).into(),
                    SingleOrVec::Vec(tys) => InternalJsonSchemaType::Multiple(
                        tys.iter().copied().map(JsonSchemaType::from).collect(),
                    ),
                }
            } else if let Some(ref constant) = obj.const_value {
                serde_value_to_own(constant).into()
            } else if !obj.object().properties.is_empty() {
                JsonSchemaType::Object.into()
            } else if let Some(ref mut any_of) = obj.subschemas().any_of {
                InternalJsonSchemaType::Multiple(
                    any_of
                        .iter_mut()
                        .flat_map(|a| Self::effective_type(a).explode())
                        .collect::<BTreeSet<_>>()
                        .into_iter()
                        .collect(),
                )
            } else if obj.subschemas().not.as_ref().map_or(false, |x| x.is_true()) {
                InternalJsonSchemaType::Never
            } else {
                InternalJsonSchemaType::Any
            }
        }
        Schema::Bool(true) => InternalJsonSchemaType::Any,
        Schema::Bool(false) => InternalJsonSchemaType::Never,
    }
}

fn as_object(&mut self) -> Option<&mut SchemaObject> {
    match self {
        Schema::Object(obj) => Some(obj),
        _ => None,
    }
}
}

A main advantage here is that we can get rid of cloning Schema just for obtaining &mut SchemaObject, as done in some places. e.g.

https://github.com/getsentry/json-schema-diff/blob/19d0fd5fec55904a7a0370a503f5cb71656b9797/src/diff_walker.rs#L146

A drawback would be increased lines of code. We would rewrite diff_properties as below:

fn diff_properties( &mut self, json_path: &str, lhs: &mut Schema, rhs: &mut Schema, ) -> Result<(), Error> { let lhsprops: BTreeSet<> = lhs .as_object() .map(|obj| obj.object().properties.keys().cloned().collect()) .unwrap_or_default(); let rhsprops: BTreeSet<> = rhs .as_object() .map(|obj| obj.object().properties.keys().cloned().collect()) .unwrap_or_default();
    let lhs_additional_properties = lhs
        .as_object()
        .and_then(|obj| obj.object().additional_properties.as_ref())
        .map_or(true, |x| x.is_true());

    for removed in lhs_props.difference(&rhs_props) {
        (self.cb)(Change {
            path: json_path.to_owned(),
            change: ChangeKind::PropertyRemove {
                lhs_additional_properties,
                removed: removed.clone(),
            },
        });
    }
— Reply to this email directly, view it on GitHub https://github.com/getsentry/json-schema-diff/issues/36, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGMPRL4UGX6KLUNLH3547TXHEDUJANCNFSM6AAAAAAYI4DPHE . You are receiving this because you are subscribed to this thread.Message ID: @.***>

getsentry / json-schema-diff

`as_object` on Schema? #36