different split

pkg/scd/operational_intents_handler.go

@@ -701,55 +701,20 @@ func validateKeyAndProvideConflictResponse(
 	return nil, nil
 }
 
-// dependOnExistingSubscriptionIfAllowedAndCovered ensures that:
-//   - the subscription exists
-//   - the subscription is owned by the manager
-//   - the subscription covers the requested geo-temporal extent
-//   - if the subscription is implicit, it is extended if required to cover the requested parameters
-//   - if the subscription is explicit and it is not covering the requested parameters, an error is returned
-//
-// IMPORTANT: this method currently only EXTENDS implicit subscriptions:
-//
-//	It WILL NOT shrink an implicit subscription if that subscription covers more than the area of its dependant OIRs.
-//	This is relevant for situations where clients manually specify implicit subscriptions that were previously
-//	created when creating or updating their OIRs: they may get notifications for areas they are not interested in.
-func dependOnExistingSubscriptionIfAllowedAndCovered(ctx context.Context, r repos.Repository, manager dssmodels.Manager, params *validOIRParams) (*scdmodels.Subscription, error) {
-	if params.subscriptionID.Empty() {
-		// This situation should have been caught earlier but we check again to be safe
-		return nil, stacktrace.NewError("Cannot depend on a non-specified Subscription")
-	}
-
-	sub, err := r.GetSubscription(context.Background(), params.subscriptionID)
-	if err != nil {
-		return nil, stacktrace.Propagate(err, "Unable to get depended-upon Subscription")
-	}
-	if sub == nil {
-		return nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, "depended-upon Subscription %s does not exist", params.subscriptionID)
-	}
-
-	if sub.Manager != manager {
-		return nil,
-			stacktrace.Propagate(
-				// We do a bit of wrapping gymnastics because the root error message will be sent in the response,
-				// and we don't want to include the effective manager in there.
-				stacktrace.NewErrorWithCode(
-					dsserr.PermissionDenied, "Specificed Subscription is owned by different client"),
-				// The propagation message will end in the logs and help with debugging.
-				"Subscription %s owned by %s, but %s attempted to use it for an OperationalIntent",
-				params.subscriptionID,
-				sub.Manager,
-				manager,
-			)
-	}
+// ensureSubscriptionCoversOIR ensures that the subscription covers the requested geo-temporal extent, extending it if both possible and required,
+// or failing otherwise.
+// After this method returns successfully, the subscription will cover the requested geo-temporal extent.
+func ensureSubscriptionCoversOIR(ctx context.Context, r repos.Repository, sub *scdmodels.Subscription, params *validOIRParams) (*scdmodels.Subscription, error) {
+
 	updateSub := false
-	if sub.StartTime != nil && sub.StartTime.After(*params.uExtent.StartTime) {
-		if sub.ImplicitSubscription {
-			sub.StartTime = params.uExtent.StartTime
-			updateSub = true
-		} else {
-			return nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, "Subscription does not begin until after the OperationalIntent starts")
-		}
-	}
+    if sub.StartTime != nil && sub.StartTime.After(*params.uExtent.StartTime) {
+        if sub.ImplicitSubscription {
+            sub.StartTime = params.uExtent.StartTime
+            updateSub = true
+        } else {
+            return nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, "Subscription does not begin until after the OperationalIntent starts")
+        }
+    }
 	if sub.EndTime != nil && sub.EndTime.Before(*params.uExtent.EndTime) {
 		if sub.ImplicitSubscription {
 			sub.EndTime = params.uExtent.EndTime
@@ -767,10 +732,11 @@ func dependOnExistingSubscriptionIfAllowedAndCovered(ctx context.Context, r repo
 		}
 	}
 	if updateSub {
-		sub, err = r.UpsertSubscription(ctx, sub)
+		upsertedSub, err := r.UpsertSubscription(ctx, sub)
 		if err != nil {
 			return nil, stacktrace.Propagate(err, "Failed to update existing Subscription")
 		}
+		return upsertedSub, nil
 	}
 
 	return sub, nil
@@ -830,15 +796,34 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize
 				}
 			}
 		} else {
-			// Use existing Subscription and adapt it if needed and possible
-			sub, err = dependOnExistingSubscriptionIfAllowedAndCovered(ctx, r, manager, validParams)
+			// Attempt to rely on the specified subscription
+			sub, err = r.GetSubscription(ctx, validParams.subscriptionID)
+			if err != nil {
+				return stacktrace.Propagate(err, "Unable to get depended-upon Subscription")
+			}
+			if sub == nil {
+				return stacktrace.NewErrorWithCode(dsserr.BadRequest, "Depended-upon Subscription %s does not exist", validParams.subscriptionID)
+			}
+
+			// We need to confirm that it is owned by the calling manager
+			if sub.Manager != manager {
+				return stacktrace.Propagate(
+					// We do a bit of wrapping gymnastics because the root error message will be sent in the response,
+					// and we don't want to include the effective manager in there.
+					stacktrace.NewErrorWithCode(
+						dsserr.PermissionDenied, "Specificed Subscription is owned by different client"),
+					// The propagation message will end in the logs and help with debugging.
+					"Subscription %s owned by %s, but %s attempted to use it for an OperationalIntent",
+					validParams.subscriptionID,
+					sub.Manager,
+					manager,
+				)
+			}
+
+			// We need to ensure the subscription covers the OIR's geo-temporal extent
+			sub, err = ensureSubscriptionCoversOIR(ctx, r, sub, validParams)
 			if err != nil {
-				// Propagating the code here is important for properly handling all failure cases in the caller
-				return stacktrace.PropagateWithCode(
-					err,
-					stacktrace.GetCode(err),
-					"could not depend on specified subscription: %s",
-					validParams.subscriptionID)
+				return stacktrace.Propagate(err, "Failed to ensure subscription covers OIR")
 			}
 		}