Support specifying method for instantiating builders in JsonDeserialize

[kaweston]

I request adding a parameter such as builderMethod to JsonDeserialize (e.g. @JsonDeserialize(builderMethod = "builder") ) to facilitate a way of specifying a method to instantiate a builder instance.

Example use case
When using the Lombok SuperBuilder annotation it results in the creation two builder inner classes, one public and the other private which subclasses the public; it is the private class that must be instantiated. Currently when using the SuperBuilder annotation it is necessary to declare the private inner class (which will be completed during the annotation processing stage) to set its access modifier to package private so that Jackson can instantiated it.

An example original source file and the annotation processed result follows:

Original file

@JsonDeserialize(builder = LegalDocument.LegalDocumentBuilderImpl.class)
@Getter
@SuperBuilder(toBuilder = true)
public final class LegalDocument {

    private final String code;
    private final String fileName;
    private final String description;

   // This should not be necessary
    static final class LegalDocumentBuilderImpl
            extends LegalDocument.LegalDocumentBuilder<
            LegalDocument, LegalDocumentBuilderImpl> {
    }

}

After annotation processing

@JsonDeserialize(
    builder = LegalDocument.LegalDocumentBuilderImpl.class
)
public final class LegalDocument {
    private final String code;
    private final String fileName;
    private final String description;

    @Generated
    protected LegalDocument(LegalDocument.LegalDocumentBuilder<?, ?> b) {
        this.code = b.code;
        this.fileName = b.fileName;
        this.description = b.description;
    }

    // I would like to be able reference this method in the JsonDerserialize annotation
    @Generated
    public static LegalDocument.LegalDocumentBuilder<?, ?> builder() {
        return new LegalDocument.LegalDocumentBuilderImpl();
    }

    @Generated
    public LegalDocument.LegalDocumentBuilder<?, ?> toBuilder() {
        return (new LegalDocument.LegalDocumentBuilderImpl()).$fillValuesFrom(this);
    }

    @Generated
    public String getCode() {
        return this.code;
    }

    @Generated
    public String getFileName() {
        return this.fileName;
    }

    @Generated
    public String getDescription() {
        return this.description;
    }

    @Generated
    public abstract static class LegalDocumentBuilder<C extends LegalDocument, B extends LegalDocument.LegalDocumentBuilder<C, B>> {
        @Generated
        private String code;
        @Generated
        private String fileName;
        @Generated
        private String description;

        public LegalDocumentBuilder() {
        }

        @Generated
        protected B $fillValuesFrom(C instance) {
            $fillValuesFromInstanceIntoBuilder(instance, this);
            return this.self();
        }

        @Generated
        private static void $fillValuesFromInstanceIntoBuilder(LegalDocument instance, LegalDocument.LegalDocumentBuilder<?, ?> b) {
            b.code(instance.code);
            b.fileName(instance.fileName);
            b.description(instance.description);
        }

        @Generated
        protected abstract B self();

        @Generated
        public abstract C build();

        @Generated
        public B code(String code) {
            this.code = code;
            return this.self();
        }

        @Generated
        public B fileName(String fileName) {
            this.fileName = fileName;
            return this.self();
        }

        @Generated
        public B description(String description) {
            this.description = description;
            return this.self();
        }

        @Generated
        public String toString() {
            return "LegalDocument.LegalDocumentBuilder(code=" + this.code + ", fileName=" + this.fileName + ", description=" + this.description + ")";
        }
    }

    static final class LegalDocumentBuilderImpl extends LegalDocument.LegalDocumentBuilder<LegalDocument, LegalDocument.LegalDocumentBuilderImpl> {
        @Generated
        private LegalDocumentBuilderImpl() {
        }

        @Generated
        protected LegalDocument.LegalDocumentBuilderImpl self() {
            return this;
        }

        @Generated
        public LegalDocument build() {
            return new LegalDocument(this);
        }
    }
}

[janrieke]

Is there any progress on this?

I'm the contributor of the @SuperBuilder feature for lombok. Users frequently want to use builders in combination with Jackson deserializing, because they want to have their DTOs immutable and do not want to rely on INFER_PROPERTY_MUTATORS. For that, users have to

1. run delombok on their DTOs, because the generated code of @SuperBuilder (including the builder class headers) is really complex and loaded with generics,
2. identify the correct builder class header (there are two of them!),
3. copy-paste it to their sources, and finally
4. remove the private to make it usable (e.g. @JsonDeserialize(builder = MyDtoBuilderImpl.class))

