[raft/scd] Extract update and create constraint - #1650
Conversation
barroco
added
the
dss-raft
b03192b to
2056183
Compare
mickmis
left a comment
mickmis
left a comment
There was a problem hiding this comment.
LGTM modulo clarification.
But what this shows is that we may want to come up with something better for deduplicating validation/parsing of those parameters.
LMK if you agree and have any idea. But in any case for after this PR train is merged
Maybe a Validate field on the OperationHandler?
| // Start and end times are required for each volume | ||
| // The end time may not be in the past | ||
| valid.uExtent, err = scdmodels.UnionVolumes4DFromSCDRest( | ||
| params.Extents, | ||
| scdmodels.WithRequireTimeBounds(), | ||
| scdmodels.WithRequireEndTimeAfter(now), | ||
| ) | ||
| if err != nil { | ||
| return nil, stacktrace.Propagate(err, "Invalid extents") | ||
| } | ||
|
|
||
| valid.cells, err = valid.uExtent.CalculateSpatialCovering() | ||
| if err != nil { | ||
| return nil, stacktrace.Propagate(err, "Invalid area") | ||
| } |
There was a problem hiding this comment.
What's the rationale for not validating here with UnionVolumes4DFromSCDRest and CalculateSpatialCovering anymore?
IIUC the strategy for validation is:
- keep in REST handlers whatever validation there is, even if it is a side effect of the parsing
- duplicate in
ExecuteXXXfunctions what is needed for parsing
Is this correct?
There was a problem hiding this comment.
Regarding the strategy, I aim to do as much validation as possible on the handler side, especially for bad request cases to avoid doing a consensus round just to end up rejecting the request for such a reason. Indeed, duplication occurs in the operation because the validation also comes with parsing (ID etc.).
For this specific case, not validating the volumes was a mistake on my part (fixed in latest push) as I kept these checks for the subscription side as well. I also merged the validation between the handler and the action / operation and kept the handler specific part (AllowHTTPBaseUrls check) there.
I just opened issue #1676 to track this duplication issue.
2056183 to
73a9bf9
Compare
3 participants
Chained PR: #1627 -> #1642 -> #1643 -> #1644 -> #1645 -> #1646 -> #1649 -> #1650 -> #1651 -> #1653 -> #1654 -> #1656 -> #1657 -> #1655 -> #1666 -> #1667 -> #1668 -> #1669
Extract update and create constraint business logic.