adds implementation for `WriteToIsl` #183

desaikd · 2023-06-08T23:11:31Z

Issue #173

Description of changes:

This PR works on adding implementation of WriteToIsl which serializes a schema model into a schema file using Ion writer.

Example:

Here is a quick example of how to serialize a schema model using Ion writer.

// construct an ISL type programmatically
let isl_type = named_type(
    // represents the `name` of the defined type
    "my_number_type".to_owned(),
    vec![
        // represents `one_of` constraint
        one_of(vec![
            named_type_ref("int"),
            named_type_ref("float"),
            named_type_ref("decimal"),
        ]),
    ],
);

// create an ISL schema using above `isl_type`
let isl_schema = IslSchema::schema_v_1_0(
    "my_schema.isl", // represents schem id which is usually same as the schema file name
    vec![], // represents imports
    vec![isl_type.to_owned()],
    vec![], // represents inline imports
    vec![], // represents open content
);

// use an Ion Writer to write the ISL model
let mut buffer = Vec::new();
let mut writer = TextWriterBuilder::pretty().build(&mut buffer)?;
isl_schema.write_to(&mut writer)?;

// write into schema file
let mut file = File::create("my_schema.isl")?;
file.write_all(writer.output().as_bytes())?;

Output:

$ion_schema_1_0
schema_header::{}
type::{
    name: "my_number_type",
    one_of: [
        int,
        float,
        decimal
    ]
}
schema_footer::{}

List of changes:

adds WriteToIsl to serialize isl model into schema file
- Defines WriteToIsl trait which will be sued by all ISL model structs to write schema content to an Ion writer
- Implements WriteToIsl for IslSchema
adds WriteToIsl implementation for
- UserReservedFields
- ISL imports
- ISL type references
- ISL constraints
- ISL ranges
- ISL model utils

Test:

Adds unit tests for WriteToIsl implementations.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

* Defines `WriteToIsl` trait * Implements `WriteToIsl` for `IslSchema`

popematt · 2023-06-09T23:48:22Z

ion-schema/src/isl/mod.rs

    use ion_rs::Symbol;
+    use ion_rs::{IonType, TextWriterBuilder};
+    use nom::AsBytes;


Where is this being used?

This is being used in the tests for WriteToIsl.

nom is a pretty heavy dependency to bring in for the AsBytes trait, which seems to be a helper for fixed-sized arrays. Consider either:

Removing the dependency and using Vec::as_slice to access the bytes of the Vec<u8>.

Downgrading the dependency to a dev dependency.

nom is a pretty heavy dependency to bring in for the AsBytes trait, which seems to be a helper for fixed-sized arrays. Consider either:

