Adds read-write API generation support for Java code generation #100

desaikd · 2024-04-18T19:26:20Z

Issue #87:

Description of changes:

This PR adds read-write API generation support for Java.

Example generated APIs

This example uses schema struct_with_fields from code-gen-projects/schema/.
Note: The generated code has been formatted here and actual code might have extra lines and tabs.

public static StructWithFields readFrom(IonReader reader) {
        AnonymousType2 c = null;
        int b = 0;
        String a = null;
        double d = 0;
        
        reader.stepIn();
        
        while (reader.hasNext()) {
            reader.next();
            String fieldName = reader.getFieldName();
            switch (fieldName) {
                case "C":
                    c = AnonymousType2.readFrom(reader);
                    break;
                case "B":
                    b = reader.intValue();
                    break;
                case "A":
                    a = reader.stringValue();
                    break;
                case "D":
                    d = reader.doubleValue();
                    break;
                default:
                    throw new IonException("Can not read field name:" + fieldName +
                            " for StructWithFields as it doesn't exist in the given schema type definition.");
            }
        }
        reader.stepOut();
        return new StructWithFields(a, b, c, d);
    }
    
    public void writeTo(IonWriter writer) throws IOException {
        writer.stepIn(IonType.STRUCT);

        writer.setFieldName("C");
        this.c.writeTo(writer);

        writer.setFieldName("B");
        writer.writeInt(this.b);

        writer.setFieldName("A");
        writer.writeString(this.a);

        writer.setFieldName("D");
        writer.writeFloat(this.d);

        writer.stepOut();
    }

Complete generated code can be found here.

List of changes:

Modifies class template for read API generation
Modifies current is_built_in_type filter for JavaLanguage to
change float to double and added Ion float field to verify
Adds tests for generated read API in CodeGenTest.java
Adds write API generation support and adds roundtrip tests in CodeGenTest.java

Test

Roundtrip tests are added to test generated read-write API.
The test consists of 3 steps:

Read an Ion data using IonReader into data model with readFrom API
Write the data model using IonWriter as Ion with writeTo API
Using IonLoader check equivalence of initially provided Ion data and the Ion data written by writeTo API.

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

* Adds import statement for IonReader and IonException * Adds read API for `AbstractDataType::Value`, `AbstractDataType::Sequence` and `AbstractDataType::Structure`

* Adds new tests for read APi generation * Modifies current `is_built_in_type` filter for `JavaLanguage` to change float to double * Modifies test input and schemas to use Ion float values

* Modifies class template toa dd writer API support * Adds roundtrip tests in CodeGenTest.java

desaikd · 2024-04-18T19:27:34Z

code-gen-projects/java/code-gen-demo/src/test/java/org/example/CodeGenTest.java

