-
Notifications
You must be signed in to change notification settings - Fork 207
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix exception when plugin settings are empty #1525
Fix exception when plugin settings are empty #1525
Conversation
…ting to validate that setting. Always return a non-null plugin configuration. Signed-off-by: David Venable <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #1525 +/- ##
=========================================
Coverage 94.17% 94.18%
- Complexity 1179 1181 +2
=========================================
Files 165 165
Lines 3382 3386 +4
Branches 276 277 +1
=========================================
+ Hits 3185 3189 +4
Misses 141 141
Partials 56 56
Continue to review full report at Codecov.
|
private Object convertSettings(final Class<?> pluginConfigurationType, final PluginSetting pluginSetting) { | ||
Map<String, Object> settingsMap = pluginSetting.getSettings(); | ||
if (settingsMap == null) | ||
settingsMap = Collections.emptyMap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PluginSettings
class define the default value for an empty plugin settings over the PluginConfigurationConverter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I thought about that as well. But, I didn't want to interrupt any use of getSettings
that was already using null
.
From what I can tell, plugins that use the new pluginConfigurationType
are all assuming non-null configurations. So I wanted to at least cover that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I'd like to fully remove PluginSettings
someday. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarifying!
…ting to validate that setting. Always return a non-null plugin configuration. (opensearch-project#1525) Signed-off-by: David Venable <[email protected]> Signed-off-by: Finn Roblin <[email protected]>
Description
When creating a plugin configuration with an empty plug-in settings, make that configuration non-null. This fixes a bug where the call to
validate()
was provided a null pointer. Additionally, plugins should expect that their configurations are non-null.Issues Resolved
N/A
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.