Ensure all response object modification happens in one place

Make setLink and setListLink the same, and make them happen in transformResponseObject.
Make those methods also responsible for ensuring an empty list. Then move outputMediaType
negotiation before all other calls in the specific methods, to ensure we fail fast.

Refactoring in preparation to support type conversion on watch.

staging/src/k8s.io/apimachinery/pkg/api/meta/help.go

@@ -158,6 +158,19 @@ func ExtractList(obj runtime.Object) ([]runtime.Object, error) {
 // objectSliceType is the type of a slice of Objects
 var objectSliceType = reflect.TypeOf([]runtime.Object{})
 
+// LenList returns the length of this list or 0 if it is not a list.
+func LenList(list runtime.Object) int {
+	itemsPtr, err := GetItemsPtr(list)
+	if err != nil {
+		return 0
+	}
+	items, err := conversion.EnforcePtr(itemsPtr)
+	if err != nil {
+		return 0
+	}
+	return items.Len()
+}
+
 // SetList sets the given list object's Items member have the elements given in
 // objects.
 // Returns an error if list is not a List type (does not have an Items member),

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/create.go

@@ -70,6 +70,11 @@ func createHandler(r rest.NamedCreater, scope RequestScope, admit admission.Inte
 
 		ctx := req.Context()
 		ctx = request.WithNamespace(ctx, namespace)
+		outputMediaType, _, err := negotiation.NegotiateOutputMediaType(req, scope.Serializer, &scope)
+		if err != nil {
+			scope.err(err, w, req)
+			return
+		}
 
 		gv := scope.Kind.GroupVersion()
 		s, err := negotiation.NegotiateInputSerializer(req, false, scope.Serializer)
@@ -77,6 +82,7 @@ func createHandler(r rest.NamedCreater, scope RequestScope, admit admission.Inte
 			scope.err(err, w, req)
 			return
 		}
+
 		decoder := scope.Serializer.DecoderToVersion(s.Serializer, scope.HubGroupVersion)
 
 		body, err := readBody(req)
@@ -144,17 +150,6 @@ func createHandler(r rest.NamedCreater, scope RequestScope, admit admission.Inte
 		}
 		trace.Step("Object stored in database")
 
-		requestInfo, ok := request.RequestInfoFrom(ctx)
-		if !ok {
-			scope.err(fmt.Errorf("missing requestInfo"), w, req)
-			return
-		}
-		if err := setSelfLink(result, requestInfo, scope.Namer); err != nil {
-			scope.err(err, w, req)
-			return
-		}
-		trace.Step("Self-link added")
-
 		// If the object is partially initialized, always indicate it via StatusAccepted
 		code := http.StatusCreated
 		if accessor, err := meta.Accessor(result); err == nil {
@@ -168,7 +163,7 @@ func createHandler(r rest.NamedCreater, scope RequestScope, admit admission.Inte
 		}
 
 		scope.Trace = trace
-		transformResponseObject(ctx, scope, req, w, code, result)
+		transformResponseObject(ctx, scope, req, w, code, outputMediaType, result)
 	}
 }
 

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/delete.go

@@ -64,6 +64,12 @@ func DeleteResource(r rest.GracefulDeleter, allowsOptions bool, scope RequestSco
 		ae := request.AuditEventFrom(ctx)
 		admit = admission.WithAudit(admit, ae)
 
+		outputMediaType, _, err := negotiation.NegotiateOutputMediaType(req, scope.Serializer, &scope)
+		if err != nil {
+			scope.err(err, w, req)
+			return
+		}
+
 		options := &metav1.DeleteOptions{}
 		if allowsOptions {
 			body, err := readBody(req)
@@ -160,23 +166,10 @@ func DeleteResource(r rest.GracefulDeleter, allowsOptions bool, scope RequestSco
 					Kind: scope.Kind.Kind,
 				},
 			}
-		} else {
-			// when a non-status response is returned, set the self link
-			requestInfo, ok := request.RequestInfoFrom(ctx)
-			if !ok {
-				scope.err(fmt.Errorf("missing requestInfo"), w, req)
-				return
-			}
-			if _, ok := result.(*metav1.Status); !ok {
-				if err := setSelfLink(result, requestInfo, scope.Namer); err != nil {
-					scope.err(err, w, req)
-					return
-				}
-			}
 		}
 
 		scope.Trace = trace
-		transformResponseObject(ctx, scope, req, w, status, result)
+		transformResponseObject(ctx, scope, req, w, status, outputMediaType, result)
 	}
 }
 
