[
https://issues.apache.org/jira/browse/CASSSIDECAR-504?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18126689#comment-18126689
]
Bohdan Siryk commented on CASSSIDECAR-504:
------------------------------------------
I open a pull request with Jackson based solution which just hooks into a
deserialization chain and calls Validatable#validate after deserialization.
This allows to validate the whole object entirely including cross-fields
validations when validation of an option depends on a value of another option.
Jackson based approach (Option A) has some benefits:
1. Nested configurations of subconfigs will be validated as well if they
implement Validatable, so we don't have to explicitly chain validation in
subconfigs
2. Enables cross-field validation, ValidatingDeseriazlier sees the final state
of the object
The downside is that if option is absent in yaml Jackson will skip
deserialization, which means the object won't be validated. It should not be a
problem if default configurations are valid though, but it still needs
attention.
As an alternative to this solution, we can just manage validation explicitly
(Option B) in SidecarConfigurationImpl#validate and its configuration objects.
The downside is that it is possible to miss updating the validate method so it
will fail silently, but with all AI staff we have it should not be a problem?
It will be caught on a review.
If that really matters, we can try to achieve something similar to what Jackson
does but only with a reflection so walking through all configuration fields of
SidecarConfigurationImpl, checking if they implement Validatable and call it,
but we will also need either to go through all fields of subconfigs (which
means we will go deep in a graph), either subconfig has to wire validation of
its config objects as well.
I don't really have a preference to choose between Jackson based solution
(Option A) or explicitly managed validation (Option B).
> RestoreJobConfigurationImpl bypasses validation
> -----------------------------------------------
>
> Key: CASSSIDECAR-504
> URL: https://issues.apache.org/jira/browse/CASSSIDECAR-504
> Project: Sidecar for Apache Cassandra
> Issue Type: Bug
> Components: Configuration
> Reporter: Bohdan Siryk
> Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> Spotted during review of CASSSIDECAR-501.
> Basically, *RestoreJobConfigurationImpl* is validated only through its
> builder, while Jackson takes another path to instantiate and parse values
> into config object which results in bypassing validation.
> Across the codebase, validation of each config setting is delegated to its
> setters. It's enough for single setting validation, but it's not possible for
> settings that depend on other settings like _jobDiscoveryMinimumRecencyDays_
> of {*}RestoreJobConfigurationImpl{*}. Value _jobDiscoveryMinimumRecencyDays_
> cannot be greater than
> _restoreJobTablesTtl_ value which is configurable. The order of
> deseriazliation is unknown, so we have to validate object right after it is
> deserialized (including calls of setters).
> We can achieve such by hooking into Jackson deserialize chain (using
> {*}BeanDeserializerModifier{*}) and validate object there or just by creating
> an all arg constructor with @JsonCreator annotation so removing all setters
> entirely.
> I tried to implement the first approach with help of AI
> [here|https://github.com/apache/cassandra-sidecar/compare/trunk...worryg0d:cassandra-sidecar:jackson-validating-deserializer?expand=1]
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
