Java record components should dictate written fields

[dblevins]

Given a record like this, only the name and age fields should be written.

public record Person(String name, int age) {
    public Builder toBuilder() { return ...; }
    public static Builder builder() { return ...; }
}

https://issues.apache.org/jira/browse/JOHNZON-432

[struberg]

reviewed and am-ed

[rmannibucau]

I am concerned with this PR because we do intentionally enable virtual field so this is a regression

side note: think we can upgrade to java 17 as a baseline and avoid asm workaround

tip: solution can be to make the tostring/hashcode hardcoded set confgiurable in rg.apache.johnzon.mapper.access.MethodAccessMode#doFindReaders, will work for both mapper and jsonb impl and cover this issue feature

edit: created #144 as an alternative which doesn't break current usage, any hope you have a look?

[dblevins]

On cell so can check out the reference later. What do we think about a JsonbConfig flag?

[rmannibucau]

@dblevins @struberg said the same so there is [1], the thing is it is better to use @JsonbTransient or alike cause you don't always control well the JsonbConfig depending where you run - thinking to JAX-RS where it might break others to set such a flag if you merge models in the same app.

[1] https://github.com/apache/johnzon/pull/144/changes#diff-67b3744e36ecc6edf4d2d50ab282e0899a96ddc9dd532a756751eb97061b263fR436

[struberg]

But @JsonbTransient would reverse the logic which is mostly used in the JSON-B spec. I do agree that it is not crystal clear, but the spec always talks about "..serialization of field or record component (or JavaBean property) to JSON document"

So as far as I interpret it the spec says fields, record components (that's record fields only, not it's methods!) and Java Bean property. A method toBuilder() is not a Java Bean property. We would need to further clarify whether getCompoundName() on a Java Record qualifies as 'Java Bean property' with the name "compoundName" and should get rendered as well for Records?

But toBuilder() is clearly not a Bean Property in respect to the Java Beans 1.1 specification. That was imo even a clear bug in our implementation. So if we do any changes the new behaviour is to omit Record methods by default, unless they have a @JsonbProperty annotation on them. This part is again very underspecified, the spec says nothing about it, but that seems sensible, right?

That said, Romain is correct to point out that we should have a configuration option to have applications switch back to our old Record handling for some time (maybe even mark it with a deprecation warning?) for compatibility reasons.

[rmannibucau]

@struberg to be honest i'm fine not respecting the spec there and aligning on the POJO side since otherwise it is hard to handle virtual fields and forces to materialize all values which is quite dumb. Also it is our behavior since years (since we do support records) for that exact reason so I would promote it as main behavior, add the toggle as in my PR, and make it to the spec instead of regressing and adding it to the spec later.

[struberg]

What about that option: have that flag, but always enable the spec compliant version (to not serialise Record methods, except if they are annotated with @JsonbProperty) in our jsonb module?

That way we would keep Mapper compatible with the old version by default, and JsonB compatible with the spec by default.

[rmannibucau]

have that flag, but always enable the spec compliant version

the feature is used for JSON-B models so it would be a breaking change I don't see much benefit from - and solution to ignore the additional accessors is trivial (@JsonbTransient)

not sure it was obvious but with years the mapper feature was promoted to our JSON-B implementation so I wouldn't assume we can decouple them now.

[struberg]

I've run a few tests with latest yasson (JSON-B reference implementation), and they behave like the JSON-B spec rather clearly defines:

public record CarWithExtraMethod(String type, String color) {

    public String type() {
        return type;
    }

    public String name() {
        return color + " " + type;
    }
}

results in {"color":"green","type":"skoda"}

But

public record CarWithExtraMethod(String type, String color) {

    public String type() {
        return type;
    }

    public String getOtherName() {
        return color + " " + type;
    }
}

results in {"color":"green","getOtherName":"green skoda","type":"skoda"}

I don't like this result from the RI neither, as the attribute name is clearly wrong. it should not be getOtherName but only otherName.

So our old behaviour is clearly a bug and we have to fix it!
As I said: I understand that there is a benefit for having a backward compat flag to our (OUR, not the specs!) old behaviour, but the default behaviour must spec compliant.

[rmannibucau]

So our old behaviour is clearly a bug and we have to fix it!

100% disagree on that and on the assumption the RI is right - there are a ton of converter/serializer area we do behave differently so the RI runtime is not a proof.
From my window it is an ambiguity between POJO and records and since it is our choice since years we must keep it by default and ensure the spec aligns both, not the opposite.

opened jakartaee/jsonb-api#399

[dblevins]

There've been a few references to JSON-B spec. The spec doesn't say the word "record" anywhere, which is likely a huge gap we should close.

Circling back, would we be willing to allow a configuration flag? Yes, Transient is great, but doesn't help me as the unwanted method are generated code (toBuilder() method from lombok). We use it in 100+ records and do control the config.

Any "do it this way instead" suggestions welcome as long as I do get feedback on if a config option would be welcome (disabled by default of course, for backwards compatibility).

[struberg]

The newest spec draft does in section 3.7 and others.
https://github.com/jakartaee/jsonb-api/blob/master/spec/src/main/asciidoc/jsonb.adoc
Note that this is not yet a final version and the released 3.0 spec does indeed not define anything for records.

So your use case is you use Lombok and it generates a toBuilder(). There is no way to add any @JsonbTransient annotation to this generated method, right?

We might look into configuring a list of excluded methods. We already do this hardcoded for equals and hashCode, which is code smell imo. Maybe we could add additional excludes via JsonbConfig?

[rmannibucau]

@dblevins guess you might take over #144 but the historical design and solution for that (which exists since day 0) was to set a custom access mode, potentially decorating the jsonb default one, works well and solves your issue in a few seconds of code (maybe minutes if you do use an agent to do it ;)). This is the common way to solve lombok toBuilder pattern (the side note being a nicer pattern can be to use a static method there to not break the data side of the record but it is more a design thing, technically you're already covered)

[struberg]

I think there might be even an easier workaround which also works with older Johnzon versions.
I've just created a unit test case JsonbRecordMethodVisibilityTest in TomEE to verify my idea.

Just use a custom @JsonbVisibility on your record classes. That should do the trick.
apache/tomee@ff3ce45#diff-4b16127092f642da295e68f19f9ca1721b3ff57fd8a8c9ca2f8e7318fc292c05

That way you can exclude the toBuilder(), toString(), hashCode() and equals() methods. Can you verify if that works for your case @dblevins ?

We will still have to address this properly in Johnzon, but I would like to wait for what we do in the upcoming specification before we create a hack which is broken soon again.

[rmannibucau]

@struberg would be preferred if using jsonb but not mapper only but agree

[struberg]

Once we know how the spec intends to solve it then we can also look if we want to adopt the same strategy in Mapper. But I'd prefer to not do things prematurely in Mapper and then have to implement a completely different route in Json-B.

[rmannibucau]

@struberg we'll likely not change mapper but my point was not to do anything since we do support David's feature since the first version