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

Configuration processor does not handle generics #15850

Closed
garyrussell opened this issue Feb 5, 2019 · 5 comments
Closed

Configuration processor does not handle generics #15850

garyrussell opened this issue Feb 5, 2019 · 5 comments
Assignees
Labels
type: bug A general bug
Milestone

Comments

@garyrussell
Copy link
Contributor

Given

@ConfigurationProperties("foo.bar")
public class GenericProperties extends AbstractPropeties<Foo> {

}

and

public class AbstractPropeties<T> {

	private Map<String, T> props;

	public Map<String, T> getProps() {
		return props;
	}

	public void setProps(Map<String, T> props) {
		this.props = props;
	}

}

The generated metadata is

{
  "groups": [
    {
      "name": "foo.bar",
      "type": "com.example.GenericProperties",
      "sourceType": "com.example.GenericProperties"
    }
  ],
  "properties": [
    {
      "name": "foo.bar.props",
      "type": "java.util.Map<java.lang.String,T>",
      "sourceType": "com.example.GenericProperties"
    }
  ],
  "hints": []
}

Should be

{
  "groups": [
    {
      "name": "foo.bar",
      "type": "com.example.GenericProperties",
      "sourceType": "com.example.GenericProperties"
    }
  ],
  "properties": [
    {
      "name": "foo.bar.props",
      "type": "java.util.Map<java.lang.String,com.example.Foo>",
      "sourceType": "com.example.GenericProperties"
    }
  ],
  "hints": []
}

The generator should be able to determine the generic type.

Per Kris De Volder in Slack

Generics is complex, so I'm never surprised if it breaks stuff like this. But in my opinion it should be considered a bug. The method may be inherted from a generic superclass (or overridden). But from the subclass point of view we know that type 'T' is really 'RabbitBindingProperties'. So its reasonable to expect the Spring boot annotation processor to treat 'T' as if it is equal to 'RabbitBindingProperties' in that context.
So I think you should raise a bug against spring boot configuration processor.

See spring-cloud/spring-cloud-stream#1601

@snicoll
Copy link
Member

snicoll commented Feb 5, 2019

Yes, you're right, the annotation processor should be able to resolve T. I deliberately ignored generics as I've been dealing with them in the past for another annotation processor I wrote and it's quite hard to get it right with all possible combinations.

Would it be possible to simplify that arrangement to avoid the generics? We prefer that @ConfigurationProperties to be as simple as possible and limit hierarchies whenever possible.

Flagging for team attention to see what the rest of the team thinks.

@olegz
Copy link
Contributor

olegz commented Feb 5, 2019

@snicoll we did put a fix in place, it works but it's pretty ugly.
We do have a need for generics, since we have an extended properties structure for binders that we need to enforce to ensure that most of the code operating on it sits in the base binder class(es) (i can go into more details if need to).

@snicoll snicoll added the for: team-attention An issue we'd like other members of the team to review label Feb 5, 2019
@garyrussell
Copy link
Contributor Author

I agree that it might be hard to fix (especially for all corner cases), but I was surprised to see that overriding the getter with the explicit return type didn't work either...

@ConfigurationProperties("foo.bar")
public class GenericProperties extends AbstractPropeties<Foo> {

	@Override
	public Map<String, Foo> getProps() {
		return super.getProps();
	}

}

We still get T in the metadata.

(I tried overriding both the getter and setter with the same result).

Perhaps a reasonable compromise would be to at least make this work?

@garyrussell
Copy link
Contributor Author

garyrussell commented Feb 5, 2019

Method method = ClassUtils.getMostSpecificMethod(
	ClassUtils.getMethod(GenericProperties.class, "getProps", new Class[0]), GenericProperties.class);
Type returnType = method.getGenericReturnType();
System.out.println(returnType);
java.util.Map<java.lang.String, com.example.Foo>

@wilkinsona
Copy link
Member

wilkinsona commented Feb 6, 2019

It's easy for me to say as I don't know how hard it is to implement, but when I first saw the discussion on Slack my reaction was that this is something that we should support. That's still my view now so I'd like us to take a look at what it would entail.

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

No branches or pull requests

5 participants