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

Suggest property name in error message when referring to a non-existent property in @Mapping #122

Closed
gunnarmorling opened this Issue Feb 3, 2014 · 6 comments

Comments

Projects
None yet
3 participants
@gunnarmorling
Member

gunnarmorling commented Feb 3, 2014

The AP raises an error in case a non-existent property is referenced via @Mapping#source() or target(). It would be awesome to display the name of the most similar existing property in the error message:

"No property 'foubar' exists. Did you mean 'foobar'?"
@sjaakd

This comment has been minimized.

Show comment
Hide comment
@sjaakd

sjaakd Feb 3, 2014

Contributor

What about generating a template (of the mapper interface) in the generated sources completing the target mapper interface with all suggestions?

Contributor

sjaakd commented Feb 3, 2014

What about generating a template (of the mapper interface) in the generated sources completing the target mapper interface with all suggestions?

@gunnarmorling

This comment has been minimized.

Show comment
Hide comment
@gunnarmorling

gunnarmorling Feb 3, 2014

Member

I'm not sure I'm following. What would that template look like?

What I had in mind was just a more helpful error message (or diagnostic in AP terms) which gives a hint about possibly meant properties. Similar to what git does when one mistypes a git command.

Member

gunnarmorling commented Feb 3, 2014

I'm not sure I'm following. What would that template look like?

What I had in mind was just a more helpful error message (or diagnostic in AP terms) which gives a hint about possibly meant properties. Similar to what git does when one mistypes a git command.

@sjaakd

This comment has been minimized.

Show comment
Hide comment
@sjaakd

sjaakd Feb 3, 2014

Contributor

Take for instance the following mapper:

public interface SourceTargetMapper {

    SourceTargetMapper INSTANCE = Mappers.getMapper( SourceTargetMapper.class );

    @Mappings({
        @Mapping(source = "propX", target = "propXcd"),
        @Mapping(source = "propY", target = "propYc")
    })
    Target sourceToTarget(Source source);

    Source targetToSource(Target target);
}

Now suppose that I made an error in the: @Mapping(source = "propY", target = "propYc") mapping (target should have been propYcd. I missed propZ all together. Wouldn't it be cool if MapStruct generates a file: SourceTargetMapper.template:

public interface SourceTargetMapper {

    SourceTargetMapper INSTANCE = Mappers.getMapper( SourceTargetMapper.class );

    @Mappings({
        @Mapping(source = "propX", target = "propXcd"),
        @Mapping(source = "propY", target = "propYcd"), // suggestion
        @Mapping(source = "propZ", target = "propZcd") // missing?
    })
    Target sourceToTarget(Source source);

    Source targetToSource(Target target);
}

This "template" can then be use to adapt the original SourceTargetMapper. Heck, it could even be used to generate a first attempt :). I noticed (in my case) that creating a mapper is a tedious job. Especially when its a big structure. That takes away much of the advantage of MapStruct.

Many time you don't have control over both the source and the target objects (but properties are awfully similar in name).

You could even propose mappings such as I requested in #117.

Such a suggestion feature could be turned on / off by means of a configuration param (note the .template does not harm in the generated sources, but its also nice if you could do without).

Contributor

sjaakd commented Feb 3, 2014

Take for instance the following mapper:

public interface SourceTargetMapper {

    SourceTargetMapper INSTANCE = Mappers.getMapper( SourceTargetMapper.class );

    @Mappings({
        @Mapping(source = "propX", target = "propXcd"),
        @Mapping(source = "propY", target = "propYc")
    })
    Target sourceToTarget(Source source);

    Source targetToSource(Target target);
}

Now suppose that I made an error in the: @Mapping(source = "propY", target = "propYc") mapping (target should have been propYcd. I missed propZ all together. Wouldn't it be cool if MapStruct generates a file: SourceTargetMapper.template:

public interface SourceTargetMapper {

    SourceTargetMapper INSTANCE = Mappers.getMapper( SourceTargetMapper.class );

    @Mappings({
        @Mapping(source = "propX", target = "propXcd"),
        @Mapping(source = "propY", target = "propYcd"), // suggestion
        @Mapping(source = "propZ", target = "propZcd") // missing?
    })
    Target sourceToTarget(Source source);

    Source targetToSource(Target target);
}

This "template" can then be use to adapt the original SourceTargetMapper. Heck, it could even be used to generate a first attempt :). I noticed (in my case) that creating a mapper is a tedious job. Especially when its a big structure. That takes away much of the advantage of MapStruct.

Many time you don't have control over both the source and the target objects (but properties are awfully similar in name).

You could even propose mappings such as I requested in #117.

Such a suggestion feature could be turned on / off by means of a configuration param (note the .template does not harm in the generated sources, but its also nice if you could do without).

@gunnarmorling

