#73 Implementing Immutable Constructors - #847

(Message from the pull-request-builder): Admins, please verify this patch for it to build in the pull-request-builder.

Hi Guys,

I have started work on #73.

This PR is far from being ready to merge but I wanted to check if so see if I am on the right track and ask a few questions because I have gotten stuck.

Basic Thought Process:

• Implement a constructor annotation which defines mapping targets
• Change mapstruct Type to be aware of this annotation and store the results in a simple constructor object
• Change PropertyMapping to accept a constructor write assignment
• Change bean mapping method to be aware of these extra mapping targets
• Change the BeanMapping template to take this constructor into account and generate the constructor invocation

Questions:
Should a constructor mapping be part of PropertyMapping?
How should I handle dependsOn in terms of constructors?
How should I handle default values? This will probably require a LocalVar Wrapper

Ciaran

@cliedeman That's an awesome contribution, thanks! Still recovering from vacation, but going to look into this soon. This one has been on the wish list for a very long time!

Hey @cliedeman, great that you're taking this up!

One thing that directly caught my eye was the introduction of @Constructor - until now we tried to design MapStruct to not require modifying or annotating the bean classes. This would break that. Plus, there are a couple comments in the feature requests for immutable classes being generated by other frameworks, so having to modify potentially genereated code should not even be an option provided by us.

My idea would be to try to map the properties to constructor args by type, and if that doesn't work without ambiguities, match those ambiguous properties by constructor argument name (or their source / target mapping in the @Mapping annotation). Only if there is no or more than one constructor that can be linked to the properties, an error would be shown and the user would have to specify a Builder (that's a feature on our list as well) or map it manually.

Regarding your questions:

Should a constructor mapping be part of PropertyMapping?

Hmm, I would have guessed that in the model it's part of the BeanMappingMethod, somehow alongside the factoryMethod. Speaking of which, perhaps one alternative way to tackle this feature is to extend the capabilities of factory methods (i.e. allow them to take arguments - with some sensible logic to collect candicates and select the best one -- it's also a longer standing feature request, iirc). The only thing to be done then would be to treat the bean constructors as factory methods, subject to the best-matching candidate selection. The Constructor would then only be a specialization of MethodReference. And we'd solve a whole bunch of problems and feature requests in one go.

How should I handle dependsOn in terms of constructors?

Good catch! dependsOn currently fails to always do what's expected in combination with constant already, so there's stuff to do anyway (but low on our list). But generally my feeling would be that properties passed via constructor shouldn't be declared to be dependent on properties populated with a setter... 😉. We could raise an error in those cases. And perhaps a warning for dependsOn relations between "constructor-properties"...

How should I handle default values? This will probably require a LocalVar Wrapper

Introducing local vars sounds reasonable here. Don't know if that works with the existing wrapper, though.

One thing that directly caught my eye was the introduction of @constructor - until now we tried to design MapStruct to not require modifying or annotating the bean classes

The reason is that these bean classes might be generated by a framework. I would not like to change generated code.

Hi Guys,

Thanks for the feedback.

@agudian, @sjaakd I am not too worried how we end up sourcing the bean constructor to use. I feel like there are multiple approaches but this is not the hard part of the implementation. I am happy to change it later or use an alternate method. For instance my particular use case would most likely be with jackson annotated constructor fields. In this case I would like to retrieve the jackson annotations and somehow create the constructor mapping from them.

@agudian I think enhancing the bean factory method to accept parameters might be a better approach in the long run and will experiment with this.

Ciaran

I reckon we have the following situations:

1a. The mapping requires a constructor with a set of arguments, each of them of a unique type. There's only one matching constructor, with exactly those arguments of the required type. -> Make match.

1b. The mapping requires a constructor with a set of arguments, each of them of a unique type. There's are more matching constructors, with exactly those arguments of the required type. ->
We need (the) factory mechanism + qualifiers to make a decision.

2a. The mapping requires a constructor with a set of arguments, but some of the required arguments have the same type. -> MapStruct could use constructor parameter names to make a match. I know that IDE's match the name of the constructor with the properties of the bean when you tell it to generate a constructor (at least Netbeans does). However, this is not in anyway conform java bean standard and we don't know how frameworks generate their constructors.

2b. The mapping requires a constructor with a set of arguments, but some of the required arguments have the same type, names do not match up. -> We could rely on the @Mapping to define source and target in this scenario.

3a. The mapping requires a constructor with a set of arguments, each of them of a unique type. However there's a constructor that has a superset of these arguments. -> NullValueMappingStrategy could be used to complete.

4a. The mapping requires a constructor with a set of arguments, each of them of a unique type. However there's a constructor that has a subset of these arguments. -> the remainder could be completed by the regular target accessors (setter, adder, getter-for-collections)

5a. The user wants to select deliberately an other constructor. -> We need (the) factory mechanism + qualifiers to make a decision.

In general: when do we resort to constructor matching? If there's no empty constructor?

@cliedeman, regarding

For instance my particular use case would most likely be with jackson annotated constructor fields. > In this case I would like to retrieve the jackson annotations and somehow create the constructor
mapping from them

Could you give an example of how that'd look like?

So my current inclination would be towards the following:

• If there is a default ctor and one proper one, take the latter by default
• If there are several non-default ones, have the user resolve the ambiguity using an annotation or property on @BeanMapping, accepting the array of parameter types
• Properties are associated to target properties via the constructor parameter names; if needed the existing ConstructorProperties annotation would be used to map the parameter names

This above would be the default implementation of an SPI for constructor selection. Users could plug in their own selection mechanism via that API (Ciaran's Jackson example would be one potential usage).

@gunnarmorling Jackson Example:

public class Point {
  private final int x, y;

  @JsonCreator
  public Point(@JsonProperty("x") int x, @JsonProperty("y") int y) {
   this.x = x;
   this.y = y;
  }
 }

whoops accidentally hit close....

(Message from the pull-request-builder): Admins, please verify this patch for it to build in the pull-request-builder.

Great to see you're working on this! Could you please sonsider that the constructor for the immutable object could be private access? This will be often the case if externally a Builder is used to fill the objects attributes (like generated by https://immutables.github.io/).

@mvonrenteln we have that on the radar with the separate issue #782. PRs are welcome!

Any news on this PR?
I'd love to see it merge to master some day.