+         assertEquals(false, n.getC().getD(), "n.getC().getD() should return `false`");
+    }
+
+    @Test void roundtripTestForStructWithFields() throws IOException {


(🗺️ PR tour) Added roundtrip tests for generated read-write APIs

desaikd · 2024-04-18T19:28:38Z

src/bin/ion/commands/beta/generate/templates/java/class.templ

@@ -16,4 +21,110 @@ public final class {{ target_kind_name }} {
        return this.{{ field.name | camel }};
    }
    {% endfor %}
+
+    public static {{ target_kind_name }} readFrom(IonReader reader) {


(🗺️ PR tour) Added readFrom API in the class template for reading an Ion value into the given data model.

desaikd · 2024-04-18T19:29:08Z

src/bin/ion/commands/beta/generate/templates/java/class.templ

+        return new {{ target_kind_name }}({% for field in fields | sort(attribute="name") -%}{{ field.name | camel }}{% if not loop.last %},{% endif %}{% endfor %});
+    }
+
+    public void writeTo(IonWriter writer) throws IOException {


(🗺️ PR tour) Added writeTo API in the class template for writing the given data model as Ion.

desaikd · 2024-04-18T19:34:29Z

src/bin/ion/commands/beta/generate/utils.rs

@@ -88,7 +88,7 @@ impl Language for JavaLanguage {
    }

    fn is_built_in_type(name: &str) -> bool {
-        matches!(name, "int" | "String" | "boolean" | "byte[]" | "float")
+        matches!(name, "int" | "String" | "boolean" | "byte[]" | "double")


(🗺️ PR tour) ion-java uses doubleValue API for IonReader, so changing this similar to ion-java.

desaikd · 2024-04-18T19:35:17Z

code-gen-projects/input/struct_with_fields.ion

@@ -2,4 +2,5 @@
 A: "hello",
 B: 12,
 C: ["foo", "bar", "baz"],
+ D: 10e2


(🗺️ PR tour) Added this field to test for double value in Java.

code-gen-projects/schema/struct_with_fields.isl

popematt

This is good progress!

It would be nice if you could drop the generated code into a GitHub Gist or something so that a reviewer can also look at the generated code without having to check out and build the PR locally.

The read and write code is exposing some things that are not very idiomatic Java. They don't need to be addressed in this PR, but I think they are things that must be addressed before saying that this is production-ready.

The "new type" pattern—while this is idiomatic in Rust, it is potentially expensive and awkward to use in Java. I think that anonymous, parameterized-list types should not be wrapped in a new Java type.
We need to have a builder for classes that are based on Ion structs.
I think we should try to define anonymous types as nested static classes inside their respective parent type. E.g. using the schema given here, that would mean that AnonymousType2 would actually be StructWithFields.AnonymousType2.

src/bin/ion/commands/beta/generate/templates/java/class.templ

popematt · 2024-04-18T20:36:06Z

src/bin/ion/commands/beta/generate/templates/java/class.templ

+            }
+            reader.stepOut();
+        {% endif %}
+        return new {{ target_kind_name }}({% for field in fields | sort(attribute="name") -%}{{ field.name | camel }}{% if not loop.last %},{% endif %}{% endfor %});


We really need to have a builder class so that we can avoid things like sorting the field names and having to check to see if we're on the last loop iteration to see if we need a comma.

code-gen-projects/schema/struct_with_fields.isl

src/bin/ion/commands/beta/generate/templates/java/class.templ

popematt · 2024-04-18T20:47:24Z

src/bin/ion/commands/beta/generate/templates/java/class.templ

+            {% if field.value == "boolean" %}
+                false


If you use Boolean instead of boolean, then this could be initialized to null. Similarly for int/Integer and double/Double.

However, if it's not an optional field, then we're better off using the primitive types to avoid the overhead of the boxed types.

I am going to keep this as boolean/int/double for now. I will change it in separate PR to consider nulls.

Reference: #101

desaikd · 2024-04-18T22:26:37Z

It would be nice if you could drop the generated code into a GitHub Gist or something so that a reviewer can also look at the generated code without having to check out and build the PR locally.

I added the generated code in the PR description for one of the schema files. But I will add Gists with generated code from now to make this easier.

The read and write code is exposing some things that are not very idiomatic Java. They don't need to be addressed in this PR, but I think they are things that must be addressed before saying that this is production-ready.

The "new type" pattern—while this is idiomatic in Rust, it is potentially expensive and awkward to use in Java. I think that anonymous, parameterized-list types should not be wrapped in a new Java type.

What did you mean by new type pattern. Did you mean just adding anonymous types into the same class as the one that uses it?

We need to have a builder for classes that are based on Ion structs.

Yes, builder needs to be added for sure. I have already created an issue for it and will work on it as the next PR.
Reference: #97

I think we should try to define anonymous types as nested static classes inside their respective parent type. E.g. using the schema given here, that would mean that AnonymousType2 would actually be StructWithFields.AnonymousType2.

Yes, I will change that.

* Modified indentation for template files * Adds doc comment * Minor typo fixes

popematt · 2024-04-18T23:06:22Z

I added the generated code in the PR description for one of the schema files.

I didn't even notice that... it's my fault for jumping straight into the code, I guess.

popematt · 2024-04-18T23:11:10Z

What did you mean by new type pattern.

https://doc.rust-lang.org/rust-by-example/generics/new_types.html

The Anonymous2 type is basically just a "new type" that wraps List<String>.

desaikd · 2024-04-19T00:10:33Z

What did you mean by new type pattern.

https://doc.rust-lang.org/rust-by-example/generics/new_types.html

The Anonymous2 type is basically just a "new type" that wraps List<String>.

As per offline discussion, this will be added with a separate PR.

nirosys

Awesome :) lgtm, as long as popematt's concerns are met.

* Changes Rust and Java demo project tests to run throughbad/invalid input Ion files and assert Exception is thrown * Changes Rust and java dmeo projects tests to run through each good/valid input Ion files and assert equivelnce for roundtrip test * Adds valid and invalid inpu Ion files for tests

code-gen-projects/input/good/nested_struct/valid_unordered_fields.ion

desaikd added 3 commits April 11, 2024 17:27

Adds class template changes for read API generation

4d37a96

* Adds import statement for IonReader and IonException * Adds read API for `AbstractDataType::Value`, `AbstractDataType::Sequence` and `AbstractDataType::Structure`

Adds tests for read API generation

595096b

* Adds new tests for read APi generation * Modifies current `is_built_in_type` filter for `JavaLanguage` to change float to double * Modifies test input and schemas to use Ion float values

Adds write API generation support

f335349

* Modifies class template toa dd writer API support * Adds roundtrip tests in CodeGenTest.java

desaikd commented Apr 18, 2024

View reviewed changes

desaikd requested a review from nirosys April 18, 2024 19:36

popematt reviewed Apr 18, 2024

View reviewed changes

Adds suggested changes

9f4171b

* Modified indentation for template files * Adds doc comment * Minor typo fixes

Add setter support for Java classes

475bebc

nirosys approved these changes Apr 19, 2024

View reviewed changes

desaikd added 2 commits April 23, 2024 14:46

Renamed value property of field to value_type

98db615

popematt reviewed Apr 25, 2024

View reviewed changes

code-gen-projects/input/good/nested_struct/valid_unordered_fields.ion Show resolved Hide resolved

Modifies unordered field test case

ba4e440

desaikd merged commit 515bd90 into amazon-ion:master Apr 25, 2024
4 checks passed

desaikd mentioned this pull request Apr 25, 2024

Add support for generation of class in Java #87

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds read-write API generation support for Java code generation #100

Adds read-write API generation support for Java code generation #100

desaikd commented Apr 18, 2024 •

edited

Loading

desaikd Apr 18, 2024

desaikd Apr 18, 2024

desaikd Apr 18, 2024

desaikd Apr 18, 2024

desaikd Apr 18, 2024

popematt left a comment

popematt Apr 18, 2024

popematt Apr 18, 2024

desaikd Apr 18, 2024

desaikd Apr 18, 2024

desaikd commented Apr 18, 2024

popematt commented Apr 18, 2024

popematt commented Apr 18, 2024

desaikd commented Apr 19, 2024

nirosys left a comment

Adds read-write API generation support for Java code generation #100

Adds read-write API generation support for Java code generation #100

Conversation

desaikd commented Apr 18, 2024 • edited Loading

Issue #87:

Description of changes:

Example generated APIs

List of changes:

Test

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

popematt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

desaikd commented Apr 18, 2024

popematt commented Apr 18, 2024

popematt commented Apr 18, 2024

desaikd commented Apr 19, 2024

nirosys left a comment

Choose a reason for hiding this comment

desaikd commented Apr 18, 2024 •

edited

Loading