amazon-ion / ion-cli

Apache License 2.0
31 stars 15 forks source link

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

Closed desaikd closed 4 months ago

desaikd commented 4 months ago

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:

Test

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

  1. Read an Ion data using IonReader into data model with readFrom API
  2. Write the data model using IonWriter as Ion with writeTo API
  3. 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.

desaikd commented 4 months ago

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.

  1. 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?

  1. 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

  1. 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.

popematt commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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.