* Removing the dependency and using [`Vec::as_slice`](https://doc.rust-lang.org/std/vec/struct.Vec.html#method.as_slice) to access the bytes of the `Vec<u8>`. * Downgrading the dependency to a dev dependency.

According to your suggestion and since I am using this only for tests, I will remove the dependency and just use as_slice.

popematt · 2023-06-09T23:53:02Z

ion-schema/src/isl/isl_type_reference.rs

+            IslTypeRefImpl::Named(name, nullability_modifier) => {
+                nullability_modifier.write_to(writer)?;
+                writer.write_symbol(name)?;
+            }
+            IslTypeRefImpl::TypeImport(type_import, nullability_modifier) => {
+                nullability_modifier.write_to(writer)?;
+                writer.step_in(IonType::Struct)?;
+                type_import.write_to(writer)?;
+                writer.step_out()?;
+            }


I'm assuming that it's possible to construct an IslVariableOccurringTypeRef that has a Named or TypeImport. If that's the case, then we should also be writing out the occurs in these branches, if it's not the default occurs for that constraint.

Right, for TypeImport I will modify it to add the occurs field here. For Named if it has an occurs other than the default value then it becomes an Anonymous(inline) type reference.
e.g.

// Named fields : { name: string } // Anonymous fields: { name: { type: string, occurs: required } }

popematt · 2023-06-10T00:05:52Z

ion-schema/src/isl/mod.rs

+        "ion-schema-schemas/json/json.isl", // the file contains `nan` which fails on equivalence for two schemas
+        "ion-schema-tests/ion_schema_1_0/nullable.isl", // Needs `nullable` annotation related fixes
+        "ion-schema-tests/ion_schema_1_0/schema/import/import_inline.isl", // related to order of types in the schema file
+        "ion-schema-tests/ion_schema_2_0/imports/tree/inline_import_a.isl", // related to order of types in the schema file
+        "ion-schema-tests/ion_schema_2_0/imports/tree/inline_import_c.isl", // related to order of types in the schema file


Do we have any GitHub issue to track the nan issue or the "order of types in a schema file" issue?

Created an issue for this, I will add it in the comments for these SKIP_LIST items.
#184

ion-schema/src/lib.rs

ion-schema/src/isl/mod.rs

* adds more doc comments * `write_to` writes `occurs` field for `TypeImport` in `IslVariablyOccurringTypeRef`

zslayton · 2023-06-14T17:25:51Z

ion-schema/src/isl/isl_range.rs

+                if range_boundary_type == &RangeBoundaryType::Exclusive {
+                    writer.set_annotations(["exclusive"]);
+                }
+                let element = Element::read_one(format!("{value}").as_bytes())?;


I don't have a concrete suggestion at the moment, but this bit feels a bit fragile to me. We're using the constraint T: Display but actually hoping that T is something that's serialized as text Ion, which is a narrower set of types.

ion-schema/src/isl/isl_type.rs

zslayton · 2023-06-14T17:38:36Z

ion-schema/src/isl/mod.rs

    use ion_rs::Symbol;
+    use ion_rs::{IonType, TextWriterBuilder};
+    use nom::AsBytes;


nom is a pretty heavy dependency to bring in for the AsBytes trait, which seems to be a helper for fixed-sized arrays. Consider either:

Removing the dependency and using Vec::as_slice to access the bytes of the Vec<u8>.

Downgrading the dependency to a dev dependency.

desaikd added 10 commits June 8, 2023 14:52

adds WriteToIsl to serialize isl model into schema file

79a2fa2

* Defines `WriteToIsl` trait * Implements `WriteToIsl` for `IslSchema`

adds WriteToIsl implementation for UserReservedFields

0908a3b

adds WriteToIsl implementation for ISL import

3149d43

adds writeToIsl implementation for IslTypeImpl

6b6e987

adds WriteToIsl for IslTyepRef

ce64dbe

adds WriteToIsl for model utils

f0c2a01

adds WriteToIsl for ISl ranges

df9362e

adds WriteToIsl for ISL constraint

5c2a692

adds unit tests for serializing ISL model into schema file

8778e69

adds utility for unit tests

9e302f1

desaikd requested review from popematt and zslayton June 8, 2023 23:11

popematt reviewed Jun 10, 2023

View reviewed changes

adds suggested changes

3da8eaf

* adds more doc comments * `write_to` writes `occurs` field for `TypeImport` in `IslVariablyOccurringTypeRef`

desaikd requested a review from popematt June 12, 2023 18:14

zslayton approved these changes Jun 14, 2023

View reviewed changes

adds more suggested changes

7cf626b

popematt approved these changes Jun 15, 2023

View reviewed changes

desaikd merged commit 7927b1f into main Jun 15, 2023

desaikd deleted the desaikd-model-write-to-isl branch June 29, 2023 20:57

desaikd mentioned this pull request Jul 5, 2023

Please add capability to serialise a programmatically constructed Ion Schema #173

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adds implementation for `WriteToIsl` #183

adds implementation for `WriteToIsl` #183

desaikd commented Jun 8, 2023

popematt Jun 9, 2023

desaikd Jun 12, 2023

zslayton Jun 14, 2023

desaikd Jun 14, 2023

popematt Jun 9, 2023

desaikd Jun 12, 2023

popematt Jun 10, 2023

desaikd Jun 12, 2023 •

edited

Loading

zslayton Jun 14, 2023

zslayton Jun 14, 2023

adds implementation for WriteToIsl #183

adds implementation for WriteToIsl #183

Conversation

desaikd commented Jun 8, 2023

Issue #173

Description of changes:

Example:

List of changes:

Test:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

desaikd Jun 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adds implementation for `WriteToIsl` #183

adds implementation for `WriteToIsl` #183

desaikd Jun 12, 2023 •

edited

Loading