@@ -204,6 +197,12 @@ func DeleteCollection(r rest.CollectionDeleter, checkBody bool, scope RequestSco
 		ctx = request.WithNamespace(ctx, namespace)
 		ae := request.AuditEventFrom(ctx)
 
+		outputMediaType, _, err := negotiation.NegotiateOutputMediaType(req, scope.Serializer, &scope)
+		if err != nil {
+			scope.err(err, w, req)
+			return
+		}
+
 		listOptions := metainternalversion.ListOptions{}
 		if err := metainternalversion.ParameterCodec.DecodeParameters(req.URL.Query(), scope.MetaGroupVersion, &listOptions); err != nil {
 			err = errors.NewBadRequest(err.Error())
@@ -304,17 +303,9 @@ func DeleteCollection(r rest.CollectionDeleter, checkBody bool, scope RequestSco
 					Kind: scope.Kind.Kind,
 				},
 			}
-		} else {
-			// when a non-status response is returned, set the self link
-			if _, ok := result.(*metav1.Status); !ok {
-				if _, err := setListSelfLink(result, ctx, req, scope.Namer); err != nil {
-					scope.err(err, w, req)
-					return
-				}
-			}
 		}
 
 		scope.Trace = trace
-		transformResponseObject(ctx, scope, req, w, http.StatusOK, result)
+		transformResponseObject(ctx, scope, req, w, http.StatusOK, outputMediaType, result)
 	}
 }

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/get.go

@@ -33,6 +33,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/fields"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apiserver/pkg/endpoints/handlers/negotiation"
 	"k8s.io/apiserver/pkg/endpoints/metrics"
 	"k8s.io/apiserver/pkg/endpoints/request"
 	"k8s.io/apiserver/pkg/registry/rest"
@@ -58,24 +59,21 @@ func getResourceHandler(scope RequestScope, getter getterFunc) http.HandlerFunc
 		ctx := req.Context()
 		ctx = request.WithNamespace(ctx, namespace)
 
-		result, err := getter(ctx, name, req, trace)
+		outputMediaType, _, err := negotiation.NegotiateOutputMediaType(req, scope.Serializer, &scope)
 		if err != nil {
 			scope.err(err, w, req)
 			return
 		}
-		requestInfo, ok := request.RequestInfoFrom(ctx)
-		if !ok {
-			scope.err(fmt.Errorf("missing requestInfo"), w, req)
-			return
-		}
-		if err := setSelfLink(result, requestInfo, scope.Namer); err != nil {
+
+		result, err := getter(ctx, name, req, trace)
+		if err != nil {
 			scope.err(err, w, req)
 			return
 		}
 
 		trace.Step("About to write a response")
 		scope.Trace = trace
-		transformResponseObject(ctx, scope, req, w, http.StatusOK, result)
+		transformResponseObject(ctx, scope, req, w, http.StatusOK, outputMediaType, result)
 		trace.Step("Transformed response object")
 	}
 }
@@ -187,6 +185,12 @@ func ListResource(r rest.Lister, rw rest.Watcher, scope RequestScope, forceWatch
 		ctx := req.Context()
 		ctx = request.WithNamespace(ctx, namespace)
 
+		outputMediaType, _, err := negotiation.NegotiateOutputMediaType(req, scope.Serializer, &scope)
+		if err != nil {
+			scope.err(err, w, req)
+			return
+		}
+
 		opts := metainternalversion.ListOptions{}
 		if err := metainternalversion.ParameterCodec.DecodeParameters(req.URL.Query(), scope.MetaGroupVersion, &opts); err != nil {
 			err = errors.NewBadRequest(err.Error())
@@ -267,22 +271,9 @@ func ListResource(r rest.Lister, rw rest.Watcher, scope RequestScope, forceWatch
 			return
 		}
 		trace.Step("Listing from storage done")
-		numberOfItems, err := setListSelfLink(result, ctx, req, scope.Namer)
-		if err != nil {
-			scope.err(err, w, req)
-			return
-		}
-		trace.Step("Self-linking done")
-		// Ensure empty lists return a non-nil items slice
-		if numberOfItems == 0 && meta.IsListType(result) {
-			if err := meta.SetList(result, []runtime.Object{}); err != nil {
-				scope.err(err, w, req)
-				return
-			}
-		}
 
 		scope.Trace = trace
-		transformResponseObject(ctx, scope, req, w, http.StatusOK, result)
-		trace.Step(fmt.Sprintf("Writing http response done (%d items)", numberOfItems))
+		transformResponseObject(ctx, scope, req, w, http.StatusOK, outputMediaType, result)
+		trace.Step(fmt.Sprintf("Writing http response done (%d items)", meta.LenList(result)))
 	}
 }

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go

@@ -88,6 +88,12 @@ func PatchResource(r rest.Patcher, scope RequestScope, admit admission.Interface
 		ctx := req.Context()
 		ctx = request.WithNamespace(ctx, namespace)
 
+		outputMediaType, _, err := negotiation.NegotiateOutputMediaType(req, scope.Serializer, &scope)
+		if err != nil {
+			scope.err(err, w, req)
+			return
+		}
+
 		patchJS, err := readBody(req)
 		if err != nil {
 			scope.err(err, w, req)
@@ -190,19 +196,8 @@ func PatchResource(r rest.Patcher, scope RequestScope, admit admission.Interface
 		}
 		trace.Step("Object stored in database")
 
-		requestInfo, ok := request.RequestInfoFrom(ctx)
-		if !ok {
-			scope.err(fmt.Errorf("missing requestInfo"), w, req)
-			return
-		}
-		if err := setSelfLink(result, requestInfo, scope.Namer); err != nil {
-			scope.err(err, w, req)
-			return
-		}
-		trace.Step("Self-link added")
-
 		scope.Trace = trace
-		transformResponseObject(ctx, scope, req, w, http.StatusOK, result)
+		transformResponseObject(ctx, scope, req, w, http.StatusOK, outputMediaType, result)
 	}
 }
 

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/response.go

@@ -33,17 +33,14 @@ import (
 
 // transformResponseObject takes an object loaded from storage and performs any necessary transformations.
 // Will write the complete response object.
-func transformResponseObject(ctx context.Context, scope RequestScope, req *http.Request, w http.ResponseWriter, statusCode int, result runtime.Object) {
-	// TODO: fetch the media type much earlier in request processing and pass it into this method.
-	trace := scope.Trace
-	mediaType, _, err := negotiation.NegotiateOutputMediaType(req, scope.Serializer, &scope)
-	if err != nil {
-		status := responsewriters.ErrorToAPIStatus(err)
-		trace.Step("Writing raw JSON response")
-		responsewriters.WriteRawJSON(int(status.Code), status, w)
+func transformResponseObject(ctx context.Context, scope RequestScope, req *http.Request, w http.ResponseWriter, statusCode int, mediaType negotiation.MediaTypeOptions, result runtime.Object) {
+	if err := setObjectSelfLink(ctx, result, req, scope.Namer); err != nil {
+		scope.err(err, w, req)
 		return
 	}
 
+	trace := scope.Trace
+
 	// If conversion was allowed by the scope, perform it before writing the response
 	if target := mediaType.Convert; target != nil {
 		switch {
@@ -51,7 +48,7 @@ func transformResponseObject(ctx context.Context, scope RequestScope, req *http.
 		case target.Kind == "PartialObjectMetadata" && target.GroupVersion() == metav1beta1.SchemeGroupVersion:
 			if meta.IsListType(result) {
 				// TODO: this should be calculated earlier
-				err = newNotAcceptableError(fmt.Sprintf("you requested PartialObjectMetadata, but the requested object is a list (%T)", result))
+				err := newNotAcceptableError(fmt.Sprintf("you requested PartialObjectMetadata, but the requested object is a list (%T)", result))
 				scope.err(err, w, req)
 				return
 			}
@@ -77,7 +74,7 @@ func transformResponseObject(ctx context.Context, scope RequestScope, req *http.
 		case target.Kind == "PartialObjectMetadataList" && target.GroupVersion() == metav1beta1.SchemeGroupVersion:
 			if !meta.IsListType(result) {
 				// TODO: this should be calculated earlier
-				err = newNotAcceptableError(fmt.Sprintf("you requested PartialObjectMetadataList, but the requested object is not a list (%T)", result))
+				err := newNotAcceptableError(fmt.Sprintf("you requested PartialObjectMetadataList, but the requested object is not a list (%T)", result))
 				scope.err(err, w, req)
 				return
 			}

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go

@@ -280,23 +280,30 @@ func checkName(obj runtime.Object, name, namespace string, namer ScopeNamer) err
 	return nil
 }
 
-// setListSelfLink sets the self link of a list to the base URL, then sets the self links
-// on all child objects returned. Returns the number of items in the list.
-func setListSelfLink(obj runtime.Object, ctx context.Context, req *http.Request, namer ScopeNamer) (int, error) {
+// setObjectSelfLink sets the self link of an object as needed.
+func setObjectSelfLink(ctx context.Context, obj runtime.Object, req *http.Request, namer ScopeNamer) error {
 	if !meta.IsListType(obj) {
-		return 0, nil
+		// status needs no self link
+		if _, ok := obj.(*metav1.Status); ok {
+			return nil
+		}
+		requestInfo, ok := request.RequestInfoFrom(ctx)
+		if !ok {
+			return fmt.Errorf("missing requestInfo")
+		}
+		return setSelfLink(obj, requestInfo, namer)
 	}
 
 	uri, err := namer.GenerateListLink(req)
 	if err != nil {
-		return 0, err
+		return err
 	}
 	if err := namer.SetSelfLink(obj, uri); err != nil {
 		klog.V(4).Infof("Unable to set self link on object: %v", err)
 	}
 	requestInfo, ok := request.RequestInfoFrom(ctx)
 	if !ok {
-		return 0, fmt.Errorf("missing requestInfo")
+		return fmt.Errorf("missing requestInfo")
 	}
 
 	count := 0
@@ -304,7 +311,14 @@ func setListSelfLink(obj runtime.Object, ctx context.Context, req *http.Request,
 		count++
 		return setSelfLink(obj, requestInfo, namer)
 	})
-	return count, err
+
+	if count == 0 {
+		if err := meta.SetList(obj, []runtime.Object{}); err != nil {
+			return err
+		}
+	}
+
+	return err
 }
 
 func summarizeData(data []byte, maxLength int) string {

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go

@@ -64,6 +64,12 @@ func UpdateResource(r rest.Updater, scope RequestScope, admit admission.Interfac
 		ctx := req.Context()
 		ctx = request.WithNamespace(ctx, namespace)
 
+		outputMediaType, _, err := negotiation.NegotiateOutputMediaType(req, scope.Serializer, &scope)
+		if err != nil {
+			scope.err(err, w, req)
+			return
+		}
+
 		body, err := readBody(req)
 		if err != nil {
 			scope.err(err, w, req)
@@ -174,24 +180,13 @@ func UpdateResource(r rest.Updater, scope RequestScope, admit admission.Interfac
 		}
 		trace.Step("Object stored in database")
 
-		requestInfo, ok := request.RequestInfoFrom(ctx)
-		if !ok {
-			scope.err(fmt.Errorf("missing requestInfo"), w, req)
-			return
-		}
-		if err := setSelfLink(result, requestInfo, scope.Namer); err != nil {
-			scope.err(err, w, req)
-			return
-		}
-		trace.Step("Self-link added")
-
 		status := http.StatusOK
 		if wasCreated {
 			status = http.StatusCreated
 		}
 
 		scope.Trace = trace
-		transformResponseObject(ctx, scope, req, w, status, result)
+		transformResponseObject(ctx, scope, req, w, status, outputMediaType, result)
 	}
 }