To pick up a question from the issue discussion:

[...] Also I would like to to see what happens if you cannot make a direct mapping from a map to a target.. E.g. is an available mapping method / typeconversion called correctly? [...]

Yes it is, it worked out of the box. In the unit test i added, the method String map(Object object) is called correctly. To map from the Maps values to the String properties of the target object. But i will add some more unit tests.

@sjaakd since i am using mapstruct in projects always for the same (rather narrow) scenario, i am lacking creativity here regarding the unit tests. Also i am relatively new to mapstructs code base so i don't know where "the Pain" is.

If you have any suggestions for further important unit test scenarios, just mention them here, i will add them then.

I was thinking, one of the things I considered was the type you cast it to obtaining from the map (I was considering a new field on @mapping) as input for target property mapping) .

But before you dive into that I should first do a proper review. In the mean time I think a bit more on it. (Not behind my computer now)

@ckosmowski I've thought a bit more on the unit test..

First some comments on the current UT.

1. Try to limit the scope of a test. That makes troubleshooting a whole lot more easy. The strategy we use nowadays is to include a mapper with inner classes (the source and the target). See for an example https://github.com/mapstruct/mapstruct/tree/master/processor/src/test/java/org/mapstruct/ap/test/bugs/_2253. You only need to include the mapper, the static member classes are imported automatically.
2. Stick the @WithClasses on the method instead of on the entire test if you have more than one test method
3. limit the amount of fields to one if the test does not demand more.
4. Name the mapper to what you want to test (line up with the test method).

Then some ideas for testcases:

1. test calling mapping methods, so
2. test source side nesting
3. constant test
4. presence checker tests (including defaults)
5. error scenarios: what if you cannot find a proper mapping method / type conversion?
6. test different target accessing (field, setter, builder, constructor)
7. test multiple sources (also 2 maps), address the proper source by means of including the paramter name.
8. dependends on constructions
9. mix strategies (ADDER on the target).
10. ..

Okay, thanks for the details, will carry on with the unit tests next year.

Happy new Year!

Please let me know, if i am on the right track with the overall fashion of the unit tests. I'd further need some clarification on the following topics:

By source side nesting do you mean Maps in Maps on the source side? And if yes, what would be the desired behaviour? I would expect mapstruct to call a mapping method if present to convert from the nested Map to the target type.

Regarding the error scenarios, if there is no proper mapping method, the usual mapstruct behaviour kicks in (Reporting that a value cannot be converted due to a missing conversion method). Just to be sure, is the correct way of testing this still the @ExpectedCompilationOutcome annotation?

And last but not least:

dependends on constructions
mix strategies (ADDER on the target).

I don't know what that could be about :-) Is it possible, that you point me to sections in the documentation regarding this features?

@ckosmowski sorry it takes some time, but first some quick answers

By source side nesting do you mean Maps in Maps on the source side? And if yes, what would be the desired behaviour? I would expect mapstruct to call a mapping method if present to convert from the nested Map to the target type.

No.. I meant that you could store beans that refer to beans.. e.g.

Car {
   
   private Steer steer;

  // getters /setters
}

Steer {

    private boolean isRightHanded;
   // getters / setters
}

So you would first call a getter and next generate methods (which should happen automatically)..

Regarding the error scenarios, if there is no proper mapping method, the usual mapstruct behaviour kicks in (Reporting that a value cannot be converted due to a missing conversion method). Just to be sure, is the correct way of testing this still the @ExpectedCompilationOutcome annotation?

I need to think of some scenarios.. But, in essence this is the type of tests I was after:

https://github.com/mapstruct/mapstruct/blob/master/processor/src/test/java/org/mapstruct/ap/test/collection/erroneous/ErroneousCollectionMappingTest.java

Happy new Year!

Of course.. the same to you (although somewhat late from my side)

An interesting approach @ckosmowski. I like the idea.

A small note from me, from what I understood this only works when the source parameters are maps not when mapping nested sources. In addition to that this works one sided as well (from Map to Bean), I think it would be a good think to support the reverse as well, although that might warrant a new PR.

