[scd] properly cleanup implicit sub on oir update #1104
Conversation
Shastick
force-pushed
the
fix-implicit-sub-oir-update-clean
branch
from
03bd2b6
to
00a7e0c
Compare
| // See https://github.com/interuss/dss/issues/1059 for more details | ||
| func subscriptionIsImplicitAndOnlyAttachedToOIR(ctx context.Context, r repos.Repository, oirID *dssmodels.ID, subscription *scdmodels.Subscription) (bool, error) { | ||
| if subscription == nil { | ||
| return false, nil |
There was a problem hiding this comment.
This looks like a sanity check: return an error if subscription is nil. It does not make sense invoking that function with a nil subscription.
Or remove the check altogether since it is private and use twice where a non-nil check is already performed.
There was a problem hiding this comment.
- Subscriptions are allowed to be
nilwhen the state of an OIR is 'accepted' so receiving a nil here does not necessarily indicate a problem - when an OIR is created there is no previous subscription to speak of, so a nil makes sense as well
My intent of having the check here was probably to avoid the extra checks in the caller, but conciseness consideration taken aside:
I may have originally called the method subscriptionNeedsCleanup: at that time the early return with false would fit pretty well with the method name. But even now, if a subscription does not exist it feels Ok to just return false, as a non-existing subscription can't be implicit or attached to the OIR?
There was a problem hiding this comment.
Looking at the context where this method is called, I feel like moving the check out of it will not bring any benefits: I'd rather leave it as-is, especially knowing that an upcoming PR is going to remove this code.
There was a problem hiding this comment.
if a subscription does not exist it feels Ok to just return false, as a non-existing subscription can't be implicit or attached to the OIR?
I guess that's a possible interpretation, but then I'd say that the root issue here is that the abstraction defined by this function is a bit awkward. Not a huge issue in itself here though.
As long as we are not silencing errors I'm OK, and looks like it's not the case here.
especially knowing that an upcoming PR is going to remove this code
Is this actually planned for?
(but not a valid reason anyway)
There was a problem hiding this comment.
Yes, in #1059 and it's part of the work we'd like to possibly get done until the 14th
Shastick
force-pushed
the
fix-implicit-sub-oir-update-clean
branch
from
00a7e0c
to
557c8eb
Compare
This is a rework of #1057 after most changes for #1088 have landed.
The DSS built from this PR successfully passed the new qualifier's implicit subscription handling scenario, although that scenario still needs to be extended to cover all corner cases.
The PR is open for review, as it addresses #1056