Wow, thanks a lot for this PR @Zegveld. I haven't gotten around in reviewing this (picking the low hanging fruits first). However, we would look into this to make the 1.5.0.Beta2 release

By the way @Zegveld in #131 (comment) you are saying that you are not happy with the implementation route you took? Can you explain why? I personally thought that it is quite neat 😄 .

By the way @Zegveld in #131 (comment) you are saying that you are not happy with the implementation route you took? Can you explain why? I personally thought that it is quite neat .

Well that was before I made a lot more changes where in I made it quite neat ;)
Fixed it in this commit: rigd-loxia@98c3d8e

Only done the bits so far where I've added a thumbs up.
Still got a few review items to go, but that will have to wait till when I have time again later this week.

Only done the bits so far where I've added a thumbs up.
Still got a few review items to go, but that will have to wait till when I have time again later this week.

No problem @Zegveld, take as much time as you need. Please ping me once the PR is fully ready for the next iteration of review.

@filiphr , Think I've done all the adjustments. So that part is ready for a next iteration of review. Hopefully I haven't missed anything.

Just read the first review again and noticed the bit about:
Can we also add some fixture tests, to see how the generated code looks like? As you know we strive to generate code which looks human readable smile .
Where can I find an example of fixture tests so that I can add them for the subclass mappings as well? Edit: Think I've found examples at other tests. Took me a while to understand where to look for the classes with which the result is compared.

Also added a warning for Something that could be good, but maybe we need to do it in a separate PR is to validate that the SubClassMapping make sense. For example if the first sub class mapping does not hide the second one.

I've added fixture tests. Made these separate so that the main variants are covered and not impacted by changes within the general tests.
@filiphr, hope to hear from you soon for the 2 remaining issues, and also what you think of my solution for the annotation handling order one.

Thanks for the review @sjaakd. However, I have to say that I do not quite agree with moving @SubclassMapping to the BeanMapping. I believe that @SubclassMapping is on the same level as @ValueMapping and @Mapping.

Adding it to the @BeanMapping to only remove it later on doesn't bring much value I think. @Zegveld also did a really nice refactoring which makes supporting meta annotations work out of the box already.

and also what you think of my solution for the annotation handling order one.

@Zegveld you mean the refactoring you did, right? I found it really great, already said that in #2512 (comment) 😉 .

@Zegveld I've been a bit busy, so I haven't gotten around in finishing the review with your last changes. I haven't forgotten about this one yet.

First of all: what a tremendous feat of work. Thanks!. When discussing this with @filiphr I was amazed by the effort done on this.

I did not check the complete implementation. I recon that @filiphr did that already. So I focussed on the API extensions and the implications there.

Now: we do like to go more into meta-annotations in 2.0. So, isolating functionality (sourceClass and targetClass) into a new annotation is a good idea. I've one consideration here though.

MapStruct recognises a few major classes of mappings: mapping bean-to-bean, mapping iterable-to-iterable, map-to-map and value-to-value. BeanMapping obviously the most important one. Also, each comes with its own annotation for doing configuration: @BeanMapping, @IterableMapping, @MapMapping, @ValueMapping.

There is check functionality that guarantees that a MapMapping is not accidentally applied to @BeanMapping. In fact, we do have a small problem with the just released map-to-bean-mapping, which under some conditions gets mixed up with bean mapping.

Having said that: would it not be better to make @SubclassMapping a property of @BeanMapping? Although more verbose, it would keep functionality together and make sure @SubclassMapping is not accidentally applied to another form of mapping.

I didn't think about this initially, but it would be possible to do submappings for iterables or maps as well. This would be useful if the mapping is reused multiple times, but the implementation collection varies. And you could even support implementations of the Collection framework that you do not know exist. (e.g. project specific ones)
For @ValueMappings I'm not that certain, if this is only enums then no. But if this is also static fields than the same thing could apply. Because static fields can refer to subclassed instances of the source.
It would require some more work to support the @IterableMapping, @MapMapping and @ValueMapping annotations with subclasses but I do believe that would be possible. Not certain as to how often it would be used if at all though, because the above is pure theoretical.

In 2.0 we could give those mappings a more " independent life", perhaps making even use of the fact that they are part of a @BeanMapping. But lets do that step-by-step.

Next, I was thinking how this functionality would interplay with the new MapToBean functionality. Do we have an idea 'bout that?

I think that could turn out to be an adjusted variant of the @SubclassMapping as is, but instead of supplying a source class you would supply a sourceCondition. If this sourceCondition would result in a true value a more specific MapToBean method would be called. Something for in another PR but here is the gist of it:

@SubclassMapping( sourceCondition="java(input.containsKey('subBeanSpecificField'))", target=SubBean.class )
Bean mapToBean(Map<String, String> input);

@Mapping(/*specific mapping for this condition*/
SubBean mapToSubBean(Map<String, String> input);

which could result in something like:

Bean mapToBean(Map<String, String> input) {
    if (input.containsKey('subBeanSpecificField')) {
        return mapToSubBean(input);
    }
    // default mapToBean behaviour
}

SubBean mapToSubBean(Map<String, String> input) {
    // mapToSubBean behaviour
}

anyway @Zegveld : what a nice piece of work on an issue which has a long history. Really looking forward to work with you on more issues in the future.

Likewise.

and also what you think of my solution for the annotation handling order one.

@Zegveld you mean the refactoring you did, right? I found it really great, already said that in #2512 (comment) wink .

@filiphr, no I did a refactor which changes several Set's into specifically LinkedHashSet's. Was talking about that one.

@Zegveld I've been a bit busy, so I haven't gotten around in finishing the review with your last changes. I haven't forgotten about this one yet.

No problem I've got plenty of waiting time. Currently implementing just a lot of instanceof checks manually when I'm using it instead of generating them. :)

I didn't think about this initially, but it would be possible to do submappings for iterables or maps as well. This would be useful if the mapping is reused multiple times, but the implementation collection varies. And you could even support implementations of the Collection framework that you do not know exist. (e.g. project specific ones)

You have a point here. Did not consider that. I think @filiphr and I were discussing the change of the default implementation (don't remember if it was Map or List) a while ago. And this would open up changing it back or having your own preferred default implementation.

For @ValueMappings I'm not that certain, if this is only enums then no. But if this is also static fields than the same thing could apply. Because static fields can refer to subclassed instances of the source.

That would be really a niche feature.

I was discussing mis-use of annotations with @filiphr just earlier and he pointed out that we don't have (presumably) checks for @Mapping either. So we're a bit half-hearted with checks here anyway.

Finally, applying it to the new map-to-bean would indeed qualify as a follow up PR.

So, as far as I'm concerned: go ahead.

I checked out the PR locally today and did some changes in it. I've pushed the changes to https://github.com/filiphr/mapstruct/tree/feature/PR-131 (I couldn't push it to your branch for some reason, most likely you have not allowed maintainers to push changes to it). Can you please have a look at them and let me know whether you agree with them @Zegveld? I tried to do them atomically in order to show my thought process for some of them, everything will be merged into one commit in the end.

@filiphr, no I did a refactor which changes several Set's into specifically LinkedHashSet's. Was talking about that one.

I saw that one, and I didn't really like it. I do not like having collection implementation types as fields of classes or parameters. I reverted some of that change in a separate PR.

Some other more notable changes I did are:

• We do not support all update methods, not just the ones that are void. Otherwise it is too confusing
• Small Refactoring in the RepeatableMappingAnnotations to reduce the complexity a bit
• Remove obsolete changes in MapperCreationProcessor

Some final things to address before merging:

• I am not sure that the throwing of the IllegalArgumentException is a good approach. The reason for that is that if you remove the SubclassMapping(s) we will have a compile error, otherwise we throw a runtime error. Can we keep this behaviour consistent? In a follow up PR we can add support for throwing the exception if the user wants it by explicitly asking for it via @BeanMapping and in another PR to support sealed classes, i.e. we can know whether the SubclassMapping(s) have all classes for the source or not.

I think that this would be the last change I request, together with my changes the PR would be ready for merging and doing a beta release shortly after that. It would be a great addition to the 1.5 release 😄

Side note: Since your email address is private, can you please reach out to me somehow? You could use Twitter or my email address (it is in my commits). I'd like to discuss something with you.

I've looked through the changes and they are indeed an improvement as to what I did.
The bit about the Set we could discuss, but that shouldn't hinder this PR.

Concerning the IllegalStateException:
So if I understand it correctly, since this is generated code it is better to throw an UnsupportedSubclassError that extends Error (Edit: or perhaps a NotImplementedError?) instead of an IllegalStateException?
Did some reading up on the definition of Error vs RuntimeException. I've never used error before but then again this is my first time interacting at compiler-level.
I think that would indeed be an improvement here.

side note: I've sent you an e-mail.

Concerning the IllegalStateException:
So if I understand it correctly, since this is generated code it is better to throw an UnsupportedSubclassError that extends Error (Edit: or perhaps a NotImplementedError?) instead of an IllegalStateException?
Did some reading up on the definition of Error vs RuntimeException. I've never used error before but then again this is my first time interacting at compiler-level.
I think that would indeed be an improvement here.

That's not what I had in mind. All of that is runtime behaviour and not compile time behaviour. From a runtime point of view the runtime the IllegalArgumentException is OK. However, from a point of view of MapStruct, the way our users use it, I do not think that we need to have such behaviour on the runtime. We are more towards compile time safety and telling the user that they have made a mistake.

What I had in mind is that we do not do some of the changes you did in the BeanMappingMethod such as the hasSubclassMappingsAndIsEitherAbstractOrCanBeConstructed and we keep everything as it was. That way the user will get a compile error saying something like

• Fruit does not have an accessible constructor.
• The return type Fruit is an abstract class or interface. Provide a non abstract / non interface result type or a factory method.

Once we have support for sealed types we can avoid that part if all of them have been covered.

In order to see what I mean you can remove the @SubclassMapping for the AbstractSuperClassTest, SubclassFixtureTest and SubclassMappingTest and see what happens. I think that we need to keep that behaviour consistent. We could offer a possibility to throw the exception that is now thrown through something on the BeanMapping.

side note: I've sent you an e-mail.

Thanks for reaching out to me.

I did take that route under consideration, but the use case that I had which caused me to write this enhancement was one where I never have an implementation for general usage. In this use case the classes are generated from two xsd's and I need to transfer information between the two models.

The best solution I can think of usage side without having the exception would be to manually create an implementation/extension of the base class which then would throw an IllegalStateException in it's constructor because it should not be created. That one would then be the default implementation that mapstruct falls back to. This does not feel that good as a solution for this problem.

What if we add another annotation @SubclassMappingStrategy in which you can configure whether you get an exception or compile error. Hmmm maybe with a field/property named .... can't think of a good name for the property atm. But that property would then influence the behavior. Just like how the other MappingStrategy annotations do.

What if we add another annotation @SubclassMappingStrategy in which you can configure whether you get an exception or compile error. Hmmm maybe with a field/property named .... can't think of a good name for the property atm. But that property would then influence the behavior. Just like how the other MappingStrategy annotations do.

That's what I head in mind when I was saying about the BeanMapping, but I couldn't come up with a good name 😄 . Your naming is really good. I think that for now we can expose subclassMappingStrategy or subclassExhaustiveStrategy and there we can have COMPILE_ERROR (as default) and RUNTIME_EXCEPTION as enums. This can be exposed to @MapperConfig, @Mapper and @BeanMapping, same as the other strategies. The exhaustive comes from the Java Sealed Classes JEP 409. I think that this would also solve your use case, by you configuring your mappers to have SubclassExhaustiveStrategy.RUNTIME_EXCEPTION.

We have ideas for the future (2.0) to have a different approach for our APIs and have a single @SubclassMappingStrategy as an annotation instead, but let's not do that now.

@filiphr , I've introduced the SubclassExhaustiveStrategy and did some more renaming/refactoring. Also updated the documentation to have it mentioned.
Hope you have time to review the new changes.

And done one of our oldest feature request has been implemented. Thanks a lot for the huge effort you put in this @Zegveld.