Make ImmutablePluginConfigVariable.refresh() a no-op instead of throwing#6869
Conversation
dlvenable
requested review from
KarstenSchnitter,
Zhangxunmt,
dinujoh,
divbok,
graytaylor0,
kkondaka,
oeyh,
san81,
sb2k16 and
srikanthjg
as code owners
…ing. The refresh() method semantically means "re-read from your backing store." For an immutable variable with no backing store, this is naturally a no-op. Only setValue() should throw since it attempts to mutate the value. This allows callers to safely call refresh() without needing to guard with isUpdatable() checks. Signed-off-by: David Venable <dlv@amazon.com>
dlvenable
force-pushed
the
fix/immutable-plugin-config-variable-refresh
branch
from
639b639 to
30308b2
Compare
|
LGTM. Making |
| return objectMapper.readValue(jsonParser, destinationType); | ||
| } | ||
|
|
||
| private static class ImmutablePluginConfigVariable implements PluginConfigVariable { |
There was a problem hiding this comment.
Thanks for this fix — making refresh() a no-op is the right behavior for immutable variables.
While we're touching this area, I think we should also consider making ImmutablePluginConfigVariable a public top-level class in data-prepper-api (alongside the PluginConfigVariable interface) as a follow-up or as part of this PR.
Rationale: Plugin authors sometimes need to programmatically construct a PluginConfigVariable holding a static/constant value — for example, when building config objects in code rather than through YAML deserialization. Today there's no framework-provided way to do this. The only implementation lives as a private inner class inside VariableExpander in data-prepper-plugin-framework, which plugins cannot access (and shouldn't depend on due to its heavy transitive dependencies).
Making this class public in data-prepper-api gives plugin developers a first-class way to create immutable config variables without each plugin having to write its own wrapper around the interface.
What do you think — should we include that in this PR or track it separately?
There was a problem hiding this comment.
Agree it would be useful for plugin authors constructing config objects programmatically. Should be tracked separately though, this PR is just the behavioral fix for refresh().
There was a problem hiding this comment.
@bagmarnikhil , Thanks for the feedback on this!
I want to understand this a little better. Is this for testing code? Or is this something to generate default config objects? I think we need this for main code, it should probably come from a static factory method on PluginConfigVariable. But if it is for testing, we could offer a different mechanism.
4 participants
Description
The
refresh()method semantically means "re-read from your backing store." For an immutable variable with no backing store, this is naturally a no-op. OnlysetValue()should throw since it attempts to mutate the value. This allows callers to safely callrefresh()without needing to guard withisUpdatable()checks.I also updated the Javadocs on these to make this clearer.
I took note of this issue while reviewing PR #6856. We should be able to call
refresh()safely.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.