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 support for Java enum generation #158

Merged
merged 10 commits into from
Oct 24, 2024
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
FoobarBaz // expected FooBarBaz, found FoobarBaz
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
hello // expected (foo, bar, baz or FooBarBaz) found hello
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
"foo" // expected a symbol value foo for enum, found string "foo"
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// struct with mismatched sequence element
{
A: "hello",
B: 12,
C: (1 2 3), // expected sexpression of strings
D: 10e2
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// simple struct with type mismatched sequence type
{
A: "hello",
B: 12,
C: ["foo", "bar", "baz"], // expected sexp
D: 10e2
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// simple struct with type mismatched fields
{
A: "hello",
B: false, // expected field type: int
C: ("foo" "bar" "baz"),
D: 10e2
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// simple struct with all valid fields
{
A: "hello",
B: 12,
// C: ("foo" "bar" "baz"), // since `C` is a required field, this is an invalid struct
D: 10e2
}
1 change: 1 addition & 0 deletions code-gen-projects/input/good/enum_type/valid_value_1.ion
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
foo
1 change: 1 addition & 0 deletions code-gen-projects/input/good/enum_type/valid_value_2.ion
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
bar
1 change: 1 addition & 0 deletions code-gen-projects/input/good/enum_type/valid_value_3.ion
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
baz
1 change: 1 addition & 0 deletions code-gen-projects/input/good/enum_type/valid_value_4.ion
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
FooBarBaz
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// struct with empty list, empty string and zeros
{
C: (),
A: "",
B: 0,
D: 0e0,
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// simple struct with all valid fields
{
A: "hello",
B: 12,
C: ("foo" "bar" "baz"),
D: 10e2,
E: foo
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// simple struct with all valid fields
{
A: "hello",
B: 12,
C: ("foo" "bar" "baz"),
// D: 10e2, // since `D` is optional field, this is a valid struct
E: foo,
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// struct with unordered fields
{
C: ("foo" "bar" "baz"),
A: "hello",
B: 12,
E: foo,
D: 10e2,
}
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,16 @@ void roundtripBadTestForNestedStruct() throws IOException {
runRoundtripBadTest("/bad/nested_struct", NestedStruct::readFrom);
}

@Test
void roundtripBadTestForStructWithEnumFields() throws IOException {
runRoundtripBadTest("/bad/struct_with_enum_fields", StructWithEnumFields::readFrom);
}

@Test
void roundtripBadTestForEnumType() throws IOException {
runRoundtripBadTest("/bad/enum_type", EnumType::readFrom);
}

private <T> void runRoundtripBadTest(String path, ReaderFunction<T> readerFunction) throws IOException {
File dir = new File(System.getenv("ION_INPUT") + path);
String[] fileNames = dir.list();
Expand Down Expand Up @@ -161,6 +171,17 @@ void roundtripGoodTestForNestedStruct() throws IOException {
runRoundtripGoodTest("/good/nested_struct", NestedStruct::readFrom, (item, writer) -> item.writeTo(writer));
}

@Test
void roundtripGoodTestForStructWithEnumFields() throws IOException {
runRoundtripGoodTest("/good/struct_with_enum_fields", StructWithEnumFields::readFrom, (item, writer) -> item.writeTo(writer));
}


@Test
void roundtripGoodTestForEnumType() throws IOException {
runRoundtripGoodTest("/good/enum_type", EnumType::readFrom, (item, writer) -> item.writeTo(writer));
}

private <T> void runRoundtripGoodTest(String path, ReaderFunction<T> readerFunction, WriterFunction<T> writerFunction) throws IOException {
File dir = new File(System.getenv("ION_INPUT") + path);
String[] fileNames = dir.list();
Expand Down
4 changes: 4 additions & 0 deletions code-gen-projects/schema/enum_type.isl
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
type::{
name: enum_type,
valid_values: [foo, bar, baz, FooBarBaz]
}
12 changes: 12 additions & 0 deletions code-gen-projects/schema/struct_with_enum_fields.isl
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
type::{
name: struct_with_enum_fields,
type: struct,
fields: {
A: string,
B: int,
C: { element: string, type: sexp, occurs: required },
D: float,
E: { valid_values: [foo, bar, baz] }
}
}

101 changes: 97 additions & 4 deletions src/bin/ion/commands/generate/generator.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
use crate::commands::generate::context::{CodeGenContext, SequenceType};
use crate::commands::generate::model::{
AbstractDataType, DataModelNode, FieldPresence, FieldReference, FullyQualifiedTypeReference,
ScalarBuilder, SequenceBuilder, StructureBuilder, WrappedScalarBuilder, WrappedSequenceBuilder,
AbstractDataType, DataModelNode, EnumBuilder, FieldPresence, FieldReference,
FullyQualifiedTypeReference, ScalarBuilder, SequenceBuilder, StructureBuilder,
WrappedScalarBuilder, WrappedSequenceBuilder,
};
use crate::commands::generate::result::{
invalid_abstract_data_type_error, invalid_abstract_data_type_raw_error, CodeGenResult,
Expand All @@ -10,12 +11,14 @@ use crate::commands::generate::templates;
use crate::commands::generate::utils::{IonSchemaType, Template};
use crate::commands::generate::utils::{JavaLanguage, Language, RustLanguage};
use convert_case::{Case, Casing};
use ion_schema::external::ion_rs::element::Value;
use ion_schema::isl::isl_constraint::{IslConstraint, IslConstraintValue};
use ion_schema::isl::isl_type::IslType;
use ion_schema::isl::isl_type_reference::IslTypeRef;
use ion_schema::isl::util::ValidValue;
use ion_schema::isl::IslSchema;
use ion_schema::system::SchemaSystem;
use std::collections::HashMap;
use std::collections::{BTreeSet, HashMap};
use std::fs;
use std::fs::OpenOptions;
use std::io::Write;
Expand Down Expand Up @@ -46,6 +49,7 @@ impl<'a> CodeGenerator<'a, RustLanguage> {
("struct.templ", templates::rust::STRUCT),
("scalar.templ", templates::rust::SCALAR),
("sequence.templ", templates::rust::SEQUENCE),
("enum.templ", templates::rust::ENUM),
("util_macros.templ", templates::rust::UTIL_MACROS),
("import.templ", templates::rust::IMPORT),
("nested_type.templ", templates::rust::NESTED_TYPE),
Expand Down Expand Up @@ -89,6 +93,7 @@ impl<'a> CodeGenerator<'a, JavaLanguage> {
("class.templ", templates::java::CLASS),
("scalar.templ", templates::java::SCALAR),
("sequence.templ", templates::java::SEQUENCE),
("enum.templ", templates::java::ENUM),
("util_macros.templ", templates::java::UTIL_MACROS),
("nested_type.templ", templates::java::NESTED_TYPE),
])
Expand Down Expand Up @@ -328,7 +333,7 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> {
self.generate_abstract_data_type(&isl_type_name, isl_type)?;
// Since the fully qualified name of this generator represents the current fully qualified name,
// remove it before generating code for the next ISL type.
self.current_type_fully_qualified_name.pop();
L::reset_namespace(&mut self.current_type_fully_qualified_name);
}

Ok(())
Expand Down Expand Up @@ -455,6 +460,11 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> {
isl_type,
)?
}
} else if constraints
.iter()
.any(|it| matches!(it.constraint(), IslConstraintValue::ValidValues(_)))
Copy link
Contributor

Choose a reason for hiding this comment

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

The way this is implemented, if I defined something like this, it would fail because it's not a supported way of defining an enum, even though a user would expect this to be a perfectly valid scalar.

type::{
  name: uint,
  type: int,
  valid_values: range::[0, max],
}

In order to classify something as an enum, it must have a valid_values constraint, and all of its valid values should be symbols so that we don't incorrectly classify something such as this example. (I would be inclined to limit it specifically to symbols unless we specifically get a request to allow other things.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made changes to allow only symbol with valid_values for enum generation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I explained myself well enough. The way you have implemented this, the categorization (i.e. this if condition) is overly broad. That means it could catch other type definitions that are not intended to be enums in the first place.

In order to even be categorized as an enum, it should have a valid_values constraint with only symbols. Checking for symbols in the valid_values constraint needs to happen here, or else the enum code will reject things like

type::{
  name: uint,
  type: int,
  valid_values: range::[0, max],
}

I believe it should be something like this (although this is quite long for an if condition, and should probably be extracted to a helper function).

Suggested change
} else if constraints
.iter()
.any(|it| matches!(it.constraint(), IslConstraintValue::ValidValues(_)))
} else if constraints
.iter()
.any(|it| {
if let IslConstraintValue::ValidValues(valid_values) = it.constraint() {
valid_values.values().iter().all(|val| {
matches!(val, ValidValue::Element(Value::Symbol(_)))
})
} else {
false
}
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did have this check inside the build_enum_from_constraints. But I understand that adding it here keeps the generation separate from these checks. I will change this.

{
self.build_enum_from_constraints(constraints, code_gen_context, isl_type)?
} else if Self::contains_scalar_constraints(constraints) {
if is_nested_type {
self.build_scalar_from_constraints(constraints, code_gen_context, isl_type)?
Expand Down Expand Up @@ -697,6 +707,89 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> {
Ok(AbstractDataType::Structure(structure_builder.build()?))
}

/// Builds `AbstractDataType::Enum` from the given constraints.
/// e.g. for a given type definition as below:
/// ```
/// type::{
/// name: Foo,
/// type: symbol,
/// valid_values: [foo, bar, baz]
/// }
/// ```
/// This method builds `AbstractDataType`as following:
/// ```
/// AbstractDataType::Enum(
/// Enum {
/// name: vec!["org", "example", "Foo"], // assuming the namespace is `org.example`
/// variants: HashSet::from_iter(
/// vec![
/// "foo",
/// "bar",
/// "baz"
/// ].iter()) // Represents enum variants
/// doc_comment: None // There is no doc comment defined in above ISL type def
/// source: IslType {name: "foo", .. } // Represents the `IslType` that is getting converted to `AbstractDataType`
/// }
/// )
/// ```
fn build_enum_from_constraints(
&mut self,
constraints: &[IslConstraint],
code_gen_context: &mut CodeGenContext,
parent_isl_type: &IslType,
) -> CodeGenResult<AbstractDataType> {
let mut enum_builder = EnumBuilder::default();
enum_builder
.name(self.current_type_fully_qualified_name.to_owned())
.source(parent_isl_type.to_owned());
let mut found_base_type = false;

for constraint in constraints {
match constraint.constraint() {
IslConstraintValue::ValidValues(valid_values_constraint) => {
let valid_values = valid_values_constraint
.values()
.iter()
.map(|v| match v {
ValidValue::Element(Value::Symbol(symbol_val) ) => {
symbol_val.text().map(|s| s.to_string()).ok_or(invalid_abstract_data_type_raw_error(
"Could not determine enum variant name",
))
}
_ => invalid_abstract_data_type_error(
"Only `valid_values` constraint with values of type `symbol` are supported yet!"
),
})
.collect::<CodeGenResult<Vec<String>>>()?;
enum_builder.variants(BTreeSet::from_iter(valid_values));
}
IslConstraintValue::Type(isl_type_ref) => {
if isl_type_ref.name() != "symbol" {
return invalid_abstract_data_type_error(
"Only `valid_values` constraint with values of type `symbol` are supported yet!"
);
}

let _type_name = self.handle_duplicate_constraint(
found_base_type,
"type",
isl_type_ref,
FieldPresence::Required,
code_gen_context,
)?;
found_base_type = true;
}
_ => {
return invalid_abstract_data_type_error(
"Could not determine the abstract data type due to conflicting constraints",
)
}
}
}

Ok(AbstractDataType::Enum(enum_builder.build()?))
}

/// Builds `AbstractDataType::WrappedScalar` from the given constraints.
/// ```
/// type::{
Expand Down
43 changes: 42 additions & 1 deletion src/bin/ion/commands/generate/model.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use derive_builder::Builder;
use ion_schema::isl::isl_type::IslType;
use std::collections::HashMap;
use std::collections::{BTreeSet, HashMap};
use std::fmt::Debug;
// This module contains a data model that the code generator can use to render a template based on the type of the model.
// Currently, this same data model is represented by `AbstractDataType` but it doesn't hold all the information for the template.
Expand Down Expand Up @@ -182,6 +182,8 @@ pub enum AbstractDataType {
WrappedSequence(WrappedSequence),
// A collection of field name/value pairs (e.g. a map)
Structure(Structure),
// Represents an enum type
Enum(Enum),
}

impl AbstractDataType {
Expand All @@ -203,6 +205,9 @@ impl AbstractDataType {
AbstractDataType::Structure(Structure { doc_comment, .. }) => {
doc_comment.as_ref().map(|s| s.as_str())
}
AbstractDataType::Enum(Enum { doc_comment, .. }) => {
doc_comment.as_ref().map(|s| s.as_str())
}
}
}

Expand All @@ -219,6 +224,7 @@ impl AbstractDataType {
Some(L::target_type_as_sequence(seq.element_type.to_owned()))
}
AbstractDataType::Structure(structure) => Some(structure.name.to_owned().into()),
AbstractDataType::Enum(enum_type) => Some(enum_type.name.to_owned().into()),
}
}
}
Expand Down Expand Up @@ -448,6 +454,41 @@ pub struct FieldReference(
pub(crate) FieldPresence,
);

/// Represents an enum type
/// e.g. Given below ISL,
/// ```
/// type::{
/// name: enum_type,
/// valid_values: [foo, bar, baz]
/// }
/// ```
/// Corresponding generated code in Rust would look like following:
/// ```
/// enum EnumType {
/// Foo,
/// Bar,
/// Baz
/// }
/// ```
#[allow(dead_code)]
#[derive(Debug, Clone, Builder, PartialEq, Serialize)]
#[builder(setter(into))]
pub struct Enum {
// Represents the fully qualified name for this data model
pub(crate) name: FullyQualifiedTypeName,
// The variants of this enum
variants: BTreeSet<String>,
// Represents doc comment for the generated code
#[builder(default)]
doc_comment: Option<String>,
// Represents the source ISL type which can be used to get other constraints useful for this type.
// For example, getting the length of this sequence from `container_length` constraint or getting a `regex` value for string type.
// This will also be useful for `text` type to verify if this is a `string` or `symbol`.
#[serde(skip_serializing_if = "is_anonymous")]
#[serde(serialize_with = "serialize_type_name")]
source: IslType,
}

#[cfg(test)]
mod model_tests {
use super::*;
Expand Down
12 changes: 10 additions & 2 deletions src/bin/ion/commands/generate/result.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::commands::generate::model::{
ScalarBuilderError, SequenceBuilderError, StructureBuilderError, WrappedScalarBuilderError,
WrappedSequenceBuilderError,
EnumBuilderError, ScalarBuilderError, SequenceBuilderError, StructureBuilderError,
WrappedScalarBuilderError, WrappedSequenceBuilderError,
};
use ion_schema::result::IonSchemaError;
use thiserror::Error;
Expand Down Expand Up @@ -87,3 +87,11 @@ impl From<StructureBuilderError> for CodeGenError {
}
}
}

impl From<EnumBuilderError> for CodeGenError {
fn from(value: EnumBuilderError) -> Self {
CodeGenError::DataModelBuilderError {
description: value.to_string(),
}
}
}
Loading
Loading