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

Ability to specify Field Annotations that gets taken over with @XArgsConstructor #1528

Open
marcus-held opened this issue Nov 30, 2017 · 11 comments

Comments

@marcus-held
Copy link

Hi,

I use Lombok together with Spring and I inject all dependencies via constructor injection. But because I'm lazy I don't want to write the constructor on my own and rather use one of the @XArgsConstructor's. But with Spring you might want to specify some dependecies by name. In order to do so you need to annotate the parameter in the constructor with @qualified("someName"). And here lies the problem, of course I can not use the generated constructor and annotate just the field, because the annotation must be on the parameter of the constructor. So what I would like to have is the ability to define specific annotations in the lombok.config file which are taken over from the field into the parameter of the constructor. That way I would be able to build up the behavior that feels more natural.

Here are some examples:

@RequiredArgsConstructor
public class WorksFine {

  private final SomeDependency dependency; // Gets injected by Spring

}
@RequiredArgsConstructor
public class DoesNotWork {

  @Qualifier("specificDependency")
  private final SpecificDependency dependency; // Can not find the dependency by the qualifier

}
// :'-( Missing annotation
public class CurrentWorkaround {

  private final SpecificDependency dependency; 

  public CurrentWorkaround(@Qualifier("specificDependency") SpecificDependency dependency) {
    this.dependency = dependency;
  }

}

Proposed Solution:
lombok.config

lombok.xArgsConstructor.fieldAnnotationTakeOver += org.springframework.beans.factory.annotation.Qualifier

java class:

@RequiredArgsConstructor
public class Works {

  @Qualifier("specificDependency")
  private final SpecificDependency dependency;

}

generated java class:

public class Works {

  private final SpecificDependency dependency;

  public Works(@Qualifier("specificDependency") SpecificDependency dependency) {
    this.dependency = dependency;
  }
}
@ElderMael
Copy link

ElderMael commented Dec 9, 2017

@mld-ger I think you are mistakenly using @Qualifier instead of @Resource. I quote from the documentation:

[...] if you intend to express annotation-driven injection by name, do not primarily use @Autowired, even if is capable of selecting by bean name among type-matching candidates [using @Qualifier ]. Instead, use the JSR-250 @resource annotation, which is semantically defined to identify a specific target component by its unique name, [...]

Anyway, the proposed example gives the problem of having no way to filter the annotations that you are going to add to the constructor arguments, e.g.:


@RequiredArgsConstructor
public class Works {

  @Qualifier("specificDependency")
  private final SpecificDependency dependency;

  @Getter
  private final someProperty;
}

Will result in:

public class Works {

  private final SpecificDependency dependency;

  private final Type someProperty;

  public Works(@Qualifier("specificDependency") SpecificDependency dependency,
                         @Getter Type someProperty ) {
    this.dependency = dependency;
  }

  // Other stuff
}

This won't compile, @Getter cannot be used in parameters because it is annotated with @Target(value={FIELD,TYPE})

@marcus-held
Copy link
Author

marcus-held commented Dec 11, 2017

Thanks for your response @ElderMael.
Maybe through the given example you could assume, that @Resource is the annotation to go with, but in the end the problem that I describe stays the same. I want to use constructor injection and I might need to annotate the parameters of the constructors to further specify them. As far as I know @Resource only works with field injection and that`s not what I want to do.

Anyway, the proposed example gives the problem of having no way to filter the annotations that you are going to add to the constructor arguments, e.g.:

In my proposed example I spoke about specifying the annotations that will be taken over to the constructor via a lombok.config. In my example it could look like this:

lombok.xArgsConstructor.fieldAnnotationTakeOver += org.springframework.beans.factory.annotation.Qualifier

This would specify exactly which annotations should go on the constructor and not remain on the field itself (see the example I gave above, you might overlooked that). So having this specification in the config would result in the following with your example:

public class Works {

  private final SpecificDependency dependency;

  @Getter
  private final Type someProperty;

  public Works(@Qualifier("specificDependency") SpecificDependency dependency,
                          Type someProperty ) {
    this.dependency = dependency;
  }

  // Other stuff
}

@Maaartinus
Copy link
Contributor

This is a duplicate of #1335, isn't it?

@marcus-held
Copy link
Author

Thanks @Maaartinus for pulling this out. When I searched for the issue I didn't see that one. But in its essence its the same request, just that I proposed the config approach that gets discussed in this issue at a later point as well.

@ptanov
Copy link

ptanov commented Apr 2, 2020

This will be useful when using @JacksonInject. Without it we need to create explicit construct and put @JacksonInject next to the parameter, instead to the property. Check this passing test:

@Value
protected static class Some {
	@NonNull
	private final String field1;

	@NonNull
	@JacksonInject(value = "defaultValueForField2", useInput = OptBoolean.TRUE)
	private final String field2;
}

@Test
public void testReadValueInjectablesIncorrectBehavior() throws JsonParseException, JsonMappingException, IOException {
	final ObjectMapper mapper = new ObjectMapper();
	final InjectableValues injectableValues =
			new InjectableValues.Std().addValue("defaultValueForField2", "somedefaultValue");
	mapper.setInjectableValues(injectableValues);

	final Some actualValueMissing = mapper.readValue("{\"field1\": \"field1value\"}", Some.class);
	assertEquals(actualValueMissing, new Some("field1value", "somedefaultValue"));

	final Some actualValuePresent =
			mapper.readValue("{\"field1\": \"field1value\", \"field2\": \"field2value\"}", Some.class);
	assertEquals(actualValuePresent.getField1(), "field1value");
	// unfortunately "field2value" is overrided because of putting "@JacksonInject" to the field
	assertEquals(actualValuePresent.getField2(), "somedefaultValue");
}

so we need to explicitly create constructor and put @JacksonInject there in order to have correct functionality of Jackson:

@Value
protected static class Some {
	@NonNull
	private final String field1;

	@NonNull
	private final String field2;

	// we need to write it manually:
	public Some(@JsonProperty("field1") final String field1,
			@JsonProperty("field2") @JacksonInject(value = "defaultValueForField2",
					useInput = OptBoolean.TRUE) final String field2) {
		this.field1 = requireNonNull(field1);
		this.field2 = requireNonNull(field2);
	}
}

@Test
public void testReadValueInjectablesCorrectBehavior() throws JsonParseException, JsonMappingException, IOException {
	final ObjectMapper mapper = new ObjectMapper();
	final InjectableValues injectableValues =
			new InjectableValues.Std().addValue("defaultValueForField2", "somedefaultValue");
	mapper.setInjectableValues(injectableValues);

	final Some actualValueMissing = mapper.readValue("{\"field1\": \"field1value\"}", Some.class);
	assertEquals(actualValueMissing, new Some("field1value", "somedefaultValue"));

	final Some actualValuePresent =
			mapper.readValue("{\"field1\": \"field1value\", \"field2\": \"field2value\"}", Some.class);
	// correct behavior:
	assertEquals(actualValuePresent, new Some("field1value", "field2value"));
}

see https://stackoverflow.com/a/60987798/2054634 for more information on this problem and why this happens.

P.S. I'm using lombok v. 1.18.8 and jackson v. 2.9.6

@janrieke
Copy link
Contributor

janrieke commented Apr 2, 2020

You can add that annotation to the list of copyable annotations using a lombok.config:

lombok.copyableAnnotations += com.fasterxml.jackson.annotation.JacksonInject

@ptanov
Copy link

ptanov commented Apr 3, 2020

First - thanks for your answer.

Yes, I know about this configuration, but when it is used then annotation will not be removed from the property so the same problem will happen (somedefaultValue will be set to field1value even there is such element in the json).
Here is de-lomboked class with copyableAnnotation set:

public final class Some {
	@NonNull
	private final String field1;
	@NonNull
// still here:
	@JacksonInject(value = "defaultValueForField2", useInput = OptBoolean.TRUE)
	private final String field2;

	@java.beans.ConstructorProperties({"field1", "field2"})
	@java.lang.SuppressWarnings("all")
	@javax.annotation.Generated("lombok")
	@lombok.Generated
	public Some(@NonNull final String field1, @NonNull @JacksonInject(value = "defaultValueForField2", useInput = OptBoolean.TRUE) final String field2) {
		if (field1 == null) {
			throw new java.lang.NullPointerException("field1 is marked non-null but is null");
		}
		if (field2 == null) {
			throw new java.lang.NullPointerException("field2 is marked non-null but is null");
		}
		this.field1 = field1;
		this.field2 = field2;
	}
//...
}

@janrieke
Copy link
Contributor

janrieke commented Apr 3, 2020

That does not work? Sounds like a bug in Jackson to me, because it works for all other annotations. I don't see anything special why it should not work in this case.

Moving the annotation instead of copying is probably not a very common use case. Furthermore, it's not that straightforward: What happens when you have more than one possible target, for example, @AllArgsConstructor, @RequiredArgsConstructor, @Setter, @Builder?

@ptanov
Copy link

ptanov commented Apr 3, 2020

Yes, this does not work (actually I have set this setting for the tests I mentioned). Probably you are right for not so straightforward moving.

@janrieke
Copy link
Contributor

janrieke commented Apr 3, 2020

I suggest you file an issue at the Jackson project for that. Similar Jackson annotations can be duplicated and still work, so this is quite surprising to me. Let's see what the Jackson people say.

@ptanov
Copy link

ptanov commented Apr 4, 2020

Thanks for the suggestion, I created an issue (FasterXML/jackson-databind#2678)

btw what about using onMethod=@__({@AnnotationsHere}) in @Setter (there is no onConstructorParameter) to annotate the parameter in the constructor (instead of moving)?

Thanks for your quick feedback!

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

No branches or pull requests

5 participants