Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow constructor injection. #3

Open
evant opened this issue Feb 17, 2015 · 13 comments
Open

Allow constructor injection. #3

evant opened this issue Feb 17, 2015 · 13 comments

Comments

@evant
Copy link

evant commented Feb 17, 2015

Oftentimes you want your model object to be immutable so you don't want to make your fields public or provide setters. Would it be possible to support constructor injection for this usecase? Something like:

@JsonObject
public class Image {

    @JsonField(name = "_id")
    private int imageId;

    @JsonField
    private String format;

    @JsonField
    private String url;

    @JsonField
    private String description;

    @JsonField(name = "similar_images")
    private List<Image> similarImages;

   // Not sure if the arguments need to be annotated instead/as well.
   @JsonConstructor
   public Image(int imageId, String format, String url, String description, List<Image> similarImages) {
       this.imageId = imageId;
       this.format = foramt;
       this.url = url;
       this.description = description;
       this.similarImages = similarImages;
   }

   public int getImageId() {
      return getImageId;
   }

   public String getFormat() {
      return format;
   }

   public String getUrl() {
      return url;
   }

   public List<Image> getSimilarImages() {
      return Collections.unmodifiableList(similarImages);
   }
}
@EricKuck
Copy link
Member

Interesting idea. I think this has to be fleshed out a bit to make sure that it isn't more error prone than what we have now and that resulting code isn't a big pile of ugly. I see lots of utility in something like this though.

@andaag
Copy link

andaag commented Feb 17, 2015

I suspect you'd need annotations on all the arguments as well, which would make it quite annotation .. verbose.

How about not making fields private, but instead package local. Then the annotator could create the constructor file in the same package and set the fields? Not quite immutable, but better than public on everything.

@evant
Copy link
Author

evant commented Feb 17, 2015

I'd be perfectly ok with the annotation on the constructor arguments instead of the fields. That should be sufficient right? Since the fields are manually set in the class anyway.

@EricKuck
Copy link
Member

@andaag You can actually already do package local everything (constructor, fields, getters/setters). You can even do private fields with public/package accessors. Doing this is good enough for all the scenarios I can think of off the top of my head.

@evant do you have an example of where this wouldn't suffice?

@evant
Copy link
Author

evant commented Feb 17, 2015

Hm, that does alleviate most of my concerns. Package local doesn't quite logically match the semantics I would want to have, but it would be a workable solution. It might be more convenient for constructing instances of the class not through serialization, (unit tests for example) though I guess you can just add that in addition to an empty constructor. You will have to check your constraints both in the constructor and in onParseComplete() though again, that could be workable.

All in all, I think this would make some cases slightly cleaner, but it's not strictly necessary.

@evant
Copy link
Author

evant commented Feb 17, 2015

One thing I thought of is you could chose not to have a backing field, or choose to store it in a different way, as long as you have an argument and getter your good. I'm not sure how often that would come up in practice though.

@EricKuck
Copy link
Member

Another possible solution for this would be to use something akin to the Builder pattern. Here's kinda what I'm thinking:

public class ImmutableClassExample {
    private final String mImmutableString;
    private final int mImmutableInt;

    public ImmutableClassExample(ImmutableClassBuilder builder) {
        mImmutableString = builder.getString();
        mImmutableInt = builder.getInteger();
    }

    @JsonObject
    public static class ImmutableClassBuilder {

        @JsonField
        private String mString;

        @JsonField
        private int mInt;

        public String getString() {
            return mString;
        }

        public void setString(String string) {
            mString = string;
        }

        public int getInteger() {
            return mInt;
        }

        public void setInteger(int integer) {
            mInt = integer;
        }
    }
}

In this case, you'd be parsing and serializing the ImmutableClassBuilder instead of the actual ImmutableClass object. Since the Builder pattern is used so much throughout Java, this seems like a pretty natural solution to me.

@mannodermaus
Copy link
Contributor

The proposition made by @evant could also allow for generic types being compatible with LoganSquare, right? Jackson can deduce the type of a generic object from its annotation in the constructor, so would that be a possibility for this as well?

@JsonObject
public class GenericObject<T> {

    @JsonField(name="value")
    public int value;   // Not generic, will be parsed without any complications

    public T field;     // Generic, couldn't be deduced by LoganSquare if it were to be annotated here

    public GenericObject(@JsonField(name="field") T field) {
        this.field = field;
    }
}

@EricKuck
Copy link
Member

@aurae Jackson gets away with this because it uses runtime annotation processing (which is why it's so much slower than this library). Since this library does everything at compile-time, there's no way to know what types you're going to pass in.

@mannodermaus
Copy link
Contributor

I see, that makes total sense!

@kunjan-a
Copy link

kunjan-a commented Oct 24, 2016

I agree with @evant . Annotating constructor arguments seems a better solution than the current situation. The only issue with the builder approach @EricKuck suggested is that while it helps during deserialization, while serializing you have to again create an object of builder from the immutable object and then serialize the builder. This seems counter intuitive as a design pattern.

Honesty I have used @JsonCreator exposed by Jackson and although its a little verbose at least it ensures that we can use immutable objects naturally.

@renannprado
Copy link

any news on this?
I would not like to have to switch to jackson to have this feature :(

@Alexander--
Copy link

Implemented in this PR — #213

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants