NullPointerException in RoutingDataSourceHealthContributor when a routing data source has a target with a null routing key

[thegeekyasian]

The RoutingDataSourceHealthContributor constructor throws a NullPointerException (NPE) if the provided map has a null key. With this commit the NPE issue has been fixed by filtering the entries that are not-null.

Alternatively, a constant value can be added when the key for a datasource in the ‘Map’ is null.

This closes #27694

[philwebb]

Thanks for the pull-request @thegeekyasian. I'm wondering if we'd be better off changing e.getKey().toString() to something else so that null entries aren't removed? Perhaps null could be replaced with "unnamed" or something similar.

We should also add some tests to DataSourceHealthContributorAutoConfigurationTests.

Are you interested in updating your pull request in that direction?

[thegeekyasian]

Thanks for the pull-request @thegeekyasian. I'm wondering if we'd be better off changing e.getKey().toString() to something else so that null entries aren't removed? Perhaps null could be replaced with "unnamed" or something similar.

We should also add some tests to DataSourceHealthContributorAutoConfigurationTests.

Are you interested in updating your pull request in that direction?

Thanks @philwebb Definitely, we can replace the null one with a public constant string value 'unnamed'. I would like to do the suggested changes and update the PR.

Another option that we have is to keep null values with 'null' key with the below code snippet:

.collect(Collectors.toMap((e) -> Optional.of(e.getKey()).map(Object::toString) .orElse(null), Map.Entry::getValue));

Thoughts?

[philwebb]

@thegeekyasian I don't think null keys will work because of this assertion.

[thegeekyasian]

@philwebb Ah, I see! 'unnamed' key is the way to go then.

Should a warning be logged in this case though, when setting the datasource for unnamed?

[philwebb]

From @timmalich in #27694 (comment)

Thanks @thegeekyasian, I've added a comment to the pull-request.

@philwebb, a PR has been created to fix the null pointer by filtering the entries with the non-null only. Can you please review and share your thoughts?

Thank you both very much.

Wouldn't that filter just drop an otherwise valid configuration? Wouldn't it be better to check null within the next line? Like:
e.getKey()==null?null:e.getKey().toString()

Option 2) Would it be possible to at least log a warning?

[philwebb]

I think the warning is unnecessary since the health info name is only for reference. The only problem might be if Map happens to contain null and "unnamed" which seem quite unlikely.

[thegeekyasian]

@philwebb, the PR has been updated with the suggested changes and the tests have been updated to assert the 'unnamed' datasource. With the change done, even if the unnamed key is used in the Map, it would work as expected by the user.

[wilkinsona]

@thegeekyasian Thanks very much for making your first contribution to Spring Boot.