Spring Boot Properties Migrator - Detect properties of Map type that are marked as deprecated #28796
Conversation
|
@dominiccroce Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
|
@dominiccroce Thank you for signing the Contributor License Agreement! |
spring-projects-issues
added
the
status: waiting-for-triage
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Thanks for the PR but it doesn't address the issue.
It looks like there is some confusion between MapPropertySource (an implementation detail, irrelevant to this issue) and a property that happens to be of type map.
I think one way of fixing this would be to patch getMatchingProperties. Right now, it looks for an exact match but we could also look for properties that would match the prefix if the property is deprecated.
Would you be willing to give it another try? No problem if you can't, I can take over.
| })); | ||
| return result; | ||
| } | ||
|
|
||
| private void getMatchingPropertiesForMapEntries(String name, ConfigurationPropertySource source, | ||
| ConfigurationMetadataProperty metadata, MultiValueMap<String, PropertyMigration> result) { | ||
| if (source.getUnderlyingSource() instanceof MapPropertySource && name.equals(metadata.getName())) { |
There was a problem hiding this comment.
I don't understand what this is doing. MapPropertySource is an implementation detail and you shouldn't be looking at it. It has nothing to do with a deprecated property that is of type Map.
There was a problem hiding this comment.
I see, I thought MapPropertySource indicated the Map deprecation. But if we don't have a way to filter for Map deprecations than agreed it must be done by prefix matching on the name itself.
| content.put("status.http-mapping.down", 500); | ||
| content.put("status.http-mapping.out-of-service", 503); | ||
| content.put("status.http-mapping.warning", 500); | ||
| propertySources.addFirst(new MapPropertySource("status.http-mapping", content)); |
There was a problem hiding this comment.
If I change the name of the property source to be something else than this, the test fails. The PropertySource implementation and its name are irrelevant for this, it could be anything.
There was a problem hiding this comment.
Nothing is name specific, but yes it is specific to MapPropertySource
There was a problem hiding this comment.
It is. Replace new MapPropertySource("status.http-mapping", content) by new MapPropertySource("test", content) and you'll see. The name of the property source has nothing to do whatsoever with the prefix used by the keys.
snicoll
added
the
status: waiting-for-feedback
I have pushed a new commit to address the issues. The main problem I am facing is for any PropertyResource, I want to be able to iterate through the ConfigurationPropertyNames. Once we have those, I rely on existing code to handle prefix matching. From there I handle prefix matching. I am lacking some context on PropertySource but feels like there should be a way to get all PropertyNames without casting to IterableConfigurationPropertySource. |
spring-projects-issues
added
status: feedback-provided
| if (source instanceof IterableConfigurationPropertySource) | ||
| ((IterableConfigurationPropertySource) source).iterator().forEachRemaining(configurationPropertyName -> { | ||
| ConfigurationProperty configurationProperty = source.getConfigurationProperty(configurationPropertyName); |
There was a problem hiding this comment.
This should be easier, something like source.getConfigurationProperties() would be nice API to add. Not sure if it belongs on PropertySource though
There was a problem hiding this comment.
@dominiccroce do you want to get another look at it? filter takes a predicate and should lets you filter the propertysource with only the keys that are relevant. I haven't tried but I think it's worth exploring.
snicoll
added
status: waiting-for-feedback
|
@dominiccroce thanks but I think we will have to restart this one from scratch I am afraid. The last changes are breaking existing tests and we should really consider looking at |
snicoll
added
status: declined
Addresses #27854 with unit test demonstrating desired functionality