Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adds inital support for code generation #61

Merged
merged 4 commits into from
Dec 15, 2023

Conversation

desaikd
Copy link
Contributor

@desaikd desaikd commented Nov 10, 2023

Issue #66

Description of changes:

This PR adds an initial support generate subcommand in ion-cli.generate subcommand intends to provide a code generation CLI tool that can generate code from Ion schema.

Example:

In this example, it shows how the generated Rust code will look like for the given schema. It generates the data model along with read and write APIs to read from Ion data into the data model and write as Ion data from the data model.

type:: {
  name: Address,
  fields: {
      street: string,
      city: string,
      country: string,
      postal_code: int
  }
}
#[derive(Debug, Clone, Default)]
pub struct Address {
    street: String,
    city: String,
    country: String,
    postal_code: i64,

}

impl Address {
    pub fn new(street: String, city: String, country: String, postal_code: i64) -> Self {
        Self {
            street,
            city,
            country,
            postal_code,

        }
    }

    pub fn street(&self) -> &String {
        &self.street
    }
    pub fn city(&self) -> &String {
        &self.city
    }
    pub fn country(&self) -> &String {
        &self.country
    }
    pub fn postal_code(&self) -> &i64 {
        &self.postal_code
    }

    pub fn read_from(reader: &mut Reader) -> IonResult<Self> {
        reader.next()?;
        reader.step_in()?;
        let mut data_model = Address::default();
        while reader.next()? != StreamItem::Nothing {
            if let Some(field_name) = reader.field_name()?.text() {
                match field_name {
                    "street" => { data_model.street = reader.read_string()?; }
                    "city" => { data_model.city = reader.read_string()?; }
                    "country" => { data_model.country = reader.read_string()?; }
                    "postal_code" => { data_model.postal_code = reader.read_i64()?; }
                    _ => {}
                }
            }
        }
        reader.step_out()?;

        Ok(data_model)
    }

    pub fn write_to<W: IonWriter>(&self, writer: &mut W) -> IonResult<()> {
        writer.step_in(IonType::Struct)?;
        writer.set_field_name("street");
        writer.write_string(self.street)?;
        writer.set_field_name("city");
        writer.write_string(self.city)?;
        writer.set_field_name("country");
        writer.write_string(self.country)?;
        writer.set_field_name("postal_code");
        writer.write_i64(self.postal_code)?;
        writer.step_out()?;

        Ok(())
    }
}

List of changes:

  • Creates a feature for all the beat subcommands (beta-subcommands).
  • Updates ion-schema-rust dependency to v0.10.0.
  • Creates a new generate subcommand
  • Creates class and struct templates for code generation inside templates directrory
  • Adds various generate_*() that generate code based on programming language(More details will be added in PR tour comments 🗺️ )
  • Adds util structures for code generation
    • Adds Field struct which will be used to fill in fields in struct/class for defined templates
    • Adds Language which represents which programming language is selected for code generation.
    • Adds CodeGenContext which will be used as a context for code generation related properties.
    • Adds Import which will be used to fill import details in struct/class for defined templates.

What is supported with this PR?:

  • Rust
    • Supports generating struct data models
    • Supports generating read and write APIs for struct data models
    • Supports built-in types in ISL (Does not support nulls)
    • Currently only supports element, fields and type constraints in ISL
  • Java
    • Supports generating struct data models
    • Supports built-in types in ISL (Does not support nulls)
    • Currently only supports element, fields and type constraints in ISL

Next steps:

  • Add support for more constraints in ISL (valid_values, one_of etc.)
  • Add support for generic container types (list, sexp, struct with any type in ISL)
  • Add support for imports in ISL
  • Support for null values

Tests:

Adds unit tests for rust and java code generation. Unit tests are done using string values to see if the generated code contains the expected string value.


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

use std::path::Path;
use tera::Context;

pub struct GenerateCommand;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ Adds generate subcommand with options to provide schema file and language of choice for code generation.

