From c38635849c3a62caaf94a2619a01c5df2fefcadb Mon Sep 17 00:00:00 2001 From: Khushboo Desai Date: Tue, 30 Apr 2024 16:12:07 -0700 Subject: [PATCH 1/8] Adds changes to support sequence types with `type` constraint * Modifies `AbstractDataType::Sequence` to store sequence type (sexp or list) and element type * Adds `element_type()` and `sequence_type()` methods on `AbstractDataType` to get element type and sequence type details * Renames `verify_abstract_data_type_consistency` to `verify_and_update_abstract_data_type` * `verify_and_update_abstract_data_type` modified to check for special cases when `type` and `element` cosntraints occur together * `JavaLanguage` and `RustLanguage` updated `target_type` to return generic type for `List` and `SExp` * Adds `wrapper_class` in `JavaLanguage` to be sued within `ArrayList` --- src/bin/ion/commands/beta/generate/context.rs | 26 +++- .../ion/commands/beta/generate/generator.rs | 134 ++++++++++++++++-- src/bin/ion/commands/beta/generate/utils.rs | 32 ++++- 3 files changed, 178 insertions(+), 14 deletions(-) diff --git a/src/bin/ion/commands/beta/generate/context.rs b/src/bin/ion/commands/beta/generate/context.rs index 7acf77d..7f59564 100644 --- a/src/bin/ion/commands/beta/generate/context.rs +++ b/src/bin/ion/commands/beta/generate/context.rs @@ -38,7 +38,8 @@ pub enum AbstractDataType { // } // ``` Value, - // A series of zero or more values whose type is described by the nested `String` (e.g. a list) + // A series of zero or more values whose type is described by the nested `element_type` + // and sequence type is described by nested `sequence_type` (e.g. a list) // e.g. Given below ISL, // ``` // type::{ @@ -52,7 +53,10 @@ pub enum AbstractDataType { // value: Vec // } // ``` - Sequence(String), + Sequence { + element_type: String, + sequence_type: String, + }, // A collection of field name/value pairs (e.g. a map) // the nested boolean represents whether the struct has closed fields or not // e.g. Given below ISL, @@ -75,6 +79,22 @@ pub enum AbstractDataType { Structure(bool), } +impl AbstractDataType { + pub fn element_type(&self) -> Option<&String> { + match self { + AbstractDataType::Sequence { element_type, .. } => Some(element_type), + _ => None, + } + } + + pub fn sequence_type(&self) -> Option<&String> { + match self { + AbstractDataType::Sequence { sequence_type, .. } => Some(sequence_type), + _ => None, + } + } +} + impl Display for AbstractDataType { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { write!( @@ -82,7 +102,7 @@ impl Display for AbstractDataType { "{}", match self { AbstractDataType::Value => "single value struct", - AbstractDataType::Sequence(_) => "sequence value struct", + AbstractDataType::Sequence { .. } => "sequence value struct", AbstractDataType::Structure(_) => "struct", } ) diff --git a/src/bin/ion/commands/beta/generate/generator.rs b/src/bin/ion/commands/beta/generate/generator.rs index 6695be4..2c91021 100644 --- a/src/bin/ion/commands/beta/generate/generator.rs +++ b/src/bin/ion/commands/beta/generate/generator.rs @@ -312,8 +312,13 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> { match constraint.constraint() { IslConstraintValue::Element(isl_type, _) => { let type_name = self.type_reference_name(isl_type)?; - self.verify_abstract_data_type_consistency( - AbstractDataType::Sequence(type_name.to_owned()), + + self.verify_and_update_abstract_data_type( + AbstractDataType::Sequence { + element_type: type_name.to_owned(), + sequence_type: "list".to_string(), + }, + tera_fields, code_gen_context, )?; self.generate_struct_field( @@ -325,8 +330,9 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> { } IslConstraintValue::Fields(fields, content_closed) => { // TODO: Check for `closed` annotation on fields and based on that return error while reading if there are extra fields. - self.verify_abstract_data_type_consistency( + self.verify_and_update_abstract_data_type( AbstractDataType::Structure(*content_closed), + tera_fields, code_gen_context, )?; for (name, value) in fields.iter() { @@ -343,11 +349,39 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> { IslConstraintValue::Type(isl_type) => { let type_name = self.type_reference_name(isl_type)?; - self.verify_abstract_data_type_consistency( - AbstractDataType::Value, + self.verify_and_update_abstract_data_type( + if isl_type.name() == "list" { + let abstract_data_type = AbstractDataType::Sequence { + element_type: type_name.clone(), + sequence_type: "list".to_string(), + }; + abstract_data_type + } else if isl_type.name() == "sexp" { + let abstract_data_type = AbstractDataType::Sequence { + element_type: type_name.clone(), + sequence_type: "sexp".to_string(), + }; + abstract_data_type + } else { + AbstractDataType::Value + }, + tera_fields, code_gen_context, )?; - self.generate_struct_field(tera_fields, type_name, isl_type.name(), "value")?; + + // if the abstract data type is a sequence then pass the type name as the updated `element_type`. + if let Some(AbstractDataType::Sequence { element_type, .. }) = + &code_gen_context.abstract_data_type + { + self.generate_struct_field( + tera_fields, + L::target_type_as_sequence(element_type), + isl_type.name(), + "value", + )?; + } else { + self.generate_struct_field(tera_fields, type_name, isl_type.name(), "value")?; + } } _ => {} } @@ -373,6 +407,7 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> { /// Verify that the current abstract data type is same as previously determined abstract data type /// This is referring to abstract data type determined with each constraint that is verifies /// that all the constraints map to a single abstract data type and not different abstract data types. + /// Also, updates the underlying `element_type` for List and SExp. /// e.g. /// ``` /// type::{ @@ -386,14 +421,95 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> { /// ``` /// For the above schema, both `fields` and `type` constraints map to different abstract data types /// respectively Struct(with given fields `source` and `destination`) and Value(with a single field that has String data type). - fn verify_abstract_data_type_consistency( + fn verify_and_update_abstract_data_type( &mut self, current_abstract_data_type: AbstractDataType, + tera_fields: &mut Vec, code_gen_context: &mut CodeGenContext, ) -> CodeGenResult<()> { if let Some(abstract_data_type) = &code_gen_context.abstract_data_type { - if abstract_data_type != ¤t_abstract_data_type { - return invalid_abstract_data_type_error(format!("Can not determine abstract data type as current constraint {} conflicts with prior constraints for {}.", current_abstract_data_type, abstract_data_type)); + match abstract_data_type { + // In the case when a `type` constraint occurs before `element` constraint. The element type for the sequence + // needs to be updated based on `element` constraint whereas sequence type will be used as per `type` constraint. + // e.g. For a schema as below: + // ``` + // type::{ + // name: sequence_type, + // type: sexp, + // element: string, + // } + // ``` + // Here, first `type` constraint would set the `AbstractDataType::Sequence{ element_type: T, sequence_type: "sexp"}` + // which uses generic type T and sequence type is sexp. Next `element` constraint would + // set the `AbstractDataType::Sequence{ element_type: String, sequence_type: "list"}`. + // Now this method performs verification that if the above described case occurs + // then it updates the `element_type` as per `element` constraint + // and `sequence_type` as per `type` constraint. + AbstractDataType::Sequence { + element_type, + sequence_type, + } if abstract_data_type != ¤t_abstract_data_type + && (element_type == "Object" || element_type == "T") + && matches!( + ¤t_abstract_data_type, + &AbstractDataType::Sequence { .. } + ) => + { + // if current abstract data type is sequence and element_type is generic T or Object, + // then this was set by a `type` constraint in sequence field, + // so remove all previous fields that allows `Object` and update with current abstract_data_type. + tera_fields.pop(); + code_gen_context.with_abstract_data_type(AbstractDataType::Sequence { + element_type: current_abstract_data_type + .element_type() + .unwrap() + .to_string(), + sequence_type: sequence_type.to_string(), + }); + } + // In the case when a `type` constraint occurs before `element` constraint. The element type for the sequence + // needs to be updated based on `element` constraint whereas sequence type will be used as per `type` constraint. + // e.g. For a schema as below: + // ``` + // type::{ + // name: sequence_type, + // element: string, + // type: sexp, + // } + // ``` + // Here, first `element` constraint would set the `AbstractDataType::Sequence{ element_type: String, sequence_type: "list"}` , + // Next `type` constraint would set the `AbstractDataType::Sequence{ element_type: T, sequence_type: "sexp"}` + // which uses generic type `T` and sequence type is sexp. Now this method performs verification that + // if the above described case occurs then it updates the `element_type` as per `element` constraint + // and `sequence_type` as per `type` constraint. + AbstractDataType::Sequence { element_type, .. } + if abstract_data_type != ¤t_abstract_data_type + && (current_abstract_data_type.element_type() + == Some(&"Object".to_string()) + || current_abstract_data_type.element_type() + == Some(&"T".to_string())) + && matches!( + ¤t_abstract_data_type, + &AbstractDataType::Sequence { .. } + ) => + { + // if `element` constraint has already set the abstract data_type to `Sequence` + // then remove previous fields as new fields will be added again after updating `element_type`. + // `type` constraint does update the ISL type name to either `list` or `sexp`, + // which needs to be updated within `abstract_data_type` as well. + tera_fields.pop(); + code_gen_context.with_abstract_data_type(AbstractDataType::Sequence { + element_type: element_type.to_string(), + sequence_type: current_abstract_data_type + .sequence_type() + .unwrap() + .to_string(), + }) + } + _ if abstract_data_type != ¤t_abstract_data_type => { + return invalid_abstract_data_type_error(format!("Can not determine abstract data type as current constraint {} conflicts with prior constraints for {}.", current_abstract_data_type, abstract_data_type)); + } + _ => {} } } else { code_gen_context.with_abstract_data_type(current_abstract_data_type); diff --git a/src/bin/ion/commands/beta/generate/utils.rs b/src/bin/ion/commands/beta/generate/utils.rs index d66594c..67ab269 100644 --- a/src/bin/ion/commands/beta/generate/utils.rs +++ b/src/bin/ion/commands/beta/generate/utils.rs @@ -74,13 +74,21 @@ impl Language for JavaLanguage { Float => "double", Bool => "boolean", Blob | Clob => "byte[]", + List | SExp => "Object", SchemaDefined(name) => name, } .to_string() } fn target_type_as_sequence(target_type: &str) -> String { - format!("ArrayList<{}>", target_type) + match JavaLanguage::wrapper_class(target_type) { + Some(wrapper_class_name) => { + format!("ArrayList<{}>", wrapper_class_name) + } + None => { + format!("ArrayList<{}>", target_type) + } + } } fn field_name_case() -> Case { @@ -98,6 +106,21 @@ impl Language for JavaLanguage { } } +impl JavaLanguage { + fn wrapper_class(primitive_data_type_name: &str) -> Option<&str> { + match primitive_data_type_name { + "int" => Some("Integer"), + "bool" => Some("Boolean"), + "double" => Some("Double"), + "long" => Some("Long"), + _ => { + // for any other non-primitive types return None + None + } + } + } +} + impl Display for JavaLanguage { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { write!(f, "java") @@ -127,6 +150,7 @@ impl Language for RustLanguage { Float => "f64", Bool => "bool", Blob | Clob => "Vec", + List | SExp => "T", SchemaDefined(name) => name, } .to_string() @@ -192,6 +216,8 @@ pub enum IonSchemaType { Bool, Blob, Clob, + SExp, + List, SchemaDefined(String), // A user defined schema type } @@ -215,9 +241,11 @@ impl From<&str> for IonSchemaType { "decimal" | "timestamp" => { unimplemented!("Decimal, Number and Timestamp aren't support yet!") } - "list" | "struct" | "sexp" => { + "struct" => { unimplemented!("Generic containers aren't supported yet!") } + "list" => List, + "sexp" => SExp, _ => SchemaDefined(value.to_case(Case::UpperCamel)), } } From 938b0dbe5a550741f40eecf97af8b53021631209 Mon Sep 17 00:00:00 2001 From: Khushboo Desai Date: Tue, 30 Apr 2024 16:16:46 -0700 Subject: [PATCH 2/8] Adds result template to generate `SerdeResult` in Rust * Adds template to generate `SerdeResult` in Rust * Changes current tests to use `SerdeResult` * Changes generator to render `SerdeReuslt` in output module --- .../rust/code-gen-demo/src/lib.rs | 16 +++++----- .../ion/commands/beta/generate/generator.rs | 8 +++-- .../beta/generate/templates/rust/result.templ | 32 +++++++++++++++++++ tests/cli.rs | 4 +-- 4 files changed, 48 insertions(+), 12 deletions(-) create mode 100644 src/bin/ion/commands/beta/generate/templates/rust/result.templ diff --git a/code-gen-projects/rust/code-gen-demo/src/lib.rs b/code-gen-projects/rust/code-gen-demo/src/lib.rs index 8741095..dc9597f 100644 --- a/code-gen-projects/rust/code-gen-demo/src/lib.rs +++ b/code-gen-projects/rust/code-gen-demo/src/lib.rs @@ -21,8 +21,8 @@ mod tests { } #[test_resources("../../input/good/struct_with_fields/**/*.ion")] - fn roundtrip_good_test_generated_code_structs_with_fields(file_name: &str) -> IonResult<()> { - let ion_string = fs::read_to_string(file_name)?; + fn roundtrip_good_test_generated_code_structs_with_fields(file_name: &str) -> SerdeResult<()> { + let ion_string = fs::read_to_string(file_name).unwrap(); let mut reader = ReaderBuilder::new().build(ion_string.clone())?; let mut buffer = Vec::new(); let mut text_writer = TextWriterBuilder::default().build(&mut buffer)?; @@ -42,8 +42,8 @@ mod tests { } #[test_resources("../../input/bad/struct_with_fields/**/*.ion")] - fn roundtrip_bad_test_generated_code_structs_with_fields(file_name: &str) -> IonResult<()> { - let ion_string = fs::read_to_string(file_name)?; + fn roundtrip_bad_test_generated_code_structs_with_fields(file_name: &str) -> SerdeResult<()> { + let ion_string = fs::read_to_string(file_name).unwrap(); let mut reader = ReaderBuilder::new().build(ion_string.clone())?; // read given Ion value using Ion reader reader.next()?; @@ -54,8 +54,8 @@ mod tests { } #[test_resources("../../input/good/nested_struct/**/*.ion")] - fn roundtrip_good_test_generated_code_nested_structs(file_name: &str) -> IonResult<()> { - let ion_string = fs::read_to_string(file_name)?; + fn roundtrip_good_test_generated_code_nested_structs(file_name: &str) -> SerdeResult<()> { + let ion_string = fs::read_to_string(file_name).unwrap(); let mut reader = ReaderBuilder::new().build(ion_string.clone())?; let mut buffer = Vec::new(); let mut text_writer = TextWriterBuilder::default().build(&mut buffer)?; @@ -75,8 +75,8 @@ mod tests { } #[test_resources("../../input/bad/nested_struct/**/*.ion")] - fn roundtrip_bad_test_generated_code_nested_structs(file_name: &str) -> IonResult<()> { - let ion_string = fs::read_to_string(file_name)?; + fn roundtrip_bad_test_generated_code_nested_structs(file_name: &str) -> SerdeResult<()> { + let ion_string = fs::read_to_string(file_name).unwrap(); let mut reader = ReaderBuilder::new().build(ion_string.clone())?; // read given Ion value using Ion reader reader.next()?; diff --git a/src/bin/ion/commands/beta/generate/generator.rs b/src/bin/ion/commands/beta/generate/generator.rs index 2c91021..30e7f4e 100644 --- a/src/bin/ion/commands/beta/generate/generator.rs +++ b/src/bin/ion/commands/beta/generate/generator.rs @@ -38,7 +38,10 @@ impl<'a> CodeGenerator<'a, RustLanguage> { .unwrap(); // Render the imports into output file - let rendered = tera.render("import.templ", &Context::new()).unwrap(); + let rendered_import = tera.render("import.templ", &Context::new()).unwrap(); + // Render the SerdeResult that is used in generated read-write APIs + let rendered_result = tera.render("result.templ", &Context::new()).unwrap(); + let mut file = OpenOptions::new() // In order for the file to be created, OpenOptions::write or OpenOptions::append access must be used // reference: https://doc.rust-lang.org/std/fs/struct.OpenOptions.html#method.create @@ -47,7 +50,8 @@ impl<'a> CodeGenerator<'a, RustLanguage> { .create(true) .open(output.join("ion_generated_code.rs")) .unwrap(); - file.write_all(rendered.as_bytes()).unwrap(); + file.write_all(rendered_import.as_bytes()).unwrap(); + file.write_all(rendered_result.as_bytes()).unwrap(); Self { output, diff --git a/src/bin/ion/commands/beta/generate/templates/rust/result.templ b/src/bin/ion/commands/beta/generate/templates/rust/result.templ new file mode 100644 index 0000000..7ae37dc --- /dev/null +++ b/src/bin/ion/commands/beta/generate/templates/rust/result.templ @@ -0,0 +1,32 @@ +/// Represents serde result +pub type SerdeResult = Result; + +/// Represents an error found during code generation +#[derive(Debug)] +pub enum SerdeError { + IonError { source: IonError }, + SerializationError { description: String }, + DeserializationError { description: String }, +} + +/// A convenience method for creating an SerdeError::SerializationError +/// with the provided description text. +pub fn serialization_error>(description: S) -> SerdeResult { + Err(SerdeError::SerializationError { + description: description.as_ref().to_string(), + }) +} + +/// A convenience method for creating an SerdeError::DeserializationError +/// with the provided description text. +pub fn deserialization_error>(description: S) -> SerdeResult { + Err(SerdeError::DeserializationError { + description: description.as_ref().to_string(), + }) +} + +impl From for SerdeError { + fn from(value: IonError) -> Self { + SerdeError::IonError { source: value } + } +} diff --git a/tests/cli.rs b/tests/cli.rs index 1beb817..3861b91 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -317,9 +317,9 @@ fn test_code_generation_in_rust( assert!(contents.contains(expected_accessor)); } // verify that it generates read-write APIs - assert!(contents.contains("pub fn read_from(reader: &mut Reader) -> IonResult {")); + assert!(contents.contains("pub fn read_from(reader: &mut Reader) -> SerdeResult {")); assert!(contents - .contains("pub fn write_to(&self, writer: &mut W) -> IonResult<()> {")); + .contains("pub fn write_to(&self, writer: &mut W) -> SerdeResult<()> {")); Ok(()) } From 6e2d9610aaacd8fe913000b3d197741e285bd0d0 Mon Sep 17 00:00:00 2001 From: Khushboo Desai Date: Tue, 30 Apr 2024 16:19:13 -0700 Subject: [PATCH 3/8] Adds template file changes for SExp support * Modifies class and struct template to add check for sequence type in write_to API * Modifies class and struct template to add check for sequence type in read_from API * Modifies struct template to use the new `SerdeResult` in `read_from` and `write_to` API --- .../beta/generate/templates/java/class.templ | 29 ++++++++++---- .../beta/generate/templates/rust/import.templ | 2 +- .../beta/generate/templates/rust/struct.templ | 38 ++++++++++++++----- 3 files changed, 50 insertions(+), 19 deletions(-) diff --git a/src/bin/ion/commands/beta/generate/templates/java/class.templ b/src/bin/ion/commands/beta/generate/templates/java/class.templ index 4a457c0..271b8b0 100644 --- a/src/bin/ion/commands/beta/generate/templates/java/class.templ +++ b/src/bin/ion/commands/beta/generate/templates/java/class.templ @@ -84,18 +84,27 @@ public final class {{ target_kind_name }} { reader.stepOut(); {% elif abstract_data_type is object and abstract_data_type is containing("Sequence") %} {# Reads `Sequence` class with a single field `value` that is an `ArrayList` #} + {% if abstract_data_type["Sequence"].sequence_type == "list" %} + if(reader.getType() != IonType.LIST) { + throw new IonException("Expected list, found" + reader.getType() + "while reading {{ target_kind_name }}."); + } + {% elif abstract_data_type["Sequence"].sequence_type == "sexp" %} + if(reader.getType() != IonType.SEXP) { + throw new IonException("Expected sexpression, found" + reader.getType() + "while reading {{ target_kind_name }}."); + } + {% endif %} reader.stepIn(); value = new {{ fields[0].value_type }}(); {# Iterate through the `ArraList` and read each element in it based on the data type provided in `abstract_data_type[Sequence]` #} while (reader.hasNext()) { reader.next(); - {% if abstract_data_type["Sequence"] | is_built_in_type == false %} - value.add({{ abstract_data_type["Sequence"] }}.readFrom(reader)); + {% if abstract_data_type["Sequence"].element_type | is_built_in_type == false %} + value.add({{ abstract_data_type["Sequence"].element_type }}.readFrom(reader)); {% else %} - {% if abstract_data_type["Sequence"] == "bytes[]" %} + {% if abstract_data_type["Sequence"].element_type == "bytes[]" %} value.add(reader.newBytes()); {% else %} - value.add(reader.{{ abstract_data_type["Sequence"] | camel }}Value()); + value.add(reader.{{ abstract_data_type["Sequence"].element_type | camel }}Value()); {% endif %} {% endif %} } @@ -135,12 +144,16 @@ public final class {{ target_kind_name }} { writer.stepOut(); {% elif abstract_data_type is object and abstract_data_type is containing("Sequence") %} {# Writes `Sequence` class with a single field `value` that is an `ArrayList` as an Ion sequence #} - writer.stepIn(IonType.LIST); - for ({{ abstract_data_type["Sequence"] }} value: this.value) { - {% if abstract_data_type["Sequence"] | is_built_in_type == false %} + {% if abstract_data_type["Sequence"].sequence_type == "list" %} + writer.stepIn(IonType.LIST); + {% else %} + writer.stepIn(IonType.SEXP); + {% endif %} + for ({{ abstract_data_type["Sequence"].element_type }} value: this.value) { + {% if abstract_data_type["Sequence"].element_type | is_built_in_type == false %} value.writeTo(writer); {% else %} - writer.write{{ abstract_data_type["Sequence"] | upper_camel }}(value); + writer.write{{ abstract_data_type["Sequence"].element_type | upper_camel }}(value); {% endif %} } writer.stepOut(); diff --git a/src/bin/ion/commands/beta/generate/templates/rust/import.templ b/src/bin/ion/commands/beta/generate/templates/rust/import.templ index 059b9ca..c9d5993 100644 --- a/src/bin/ion/commands/beta/generate/templates/rust/import.templ +++ b/src/bin/ion/commands/beta/generate/templates/rust/import.templ @@ -1 +1 @@ -use ion_rs::{IonResult, IonReader, Reader, IonWriter, StreamItem}; +use ion_rs::{IonResult, IonError, IonReader, Reader, IonWriter, StreamItem}; diff --git a/src/bin/ion/commands/beta/generate/templates/rust/struct.templ b/src/bin/ion/commands/beta/generate/templates/rust/struct.templ index 6e1e265..caeefcd 100644 --- a/src/bin/ion/commands/beta/generate/templates/rust/struct.templ +++ b/src/bin/ion/commands/beta/generate/templates/rust/struct.templ @@ -21,7 +21,7 @@ impl {{ target_kind_name }} { {% endfor %} - pub fn read_from(reader: &mut Reader) -> IonResult { + pub fn read_from(reader: &mut Reader) -> SerdeResult { let mut abstract_data_type = {{ target_kind_name }}::default(); {% if abstract_data_type == "Value"%} abstract_data_type.value = {% if fields[0].value_type | is_built_in_type == false %} @@ -43,7 +43,7 @@ impl {{ target_kind_name }} { {% endfor %} _ => { {% if abstract_data_type["Structure"] %} - return IonResult::decoding_error( + return deserialization_error( "Can not read field name:{{ field.name }} for {{ target_kind_name }} as it doesn't exist in the given schema type definition." ); {% endif %} @@ -53,27 +53,41 @@ impl {{ target_kind_name }} { } reader.step_out()?; {% elif abstract_data_type is object and abstract_data_type is containing("Sequence") %} + {% if abstract_data_type["Sequence"].sequence_type == "list" %} + if reader.ion_type() != Some(IonType::List) { + return deserialization_error(format!( + "Expected list, found {} while reading {{ target_kind_name }}.", reader.ion_type().unwrap() + )); + } + {% elif abstract_data_type["Sequence"].sequence_type == "sexp" %} + if reader.ion_type() != Some(IonType::SExp) { + return deserialization_error(format!( + "Expected sexpression, found {} while reading {{ target_kind_name }}.", reader.ion_type().unwrap() + )); + } + {% endif %} reader.step_in()?; + abstract_data_type.value = { let mut values = vec![]; while reader.next()? != StreamItem::Nothing { - {% if abstract_data_type["Sequence"] | is_built_in_type == false %} - values.push({{ abstract_data_type["Sequence"] }}::read_from(reader)?); + {% if abstract_data_type["Sequence"].element_type | is_built_in_type == false %} + values.push({{ abstract_data_type["Sequence"].element_type }}::read_from(reader)?); {% else %} - values.push(reader.read_{% if fields[0].isl_type_name == "symbol" %}symbol()?.text().unwrap(){% else %}{{ abstract_data_type["Sequence"] | lower | replace(from="string", to ="str") }}()?{% endif %}{% if abstract_data_type["Sequence"] | lower== "string" %} .to_string() {% endif %}); + values.push(reader.read_{% if fields[0].isl_type_name == "symbol" %}symbol()?.text().unwrap(){% else %}{{ abstract_data_type["Sequence"].element_type | lower | replace(from="string", to ="str") }}()?{% endif %}{% if abstract_data_type["Sequence"].element_type | lower== "string" %} .to_string() {% endif %}); {% endif %} } values }; reader.step_out()?; {% else %} - return IonResult::decoding_error("Can not resolve read API template for {{ target_kind_name }}"); + return deserialization_error("Can not resolve read API template for {{ target_kind_name }}"); {% endif %} Ok(abstract_data_type) } - pub fn write_to(&self, writer: &mut W) -> IonResult<()> { + pub fn write_to(&self, writer: &mut W) -> SerdeResult<()> { {% if abstract_data_type == "Value" %} {% for field in fields %} {% if field.value_type | is_built_in_type == false %} @@ -95,12 +109,16 @@ impl {{ target_kind_name }} { {% endfor %} writer.step_out()?; {% elif abstract_data_type is object and abstract_data_type is containing("Sequence") %} - writer.step_in(IonType::List)?; + {% if abstract_data_type["Sequence"].sequence_type == "list" %} + writer.step_in(IonType::List)?; + {% else %} + writer.step_in(IonType::SExp)?; + {% endif %} for value in &self.value { - {% if abstract_data_type["Sequence"] | is_built_in_type == false %} + {% if abstract_data_type["Sequence"].element_type | is_built_in_type == false %} value.write_to(writer)?; {% else %} - writer.write_{% if fields[0].isl_type_name == "symbol" %}symbol{% else %}{{ abstract_data_type["Sequence"] | lower }}{% endif %}(value.to_owned())?; + writer.write_{% if fields[0].isl_type_name == "symbol" %}symbol{% else %}{{ abstract_data_type["Sequence"].element_type | lower }}{% endif %}(value.to_owned())?; {% endif %} } writer.step_out()?; From 5710a9275ebd697bd141f127276b5aedbde996f6 Mon Sep 17 00:00:00 2001 From: Khushboo Desai Date: Tue, 30 Apr 2024 16:21:47 -0700 Subject: [PATCH 4/8] Adds new test files for SExp support * Adds new fields that uses SExpression in Ion files * Adds bad Ion files to verify SExpression and List are properly serialized-deserialized * Adds ISL files with Sexpression field changes * Adds changes to Java setter tests --- .../bad/nested_struct/mismatched_sequence_type.ion | 9 +++++++++ .../input/bad/nested_struct/mismatched_type.ion | 1 + .../mismatched_sequence_element.ion | 2 +- .../struct_with_fields/mismatched_sequence_type.ion | 7 +++++++ .../input/bad/struct_with_fields/mismatched_type.ion | 2 +- .../input/good/nested_struct/empty_values.ion | 3 ++- .../input/good/nested_struct/valid_fields.ion | 1 + .../good/nested_struct/valid_unordered_fields.ion | 1 + .../input/good/struct_with_fields/empty_values.ion | 2 +- .../input/good/struct_with_fields/valid_fields.ion | 2 +- .../struct_with_fields/valid_unordered_fields.ion | 2 +- .../src/test/java/org/example/CodeGenTest.java | 11 ++++++++--- code-gen-projects/schema/nested_struct.isl | 3 ++- code-gen-projects/schema/struct_with_fields.isl | 2 +- 14 files changed, 37 insertions(+), 11 deletions(-) create mode 100644 code-gen-projects/input/bad/nested_struct/mismatched_sequence_type.ion create mode 100644 code-gen-projects/input/bad/struct_with_fields/mismatched_sequence_type.ion diff --git a/code-gen-projects/input/bad/nested_struct/mismatched_sequence_type.ion b/code-gen-projects/input/bad/nested_struct/mismatched_sequence_type.ion new file mode 100644 index 0000000..aa1c307 --- /dev/null +++ b/code-gen-projects/input/bad/nested_struct/mismatched_sequence_type.ion @@ -0,0 +1,9 @@ +// nested struct with mismatched sequence type +{ + A: "hello", + B: 12, + C: { + D: false, + E: (1 2 3) // expected list + } +} \ No newline at end of file diff --git a/code-gen-projects/input/bad/nested_struct/mismatched_type.ion b/code-gen-projects/input/bad/nested_struct/mismatched_type.ion index e8c48db..30ae03b 100644 --- a/code-gen-projects/input/bad/nested_struct/mismatched_type.ion +++ b/code-gen-projects/input/bad/nested_struct/mismatched_type.ion @@ -4,5 +4,6 @@ B: 12, C: { D: 1e0, // expected type: bool + E: [1, 2, 3] } } diff --git a/code-gen-projects/input/bad/struct_with_fields/mismatched_sequence_element.ion b/code-gen-projects/input/bad/struct_with_fields/mismatched_sequence_element.ion index 9d60dd2..bcb578d 100644 --- a/code-gen-projects/input/bad/struct_with_fields/mismatched_sequence_element.ion +++ b/code-gen-projects/input/bad/struct_with_fields/mismatched_sequence_element.ion @@ -2,7 +2,7 @@ { A: "hello", B: 12, - C: [1, 2, 3], + C: (1 2 3), D: 10e2 } diff --git a/code-gen-projects/input/bad/struct_with_fields/mismatched_sequence_type.ion b/code-gen-projects/input/bad/struct_with_fields/mismatched_sequence_type.ion new file mode 100644 index 0000000..300a99f --- /dev/null +++ b/code-gen-projects/input/bad/struct_with_fields/mismatched_sequence_type.ion @@ -0,0 +1,7 @@ +// simple struct with type mismatched sequence type +{ + A: "hello", + B: 12, + C: ["foo", "bar", "baz"], // expected sexp + D: 10e2 +} diff --git a/code-gen-projects/input/bad/struct_with_fields/mismatched_type.ion b/code-gen-projects/input/bad/struct_with_fields/mismatched_type.ion index 675df97..3cad533 100644 --- a/code-gen-projects/input/bad/struct_with_fields/mismatched_type.ion +++ b/code-gen-projects/input/bad/struct_with_fields/mismatched_type.ion @@ -2,7 +2,7 @@ { A: "hello", B: false, // expected field type: int - C: ["foo", "bar", "baz"], + C: ("foo" "bar" "baz"), D: 10e2 } diff --git a/code-gen-projects/input/good/nested_struct/empty_values.ion b/code-gen-projects/input/good/nested_struct/empty_values.ion index 545e6b2..798cc96 100644 --- a/code-gen-projects/input/good/nested_struct/empty_values.ion +++ b/code-gen-projects/input/good/nested_struct/empty_values.ion @@ -1,7 +1,8 @@ -// nested struct with empty string and zeros +// nested struct with empty string, list and zeros { C: { D: false, + E: [], }, A: "", B: 0, diff --git a/code-gen-projects/input/good/nested_struct/valid_fields.ion b/code-gen-projects/input/good/nested_struct/valid_fields.ion index 391c8cd..f63ab8a 100644 --- a/code-gen-projects/input/good/nested_struct/valid_fields.ion +++ b/code-gen-projects/input/good/nested_struct/valid_fields.ion @@ -4,5 +4,6 @@ B: 12, C: { D: false, + E: [1, 2, 3] } } diff --git a/code-gen-projects/input/good/nested_struct/valid_unordered_fields.ion b/code-gen-projects/input/good/nested_struct/valid_unordered_fields.ion index da21667..cca4717 100644 --- a/code-gen-projects/input/good/nested_struct/valid_unordered_fields.ion +++ b/code-gen-projects/input/good/nested_struct/valid_unordered_fields.ion @@ -4,6 +4,7 @@ A: "hello", C: { D: false, + E: [1, 2, 3] } } diff --git a/code-gen-projects/input/good/struct_with_fields/empty_values.ion b/code-gen-projects/input/good/struct_with_fields/empty_values.ion index 67986dc..a5c1386 100644 --- a/code-gen-projects/input/good/struct_with_fields/empty_values.ion +++ b/code-gen-projects/input/good/struct_with_fields/empty_values.ion @@ -1,6 +1,6 @@ // struct with empty list, empty string and zeros { - C: [], + C: (), A: "", B: 0, D: 0e0, diff --git a/code-gen-projects/input/good/struct_with_fields/valid_fields.ion b/code-gen-projects/input/good/struct_with_fields/valid_fields.ion index f306089..1ccd777 100644 --- a/code-gen-projects/input/good/struct_with_fields/valid_fields.ion +++ b/code-gen-projects/input/good/struct_with_fields/valid_fields.ion @@ -2,6 +2,6 @@ { A: "hello", B: 12, - C: ["foo", "bar", "baz"], + C: ("foo" "bar" "baz"), D: 10e2 } diff --git a/code-gen-projects/input/good/struct_with_fields/valid_unordered_fields.ion b/code-gen-projects/input/good/struct_with_fields/valid_unordered_fields.ion index 80b9a4e..62d974f 100644 --- a/code-gen-projects/input/good/struct_with_fields/valid_unordered_fields.ion +++ b/code-gen-projects/input/good/struct_with_fields/valid_unordered_fields.ion @@ -1,7 +1,7 @@ // struct with unordered fields { - C: ["foo", "bar", "baz"], + C: ("foo" "bar" "baz"), A: "hello", B: 12, D: 10e2, diff --git a/code-gen-projects/java/code-gen-demo/src/test/java/org/example/CodeGenTest.java b/code-gen-projects/java/code-gen-demo/src/test/java/org/example/CodeGenTest.java index 3e093bc..742cc29 100644 --- a/code-gen-projects/java/code-gen-demo/src/test/java/org/example/CodeGenTest.java +++ b/code-gen-projects/java/code-gen-demo/src/test/java/org/example/CodeGenTest.java @@ -26,7 +26,7 @@ class CodeGenTest { a.add("foo"); a.add("bar"); a.add("baz"); - StructWithFields s = new StructWithFields("hello", 12, new AnonymousType2(a), 10e2); + StructWithFields s = new StructWithFields("hello", 12, new AnonymousType3(a), 10e2); // getter tests for `StructWithFields` assertEquals("hello", s.getA(), "s.getA() should return \"hello\""); @@ -39,7 +39,7 @@ class CodeGenTest { assertEquals("hi", s.getA(), "s.getA() should return \"hi\""); s.setB(6); assertEquals(6, s.getB(), "s.getB() should return `6`"); - s.setC(new AnonymousType2(new ArrayList())); + s.setC(new AnonymousType3(new ArrayList())); assertEquals(true, s.getC().getValue().isEmpty(), "s.getC().isEmpty() should return `true`"); s.setD(11e3); assertEquals(11e3 ,s.getD(), "s.getD() should return `11e3`"); @@ -47,10 +47,15 @@ class CodeGenTest { @Test void getterAndSetterTestForNestedStruct() { // getter tests for `NestedStruct` - NestedStruct n = new NestedStruct("hello", 12, new AnonymousType1(false)); + ArrayList a = new ArrayList(); + a.add(1); + a.add(2); + a.add(3); + NestedStruct n = new NestedStruct("hello", 12, new AnonymousType1(false, new AnonymousType2(a))); assertEquals("hello", n.getA(), "n.getA() should return \"hello\""); assertEquals(12, n.getB(), "n.getB() should return `12`"); assertEquals(false, n.getC().getD(), "n.getC().getD() should return `false`"); + assertEquals(3, n.getC().getE().getValue().size(), "n.getC().getE().getValue().size() should return ArrayList fo size 3"); // setter tests for `NestedStruct` n.setA("hi"); diff --git a/code-gen-projects/schema/nested_struct.isl b/code-gen-projects/schema/nested_struct.isl index bdea322..7e0dd53 100644 --- a/code-gen-projects/schema/nested_struct.isl +++ b/code-gen-projects/schema/nested_struct.isl @@ -5,7 +5,8 @@ type::{ B: int, C: { fields: { - D: bool + D: bool, + E: { element: int } // default sequence type is `list` } } } diff --git a/code-gen-projects/schema/struct_with_fields.isl b/code-gen-projects/schema/struct_with_fields.isl index f03fbb0..f1eec2c 100644 --- a/code-gen-projects/schema/struct_with_fields.isl +++ b/code-gen-projects/schema/struct_with_fields.isl @@ -3,7 +3,7 @@ type::{ fields: { A: string, B: int, - C: { element: string }, + C: { element: string, type: sexp }, D: float, } } From da54b15177cf0e8dd3c6ba2b55687a7d2276601b Mon Sep 17 00:00:00 2001 From: Khushboo Desai Date: Thu, 2 May 2024 14:42:41 -0700 Subject: [PATCH 5/8] Adds clippy changes --- src/bin/ion/commands/beta/generate/generator.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/bin/ion/commands/beta/generate/generator.rs b/src/bin/ion/commands/beta/generate/generator.rs index 30e7f4e..b21985e 100644 --- a/src/bin/ion/commands/beta/generate/generator.rs +++ b/src/bin/ion/commands/beta/generate/generator.rs @@ -355,17 +355,15 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> { self.verify_and_update_abstract_data_type( if isl_type.name() == "list" { - let abstract_data_type = AbstractDataType::Sequence { + AbstractDataType::Sequence { element_type: type_name.clone(), sequence_type: "list".to_string(), - }; - abstract_data_type + } } else if isl_type.name() == "sexp" { - let abstract_data_type = AbstractDataType::Sequence { + AbstractDataType::Sequence { element_type: type_name.clone(), sequence_type: "sexp".to_string(), - }; - abstract_data_type + } } else { AbstractDataType::Value }, From 667c1449d2b2c6065ecf917c1593ee58f6b3108c Mon Sep 17 00:00:00 2001 From: Khushboo Desai Date: Tue, 7 May 2024 14:18:07 -0700 Subject: [PATCH 6/8] Adds suggested changes * Removes extra tab space * Uses `Option` to represent if `element_type` or `sequence_type` is not present for AbtsractDataType * Adds a check in code gen to return error when any field in `tera_fields` have `None` value in `value_type` * Adds tests for `type` and `element` constraint absence * Modifies tests to include `type` constraint along with `element` constraint --- .../test/java/org/example/CodeGenTest.java | 6 +- code-gen-projects/schema/nested_struct.isl | 2 +- src/bin/ion/commands/beta/generate/context.rs | 16 +-- .../ion/commands/beta/generate/generator.rs | 68 +++++++----- src/bin/ion/commands/beta/generate/utils.rs | 102 +++++++++++------- tests/cli.rs | 6 +- tests/code-gen-tests.rs | 51 +++++++++ 7 files changed, 170 insertions(+), 81 deletions(-) diff --git a/code-gen-projects/java/code-gen-demo/src/test/java/org/example/CodeGenTest.java b/code-gen-projects/java/code-gen-demo/src/test/java/org/example/CodeGenTest.java index 742cc29..a731f14 100644 --- a/code-gen-projects/java/code-gen-demo/src/test/java/org/example/CodeGenTest.java +++ b/code-gen-projects/java/code-gen-demo/src/test/java/org/example/CodeGenTest.java @@ -48,9 +48,9 @@ class CodeGenTest { @Test void getterAndSetterTestForNestedStruct() { // getter tests for `NestedStruct` ArrayList a = new ArrayList(); - a.add(1); - a.add(2); - a.add(3); + a.add(1); + a.add(2); + a.add(3); NestedStruct n = new NestedStruct("hello", 12, new AnonymousType1(false, new AnonymousType2(a))); assertEquals("hello", n.getA(), "n.getA() should return \"hello\""); assertEquals(12, n.getB(), "n.getB() should return `12`"); diff --git a/code-gen-projects/schema/nested_struct.isl b/code-gen-projects/schema/nested_struct.isl index 7e0dd53..2e37fbc 100644 --- a/code-gen-projects/schema/nested_struct.isl +++ b/code-gen-projects/schema/nested_struct.isl @@ -6,7 +6,7 @@ type::{ C: { fields: { D: bool, - E: { element: int } // default sequence type is `list` + E: { type: list, element: int } } } } diff --git a/src/bin/ion/commands/beta/generate/context.rs b/src/bin/ion/commands/beta/generate/context.rs index 7f59564..2ce4420 100644 --- a/src/bin/ion/commands/beta/generate/context.rs +++ b/src/bin/ion/commands/beta/generate/context.rs @@ -39,7 +39,9 @@ pub enum AbstractDataType { // ``` Value, // A series of zero or more values whose type is described by the nested `element_type` - // and sequence type is described by nested `sequence_type` (e.g. a list) + // and sequence type is described by nested `sequence_type` (e.g. a list). + // If there is no `element` constraint present in schema type then `element_type` will be None. + // If there is no `type` constraint present in schema type then `sequence_type` will be None. // e.g. Given below ISL, // ``` // type::{ @@ -54,8 +56,8 @@ pub enum AbstractDataType { // } // ``` Sequence { - element_type: String, - sequence_type: String, + element_type: Option, + sequence_type: Option, }, // A collection of field name/value pairs (e.g. a map) // the nested boolean represents whether the struct has closed fields or not @@ -80,16 +82,16 @@ pub enum AbstractDataType { } impl AbstractDataType { - pub fn element_type(&self) -> Option<&String> { + pub fn element_type(&self) -> Option { match self { - AbstractDataType::Sequence { element_type, .. } => Some(element_type), + AbstractDataType::Sequence { element_type, .. } => element_type.to_owned(), _ => None, } } - pub fn sequence_type(&self) -> Option<&String> { + pub fn sequence_type(&self) -> Option { match self { - AbstractDataType::Sequence { sequence_type, .. } => Some(sequence_type), + AbstractDataType::Sequence { sequence_type, .. } => sequence_type.to_owned(), _ => None, } } diff --git a/src/bin/ion/commands/beta/generate/generator.rs b/src/bin/ion/commands/beta/generate/generator.rs index b21985e..54a6996 100644 --- a/src/bin/ion/commands/beta/generate/generator.rs +++ b/src/bin/ion/commands/beta/generate/generator.rs @@ -231,6 +231,19 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> { )?; } + // if any field in `tera_fields` contains a `None` `value_type` then it means there is a constraint that leads to open ended types. + // Return error in such case. + if tera_fields.iter().any( + |Field { + name: _, + value_type, + isl_type_name: _, + }| value_type.is_none(), + ) { + return invalid_abstract_data_type_error("Currently code generation does not support open ended types. \ + Error can be due to a missing `type` constraint or `element` constraint in the type definition."); + } + // add fields for template // TODO: verify the `occurs` value within a field, by default the fields are optional. if let Some(abstract_data_type) = &code_gen_context.abstract_data_type { @@ -281,7 +294,7 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> { } /// Provides name of the type reference that will be used for generated abstract data type - fn type_reference_name(&mut self, isl_type_ref: &IslTypeRef) -> CodeGenResult { + fn type_reference_name(&mut self, isl_type_ref: &IslTypeRef) -> CodeGenResult> { Ok(match isl_type_ref { IslTypeRef::Named(name, _) => { let schema_type: IonSchemaType = name.into(); @@ -294,7 +307,7 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> { let name = self.next_anonymous_type_name(); self.generate_abstract_data_type(&name, type_def)?; - name + Some(name) } }) } @@ -320,17 +333,27 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> { self.verify_and_update_abstract_data_type( AbstractDataType::Sequence { element_type: type_name.to_owned(), - sequence_type: "list".to_string(), + sequence_type: None, }, tera_fields, code_gen_context, )?; - self.generate_struct_field( - tera_fields, - L::target_type_as_sequence(&type_name), - isl_type.name(), - "value", - )?; + + // if the abstract data type is a sequence then pass the type name as the updated `element_type`. + if let Some(AbstractDataType::Sequence { + element_type, + sequence_type: Some(_), + }) = &code_gen_context.abstract_data_type + { + self.generate_struct_field( + tera_fields, + L::target_type_as_sequence(element_type), + isl_type.name(), + "value", + )?; + } else { + self.generate_struct_field(tera_fields, None, isl_type.name(), "value")?; + } } IslConstraintValue::Fields(fields, content_closed) => { // TODO: Check for `closed` annotation on fields and based on that return error while reading if there are extra fields. @@ -357,12 +380,12 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> { if isl_type.name() == "list" { AbstractDataType::Sequence { element_type: type_name.clone(), - sequence_type: "list".to_string(), + sequence_type: Some("list".to_string()), } } else if isl_type.name() == "sexp" { AbstractDataType::Sequence { element_type: type_name.clone(), - sequence_type: "sexp".to_string(), + sequence_type: Some("sexp".to_string()), } } else { AbstractDataType::Value @@ -394,7 +417,7 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> { fn generate_struct_field( &mut self, tera_fields: &mut Vec, - abstract_data_type_name: String, + abstract_data_type_name: Option, isl_type_name: String, field_name: &str, ) -> CodeGenResult<()> { @@ -451,7 +474,7 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> { element_type, sequence_type, } if abstract_data_type != ¤t_abstract_data_type - && (element_type == "Object" || element_type == "T") + && (element_type.is_none()) && matches!( ¤t_abstract_data_type, &AbstractDataType::Sequence { .. } @@ -462,11 +485,8 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> { // so remove all previous fields that allows `Object` and update with current abstract_data_type. tera_fields.pop(); code_gen_context.with_abstract_data_type(AbstractDataType::Sequence { - element_type: current_abstract_data_type - .element_type() - .unwrap() - .to_string(), - sequence_type: sequence_type.to_string(), + element_type: current_abstract_data_type.element_type(), + sequence_type: sequence_type.to_owned(), }); } // In the case when a `type` constraint occurs before `element` constraint. The element type for the sequence @@ -486,10 +506,7 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> { // and `sequence_type` as per `type` constraint. AbstractDataType::Sequence { element_type, .. } if abstract_data_type != ¤t_abstract_data_type - && (current_abstract_data_type.element_type() - == Some(&"Object".to_string()) - || current_abstract_data_type.element_type() - == Some(&"T".to_string())) + && (current_abstract_data_type.element_type().is_none()) && matches!( ¤t_abstract_data_type, &AbstractDataType::Sequence { .. } @@ -501,11 +518,8 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> { // which needs to be updated within `abstract_data_type` as well. tera_fields.pop(); code_gen_context.with_abstract_data_type(AbstractDataType::Sequence { - element_type: element_type.to_string(), - sequence_type: current_abstract_data_type - .sequence_type() - .unwrap() - .to_string(), + element_type: element_type.to_owned(), + sequence_type: current_abstract_data_type.sequence_type(), }) } _ if abstract_data_type != ¤t_abstract_data_type => { diff --git a/src/bin/ion/commands/beta/generate/utils.rs b/src/bin/ion/commands/beta/generate/utils.rs index 67ab269..1328bd3 100644 --- a/src/bin/ion/commands/beta/generate/utils.rs +++ b/src/bin/ion/commands/beta/generate/utils.rs @@ -9,7 +9,23 @@ use std::fmt::{Display, Formatter}; #[derive(Serialize)] pub struct Field { pub(crate) name: String, - pub(crate) value_type: String, + // The value_type represents the AbstractDatType for given field. When given ISL has constraints, that lead to open ended types, + // this will be ste to None, Otherwise set to Some(ABSTRACT_DATA_TYPE_NAME). + // e.g For below ISL type: + // ``` + // type::{ + // name: list_type, + // type: list // since this doesn't have `element` constraint defined it will be set `value_type` to None + // } + // ``` + // Following will be the `Field` value for this ISL type: + // Field { + // name: value, + // value_type: None, + // isl_type_name: "list" + // } + // Code generation process results into an Error when `value_type` is set to `None` + pub(crate) value_type: Option, pub(crate) isl_type_name: String, } @@ -27,13 +43,13 @@ pub trait Language { fn file_name_for_type(name: &str) -> String; /// Maps the given ISL type to a target type name - fn target_type(ion_schema_type: &IonSchemaType) -> String; + fn target_type(ion_schema_type: &IonSchemaType) -> Option; /// Provides given target type as sequence /// e.g. /// target_type = "Foo" returns "ArrayList" /// target_type = "Foo" returns "Vec" - fn target_type_as_sequence(target_type: &str) -> String; + fn target_type_as_sequence(target_type: &Option) -> Option; /// Returns the [Case] based on programming languages /// e.g. @@ -66,29 +82,29 @@ impl Language for JavaLanguage { name.to_case(Case::UpperCamel) } - fn target_type(ion_schema_type: &IonSchemaType) -> String { + fn target_type(ion_schema_type: &IonSchemaType) -> Option { use IonSchemaType::*; - match ion_schema_type { - Int => "int", - String | Symbol => "String", - Float => "double", - Bool => "boolean", - Blob | Clob => "byte[]", - List | SExp => "Object", - SchemaDefined(name) => name, - } - .to_string() + Some( + match ion_schema_type { + Int => "int", + String | Symbol => "String", + Float => "double", + Bool => "boolean", + Blob | Clob => "byte[]", + List | SExp => return None, + SchemaDefined(name) => name, + } + .to_string(), + ) } - fn target_type_as_sequence(target_type: &str) -> String { - match JavaLanguage::wrapper_class(target_type) { - Some(wrapper_class_name) => { - format!("ArrayList<{}>", wrapper_class_name) - } - None => { - format!("ArrayList<{}>", target_type) + fn target_type_as_sequence(target_type: &Option) -> Option { + target_type.as_ref().map(|target_type_name| { + match JavaLanguage::wrapper_class(target_type_name) { + Some(wrapper_name) => format!("ArrayList<{}>", wrapper_name), + None => format!("ArrayList<{}>", target_type_name), } - } + }) } fn field_name_case() -> Case { @@ -107,12 +123,12 @@ impl Language for JavaLanguage { } impl JavaLanguage { - fn wrapper_class(primitive_data_type_name: &str) -> Option<&str> { - match primitive_data_type_name { - "int" => Some("Integer"), - "bool" => Some("Boolean"), - "double" => Some("Double"), - "long" => Some("Long"), + fn wrapper_class(primitive_data_type: &str) -> Option { + match primitive_data_type { + "int" => Some("Integer".to_string()), + "bool" => Some("Boolean".to_string()), + "double" => Some("Double".to_string()), + "long" => Some("Long".to_string()), _ => { // for any other non-primitive types return None None @@ -142,22 +158,26 @@ impl Language for RustLanguage { "ion_generated_code".to_string() } - fn target_type(ion_schema_type: &IonSchemaType) -> String { + fn target_type(ion_schema_type: &IonSchemaType) -> Option { use IonSchemaType::*; - match ion_schema_type { - Int => "i64", - String | Symbol => "String", - Float => "f64", - Bool => "bool", - Blob | Clob => "Vec", - List | SExp => "T", - SchemaDefined(name) => name, - } - .to_string() + Some( + match ion_schema_type { + Int => "i64", + String | Symbol => "String", + Float => "f64", + Bool => "bool", + Blob | Clob => "Vec", + List | SExp => return None, + SchemaDefined(name) => name, + } + .to_string(), + ) } - fn target_type_as_sequence(target_type: &str) -> String { - format!("Vec<{}>", target_type) + fn target_type_as_sequence(target_type: &Option) -> Option { + target_type + .as_ref() + .map(|target_type_name| format!("Vec<{}>", target_type_name)) } fn field_name_case() -> Case { diff --git a/tests/cli.rs b/tests/cli.rs index 3861b91..07dee21 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -246,7 +246,8 @@ fn test_write_all_values(#[case] number: i32, #[case] expected_output: &str) -> r#" type::{ name: sequence_struct, - element: string // this will be a sequence field in struct + element: string, // this will be a sequence field in struct + type: list } "#, &["value: Vec"], @@ -355,7 +356,8 @@ fn test_code_generation_in_rust( r#" type::{ name: sequence_struct, - element: string // this will be a sequence field in struct + element: string, // this will be a sequence field in struct + type: list } "#, &["private ArrayList value;"], diff --git a/tests/code-gen-tests.rs b/tests/code-gen-tests.rs index 3c79a22..3632d2e 100644 --- a/tests/code-gen-tests.rs +++ b/tests/code-gen-tests.rs @@ -1,5 +1,10 @@ use anyhow::Result; +use assert_cmd::Command; +use rstest::rstest; +use std::fs; +use std::fs::File; use std::io::Write; +use tempfile::TempDir; #[cfg(feature = "experimental-code-gen")] #[test] @@ -84,3 +89,49 @@ fn roundtrip_tests_for_generated_code_cargo() -> Result<()> { assert!(cargo_test_output.status.success()); Ok(()) } + +#[cfg(feature = "experimental-code-gen")] +#[rstest] +#[case::any_element_list( + r#" + type::{ + name: any_element_list, + type: list, // this doesn't specify the type for elements in the list with `element` constraint + } + "#, +)] +#[case::any_sequence_type( + r#" + type::{ + name: any_sequence_type, + element: int, // this doesn't specify the type of sequence with `type` constraint + } + "# +)] +/// Calls ion-cli beta generate with different unsupported schema types. Verify that `generate` subcommand returns an error for these schema types. +fn test_unsupported_schema_types_failures(#[case] test_schema: &str) -> Result<()> { + let mut cmd = Command::cargo_bin("ion")?; + let temp_dir = TempDir::new()?; + let input_schema_path = temp_dir.path().join("test_schema.isl"); + let mut input_schema_file = File::create(&input_schema_path)?; + input_schema_file.write(test_schema.as_bytes())?; + input_schema_file.flush()?; + cmd.args([ + "beta", + "generate", + "--schema", + "test_schema.isl", + "--output", + temp_dir.path().to_str().unwrap(), + "--language", + "java", + "--namespace", + "org.example", + "--directory", + temp_dir.path().to_str().unwrap(), + ]); + let command_assert = cmd.assert(); + // Code generation process should return an error for unsupported schema types + command_assert.failure(); + Ok(()) +} From f95ec6f2edee66c9577db5ddd75f366ae09e3f81 Mon Sep 17 00:00:00 2001 From: Khushboo Desai Date: Wed, 8 May 2024 14:11:21 -0700 Subject: [PATCH 7/8] Adds suggested changes * Minor nit changes for spacing and compact representation * Changes `SerdeError` enum to include `ValidationError` and removes unused `SerializationError` and `DeserializationError` --- .../ion/commands/beta/generate/generator.rs | 11 ++++------- .../beta/generate/templates/java/class.templ | 4 ++-- .../beta/generate/templates/rust/result.templ | 19 ++++++------------- .../beta/generate/templates/rust/struct.templ | 8 ++++---- 4 files changed, 16 insertions(+), 26 deletions(-) diff --git a/src/bin/ion/commands/beta/generate/generator.rs b/src/bin/ion/commands/beta/generate/generator.rs index 54a6996..e7b3895 100644 --- a/src/bin/ion/commands/beta/generate/generator.rs +++ b/src/bin/ion/commands/beta/generate/generator.rs @@ -233,13 +233,10 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> { // if any field in `tera_fields` contains a `None` `value_type` then it means there is a constraint that leads to open ended types. // Return error in such case. - if tera_fields.iter().any( - |Field { - name: _, - value_type, - isl_type_name: _, - }| value_type.is_none(), - ) { + if tera_fields + .iter() + .any(|Field { value_type, .. }| value_type.is_none()) + { return invalid_abstract_data_type_error("Currently code generation does not support open ended types. \ Error can be due to a missing `type` constraint or `element` constraint in the type definition."); } diff --git a/src/bin/ion/commands/beta/generate/templates/java/class.templ b/src/bin/ion/commands/beta/generate/templates/java/class.templ index 271b8b0..6d32768 100644 --- a/src/bin/ion/commands/beta/generate/templates/java/class.templ +++ b/src/bin/ion/commands/beta/generate/templates/java/class.templ @@ -86,11 +86,11 @@ public final class {{ target_kind_name }} { {# Reads `Sequence` class with a single field `value` that is an `ArrayList` #} {% if abstract_data_type["Sequence"].sequence_type == "list" %} if(reader.getType() != IonType.LIST) { - throw new IonException("Expected list, found" + reader.getType() + "while reading {{ target_kind_name }}."); + throw new IonException("Expected list, found " + reader.getType() + " while reading {{ target_kind_name }}."); } {% elif abstract_data_type["Sequence"].sequence_type == "sexp" %} if(reader.getType() != IonType.SEXP) { - throw new IonException("Expected sexpression, found" + reader.getType() + "while reading {{ target_kind_name }}."); + throw new IonException("Expected sexpression, found " + reader.getType() + " while reading {{ target_kind_name }}."); } {% endif %} reader.stepIn(); diff --git a/src/bin/ion/commands/beta/generate/templates/rust/result.templ b/src/bin/ion/commands/beta/generate/templates/rust/result.templ index 7ae37dc..9514c70 100644 --- a/src/bin/ion/commands/beta/generate/templates/rust/result.templ +++ b/src/bin/ion/commands/beta/generate/templates/rust/result.templ @@ -4,23 +4,16 @@ pub type SerdeResult = Result; /// Represents an error found during code generation #[derive(Debug)] pub enum SerdeError { + // Represents error found while reading or writing Ion data using Ion reader or writer. IonError { source: IonError }, - SerializationError { description: String }, - DeserializationError { description: String }, + // Represents error found while validating Ion data in `read_from` API for given data model. + ValidationError { description: String }, } -/// A convenience method for creating an SerdeError::SerializationError +/// A convenience method for creating an SerdeError::ValidationError /// with the provided description text. -pub fn serialization_error>(description: S) -> SerdeResult { - Err(SerdeError::SerializationError { - description: description.as_ref().to_string(), - }) -} - -/// A convenience method for creating an SerdeError::DeserializationError -/// with the provided description text. -pub fn deserialization_error>(description: S) -> SerdeResult { - Err(SerdeError::DeserializationError { +pub fn validation_error>(description: S) -> SerdeResult { + Err(SerdeError::ValidationError { description: description.as_ref().to_string(), }) } diff --git a/src/bin/ion/commands/beta/generate/templates/rust/struct.templ b/src/bin/ion/commands/beta/generate/templates/rust/struct.templ index caeefcd..2f279c3 100644 --- a/src/bin/ion/commands/beta/generate/templates/rust/struct.templ +++ b/src/bin/ion/commands/beta/generate/templates/rust/struct.templ @@ -43,7 +43,7 @@ impl {{ target_kind_name }} { {% endfor %} _ => { {% if abstract_data_type["Structure"] %} - return deserialization_error( + return validation_error( "Can not read field name:{{ field.name }} for {{ target_kind_name }} as it doesn't exist in the given schema type definition." ); {% endif %} @@ -55,13 +55,13 @@ impl {{ target_kind_name }} { {% elif abstract_data_type is object and abstract_data_type is containing("Sequence") %} {% if abstract_data_type["Sequence"].sequence_type == "list" %} if reader.ion_type() != Some(IonType::List) { - return deserialization_error(format!( + return validation_error(format!( "Expected list, found {} while reading {{ target_kind_name }}.", reader.ion_type().unwrap() )); } {% elif abstract_data_type["Sequence"].sequence_type == "sexp" %} if reader.ion_type() != Some(IonType::SExp) { - return deserialization_error(format!( + return validation_error(format!( "Expected sexpression, found {} while reading {{ target_kind_name }}.", reader.ion_type().unwrap() )); } @@ -82,7 +82,7 @@ impl {{ target_kind_name }} { }; reader.step_out()?; {% else %} - return deserialization_error("Can not resolve read API template for {{ target_kind_name }}"); + return validation_error("Can not resolve read API template for {{ target_kind_name }}"); {% endif %} Ok(abstract_data_type) } From 8491361dd20e656e221e03cb792bf49d326e1623 Mon Sep 17 00:00:00 2001 From: Khushboo Desai Date: Wed, 8 May 2024 14:29:22 -0700 Subject: [PATCH 8/8] Adds changes for using a SequenceType enum in AbstractDataType::Sequence --- src/bin/ion/commands/beta/generate/context.rs | 14 +++++++++++--- src/bin/ion/commands/beta/generate/generator.rs | 6 +++--- .../beta/generate/templates/java/class.templ | 6 +++--- .../beta/generate/templates/rust/struct.templ | 6 +++--- 4 files changed, 20 insertions(+), 12 deletions(-) diff --git a/src/bin/ion/commands/beta/generate/context.rs b/src/bin/ion/commands/beta/generate/context.rs index 2ce4420..2fd7bb1 100644 --- a/src/bin/ion/commands/beta/generate/context.rs +++ b/src/bin/ion/commands/beta/generate/context.rs @@ -39,7 +39,7 @@ pub enum AbstractDataType { // ``` Value, // A series of zero or more values whose type is described by the nested `element_type` - // and sequence type is described by nested `sequence_type` (e.g. a list). + // and sequence type is described by nested `sequence_type` (e.g. List or SExp). // If there is no `element` constraint present in schema type then `element_type` will be None. // If there is no `type` constraint present in schema type then `sequence_type` will be None. // e.g. Given below ISL, @@ -57,7 +57,7 @@ pub enum AbstractDataType { // ``` Sequence { element_type: Option, - sequence_type: Option, + sequence_type: Option, }, // A collection of field name/value pairs (e.g. a map) // the nested boolean represents whether the struct has closed fields or not @@ -89,7 +89,7 @@ impl AbstractDataType { } } - pub fn sequence_type(&self) -> Option { + pub fn sequence_type(&self) -> Option { match self { AbstractDataType::Sequence { sequence_type, .. } => sequence_type.to_owned(), _ => None, @@ -110,3 +110,11 @@ impl Display for AbstractDataType { ) } } + +/// Represents a sequenced type which could either be a list or s-expression. +/// This is used by `AbstractDataType` to represent sequence type for `Sequence` variant. +#[derive(Debug, Clone, PartialEq, Serialize)] +pub enum SequenceType { + List, + SExp, +} diff --git a/src/bin/ion/commands/beta/generate/generator.rs b/src/bin/ion/commands/beta/generate/generator.rs index e7b3895..bf8dc98 100644 --- a/src/bin/ion/commands/beta/generate/generator.rs +++ b/src/bin/ion/commands/beta/generate/generator.rs @@ -1,4 +1,4 @@ -use crate::commands::beta::generate::context::{AbstractDataType, CodeGenContext}; +use crate::commands::beta::generate::context::{AbstractDataType, CodeGenContext, SequenceType}; use crate::commands::beta::generate::result::{invalid_abstract_data_type_error, CodeGenResult}; use crate::commands::beta::generate::utils::{Field, JavaLanguage, Language, RustLanguage}; use crate::commands::beta::generate::utils::{IonSchemaType, Template}; @@ -377,12 +377,12 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> { if isl_type.name() == "list" { AbstractDataType::Sequence { element_type: type_name.clone(), - sequence_type: Some("list".to_string()), + sequence_type: Some(SequenceType::List), } } else if isl_type.name() == "sexp" { AbstractDataType::Sequence { element_type: type_name.clone(), - sequence_type: Some("sexp".to_string()), + sequence_type: Some(SequenceType::SExp), } } else { AbstractDataType::Value diff --git a/src/bin/ion/commands/beta/generate/templates/java/class.templ b/src/bin/ion/commands/beta/generate/templates/java/class.templ index 6d32768..8bc6d65 100644 --- a/src/bin/ion/commands/beta/generate/templates/java/class.templ +++ b/src/bin/ion/commands/beta/generate/templates/java/class.templ @@ -84,11 +84,11 @@ public final class {{ target_kind_name }} { reader.stepOut(); {% elif abstract_data_type is object and abstract_data_type is containing("Sequence") %} {# Reads `Sequence` class with a single field `value` that is an `ArrayList` #} - {% if abstract_data_type["Sequence"].sequence_type == "list" %} + {% if abstract_data_type["Sequence"].sequence_type == "List" %} if(reader.getType() != IonType.LIST) { throw new IonException("Expected list, found " + reader.getType() + " while reading {{ target_kind_name }}."); } - {% elif abstract_data_type["Sequence"].sequence_type == "sexp" %} + {% elif abstract_data_type["Sequence"].sequence_type == "SExp" %} if(reader.getType() != IonType.SEXP) { throw new IonException("Expected sexpression, found " + reader.getType() + " while reading {{ target_kind_name }}."); } @@ -144,7 +144,7 @@ public final class {{ target_kind_name }} { writer.stepOut(); {% elif abstract_data_type is object and abstract_data_type is containing("Sequence") %} {# Writes `Sequence` class with a single field `value` that is an `ArrayList` as an Ion sequence #} - {% if abstract_data_type["Sequence"].sequence_type == "list" %} + {% if abstract_data_type["Sequence"].sequence_type == "List" %} writer.stepIn(IonType.LIST); {% else %} writer.stepIn(IonType.SEXP); diff --git a/src/bin/ion/commands/beta/generate/templates/rust/struct.templ b/src/bin/ion/commands/beta/generate/templates/rust/struct.templ index 2f279c3..5ed8692 100644 --- a/src/bin/ion/commands/beta/generate/templates/rust/struct.templ +++ b/src/bin/ion/commands/beta/generate/templates/rust/struct.templ @@ -53,13 +53,13 @@ impl {{ target_kind_name }} { } reader.step_out()?; {% elif abstract_data_type is object and abstract_data_type is containing("Sequence") %} - {% if abstract_data_type["Sequence"].sequence_type == "list" %} + {% if abstract_data_type["Sequence"].sequence_type == "List" %} if reader.ion_type() != Some(IonType::List) { return validation_error(format!( "Expected list, found {} while reading {{ target_kind_name }}.", reader.ion_type().unwrap() )); } - {% elif abstract_data_type["Sequence"].sequence_type == "sexp" %} + {% elif abstract_data_type["Sequence"].sequence_type == "SExp" %} if reader.ion_type() != Some(IonType::SExp) { return validation_error(format!( "Expected sexpression, found {} while reading {{ target_kind_name }}.", reader.ion_type().unwrap() @@ -109,7 +109,7 @@ impl {{ target_kind_name }} { {% endfor %} writer.step_out()?; {% elif abstract_data_type is object and abstract_data_type is containing("Sequence") %} - {% if abstract_data_type["Sequence"].sequence_type == "list" %} + {% if abstract_data_type["Sequence"].sequence_type == "List" %} writer.step_in(IonType::List)?; {% else %} writer.step_in(IonType::SExp)?;