This comment has been minimized.

Show comment
Hide comment
@gunnarmorling

gunnarmorling Feb 3, 2014

Member

Yes, one could think about having a "one-shot" generator which creates a first draft for a mapper interface. One big question is how one would find the corresponding type, i.e. how to know that Source should be mappable into Target.

Generally I'm a bit skeptical how well something like that would work out in the end, because deciding which types/properties need to be mapped to each other should always be a conscious decision of the user; MapStruct then helps with implementing that configured mapping as easily as possibly.

It's definitely a different thing than I'm having in mind with this issue, which just should create more meaningful error messages.

Member

gunnarmorling commented Feb 3, 2014

Yes, one could think about having a "one-shot" generator which creates a first draft for a mapper interface. One big question is how one would find the corresponding type, i.e. how to know that Source should be mappable into Target.

Generally I'm a bit skeptical how well something like that would work out in the end, because deciding which types/properties need to be mapped to each other should always be a conscious decision of the user; MapStruct then helps with implementing that configured mapping as easily as possibly.

It's definitely a different thing than I'm having in mind with this issue, which just should create more meaningful error messages.

@sjaakd

This comment has been minimized.

Show comment
Hide comment
@sjaakd

sjaakd Feb 4, 2014

Contributor

I don't believe in a true "one-shot' generator either. But channeling our output in such a fashion that it ends up in extending / improving the annotations in an existing mapper and aiding the user could be a real benefit. Now, obviously we cannot do that in the source code itself, hence the introduction of a .template in the generated-sources.

Don't get me wrong.. I think it is a very good idea to provide meaningful hints. 👍

However, now, they are 'just' messages that we report as warnings and errors. A first step could for instance be introducing - and including the errors and warnings on the spot that they occur in the model (e.g. the mapper to be generated). So in essence, treating them similar as all other model elements we discover in the creation process. Then a message can still be generated as is the case now.

In a later phase a new processor can be introduced, that takes the model again and writes either all errors to system.error or does something special with it (like the idea proposed here).
Anyway, the nice thing about MapStruct is that this can actually be done in a non-intrusive way. In any other run-time / reflection mapper implementation this is more difficult. So this could be a strong selling point :).

Contributor

sjaakd commented Feb 4, 2014

I don't believe in a true "one-shot' generator either. But channeling our output in such a fashion that it ends up in extending / improving the annotations in an existing mapper and aiding the user could be a real benefit. Now, obviously we cannot do that in the source code itself, hence the introduction of a .template in the generated-sources.

Don't get me wrong.. I think it is a very good idea to provide meaningful hints. 👍

However, now, they are 'just' messages that we report as warnings and errors. A first step could for instance be introducing - and including the errors and warnings on the spot that they occur in the model (e.g. the mapper to be generated). So in essence, treating them similar as all other model elements we discover in the creation process. Then a message can still be generated as is the case now.

In a later phase a new processor can be introduced, that takes the model again and writes either all errors to system.error or does something special with it (like the idea proposed here).
Anyway, the nice thing about MapStruct is that this can actually be done in a non-intrusive way. In any other run-time / reflection mapper implementation this is more difficult. So this could be a strong selling point :).

navpil added a commit to navpil/mapstruct that referenced this issue Apr 4, 2017

navpil added a commit to navpil/mapstruct that referenced this issue Apr 10, 2017

@filiphr filiphr added this to the 1.2.0.CR1 milestone Apr 20, 2017

@filiphr filiphr added the enhancement label Apr 20, 2017

navpil added a commit to navpil/mapstruct that referenced this issue May 10, 2017

navpil added a commit to navpil/mapstruct that referenced this issue May 10, 2017

navpil added a commit to navpil/mapstruct that referenced this issue May 15, 2017

navpil added a commit to navpil/mapstruct that referenced this issue May 19, 2017

filiphr added a commit to filiphr/mapstruct that referenced this issue May 20, 2017

filiphr added a commit to filiphr/mapstruct that referenced this issue May 20, 2017

filiphr added a commit to filiphr/mapstruct that referenced this issue May 20, 2017

General code cleanups:
* #122 Use util methods when possible
* Fix some warnings in Javadoc generation
* Don't use raw classes when not needed
* Add .yml, binding.xjb and .asciidoc files to license check exclusion
@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr May 20, 2017

Member

Fixed by #1168

Member

filiphr commented May 20, 2017

Fixed by #1168

@filiphr filiphr closed this May 20, 2017

This was referenced May 20, 2017

filiphr added a commit that referenced this issue May 24, 2017

#1213 General code cleanups:
* #122 Use util methods when possible
* Fix some warnings in Javadoc generation
* Don't use raw classes when not needed
* Add .yml, binding.xjb and .asciidoc files to license check exclusion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment