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
Comment From: 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?
Comment From: thegeekyasian
Thanks for the pull-request @thegeekyasian. I'm wondering if we'd be better off changing
e.getKey().toString()
to something else so thatnull
entries aren't removed? Perhapsnull
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?
Comment From: philwebb
@thegeekyasian I don't think null
keys will work because of this assertion.
Comment From: 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?
Comment From: philwebb
From @timmalich in https://github.com/spring-projects/spring-boot/issues/27694#issuecomment-899860315
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?
Comment From: 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.
Comment From: 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.
Comment From: wilkinsona
@thegeekyasian Thanks very much for making your first contribution to Spring Boot.