This is highly error-prone and unnecessarily clutters the source code with a class definition most users won't understand why it's there.

The only solution I see here for lombok is to make the builder implementation class package-private by default (instead of private). However, this has the major drawback of making a class visible which most people will never know what it's for. So I do not want to go that way.

If you do not see any fundamental problems with adding such a parameter to @JsonDeserialize, but simply do not have the time to implement it, I would like to have a look at it and try creating a PR by myself.

[janrieke]

Hmm, now I'm a bit confused. I just checked it again and it also works with the builder implementation class private. So unless I figure out why I thought making it package-private is necessary a few month ago, I think I simply made a mistake back then.
So from my POV, this issue can be closed.

[cowtowncoder]

@janrieke Build implementation class should work any protection level (at least with Java 8, 9 and later may limit accessibility, esp. for private, but I think package-protected and other can be opened and Jackson will use whatever is available). If not, a reproduction of this happening would be useful since access to members requires call to setAccessible() (and in the past has missed from some code paths).

[cowtowncoder]

@kaweston On original request: I guess I can see potential benefit of allowing specifying a static factory method for constructing Builder, instead of relying on Builder having a no-argument constructor. Is that what and why it is requested? As per above, access restrictions should not require separate builder implementation, and no-arg constructor can remain private or package protected, needs not be public (and same for impl class).

I also noticed that the example did not use the suggested new annotation.

[janrieke]

IIRC the problem was not within Jackson, but that I got a compilation error on the annotation when referencing the (private) builder implementation class.
So it may be indeed dependent on the Java version; I'll check that.

[janrieke]

I found the problem: Eclipse's compiler does not complain about private access, but javac does. javac gives MyDto.MyDtoBuilderImpl has private access in MyDto on @JsonDeserialize. Looks like a bug in ecj to me.

So allowing specifying a static factory method would in fact help.

[kaweston]

@cowtowncoder I raised the issue primarily on the basis that this was causing a compilation issue. I wasn't sure whether not being able to instantiate the private class was by design or a bug.

Having said that, I'm personally of the view that things marked private shouldn't have their protection level subverted unless there is no other way. In this case there is an alternative with a builder method when specified.

Additionally, the fact that builder methods for instantiating builder classes is such a common pattern (for reasons such as not forcing a no arg constructor as you pointed out) it is something, I think, would be quite widely welcomed and hence I would advocate support for specifying a builder method irrespective of whether a change is made to support instantiating private classes (with javac given @janrieke's findings).

[cowtowncoder]

@kaweston Ok. So I'll consider this an RFE that is still open.

Just one additional note: as far as I can see Jackson can access private (inner) classes as well as methods just fine. No changes needed. This with Java 8; module system may impose further limits to require module declarations to "open" access for reflection.

[janrieke]

Here is the relevant part of the JLS (section 6.6.1.):

Otherwise, the member or constructor is declared private, and access is permitted if and only if it occurs within the body of the top level type (§7.6) that encloses the declaration of the member or constructor.

The important part is "within the body of the top level type". As the annotation is placed outside the body of the outer class, javac is correct to complain that the nested private class is not accessible. If this compiles in javac 8 (I can't verify that right now), I'm quite sure the behavior of javac 8 would be incorrect (and that javac 9 complains is more likely a bugfix than a result of the module system).

The test you introduced in b448a2a only compiles because you have a top-level type around the nested class that has the annotation. Thus, the annotation is within the body of the top-level type. IMO the JLS is not very consistent at this point (my guess is that this definition existed before generics and annotations were introduced).
However, this is not the typical case: Most builders are for top-level types, not nested types.

[cowtowncoder]

Ah, interesting. My interest in test was more wrt actual dynamic access, but then again with CI system it could catch changes to ability compile too (should warnings become errors).

Either way, this is useful information as it makes the case for addition of method stronger. I'll have to see if this would be doable for 2.11, given that it requires annotation addition (but to annotation that is in databind, one of few)

[cowtowncoder]

One additional note: unfortunately AnnotationIntrospector interface for builders is bit sparse, consisting of just:

    public Class<?> findPOJOBuilder(AnnotatedClass ac);
    public JsonPOJOBuilder.Value findPOJOBuilderConfig(AnnotatedClass ac);

so slightly bigger change needed to pass information. Not a big deal, I hope; not aware of others using this extension point... but there's no good way to really figure it out.