@sjaakd regarding

I was thinking, one of the things I considered was the type you cast it to obtaining from the map (I was considering a new field on @mapping) as input for target property mapping) .

But before you dive into that I should first do a proper review. In the mean time I think a bit more on it. (Not behind my computer now)

I would not like to expand the @Mapping in order to support this. I think that we should reuse as much as possible of what is there and treat it as normal mapping. When you try to map from Map<String, Object> most likely we would need people to provide method for mapping from Object to the desired type.

An interesting approach @ckosmowski. I like the idea.

A small note from me, from what I understood this only works when the source parameters are maps not when mapping nested sources.

I'm sorry for that, but i still don't really get what you guys mean by "nested sources". Could you possibly provide an example of a mapping scenario that would represent "nested sources". If this term refers to the example that @sjaakd showed here (Car, Steer) i don't see why this shouldn't work if such a car object is used as a mapping source we should be able to refer to the nested properties with @mapping(source="car.steer.isRightHanded") i'll give this a try. If you mean something else, please let me know.

In addition to that this works one sided as well (from Map to Bean), I think it would be a good think to support the reverse as well, although that might warrant a new PR.

Yes, i thought about hitting two birds with one stone but since the target type is different with this one it was not possible to just "sneak" into the normal bean mapping process wo the other way round would need more work i guess.

I would not like to expand the @Mapping in order to support this. I think that we should reuse as much as possible of what is there and treat it as normal mapping. When you try to map from Map<String, Object> most likely we would need people to provide method for mapping from Object to the desired type.

I also like this idea, because it works already :-)

I re-read the comment from @sjaakd and I think I was thinking about something else.

I had the following in mind:

Target mapping

Car {
   
   private Steer steer;

  // getters /setters
}

Steer {

    private boolean isRightHanded;
   // getters / setters
}

Source

CarDto {
   
   private Map<String, String> steer;

  // getters /setters
}

And the mapper will look like:

@Mapper
public interface CarMapper {

    Car map(CarDto car);

}

Maybe you already have a test for this and I've missed it. If you don't it would be good to add it (maybe it even works out of the box with your changes 😄).

Yes, i thought about hitting two birds with one stone but since the target type is different with this one it was not possible to just "sneak" into the normal bean mapping process wo the other way round would need more work i guess.

Thinking more about this it feels like a bit more work and it can't really be "sneaked" into this one. Most likely we will need a dedicated mapping method (similar to the BeanMapping for this).

I'm sorry for that, but i still don't really get what you guys mean by "nested sources". Could you possibly provide an example of a mapping scenario that would represent "nested sources". If this term refers to the example that @sjaakd showed here (Car, Steer) i don't see why this shouldn't work if such a car object is used as a mapping source we should be able to refer to the nested properties with @mapping(source="car.steer.isRightHanded") i'll give this a try. If you mean something else, please let me know.

I guess that is more or less what I meant.. And I think you are right.. You are getting in the realm of default behaviour So to extend on my example:

Car {
   
   private Steer steer;

  // getters /setters
}

Steer {

    private boolean rightHanded;
    private BigDecimal maxDegreesTurn;
   // getters / setters
}

VehiclesDto{

   CarDto car;
   // getters / setters
}

CarDto {
    private boolean steerRightHanded;
     private BigDecimal steerMaxDegreesTurn;
  // getters / setters
}

public interface MyMapper{

     // I guess that car is a key in the map, MapStruct should generate methods to flatten steer.rightHanded and steer.maxDegreesTun
     @Mapping( target = "steerRightHanded", source = "car.steer.rightHanded" ) 
     @Mapping( target = "steerMaxDegreesTurn", source = "car.steer.maxDegreesTurn" ) 
     VehiclesDto mapAlt1( Map<Object, Object> vehicles);

     // including the parameter name as well
     @Mapping( target = "steerRightHanded", source = "vehicles.car.steer.rightHanded" ) 
     @Mapping( target = "steerMaxDegreesTurn", source = "vehicles.car.steer.maxDegreesTurn" ) 
     VehiclesDto mapAlt2( Map<Object, Object> vehicles);


}

mapAlt2 might be interesting. But in the end, it works probably out of the box. I'll fetch your branch and have a more detailed look at the implementation soon. Spent too many hours behind my laptop already today 😄

Maybe you already have a test for this and I've missed it. If you don't it would be good to add it (maybe it even works out of the box with your changes 😄).

I did not have a test for this, but i added one. It actually works because the original mapping method will find a property of type Map, and tries to find a second mapping method to convert from Map to Steer. Such a method could either be provided by the user (as with other type conversion methods) or only be mentioned in the interface and generated by mapstruct, both ways seem to work.

Of course it would be cool if mapstruct would just generate that method on its own without needing to specify the signature in the mapper but as it is, it feels to me like it blends in with the way other type conversions are handled in mapstruct.

Of course it would be cool if mapstruct would just generate that method on its own without needing to specify the signature in the mapper but as it is, it feels to me like it blends in with the way other type conversions are handled in mapstruct.

That is why you added I guess?

NestedTarget toNestedTarget(Map<String, String> nestedMap)

I would suggest looking at this where we are actually creating a forged mapping. Maybe you can create the intermediate method there.

I would suggest looking at this where we are actually creating a forged mapping. Maybe you can create the intermediate method there.

Thanks for the pointer, i tried some forging.

Great that looks great.

However, I think that we need to still honour some of the conditions in forgeMapping.

i.e. in

            if ( forgedNamedBased && !canGenerateAutoSubMappingBetween( sourceType, targetType ) ) {
                return null;
            }

Perhaps we need to always allow mapping from Map. Have a look in the MappingBuilderContext#canGenerateAutoSubMappingBetween

Perhaps we need to always allow mapping from Map. Have a look in the MappingBuilderContext#canGenerateAutoSubMappingBetween

If i would configure mapstruct to disable automatic sub mapping generation personally i would expect it to also omit the generation for map to bean methods as well. Is there a reason you think that this is a distinct case?

This method is called after doing the global check.

The global check is in AbstractBaseBuilder#canGenerateAutoSubMappingBetween

I can't seem to find a code path from here:

f439ab2#diff-be414f6739f2aa2771446f9ee61063cec75ddec970ffbe433164a93da222eacbR299

to MappingBuilderContext#canGenerateAutoSubMappingBetween

It seems like for BeanMappingMethods this is not checked. Am i correct? If i debug the annotation processor with my testcase it never stops there. So it works for the test case, and it should not affect the auto generetation if i understand the code correctly.

Hello again, i managed to reserve some Sprint time for this feature. What could be the next steps to help this feature progress further.

@ckosmowski I actually meant that the source would go in the map as a nested source (not as an additional source parameter).

This is the dilema that I thought about and why we need an additional parameter on @Mapping. @filiphr I know you don't want this.. But I don't really see another solution save from not building it. Checkout the comments.

@Mapper
public interface MapToBeanFromNestedSource {

    // I guess that car is a key in the map, MapStruct should generate methods to flatten steer.rightHanded and steer.maxDegreesTun. This is something you could do
    @Mapping( target = "car.steerRightHanded", source = "car.steer.rightHanded" ) // the target is a boolean, perhaps we should do a cast to boolean
    @Mapping( target = "car.steerMaxDegreesTurn", source = "car.steer.maxDegreesTurn" ) // the target is a BigDecimal in this case, perhaps we should do a cast to a BigDecimal
    VehiclesDto mapAlt2(Map<String, Object> vehicles);
    
    // however, this one is more tricky. How would MapStruct know that it should generate a mapping method CarDto toCar(Car car)?. It can't, because it cannot
    // establish the type in the source. Therefore, you would need an additional parameter to determine the type in the map.
    @Mapping ( target = "car", source = "car.steer", contentType = Car.class )
    VehiclesDto mapAlt2(Map<String, Object> vehicles);
    
    
    class Car {

        Steer steer;

        // getters /setters
    }

    class Steer {

        boolean rightHanded;
        BigDecimal maxDegreesTurn;
        // getters / setters
    }

    class VehiclesDto{

        CarDto car;
        // getters / setters
    }

    class CarDto {
        private boolean steerRightHanded;
        private BigDecimal steerMaxDegreesTurn;
        // getters / setters
    }

}

By the way: should we enforce the generic key being a String and generate an error when it isn't?

@sjaakd

I understand what you are looking for. I investigated some more in the code and i'd have a problem implicit conversion of properties of the Object type.

I managed to write a ConversionProvider that generates something like this:

Target target = new Target();

  if ( source.containsKey( "awesome" ) ) {
      target.setAwesome( (source.get( "awesome" ) instanceof java.lang.Boolean) ? (java.lang.Boolean) source.get( "awesome" ) : null );
  }
  if ( source.containsKey( "gradeOfAwesomeness" ) ) {
      target.setGradeOfAwesomeness( (source.get( "gradeOfAwesomeness" ) instanceof java.lang.Integer) ? (java.lang.Integer) source.get( "gradeOfAwesomeness" ) : null );
  }

The problems with this approach:

1. Many existing tests will fail, because they expect the compiler to fail and report an unmapped property.
2. If Objects cannot be cast to the target types it fails at runtime and what is the new value of the target property if it fails?
3. Everywhere else in the code, the user is expected to provide conversion methods in all cases where Object appears as the source type this would be an Exception for this use case only

If one would implement this casting Conversion it would be better to let the user opt-in via a setting on @Mapper don't you think? And if yes I'd appreciate some pointer on how to add new configuration properties.

With the second Mapping method (the nested one):

Unfortunately i don't have a clue on how to implement that kind of magic. What about intentionally restricting the feature to one level in the map? If someone demands this feature in the future and/or has an idea on how to solve this generically it could still be added as an extension?

If one would implement this casting Conversion it would be better to let the user opt-in via a setting on @Mapper don't you think? And if yes I'd appreciate some pointer on how to add new configuration properties.

The point that I tried to made is that you have to determine for each property what it is supposed to be. The only (logical) place to do that is in @Mapping. Take for instance the example of:

    @Mapping ( target = "car", source = "car.steer", contentType = Car.class )
    VehiclesDto mapAlt2(Map<String, Object> vehicles);

We (as in MapStruct out of the box or as in "the Mapper) can never decide that the entry in the map called Car has entry value Car.class. The only way we can cast, is to associate this in a 'hard' way to the @Mapping as is done in the example. The next entry in the Map might for all intents and purposes be a Mouse.class, a Form.class or an Organization.class just to mention some arbitrary things. How would you ever solve this in a provider? Point being: if you cannot directly derive the target type from the target property, you always need additional information.

What you can solve though (information that you do have) is if you've got a target property Cat.class you can cast it to Cat and hope the runtime value in the entry in the map is indeed a Cat.class. Its up to the user to assure this.

Now, considering this PR, I think we'll have to take it in steps. First we do the type safe solution as you did here. In other words, if the Map is of type Map<String, Integer> we can only map to Integer and leave the Map<String, Object> for a next PR.

Also: I still think you should only allow String as key (always). And that's for this current PR. We should enforce that and give a decent error message when someone tries something different.

@filiphr Can you agree with my reasoning? How do you see this?

@sjaakd
I do understand what you would like to do with this paramter, taking smaller steps is also good, but without the Map<String, Object> the whole feature is not useful to me

I would also consider a Map<String, Object> perfectly typesafe. Since Object is a type like anything else in Java,.

I do not fully understand what makes Object special in contrast to any other type mapstruct encounters throughout the mapping process.

If you had something like this:

class Target {
    private String source;
}

Target mapObject(Object source);

Mapstruct would just try to convert source to String correct?, it would not care about what the concrete Subclass of Object might be (if it has any at all);

The same situation without mentioning Object would be:

class Car extends Vehicle {
    private int streetCredibility;
}

class Bicycle extends Vehicle {
    private boolean savesTheEnvironment = true;
}

class Vehicle {  //extends Object
  private String brand;
}

class Target {
  public Car theCar;
}

Target mapObject(Vehicle theCat);

Mapstruct wouldn't know if the Vehicle is really car or not, it would try to find a conversion (which is not built in) and would then search for a mapping method. It would not go for an upcast. So what is the difference if this situation appears in a Map instead?

It's the same Situation for Map<String, Number> imagine that map would contain Doubles, Floats, BigDecimals and what not. And you were trying to map this to a target class which has integer properties only.

As of now in such a case in mapstruct you would not specify the concrete sublass of the source in order to be able to forge an implicit mapping method for it.

So, yes if you would need to forge that mapping method implicitly one would need to specify the concrete sublass like you describe, but i personally do not think that this is related to this PR or the feature of using Maps as the mapping source. I would personally see the ability of specifying the expected conrete sublasses as a new feature request all by itsself since it could be useful everywhere else this situation appears.

The current behaviour would be to warn the user about an incompatible mapping (which is a great behaviour of mapstruct here i think) and giving the user the opportunity of specifying mapping methods himself which map from Object to the desired target classes. (Which is really cool already, works like a charm, and is one of the features why we love mapstruct so much).

Regarding the Key: Yes there should be a message if it is not of Type String.

By the way @sjaakd @filiphr especially in such longer discussions think it is important to say thank you for mapstruct, and thank you for spending your time supporting this PR.

Mapstruct would just try to convert source to String correct?, it would not care about what the concrete Subclass of Object might be (if it has any at all);

MapStruct can actually make a selection choice on "inheritance distance" for a proper mapping method to map the source to target type in the selection process, in this case to get from "source" to String. But that is (perhaps) a little bit besides the point in this case.

So, consider your example, a little bit modified 😄

class Car extends Vehicle {
}

class Bicycle extends Vehicle {
}

class Vehicle {  
}

class Ford {  // obviously a car, but no inheritance relation to the type Car
}

class Target {
  public Ford  theCar;
}

And the following Mapper

@Mapper
interface MyMapper {


   @Mapping( target = "theCar" )
   Target map( Map<String, Vehicle> source);

    // helper method
   default Ford mapToFord( Car source ) {
         // some logic that maps a Car to a Ford
   }  
} 

How would you make clear to MapStruct that the Vehicle coming out of the map is in fact a Car and not a Bicycle in order for MapStruct to select Car as a source, calling mapToFord in order to complete the mapping?

So it's not about forging methods in this case in particular, its about selection of the proper type to the input process. Although, once MapStruct 'knows' you try to map a Car to a Ford and the attributes carry the same name, it probably can forge the method.

Now.. we have of course qualifiers.. So we could use a qualifer to force selecting mapToFord. But still, you'll have to upcast to Car then. This would by the way be tricky, since the current selectors work based on source / target type. And source type is exactly the unknown factor. But it would be resolvable.

Anyway.. I think you ask the right question: how much value would there be in Map<String, ?> (or type-erased: Map<String, Object> ? . I had more or less the SQL ResultSet in mind, but it has methods getString, getInteger etc to make the result type-safe. and moreover, it is a Set not a Map. Perhaps that other frameworks use such a construct.. So , I'm inclined to agree here: what is the use case?

Regarding the Key: Yes there should be a message if it is not of Type String.

👍

By the way @sjaakd @filiphr especially in such longer discussions think it is important to say thank you for mapstruct, and thank you for spending your time supporting this PR.

No problem.. We are happy that we're not the only guys contributing 😄 ..So thank you for just that. These discussions serve the purpose to get the an api that is usable by others as well.

Some nice discussions here, I like it :).

This is the dilema that I thought about and why we need an additional parameter on @Mapping. @filiphr I know you don't want this.. But I don't really see another solution save from not building it. Checkout the comments.

The car might very much be the key in the mapping, but it is also possible it is another Map<String, Object> or a CarDto or something else. I think that using such a contentType can lead to runtime exceptions, and I would like to avoid having such a possibility in MapStruct.

I agree with what @ckosmowski says in #2317 (comment). I would like to avoid having such instanceof checks in the generated code.

From #2317 (comment)

Now, considering this PR, I think we'll have to take it in steps. First we do the type safe solution as you did here. In other words, if the Map is of type Map<String, Integer> we can only map to Integer and leave the Map<String, Object> for a next PR.

I don't think that we have to do this. We should allow mapping from any value type. Since we already have the mechanism in MapStruct to pick up a method being mapped from one type in another one. A user can always write a qualified method to perform such a mapping.

e.g. (from the example in the comment)

    @Mapping ( target = "car", qualifiedByName = "toCarDto" )
    VehiclesDto mapAlt2(Map<String, Object> vehicles);

    @Named("toCarDto")
    default CarDto fromAlt2Object(Object object) {
        // Do your own magic here. Invoke other mappings methods after a cast
    }

I think that what I just said is the same summary by @ckosmowski in #2317 (comment).

Basically this:

I would also consider a Map<String, Object> perfectly typesafe. Since Object is a type like anything else in Java,.

This is also what I would expect to happen in such a scenario. I guess that this works out of the box already for this mappings, and if we don't have tests I think it would be good to add such tests to make sure we don't break it in the future as well.

The current behaviour would be to warn the user about an incompatible mapping (which is a great behaviour of mapstruct here i think) and giving the user the opportunity of specifying mapping methods himself which map from Object to the desired target classes. (Which is really cool already, works like a charm, and is one of the features why we love mapstruct so much).

Still from #2317 (comment) regarding

Also: I still think you should only allow String as key (always). And that's for this current PR. We should enforce that and give a decent error message when someone tries something different.

I agree with this and it makes sense, as everything else doesn't make much sense.

Now addressing #2317 (comment)

How would you make clear to MapStruct that the Vehicle coming out of the map is in fact a Car and not a Bicycle in order for MapStruct to select Car as a source, calling mapToFord in order to complete the mapping?

Well you wouldn't you will need to provide a mapping from Vehicle to Ford, otherwise MapStruct has no way to know your type. It is the same as mapping it without using a Map. I think that this is going in the direction of upcasting, which is an entirely different beast on its own.

The SQL ResultSet or using JsonNode from Jackson is a bit different that this as those have methods for extracting some simple types. That is a different feature and a different topic I would say.

By the way @sjaakd @filiphr especially in such longer discussions think it is important to say thank you for mapstruct, and thank you for spending your time supporting this PR.

Thanks a lot to you for the good discussion and for working on this feature. We love such discussions and we really appreciate all our contributors 😄

Hey @ckosmowski , @filiphr can I conclude that we agree on the API?

Concluding: we agree that Object should not be treated any different than any other (base) type. I think we need to do some work on the documentation though (perhaps on the FAQ as well). To explain the pattern.. WDYT?

@filiphr did you do a full review on the code?

I would say that we agree on the API. Maybe one more question. I see that currently the messages about the raw map or map not having a string parameter are only warnings. Doesn't it make sense those to be errors? And for backwards compatibility we need to only report an error if they are not mapped already as source parameters directly (e.g. used to map between the parameter and a target map).

And I agree with you @sjaakd, we need to add a section about this in the documentation as well.

I think that we have some tests missing as well. It would be good to have some tests like the examples we talked about in our discussions, especially when mapping from Object to another object.

@filiphr did you do a full review on the code?

Yes I did, all looks good from my side. There are some not needed code formatting changes, but I can fix that during the merge. And I think we need a bit more tests, I am not entirely sure that we cover everything.

A style / small thing: the convention in the new test cases is putting the target first. This is:

a. It reads better (the method return type is also left, the source right
b. The target is mandatory.

Closing this in favour of PR #2471 (which has the merge conflicts fixed and has some small polishing). @ckosmowski let me know if there is something that you do not agree with it. You'll get all the credit btw, since it is very well done 😄.

I am going close this.