Cargo.toml Outdated

[dev-dependencies]
rstest = "~0.17.0"
assert_cmd = "~1.0.5"
tempfile = "~3.5.0"

[features]
deafault = []
beta-subcommands = []
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ Adds a beta-subcommands feature for all beta subcommands (#63)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should just call this experimental to align with ion-rust's flags?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you planning to add individual features for individual commands that are part of the beta-subcommands or experimental feature?


impl GenerateCommand {
/// Maps the given type name to a native type based on programming language
pub fn map_to_base_type(name: &str, language: &Language) -> String {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ Defines mapping from ISL built-in types to native data type for programming language of choice.

}

/// Generates data model based on given ISL type definition
fn generate_data_model(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ Generates data model based on given ISL constraints. If the constraints match to different data models then returns an error.
element -> SequenceStruct(creates a struct with single field that stores the sequence as a Vec)
fields -> Struct (creates a struct with given field name and value/data type pairs)
type - >UnitStruct (creates a struct with single field that has the data type provided with type constraint)

modules.push(code_gen_context.language.file_name(&data_model_name));

// Render or generate file for the template with the given context
let rendered = code_gen_context
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ Creates files for the generated code using template for the data model.

}

/// Verify that the current data model is same as previously determined data model
fn verify_data_model_consistency(
Copy link
Contributor Author

@desaikd desaikd Nov 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ Verifies consistency between determined data model for given constraints. (e.g. if first constraint is type: string and then we get a constraint with fields: { ... }, so this will determine different data models and hence will return an error)

@@ -0,0 +1,19 @@
{% if import %}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ Template for java class this doesn't include read-write APIs. Only includes getters for the fields.

@@ -0,0 +1,3 @@
{% for module in modules -%}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ Template for Rust modules that declares all the modules generated based on given type definitions.This also includes inline type definitions(anonymous type definition) that doesn't have a name.(e.g. address: { type: string }) Naming for these types is based on a counter starting with AnonymousType1. I have selected this name to imply that these are type definitions that weren't named in schema file, but I am open to other suggestions for naming these types.
Some alternatives,

  • Name these types as InlinedType*.
  • Make a code generation constraint as open content in schema that allows naming these types (e.g. address: { $codegen_type_name: address_string, type: string } )

}

/// Represents a context that will be used for code generation
pub struct CodeGenContext {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ All the necessary context for code generation is stored here. This will be used by the generate_*() methods in generate subcommand. It includes a data_model property that points to the data model determined based on constraints and is used to get appropriate template for the ISL type definition.


/// Represents a data model type that can be used to determine which templates can be used for code generation.
#[derive(Debug, Clone, PartialEq, Serialize)]
pub enum DataModel {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ Currently only includes data models related to element, fields and type constraints. More data models will be added with other constraints support like enum.

* generator - includes all the functions required to generate code
* context - Stores context structures required for code generation
* result - Stores CodeGenResult and CodeGenError
* utils - all the necessary utilities for code generation
@@ -13,8 +13,8 @@ impl IonCliCommand for LoadCommand {
}

fn about(&self) -> &'static str {
r#"Loads an Ion Schema file indicated by a user-provided schema ID and outputs a result message.\
Shows an error message if any invalid schema syntax was found during the load process."#
r"Loads an Ion Schema file indicated by a user-provided schema ID and outputs a result message.\
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ This is related to a clippy suggestion

@@ -1,6 +1,7 @@
use crate::commands::IonCliCommand;
use anyhow::{Context, Result};
use clap::{Arg, ArgAction, ArgMatches, Command};
use ion_rs::element::writer::TextKind;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ Following changes for schema subcommand are unrelated to code generation. These changes are required due to v0.10.0 of ion-schema which includes ISL model changes necessary for code generation.

@desaikd desaikd marked this pull request as ready for review November 14, 2023 20:12
Copy link
Contributor

@zslayton zslayton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a partial review.

@@ -18,15 +18,24 @@ colored = "2.0.0"
ion-rs = "0.18.1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably bump ion-rs to the latest while we're here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still pending. I will modify this in a separate PR.

Cargo.toml Show resolved Hide resolved
Cargo.toml Outdated

[dev-dependencies]
rstest = "~0.17.0"
assert_cmd = "~1.0.5"
tempfile = "~3.5.0"

[features]
deafault = []
beta-subcommands = []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should just call this experimental to align with ion-rust's flags?

Cargo.toml Outdated Show resolved Hide resolved
src/bin/ion/commands/beta/generate/context.rs Show resolved Hide resolved
src/bin/ion/commands/beta/generate/generator.rs Outdated Show resolved Hide resolved
src/bin/ion/commands/beta/generate/generator.rs Outdated Show resolved Hide resolved
src/bin/ion/commands/beta/generate/generator.rs Outdated Show resolved Hide resolved
src/bin/ion/commands/beta/generate/generator.rs Outdated Show resolved Hide resolved
src/bin/ion/commands/beta/generate/generator.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@zslayton zslayton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review 2 of N.

src/bin/ion/commands/beta/generate/generator.rs Outdated Show resolved Hide resolved
src/bin/ion/commands/beta/generate/generator.rs Outdated Show resolved Hide resolved
src/bin/ion/commands/beta/generate/generator.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@zslayton zslayton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review 3/N

src/bin/ion/commands/beta/generate/mod.rs Outdated Show resolved Hide resolved
src/bin/ion/commands/beta/generate/mod.rs Outdated Show resolved Hide resolved
src/bin/ion/commands/beta/generate/mod.rs Outdated Show resolved Hide resolved
src/bin/ion/commands/beta/generate/generator.rs Outdated Show resolved Hide resolved
src/bin/ion/commands/beta/generate/generator.rs Outdated Show resolved Hide resolved
Comment on lines +172 to +173
/// Maps the given constraint value to a data model
fn map_constraint_to_data_model(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of this function describes a step it performs in the process of generating code for the constraint, but I think the meat of the function is the code generation itself.

src/bin/ion/commands/beta/generate/generator.rs Outdated Show resolved Hide resolved
src/bin/ion/commands/beta/generate/context.rs Show resolved Hide resolved
* adds more doc comment changes
* moves properties like annoymous type counter and tera to generator
from context
* context only stores data model property and is locally created for
each ISL type code generation
* adds Template struct which stores information about which templates
are supported and has a method to navogate to a particular template file
for code generation
Cargo.toml Outdated Show resolved Hide resolved
src/bin/ion/commands/beta/generate/mod.rs Outdated Show resolved Hide resolved
/// Maps the given type name to a native type based on programming language
pub fn map_to_base_type(name: &str, language: &Language) -> String {
match (name, language) {
("int", Language::Java) => "int".to_string(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a long or BigDecimal?

src/bin/ion/commands/beta/generate/utils.rs Show resolved Hide resolved
src/bin/ion/commands/beta/generate/utils.rs Outdated Show resolved Hide resolved
src/bin/ion/commands/beta/generate/generator.rs Outdated Show resolved Hide resolved
* Doc comment changes
* rename from `name` to `target_kind_name` for template variable
* Update Result to use `IonSchemaError` instead of `IonError`
* Add newline at the end of each template
* Other variable renaming changes
Copy link
Contributor

@zslayton zslayton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like all of my comments have been addressed or TODOed in issues. 👍

Once @popematt gets another chance to go through it I think we're set.

Copy link
Contributor

@popematt popematt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's one thing that needs to be addressed, but since this is still an unstable experimental feature, it doesn't necessarily need to be fixed right now.

use Language::*;
match (self, language) {
(Int, Rust) => "i64",
(Int, Java) => "int",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be at least a long, and probably even a BigDecimal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#78

@desaikd desaikd merged commit 8138f11 into amazon-ion:master Dec 15, 2023
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants