From b01792d9dd8db737369dce28addb083391d192e6 Mon Sep 17 00:00:00 2001
From: Sanne Raymaekers <sanne.raymaekers@gmail.com>
Date: Fri, 14 Oct 2022 18:08:49 +0200
Subject: [PATCH] internal/ostree: offload using default ostree ref to caller

If params.Ref is an empty string, it's set to the distro's default
ref. The only difference here is that the default ref also gets
verified.

It makes splitting out resolving ostree refs to a new job easier.

In the weldr and cloud apis, ostree.ResolveParams always got executed,
also for non-ostree image types. Make it more explicit by only resolving
if the image type is actually an ostree image.
---
 internal/cloudapi/v2/handler.go | 60 +++++++++++++++++++--------------
 internal/ostree/ostree.go       | 21 ++++--------
 internal/weldr/api.go           |  9 +++--
 3 files changed, 48 insertions(+), 42 deletions(-)

diff --git a/internal/cloudapi/v2/handler.go b/internal/cloudapi/v2/handler.go
index b0b2310c4..35846d0dc 100644
--- a/internal/cloudapi/v2/handler.go
+++ b/internal/cloudapi/v2/handler.go
@@ -286,37 +286,45 @@ func (h *apiHandlers) PostCompose(ctx echo.Context) error {
 			}
 		}
 
-		ostreeOptions := ostree.RequestParams{}
-		if ir.Ostree != nil {
-			if ir.Ostree.Ref != nil {
-				ostreeOptions.Ref = *ir.Ostree.Ref
+		// assume it's an ostree image if the type has a default ostree ref
+		if imageType.OSTreeRef() != "" && ir.Ostree != nil {
+			ostreeOptions := ostree.RequestParams{}
+			if ir.Ostree != nil {
+				if ir.Ostree.Ref != nil {
+					ostreeOptions.Ref = *ir.Ostree.Ref
+				}
+				if ir.Ostree.Url != nil {
+					ostreeOptions.URL = *ir.Ostree.Url
+				}
+				if ir.Ostree.Parent != nil {
+					ostreeOptions.Parent = *ir.Ostree.Parent
+				}
 			}
-			if ir.Ostree.Url != nil {
-				ostreeOptions.URL = *ir.Ostree.Url
+
+			if ostreeOptions.Ref == "" {
+				ostreeOptions.Ref = imageType.OSTreeRef()
 			}
-			if ir.Ostree.Parent != nil {
-				ostreeOptions.Parent = *ir.Ostree.Parent
+
+			ref, checksum, err := ostree.ResolveParams(ostreeOptions)
+			if err != nil {
+				switch v := err.(type) {
+				case ostree.RefError:
+					return HTTPError(ErrorInvalidOSTreeRef)
+				case ostree.ResolveRefError:
+					return HTTPErrorWithInternal(ErrorInvalidOSTreeRepo, v)
+				case ostree.ParameterComboError:
+					return HTTPError(ErrorInvalidOSTreeParams)
+				default:
+					// general case
+					return HTTPError(ErrorInvalidOSTreeParams)
+				}
 			}
-		}
-		ref, checksum, err := ostree.ResolveParams(ostreeOptions, imageType.OSTreeRef())
-		if err != nil {
-			switch v := err.(type) {
-			case ostree.RefError:
-				return HTTPError(ErrorInvalidOSTreeRef)
-			case ostree.ResolveRefError:
-				return HTTPErrorWithInternal(ErrorInvalidOSTreeRepo, v)
-			case ostree.ParameterComboError:
-				return HTTPError(ErrorInvalidOSTreeParams)
-			default:
-				// general case
-				return HTTPError(ErrorInvalidOSTreeParams)
+			imageOptions.OSTree = distro.OSTreeImageOptions{
+				ImageRef:      ref,
+				FetchChecksum: checksum,
+				URL:           ostreeOptions.URL,
 			}
 		}
-		imageOptions.OSTree = distro.OSTreeImageOptions{
-			ImageRef:      ref,
-			FetchChecksum: checksum,
-			URL:           ostreeOptions.URL,
-		}
 
 		var irTarget *target.Target
 		if ir.UploadOptions == nil {
diff --git a/internal/ostree/ostree.go b/internal/ostree/ostree.go
index 8f8d0cd3d..1cfc63f3d 100644
--- a/internal/ostree/ostree.go
+++ b/internal/ostree/ostree.go
@@ -80,22 +80,15 @@ func ResolveRef(location, ref string) (string, error) {
 // without URL results in a ParameterComboError. Failure to resolve the
 // checksum results in a ResolveRefError.
 //
-// If Ref is not specified in the RequestParams, the defaultRef is used.
-// If Parent is not specified in the RequestParams, the value of Ref is used
-// (which will be defaultRef if neither is specified).
+// If Parent is not specified in the RequestParams, the value of Ref is used.
 //
 // If any ref (Ref or Parent) is malformed, the function returns with a RefError.
-func ResolveParams(params RequestParams, defaultRef string) (ref, checksum string, err error) {
-	// Determine value of ref
+func ResolveParams(params RequestParams) (ref, checksum string, err error) {
 	ref = params.Ref
-	if ref != "" {
-		// verify format of provided ref
-		if !VerifyRef(params.Ref) {
-			return "", "", NewRefError("Invalid ostree ref %q", params.Ref)
-		}
-	} else {
-		// if ref is not provided, use distro default
-		ref = defaultRef
+
+	// Determine value of ref
+	if !VerifyRef(params.Ref) {
+		return "", "", NewRefError("Invalid ostree ref %q", params.Ref)
 	}
 
 	// Determine value of parentRef
@@ -111,7 +104,7 @@ func ResolveParams(params RequestParams, defaultRef string) (ref, checksum strin
 		}
 	} else {
 		// if parent is not provided, use ref
-		parentRef = ref
+		parentRef = params.Ref
 	}
 
 	// Resolve parent checksum
diff --git a/internal/weldr/api.go b/internal/weldr/api.go
index c4b92c745..64e604d2d 100644
--- a/internal/weldr/api.go
+++ b/internal/weldr/api.go
@@ -2357,8 +2357,13 @@ func (api *API) composeHandler(writer http.ResponseWriter, request *http.Request
 	if testMode == "1" || testMode == "2" {
 		// Fake a parent commit for test requests
 		cr.OSTree.Parent = "02604b2da6e954bd34b8b82a835e5a77d2b60ffa"
-	} else {
-		ref, checksum, err := ostree.ResolveParams(cr.OSTree, imageType.OSTreeRef())
+	} else if imageType.OSTreeRef() != "" {
+		// If the image type has a default ostree ref, assume this is an OSTree image
+		reqParams := cr.OSTree
+		if reqParams.Ref == "" {
+			reqParams.Ref = imageType.OSTreeRef()
+		}
+		ref, checksum, err := ostree.ResolveParams(reqParams)
 		if err != nil {
 			errors := responseError{
 				ID:  "OSTreeOptionsError",