google / auto

A collection of source code generators for Java.
Apache License 2.0
10.42k stars 1.2k forks source link

Generated builder can't be used for Jackson deserialization #255

Closed punya closed 8 years ago

punya commented 9 years ago

Jackson 2.x supports deserializing JSON by using the builder pattern, so it would be really cool if the following worked:

@AutoValue
@JsonDeserialize(builder = AutoValue_Project.Builder.class)
public abstract class Project {
    public abstract int getId();
    public abstract String getPath();

    public abstract Builder toBuilder();

    @AutoValue.Builder
    interface Builder {
        Project build();

        Builder withId(int id);
        Builder withPath(String path);
    }
}

Unfortunately, the class AutoValue_Project.Builder doesn't exist at the point the compiler sees the @JsonDeserialize annotation, so this fails to compile. Is there a workaround for this problem?

NB: the best workaround I have at the moment is a creator method

    @JsonCreator
    static Project create(@JsonProperty("id") int id, @JsonProperty("path") String path) {
        return builder()
                .withId(id)
                .withPath(path)
                .build();
    } 
jlcheng commented 9 years ago

There is another problem, Jackson requires the Builder class in the annotation to have a public constructor. The pattern used by AutoValue.Builder is to have a protected constructor, and the programmer is "supposed" to access an instance of the builder through a .builder() method. This pattern is just not a good fit with Jackson.

IMO Either Jackson needs to support getting access to a Builder instance via a method call, or AutoValue.Builder needs to expose the Builder constructor publicly. I'd argue it's better to patch Jackson than AutoValue.

vanvlack commented 9 years ago

You should be able to do this like the following:

@AutoValue
@JsonDeserialize(builder = Project.Builder.class)
public abstract class Project {

  public abstract int id();
  public abstract String path();
  public abstract Builder toBuilder();

  @AutoValue.Builder
  @JsonPOJOBuilder(withPrefix = "")
  interface Builder {
    Project build();

    Builder id(int id);
    Builder path(String path);
  }
}

You shouldn't be referencing your AutoValue_ class impl in the annotation at all, as Jackson annotations are inherited (https://github.com/FasterXML/jackson-annotations#improvements-over-typical-java-annotations).

-- edit

I realize this is required for it to work:

AutoValue.Builder needs to expose the Builder constructor publicly

codefromthecrypt commented 9 years ago

@cowtowncoder any thoughts on this issue?

cowtowncoder commented 9 years ago

@adriancole I don't think constructor of Builder needs to be public; a private one should work as well. And if not, I'd consider that a Jackson bug. Not sure if that'd be enough, but in general it's not good (IMO) to require public accessors when reflection can be used to access any fields / methods (although code generation may be limited to public ones).

I am also open to improvements in form of, say, allowing access to builder via static method, defined either in target class or builder. All that is needed is really figuring out how annotations should work, and it could probably be added in Jackson 2.7. Implementation should not be too difficult and I can help with a PR.

zkiss commented 9 years ago

Haven't investigated this very thoroughly, but this guy managed to make it work: https://github.com/artem-zinnatullin/AutoJackson

In case improvements are still needed in Jackson, I guess a basic auto detection mechanism wouldn't be too hard to implement; find a static method with no args which returns the builder type (maybe a builderFactory class parameter could be passed in the @JsonDeserialize annotation)?

artem-zinnatullin commented 9 years ago

Yep, I'm here if you want some help — feel free to connect me!

Actually, Jackson is powerful enough to handle classes with Builders without changes in AutoValue/Jackson.

You can check the example: https://github.com/artem-zinnatullin/AutoJackson

codefromthecrypt commented 9 years ago

Thanks for the tips, guys. I'll give this a try and if need more, raise/collaborate on Jackson side.

boliu-mobile commented 8 years ago

anyone has solution?

vanniktech commented 8 years ago

Has anyone been able to make it work with default values? Right now in our builder() method we'll set some default values.

public static Builder builder() {
    return new AutoValue_Class.Builder().a(2).b(3);
}

Unfortunately Jackson won't take the builder from the builder() method and instead create a new one where the default values for a and b would be missing.

My class is annotated with @JsonDeserialize(builder = AutoValue_Class.Builder.class). I've also tried putting @JsonCreator onto the builder() method but that won't work either.

tbroyer commented 8 years ago

@vanniktech You need to use your own builder class and put a @JsonCreator method inside the builder:

@AutoValue
@JsonDeserialize(builder = A.Builder.class)
abstract class A {
  static Builder builder() {
    return new AutoValue_A.Builder().a(true).b("default");
  }

  @JsonProperty abstract boolean a();
  @JsonProperty abstract String b();

  @AutoValue.Builder
  @JsonPOJOBuilder(withPrefix = "")
  abstract static class Builder {
    @JsonCreator private static Builder create() { return A.builder(); }
    abstract Builder a(boolean a);
    abstract Builder b(String b);
    abstract A build();
  }

  public static void main(String[] args) throws Exception {
    ObjectMapper mapper = new ObjectMapper();
    System.out.println(mapper.writeValueAsString(A.builder().build()));
    A a = mapper.readValue("{\"a\": false, \"b\": \"non-default\"}", A.class);
    System.out.println(mapper.writeValueAsString(a));
    a = mapper.readValue("{}", A.class);
    System.out.println(mapper.writeValueAsString(a));
  }
}
vanniktech commented 8 years ago

@tbroyer awesome works like a charm. I guess this issue can be closed?

eamonnmcmanus commented 8 years ago

Thanks Thomas!

codefromthecrypt commented 8 years ago

slick!

singha27 commented 7 years ago

I have requirement to build profiles with values coming in Json format

singha27 commented 7 years ago

I have written one builder using builder pattern

singha27 commented 7 years ago

any on can help how to do

feinstein commented 5 years ago

Is it necessary to put @JsonProperty behind each abstract method? Can't Jackson infer automatically?

cowtowncoder commented 5 years ago

@feinstein Jackson should be able to detect methods just fine as long as they conform to expected naming pattern, which with

@JsonPOJOBuilder(withPrefix = "")

and obeys visibility limits -- both which should be the case here. Unless I miss something.

feinstein commented 5 years ago

Well, then I can't see whan I did wrong, because if I remove @JsonProperty from my abstract methods, the field just disappears on JSON.

Here's a simplification of my code (just removed some fields):

/**
 * Value class representing a user.
 */
@AutoValue
@JsonSerialize(as = User.class)
@JsonDeserialize(builder = AutoValue_User.Builder.class)
public abstract class User {
   // is exported to JSON perfectly, as are all the others with @JsonProperty 
    @JsonProperty("id")
    public abstract UUID id();
    @JsonProperty("first_name")
    public abstract String firstName();
    @JsonProperty("last_name")
    public abstract String lastName();
    // I didn't place the @JsonProperty here since I thought it didn't need 
    // as the name of the field in JSON and in the class are the same, but if 
    // I don't place @JsonProperty, it isn't added to the JSON
    public abstract String cellphone();
    // same here
    public abstract String email();
    // same here
    public abstract float rating();
    // same here
    public abstract short gender();

    public static Builder builder() {
        return new AutoValue_User.Builder();
    }

    public abstract Builder toBuilder();

    @AutoValue.Builder
    @JsonPOJOBuilder(withPrefix = "set")
    public abstract static class Builder {
        @JsonProperty("id")
        public abstract Builder setId(UUID id);
        @JsonProperty("first_name")
        public abstract Builder setFirstName(String firstName);
        @JsonProperty("last_name")
        public abstract Builder setLastName(String lastName);
        public abstract Builder setCellphone(String cellphone);
        public abstract Builder setEmail(String email);
        public abstract Builder setRating(float rating);
        public abstract Builder setGender(short gender);
        public abstract User build();
    }
}
cowtowncoder commented 5 years ago

That is strange: as long as impl class extends base class with annotations, annotations should be "inherited" (Jackson handles that part, not JDK). About the only thing I can think of is name mismatch as builder does not complain if a property does not match a builder method: instead it is buffered and match is attempted at POJO built (using build()). Perhaps it'd make sense to have an option to require strict matching, as in many cases user probably wants to ensure all methods in builder match (similar to defaults of non-builder-built POJOs).

feinstein commented 5 years ago

Well, I can't see any mismatch in the auto generated code.

Do you want me to make a small code sample to replicate the issue, so you can infer what's going on?

My case is a Jersey webapp with Jackson, retuning an AutoValue object constructed with a Builder from AutoValue, but I imagine this would be the same without Jersey, I just didn't test it.

cowtowncoder commented 5 years ago

@feinstein yes that would be useful: filing issue against jackson-databind, with a link back to here would be useful. I would like to ensure that Jackson plays well with AutoValue, due to wide usage.

feinstein commented 5 years ago

@cowtowncoder I created the issue here.