One example of such a Map is in Spring Cloud stream. As things stand, there's no metadata for any of the properties on BindingProperties. I think it would be interesting to explore the possibility of providing metadata for those properties, perhaps using a wildcard for the key in the map:
spring.cloud.stream.bindings.*.content-type
spring.cloud.stream.bindings.*.destination
spring.cloud.stream.bindings.*.group
…
For this to be useful, we'd need a corresponding enhancement in each of the IDE plugins.
Comment From: snicoll
I've thought long and hard about this and I don't think we should do this (See also #9894). There are several reasons:
- Scalar vs. Group: the only way for us to harvest nested object is by using
@NestedConfigurationProperty(essentially, this is not a scalar value, please create a "sub-group" for all the properties defined by that type). If we support that for collections or maps now, how do we tell if the value is scalar or not? - Scope: if we find a way to fix this and we offer automatic harvesting of POJOs, where do we stop? The next step is to expect this to be somehow recursive (
BindingPropertiesmay hold another nested object). - Duplication: the use of the
*above is what I ended up considering myself but that pojo can be used in multiple places, in multiples maps and/or in different objects. It means that the metadata is duplicated. If you don't want that, we need a third root objects (to harvest the pojo) and a way to reference to it somehow).
Let's see what the rest of the team thinks.
Comment From: philwebb
For "1)" the @NestedConfigurationProperty could be the indicator that the value isn't a scalar. So @NestedConfigurationProperty Map<String, Foo> is a hint that Foo can be expanded with the * syntax, where as @NestedConfigurationProperty Map<String, Foo> cannot (and we expect to be able to convert a String to a Foo somehow.
For "2)" and "3)" I'd probably go with duplicate *. I don't see the duplication being all that much of a problem, as long as there isn't an infinite loop (e.g. Foo contains a Map<String,Foo>).
Comment From: philwebb
FWIW, I think we shouldn't invest too much time thinking about this until after 2.0.
Comment From: snicoll
and we expect to be able to convert a String to a Foo somehow.
That would break backward compatibility but I think having to add an annotation to ask the AP to "expand" is a nice way to solve the scope problem. Great idea! So 1) and 2) could be fixed by some signals that the AP should expand the metadata (and the binding should work as it does now, regardless of the annotation). That's pretty much what happens today for @NestedConfigurationProperty anyway.
As for 3, duplication is much easier but we need some feedback from IDE developers first IMO (ping @yanncebron @kdvolder and @AlexFalappa).
(By the way, I am kind of changing my mind because I realized recently there is no way to add hints for those types and implementing this would be a nice way to fix that).
Comment From: wilkinsona
Since this issue was raised, the IDEs have plugged the gap to varying degrees. A new gap will appear if we add support for immutable configuration properties that use constructor injection. Without metadata, they'd have to plug that gap and look at the constructor rather than the property setters to figure out what properties to offer.
Comment From: rwinch
@wilkinsona Perhaps I am missing something, but I haven't seen any improvements with this from a Spring Security standpoint. For example:
spring:
security:
oauth2:
client:
registration:
github:
|
I get no auto complete at | (pretend that is the cursor). This is something that I would really like for users.
Can you point me to how the IDE's have plugged the gap?
Comment From: snicoll
I can't speak for Eclipse but IJ has support for this for ages on .properties. There isn't for yaml at the moment though. To be clear, we're now considering this with higher priority given the potential binding change.
Comment From: rwinch
Thanks for the clarification @snicoll. I wasn't aware that .properties were supported by IntelliJ. Also glad to hear that this means this is a higher priority. I had misunderstood it to mean that the IDE already was taking care of the issue.
Comment From: kdvolder
Should work in Eclipse / STS, in .yml as well as .properties. If it doesn't for your particular example please raise a bug with STS4.
Comment From: kdvolder
@rwinch I couldn't get your specific example to work (presumably because I don't have the right stuff on the project's classpath). But here's a similar example, to show how it is (supposed to) work:
https://drive.google.com/open?id=1bfzAdMMV6Oi148yXxEvaKvAfxqTR4nM6
Do let us know by raising a STS bug if you find problems with your actual example.
Comment From: rwinch
Thanks @kdvolder! I will have to give it a try in the latest STS.
PS: The dependency you need is org.springframework.boot:spring-boot-starter-oauth2-client
Comment From: mhalbritter
This file is read by the spring-boot-properties-migrator too, to notify users of deprecations in the property names and on-the-fly rewriting. It would be great if this
a) would support for map-based types (i stumbled over spring.security.saml2.relyingparty.registration.*.identityprovider) and
b) would support wildcards to make renames simpler (e.g. management.metrics.* to management.*.metrics)
Comment From: wilkinsona
The lack of this metadata allowed the mistake that @dreis2211 has fixed in this pull request to slip through. Our Asciidoctor extension only validates maps on a best-effort basis. The prefix, spring.security.saml2.relyingparty.registration, matched and there's no metadata to tell it what properties each entry in the map may have so it was unable to detect and report the error.
Comment From: buksvdl
ALL our properties classes are nested this way. Without the extended functionality there is no reason to pull in the lib. The need stays, so we will go the extra mile and add the missing recursion.
Comment From: dgensert
I don't understand either why this is not implemented. The whole point of generating metadata is that i do not have to add it manually. When I reference a POJO from a List or Map it should get generated for path.pojo in the metadata, simple as that.
Comment From: philwebb
@dgensert This issue remains open and is something we'd like to fix but we haven't been able to get to it yet. Please be respectful in your comments. We're a small team trying to deliver what we can, but we only have finite resources and a certain number of hours in the day.
Comment From: dgensert
@philwebb thanks for the feedback. I did not mean to complain about the issue still being open but to rather state the point of view in light of the arguments from the discussion above, having recently experienced weird issues in this direction, particularly with the kotlin spring-boot integration. Maybe I could have chosen my words more elegantly. I fully understand that there are a lot of high priority issues.
In the end it would simply make a lot of sense to have the annotation processor generate properties for all fields involved in the chain of properties in order to provide developers with reasonable insights on how to set the right switches through IDE completions.
In my particular case I wrote a POJO for configuring spring security overloadings, more specifically for adding CORS Configurations per URI-Path (e.g. "/**", "/internal","/public", etc.) including various settings. Then I ended up getting properties not generated from the List
- val b: Boolean = true
- val list: List
= listOf()
Would fail to generate info, whereas changing to "val list: List
Wrapping it up: I hope this helps to understand why it makes sense to output generated data of properties containing potentially recurring POJOs for "type": "java.util.List/Map". I guess that it would be of great benefit to make this behavior selectable, e.g. as an additional option to the @NestedConfigurationProperty() or @ConfigurationProperties() annotation, indicating that POJOs in collections should be generated as well, thereby not changing current standard behavior in regards to the mentioned concerns.
It would even be an improvement to add the POJO class reference into the generated metadata, so that we can at least see what type this List references and then do a lookup ourselves.
I hope this feedback gives some more insight into the topic, again sorry if I came across a little too blunt earlier. Cheers.