Avoid NPE when replacement property does not exist

[hdeadman]

When the property migration report is run, if a replacement for a deprecated property doesn't exist then a NullPointerException occurs and it is difficult to determine which property is bad.

This change ignores the issue by returning null from a method (detectMapValueReplacementType) which will result in the replacement and deprecated properties being found to have incompatible types (which should be fine since the property is unknown, can't do much with that except report).

The CAS project (in next release) is using it's own "additional-spring-configuration-metadata.json" file to notify upgrading users of replacement properties but sometimes the new replacement property is not in the metadata and the server won't startup (if the user is using the old deprecated property). Another alternative would be to throw an exception with a message that indicated the bad property. That would at least allow the user to know which property was causing the problem.

I could make a unit test (e.g. ensure unknown replacement property doesn't cause error) but I don't want to do that if you would prefer that an error occurred (albeit one with a helpful message).

Should I be targeting the 2.1 branch? Or should I do a another pull request for that?

[pivotal-issuemaster]

@hdeadman Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-issuemaster]

@hdeadman Thank you for signing the Contributor License Agreement!

[snicoll]

Thanks for opening your first PR to Spring Boot. This is interesting and I'd like to see this case exercised with a test to make sure I understood the problem. If the replacement key does not exist, something must be wrong in the metadata and would be worth reporting.

Can you please add a test and clarify if I got this right?

[hdeadman]

I can try to work on a test this evening.

For some more background, here is where someone ran into problem: apereo/cas#3672 (comment)

This was a problem the CAS's spring metadata and not the metadata that comes with spring-boot. I don't fully understand how spring metadata gets generated but CAS runs some code as part of its build to supplement the metadata with properties that get missed by the normal processing.
ConfigurationMetadataGenerator Despite this additional metadata there are some properties for which metadata is still not generated.

I would like to be able to add a test to CAS that makes sure the additional metadata we are adding to CAS isn't going to blow up when the property migration report runs on app startup but the fact that the PropertiesMigrationReporter in spring boot is not a public class makes that harder. (I suppose I could make a test in CAS that uses the spring-boot package?)

In CAS we are manually adding replacements to our additional-spring-configuration-metadata.json so we can have a nice migration report and we have to make sure they aren't going to cause the PropertiesMigrationReporter to break the app startup. (It's OK if it stops the startup if it is reporting useful information about deprecated properties vs. runtime exception).

[hdeadman]

I added a test that would have resulted in an NPE prior to the change in this pull request. I don't think the report is terrible that is generated as-is, now that there is no NPE. The message says that the type of the replacement parameter is incompatible with the old value and that is true. (old type != unknown type). Report is still telling the user that the property they used in their config is no longer supported.

As an extra safeguard, what do you think about wrapping the whole PropertiesMigrationListener.onApplicationEvent() method in a try/catch so that runtime exceptions during report generation don't prevent deployment of an application? It would need to log an error and not re-throw any exception.

I must be missing something b/c I don't understand the code prior to the null check I added:

ConfigurationMetadataProperty property = this.allProperties.get(fullId.substring(0, lastDot));

Why is it chopping off the last part of the replacement property name and looking that up rather than looking up the replacement property directly? For example one of tests has a replacement value of "test.mapped.ttl" but it looks up the property "test.mapped". Why wouldn't there be a property entry for "test.mapped.ttl"?

[snicoll]

The name of the method is detectMapValueReplacementType. This method is about checking if the replacement key isn't referring to a map type. The replacement would be a.b.my-map.my-key but the actual property (for the map) would be a.b.my-map. That's why we remove my-key to look for if that property is a map. If it is, this is a valid use case that we have to take into account, checking if the target value of the map matches the value of the original property.

This is a special case and that's why I am asking for a use case. It looks like non-map keys that dot not have a valid replacement are affected by this. I'll take this over.

[snicoll]

@hdeadman thank you for making your first contribution to Spring Boot. I've polished your contribution as the error message in the test you've added was misleading. This turned out to be a bigger change than I anticipated. Hopefully, the reason provided in the log is more accurate now.