Use model constructor when necessary (enables immutable models, encapsulation, Kotlin data classes etc)

[Alexander--]

This pull request adds a fallback to non-default constructors for instantiating model class.

As of version 1.3.7 LoganSquare models are required to have either mutable public fields or public getter and setter:

@JsonObject
class Dto {
    @JsonField
    public String property1;

    @JsonField
    private int property2;

    public int getProperty2() { return property2; }

    public void setProperty2(int value) { this.property2 = value; }
}

This rubs many "enterprise specialists" wrong way and makes LoganSquare incompatible with certain use cases (such as Kotlin data classes). In comparison, various reflection-based JSON libraries have no issues with classes like this:

@JsonObject
class ImmutableDto {
    @JsonField
    private final property;

    public ImmutableDto(String property) {
        this.property = property;
    }
}

Those libraries typically use reflection to instantiate the class without calling constructor, then proceed to directly assign values to internal fields, (ab)using reflection conduct to disregard their private/final qualifiers.

Compared to those libraries, supporting non-default constructors in LoganSquare is slightly non-trivial due to it's compile-time nature.

In addition to using a constructor if assigning fields directly is impossible, this pull-request adds a check to forbid subclassing other models from models, that require constructor-based injection (subclassing in the opposite direction is harmless, so it is still allowed).

[mannodermaus]

Thank you! This is great work that paves the way going forward in a lot of directions. Preventing subclassing of constructor-injected classes seems reasonable in order to prevent accidental mis-mappings of fields. If the constructor args need to be declared in order of appearance inside the class (kind of like AutoValue in a sense, I guess), are there type checks included already?

@JsonObject
class A {
    @JsonField
    final String field1;
    @JsonField
    final int field2;

    A(int field2, String field1) {
        // ...
    }
}

ERROR: Order of constructor parameters in `example.A` doesn't match declared fields! Expected `String field1` at position 0.

I suppose it's the case out-of-the-box, but would this be caught with an error at compile-time? Also, how about the already mentioned accidental incorrect ordering of fields with the same type. Maybe we could log a statement (either INFO or even WARNING) if the parameter order doesn't seem to match the corresponding fields?

@JsonObject
class A {
    @JsonField
    String field1;
    @JsonField
    String field2;

    A(String field2, String field1) {
        // ...
    }
}

WARNING: Order of constructor parameters in `example.A` possibly doesn't match declared fields. Expected `String field1` to be declared at position 0.

Maybe this last case isn't even necessary, I'm just trying to make sure we think of everything upfront when it comes to a change of this magnitude.

[Alexander--]

@aurae

are there type checks included already?

No, there are none in this pull-request. The javac can do them better anyway. My main motivation is support for Kotlin data classes; multiple constructors with type ambiguity, varargs etc. do not matter there.

Also, how about the already mentioned accidental incorrect ordering of fields with the same type.

Rather than "incorrect ordering of fields", this is really "incorrect ordering of constructor parameters" (order of fields does not matter, as evident from widespread success of field injection).

This issue does not affect Kotlin data classes, so I haven't coded any solution yet. I agree, that warning is suitable (IntelliJ sometimes warns in case of label and variable name mismatch, so there is a good precedent for that already). A facility to suppress such warnings (via @SuppressWarnings) would have to be added at the same time.

Personally I don't see mis-ordering of constructor arguments as a deal-breaker. People can always shoot themselves in the foot with constructors like this:

DataClass(String property1, String property2) {
    this.property1 = property2;
    this.property2 = property1;
}

This is the price, that comes with writing constructors and imperative programming in general: the developer can screw it up. In absence of facility like Kotlin data classes, it is actually safer to use field-based injection, period.

[mannodermaus]

Yeah, I thought the same when it comes to accidentally assigning the parameter to the wrong field. It shouldn't really matter because of that!

@EricKuck how do you feel about this contribution?

[Alexander--]

@EricKuck It have been over two weeks already. I understand, that you might be busy with other stuff, but please consider having at least brief look at this submission.

Alternatively, if you don't really intend to incorporate this feature (or merge this particular pull-request), you can save me a lot of trouble by declining it.

Unless you have completely forgotten your way around guts of the library, in which case… it is time to fork! :D

[I60R]

No activity for 2 years. It's definitely time to fork