Java optional codegen

[aslakhellesoy]

Summary

Use custom code generation for Java

Details

Switch from using jsonschema2pojo-maven-plugin to using a custom code generator.

Motivation and Context

In order to have better type safety for non-required fields, we want the generated Java code to use Optional in getters. See cucumber/ci-environment#51 for context.

Types of changes

• Bug fix (non-breaking change which fixes an issue).
• New feature (non-breaking change which adds functionality).
• Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist:

• The change has been ported to Java.
• The change has been ported to Ruby.
• The change has been ported to JavaScript.
• The change has been ported to Go.
• The change has been ported to .NET.
• I've added tests for my code.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have updated the CHANGELOG accordingly.

[jamietanna]

Is this required? I see useOptionalForGetters is an option for the Maven/Gradle plugin so it should mean we can avoid hand-writing stuff?

[aslakhellesoy]

Oh nice! I didn't see it in the on the website and I also found joelittlejohn/jsonschema2pojo#1057 which led me to believe it wasn't supported. But here it is!

I'll take it for a spin.

[aslakhellesoy]

The custom code generator I implemented would do requireNonNull checks in getters, setters and the argument constructor (not the no-arg constructor though). This will fail earlier in case we forget to set some required fields.

I could add a #validate() method as well which would do the same, and we could call this in the code that puts the messages on the event bus.

Worthwhile restoring the custom code generator for those purposes? I think so!

[aslakhellesoy]

JSON Schema has a oneOf keyword.

I think it would be a lot of work to support this in our custom code generators, so I've opted for a simpler solution in 5daf24a.

WDYT?

[aslakhellesoy]

Consider overloading Envelope.from(PropertyName propertyName) with different parameter types instead of naming the method differently for each property. Envelope.fromPropertyName(new PropertyName(....)) is very verbose.

Done! Been doing too much TypeScript lately...

[aslakhellesoy]

I think it would be cleaner to wrap List constructor arguments in unmodifiable lists, to make the objects truly immutable. Agree?

[aslakhellesoy]

I made them unmodifiable :-)

[mpkorstanje]

I made them unmodifiable :-)

Great. It's not the same as immutable though. Nice little gotcha from way back then.

[aslakhellesoy]

Good to merge @mpkorstanje ?

[mpkorstanje]

Don't forget to remove this.

[aslakhellesoy]

Why @mpkorstanje? We're calling validate() in the constructors and the static from methods.

[mpkorstanje]

If the constructors already have the requireNonNull for each required argument then what is there left to validate?

[mpkorstanje]

Good to merge @mpkorstanje ?

I'll have to take a look at the generated code. But hard to say right now just from the git diff.

[aslakhellesoy]

The generated code is in git: https://github.com/cucumber/common/blob/54e144aa7f43d6cce36fabf29db1bebd33b2f56d/messages/java/src/main/java/io/cucumber/messages/Messages.java

[mpkorstanje]

Ah! Thanks that helps a lot!

Consider:

1. making all fields final. This will allow the compiler to enforce that all fields are set in a constructor.
2. Use the pattern this.field = requireNonNull(field); instead of a separate validate method.
3. And I think that since java 17 the message can be omitted from requireNonNull as java can see which argument was the problem. Not sure about that.

The combination of the first two make it easy to read the code and see what are the required and optional fields

[aslakhellesoy]

If we make all fields final we cannot use the current implementation of from. Instead we would have to implement from like this:

public static Envelope from(Source source) {
    return new Envelope(
            null,
            null,
            null,
            null,
            null,
            null,
            null,
            Objects.requireNonNull(source, "Envelope.source cannot be null"),
            null,
            null,
            null,
            null,
            null,
            null,
            null,
            null,
    );
}

That's fine with me - is this what you're suggesting? With this we can also remove the private empty constructors, as well as the validate() method since the validation no longer needs to be called in from - it can be inlined in the constructor.

[mpkorstanje]

Instead we would have to implement from like this:

No major objects. Though what was the reason we wanted to use from and not a 1-argument constructor?

[aslakhellesoy]

Instead we would have to implement from like this:

No major objects. Though what was the reason we wanted to use from and not a 1-argument constructor?

So we don't have to create Envelope objects like this:

return new Envelope(
            null,
            null,
            null,
            null,
            null,
            null,
            null,
            Objects.requireNonNull(source, "Envelope.source cannot be null"),
            null,
            null,
            null,
            null,
            null,
            null,
            null,
            null,
    )

[aslakhellesoy]

Oh I see what you mean - might as well be a 1-arg constructor. I'll change to that in #1879

[aslakhellesoy]

Closing in favour of #1879