[scd] Factorize volume4D union from rest#1381
Conversation
RustedBones
force-pushed
the
rest-volume-union
branch
2 times, most recently
from
a99b97c to
d712e54
Compare
RustedBones
force-pushed
the
rest-volume-union
branch
2 times, most recently
from
415c243 to
81eac87
Compare
RustedBones
force-pushed
the
rest-volume-union
branch
from
81eac87 to
c6d2e77
Compare
pkg/models/scd_conversions.go
Outdated
| for _, validator := range validators { | ||
| if err := validator(union); err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
Possible behavior change: before this PR the validators are invoked on each individual volume, and after it is on the aggregated one. Could we face a scenario where some individually invalid volumes are unionized into a valid one? Or the opposite?
Out of caution and since this is used as well as validation of parameters, I would actually do the validation on both individual and unionized volumes. And in any case, make clear in the function doc what is the expected behavior. WDYT?
Also, factor away this loop? Duplicated with function above.
There was a problem hiding this comment.
Could we face a scenario where some individually invalid volumes are unionized into a valid one? Or the opposite?
Yes, that was my concern in #1380 (comment). I think extent with some expired volumes should not be rejected by the DSS, that's why I moved the validation on the unioned volume. Is that fine ?
I'll add some doc and factor the loop.
There was a problem hiding this comment.
Just to be sure we're alligned on this one, the doc states:
Spacetime extents that bound this operational intent. Start and end times, as well as lower and upper altitudes, are required for each volume. The end time may not be in the past.
I think to move the validation on the unioned extent so we can accept composite extens with some expired volumes but not all. is that correct ?
Hum that's an interesting case. IMO with a strict interpretation of the wording, the API would require all individual volumes to not end in the past. Now, implementation-wise, I do agree we should accept it as long as the aggregated volume is not in the past. And that's indeed what the current implementation is doing. So, agree with you for the 'end time not in the past' check.
As far as the other validations are concerned (time and altitude bounds), if they fail on an individual volume, they will fail for the aggregated one. So there will be no behavior change with the changes in this PR.
All in all LGTM with the clarification in the function doc of the behavior.
2 participants
Fix TODO from the code:
Remove unused
extentsfield when unioningAdd validator for expired 4D extent