Streamlines ISL model using version maker traits #191

desaikd · 2023-06-28T19:20:17Z

Issue #187

Description of changes:

This PR works on streamlining the ISL model using version maker traits.

List of changes:

adds version marker traits which will be used to construct schema,
types and constraints for a particular ISL version
adds logic constraints implementations
adds type reference, type and import implementations based on version
marker trait
adds example usage of this new change for the ISL model

Note for reviewers:

This PR creates a new module version_based_isl which will eventually be placed as the current isl module. This is because the change is too big and involves changing mostly entire ISL model. In order to divide the change into pieces created version_based_isl.
All the structs/enums for ISL model are same as previous ISL model except they now accept a generic V(version marker trait).

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

* adds verison marker traits which will be used to construct schema, types and constraints for a particular ISL version * adds logic constraints implementations * adds type reference, type and import implementations based on verison marker trait * adds example usage of this new change for the ISL model

…aikd-isl-model-revamp

desaikd · 2023-06-28T19:21:59Z

ion-schema/src/isl/version_based_isl/constraints/all_of.rs

+use crate::isl::version_based_isl::{IslTypeRef, IslVersionTrait};
+
+#[derive(Debug, Clone, PartialEq)]
+pub struct AllOf<V: IslVersionTrait> {


🗺️ All the constraints are now placed inside constraints module. Each constraint will now accept a generic V that represents the ISL version using version marker trait.

desaikd · 2023-06-28T19:37:28Z

ion-schema/src/isl/version_based_isl/mod.rs

+    phantom: PhantomData<V>,
+}
+
+impl UserReservedFields<IslV2_0> {


🗺️ This is only defined for ISL 2.0 and hence would result in a compile time error when user tries to create this with ISL 1.0 schema. Please look at the IslSchema#with_user_reserved_fields() for more details.

desaikd · 2023-06-28T19:43:59Z

ion-schema/src/isl/version_based_isl/mod.rs

+    }
+
+    /// Creates a nullable [IslTypeRef] using the name of the type referenced inside it for ISL 2.0
+    pub fn null_or_named_type_ref<A: Into<String>>(name: A) -> IslTypeRef<IslV2_0> {


🗺️ This will only be allowed to use with ISL 2.0 schema. Using it with an ISL 1.0 schema would result in compile time error.
Example:

let isl_type = IslType::named( // represents the `name` of the defined type "my_type_name".to_owned(), vec![ // represents the `type: $null_or::int` constraint IslConstraint::type_constraint( IslTypeRef::null_or_named_type_ref("int") ), ] ); // create an ISL schema using above IslType let mut isl_schema = IslSchema::<IslV1_0>::new("my_schema", vec![], vec![isl_type], vec![], vec![]);

Output:

| let mut isl_schema = IslSchema::<IslV1_0>::new("my_schema", vec![], vec![isl_type], vec![], vec![]); | ^^^^^^^^ expected `IslType<IslV1_0>`, found `IslType<IslV2_0>`

popematt

I have a few high level comments. I'd like to hear your thoughts on these before I dive into the actual code.

Eventually, we really need to address the fact that some constraints have their own structs, and others are just an enum variant that's a tuple. If you're rewriting the model, this seems like an ideal time to fix it.
I'd recommend something like this to eliminate some of the boilerplate for constraints. https://gist.github.com/popematt/11b1e632ed487d11ba50eda1bb43a78a (Note that this will not work exactly because it doesn't have the version it it.) As a bonus, a macro like this will mean that we also get a proper struct for each constraint.
What other solutions did you consider? (Have you considered creating versioned builders for a versionless model?) What made you choose a versioned model over those other solutions? (I'd like to understand any tradeoffs, etc.)

desaikd · 2023-06-28T21:12:53Z

Overall versioned ISL model gives an ease of use, maintainability and gives compile time checks for ISL version on types, constraints and schema.

Eventually, we really need to address the fact that some constraints have their own structs, and others are just an enum variant that's a tuple. If you're rewriting the model, this seems like an ideal time to fix it.

Previous ISL model had tuples for enum variants for all the constraints. I have created a struct for each constraint so that we have the same pattern throughout for all constraints be it a constraint thats same for both ISL versions or its different for different ISL versions. It gives us a nice distinction between how 2 different versions would work for a particular constraint. Also eventually if we decide to have single model that will be used for representation as well as validation then it makes sense to create different structs for all the constraints as each one will have its own validation.

I'd recommend something like this to eliminate some of the boilerplate for constraints. https://gist.github.com/popematt/11b1e632ed487d11ba50eda1bb43a78a (Note that this will not work exactly because it doesn't have the version it it.) As a bonus, a macro like this will mean that we also get a proper struct for each constraint.

Adding a macro to create the structs for constraints does sound like good idea. I could create an issue for it.

What other solutions did you consider? (Have you considered creating versioned builders for a versionless model?)
What made you choose a versioned model over those other solutions? (I'd like to understand any tradeoffs, etc.)

I have chosen to go with versioned model using the version maker trait because that gives a better API to construct ISL model constraints/types and schema. This allows us to have compile time errors for when a user tries to mix two different versions of schema. I think this provides an ease of use for customers as well as ensures the different version constraints aren't mixed.

popematt · 2023-06-28T20:55:45Z

ion-schema/src/isl/version_based_isl/constraints/mod.rs

+    /// Creates a `type` constraint using the [IslTypeRef] referenced inside it
+    // type is rust keyword hence this method is named type_constraint unlike other ISL constraint methods
+    pub fn type_constraint(isl_type_ref: IslTypeRef<V>) -> IslConstraint<V> {
+        IslConstraint::Type(Type::new(isl_type_ref))
+    }
+
+    /// Creates an `all_of` constraint using the [IslTypeRef] referenced inside it
+    pub fn all_of<A: Into<Vec<IslTypeRef<V>>>>(isl_type_refs: A) -> IslConstraint<V> {
+        IslConstraint::AllOf(AllOf::new(isl_type_refs.into()))
+    }
+
+    /// Creates an `any_of` constraint using the [IslTypeRef] referenced inside it
+    pub fn any_of<A: Into<Vec<IslTypeRef<V>>>>(isl_type_refs: A) -> IslConstraint<V> {
+        IslConstraint::AnyOf(AnyOf::new(isl_type_refs.into()))
+    }
+
+    /// Creates a `one_of` constraint using the [IslTypeRef] referenced inside it
+    pub fn one_of<A: Into<Vec<IslTypeRef<V>>>>(isl_type_refs: A) -> IslConstraint<V> {
+        IslConstraint::OneOf(OneOf::new(isl_type_refs.into()))
+    }
+
+    /// Creates a `not` constraint using the [IslTypeRef] referenced inside it
+    pub fn not(isl_type_ref: IslTypeRef<V>) -> IslConstraint<V> {
+        IslConstraint::Not(Not::new(isl_type_ref))
+    }
+
+    /// Creates an `open_content` using the field name and value referenced inside it
+    /// Note: This open content has no effect
+    pub fn open_content(field_name: String, field_value: Element) -> IslConstraint<V> {
+        IslConstraint::Unknown(OpenContent::new(field_name, field_value))
+    }


suggestion—I would leave these out in favor of using the constructor functions on the constraint structs and then having a trivial From implementation. That makes it more flexible in case we ever need to add a new field to a constraint in the future.

Makes sense.
These builder functions give a less verbose API then using the constructor for each constraint but its not a huge difference overall.

IslCosntraint::Type(Type::new(IslTypeRef::named("int")) // vs. IslCosntraint::type_constraint(IslTypeRef::named("int"))

ion-schema/src/isl/version_based_isl/mod.rs

popematt · 2023-06-28T21:03:33Z

ion-schema/src/isl/version_based_isl/mod.rs

+    pub fn open_content(&self) -> Vec<(&String, &Element)> {
+        let mut open_contents = vec![];
+        for constraint in &self.constraints {
+            if let IslConstraint::Unknown(open_content) = constraint {
+                open_contents.push((open_content.field_name(), open_content.field_value()))
+            }
+        }
+        open_contents
+    }


I think this is a hint that open content should not be modeled as a constraint. Perhaps something like this instead?

pub struct IslType<V: IslVersionTrait> { name: Option<String>, constraints: Vec<IslConstraint<V>>, open_content: Vec<OpenContent<V>>, pub(crate) isl_type_struct: Option<Element>, phantom: PhantomData<V>, }

+1, I encountered some mental dissonance when I found OpenContent in the IslConstraint enum earlier in my review. Separating it would be helpful.

ion-schema/src/isl/version_based_isl/mod.rs

ion-schema/src/isl/version_based_isl/constraints/open_content.rs

popematt · 2023-06-28T21:14:43Z

ion-schema/src/isl/version_based_isl/constraints/all_of.rs

+
+#[derive(Debug, Clone, PartialEq)]
+pub struct AllOf<V: IslVersionTrait> {
+    type_refs: Vec<IslTypeRef<V>>,


suggestion—this should be a Bag data type, but I'm fine with punting on it for now.

* renames `IslTypeRef` to `IslTypeArgument` * removes version from `OpenContent` * renames all the phantom properties with _version * adds schema header and footer structs * moves `variably_occurring()` to `IslVariablyOccuringTypeArgument`

ion-schema/src/isl/version_based_isl/constraints/any_of.rs

ion-schema/src/isl/version_based_isl/constraints/mod.rs

ion-schema/src/isl/version_based_isl/mod.rs

zslayton · 2023-07-26T13:37:14Z

ion-schema/src/isl/version_based_isl/mod.rs

+//! //      ],
+//! //      open_content: "This is an open content!"
+//! //  }
+//! let isl_type: IslType<IslV1_0> = IslType::named(


ion-schema/src/isl/version_based_isl/mod.rs

zslayton · 2023-07-26T13:55:44Z

ion-schema/src/isl/version_based_isl/mod.rs

+    /// Represents all the inline IslImportTypes in this schema file.
+    inline_imported_types: Vec<IslImportType>,
+    /// Represents open content as `Element`s
+    /// Note: This doesn't preserve the information about where the open content is placed in the schema file.


This is reasonable, but has interesting effects on how we serialize a schema later. We should talk through the impact it'll have. No TODO here.

ion-schema/src/isl/version_based_isl/mod.rs

* adds changes for doc comments * adds changes to accept Into<String> and IntoIterator for constructors * Modifies return value of &Vec<_> to be &[_]

popematt

I am unsure about the maintainability of having a separate file for each individual constraint—I suspect that we may end up repeating a lot of very similar code across all of the files—but it's better than having a single file with thousands of lines of code.

I did leave a comment about adding trivial From implementations to convert from the inner types of the enum variants to the enum type for IslConstraint. I think it should be done, but it doesn't need to be done immediately.

Overall though, this is looking good.

popematt · 2023-08-01T18:11:56Z

ion-schema/src/isl/version_based_isl/constraints/not.rs

+use crate::isl::version_based_isl::{IslTypeArgument, IslVersionTrait};
+
+#[derive(Debug, Clone, PartialEq)]
+pub struct Not<V: IslVersionTrait> {


Add impl <V: IslVersionTrait> From<Not<V>> for IslConstraint<V>, and similarly for all other constraints.

popematt · 2023-08-01T18:17:22Z

ion-schema/src/isl/version_based_isl/mod.rs

+#[derive(Debug, Clone, PartialEq)]
+pub struct IslType<V: IslVersionTrait> {
+    name: Option<String>,
+    constraints: Vec<IslConstraint<V>>,


We don't have a bag type. There are plenty of 3P bag/multiset types available, but I don't have a particular one in mind. Either way, we need it so that we have proper equality since the order of the constraints is unimportant.

If open content was modeled separately from the constraints, we could use a set instead of a bag since two completely identical constraints are redundant and functionally equivalent to having only one of them.

desaikd added 3 commits June 28, 2023 12:10

Merge branch 'main' of github.com:amazon-ion/ion-schema-rust into des…

910e24a

…aikd-isl-model-revamp

adds changes for ISl 2.0 example

20100ed

desaikd commented Jun 28, 2023

View reviewed changes

desaikd marked this pull request as ready for review June 28, 2023 19:44

desaikd requested review from zslayton and popematt June 28, 2023 19:44

popematt reviewed Jun 28, 2023

View reviewed changes

adds suggested changes

7fbd511

* renames `IslTypeRef` to `IslTypeArgument` * removes version from `OpenContent` * renames all the phantom properties with _version * adds schema header and footer structs * moves `variably_occurring()` to `IslVariablyOccuringTypeArgument`

zslayton reviewed Jul 26, 2023

View reviewed changes

adds suggested changes

4009336

* adds changes for doc comments * adds changes to accept Into<String> and IntoIterator for constructors * Modifies return value of &Vec<_> to be &[_]

popematt approved these changes Aug 1, 2023

View reviewed changes

This was referenced Aug 14, 2023

Add impl <V: IslVersionTrait> From<CONSTRAINT<V>> for IslConstraint<V> for all constraints. #198

Open

Add bag type for storing constraints and type arguments #199

Open

desaikd changed the base branch from main to streamline-isl-model August 14, 2023 19:03

desaikd merged commit de36ea0 into streamline-isl-model Aug 14, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Streamlines ISL model using version maker traits #191

Streamlines ISL model using version maker traits #191

desaikd commented Jun 28, 2023 •

edited

Loading

desaikd Jun 28, 2023

desaikd Jun 28, 2023 •

edited

Loading

desaikd Jun 28, 2023

popematt left a comment

desaikd commented Jun 28, 2023

popematt Jun 28, 2023

desaikd Jun 28, 2023

popematt Jun 28, 2023

zslayton Jul 26, 2023

popematt Jun 28, 2023

zslayton Jul 26, 2023

zslayton Jul 26, 2023

popematt left a comment •

edited

Loading

popematt Aug 1, 2023

popematt Aug 1, 2023

Streamlines ISL model using version maker traits #191

Streamlines ISL model using version maker traits #191

Conversation

desaikd commented Jun 28, 2023 • edited Loading

Issue #187

Description of changes:

List of changes:

Choose a reason for hiding this comment

desaikd Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

popematt left a comment

Choose a reason for hiding this comment

desaikd commented Jun 28, 2023

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

popematt left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

desaikd commented Jun 28, 2023 •

edited

Loading

desaikd Jun 28, 2023 •

edited

Loading

popematt left a comment •

edited

Loading