Skip to content
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

Support @Named variables to address mapper inheritance when super is not located in same jar #1198

Closed
jmuis opened this issue May 10, 2017 · 8 comments
Labels

Comments

@jmuis
Copy link

jmuis commented May 10, 2017

Hi,

We have a case where we need to have a mapper interface with 2 different implementations, which implementation will be chosen runtime is basically dependent upon the way we need to retrieve additional data for the mapper (target property uses a java expression to fetch mapping data).

Because these 2 implementations resist in 2 different jars and the mapper interface in a 3rd jar reflection the actual parameter names get lots through the java compiler and end up as arg0, arg1, arg2, etc. Example mapper method: void mapMyData(Type1 arg0, Type2 arg2, ....) where the original interface specifies mapMyData(Type1 type1, Type2 type2, ... )

You will now have to use arg0, arg1, etc. in your java expression or you will get a compile error.
You can avoid this when using java 8 and adding parameter names compiler options, but we would like to avoid this very much.

Project structure:

  • api
    • interface my-mapper-interface(...) {}
  • implementation-variant-1
    +@mapping(target="...", expression="java(...)")
    interface mapper(...) extends my-mapper-interface {}
  • implementation-variant-2
    @mapping(target="...", expression="java(...)")
    • interface mapper(...) extends my-mapper-interface {}

Possible solution (will add a merge request later) is to use @nAmed annotations on the parameter, which would then be used by the annotation processor to not use the java generated parameter name (arg0) but the value of the @nAmed annotation.

jmuis added a commit to jmuis/mapstruct that referenced this issue May 10, 2017
@filiphr
Copy link
Member

filiphr commented May 10, 2017

As I wrote in the PR #1199 I find this feature really interesting and useful. The implementation looks pretty simple as well.

I would only prefer using some other annotation like @Param or something similar instead of the @Named annotation. @gunnarmorling, @agudian, @sjaakd what do you think?

@jmuis is using a different annotation acceptable for you?

@agudian
Copy link
Member

agudian commented May 13, 2017

Hmm, interesting. We talked about a similar problem in #847

@filiphr
Copy link
Member

filiphr commented May 14, 2017

@agudian where is this talked about in the linked PR? Sorry, I couldn't find it :(.

Are you talking about this:

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

If that is the case, it is similar, but not that much. I also didn't understand what was the consensus there. Did you agree on something or not?

@gunnarmorling
Copy link
Member

Hi @jmuis it's not quite clear to me yet how reflection plays into this. MapStruct works at compile time, so it should have access to the parameter names of the methods for which it generates implementations. Could you provide some more context perhaps? Thanks!

@gunnarmorling
Copy link
Member

@jmuis, got it now - when creating an implementation for a method defined in another JAR the parameter name will be lost. But as you said, Java 8 allows to retain parameter names. I'd strongly prefer to take advantage of that standard facility instead of adding our own solution. What is it that speaks against it?

@jmuis
Copy link
Author

jmuis commented May 18, 2017

Hi @gunnarmorling ,
First of all it adds extra size to the class metadata, but more importantly, we don't wish to 'leak' that kind of information from our implementation as you would expose all parameter names in the entire jar. Given the size and impact of the change we believe it's a welcome addition to the JDK8 option.

@sjaakd
Copy link
Contributor

sjaakd commented May 20, 2017

I see an added value.. Certainly in the context of #847 . I'm not sure whether we should use @Named for it for 2 reasons:

  1. @Named derives its use from CDI and other DI frameworks. It relates to methods and method selection.
  2. If we want to add arguments to the annotation in the future because of for instance #73 Implementing Immutable Constructors #847, they might not be applicable to the method selection use case and vice versa.

So I would argue that we need a new annotation. Since they would be in line in a method definition they should be short. What about @Arg?

@gunnarmorling
Copy link
Member

Hey, after some more discussion with the team we've decided to not support this for now. Java 8 provides a way to retain parameters in class files and this should be the preferred approach. I hear what you are saying about the class file size, but I don't think it'll make a significant difference in practice.

Should we end up adding a new naming annotation in the context of that issue mentioned by @sjaakd, we still can re-evaluate, but for now I'm going to close this issue.

Thanks nevertheless for coming up with the proposal and implementation, your efforts are much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants