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