Fix sorting printer with missing fields

pkg/kubectl/BUILD

@@ -174,6 +174,7 @@ go_test(
         "//vendor:github.com/spf13/cobra",
         "//vendor:k8s.io/apimachinery/pkg/api/errors",
         "//vendor:k8s.io/apimachinery/pkg/apis/meta/v1",
+        "//vendor:k8s.io/apimachinery/pkg/apis/meta/v1/unstructured",
         "//vendor:k8s.io/apimachinery/pkg/runtime",
         "//vendor:k8s.io/apimachinery/pkg/runtime/schema",
         "//vendor:k8s.io/apimachinery/pkg/runtime/serializer/yaml",

pkg/kubectl/sorting_printer.go

@@ -88,17 +88,6 @@ func (s *SortingPrinter) sortObj(obj runtime.Object) error {
 }
 
 func SortObjects(decoder runtime.Decoder, objs []runtime.Object, fieldInput string) (*RuntimeSort, error) {
-	parser := jsonpath.New("sorting")
-
-	field, err := massageJSONPath(fieldInput)
-	if err != nil {
-		return nil, err
-	}
-
-	if err := parser.Parse(field); err != nil {
-		return nil, err
-	}
-
 	for ix := range objs {
 		item := objs[ix]
 		switch u := item.(type) {
@@ -112,18 +101,38 @@ func SortObjects(decoder runtime.Decoder, objs []runtime.Object, fieldInput stri
 		}
 	}
 
-	var values [][]reflect.Value
-	if unstructured, ok := objs[0].(*unstructured.Unstructured); ok {
-		values, err = parser.FindResults(unstructured.Object)
-	} else {
-		values, err = parser.FindResults(reflect.ValueOf(objs[0]).Elem().Interface())
-	}
-
+	field, err := massageJSONPath(fieldInput)
 	if err != nil {
 		return nil, err
 	}
-	if len(values) == 0 {
-		return nil, fmt.Errorf("couldn't find any field with path: %s", field)
+
+	parser := jsonpath.New("sorting").AllowMissingKeys(true)
+	if err := parser.Parse(field); err != nil {
+		return nil, err
+	}
+
+	// We don't do any model validation here, so we traverse all objects to be sorted
+	// and, if the field is valid to at least one of them, we consider it to be a
+	// valid field; otherwise error out.
+	// Note that this requires empty fields to be considered later, when sorting.
+	var fieldFoundOnce bool
+	for _, obj := range objs {
+		var values [][]reflect.Value
+		if unstructured, ok := obj.(*unstructured.Unstructured); ok {
+			values, err = parser.FindResults(unstructured.Object)
+		} else {
+			values, err = parser.FindResults(reflect.ValueOf(obj).Elem().Interface())
+		}
+		if err != nil {
+			return nil, err
+		}
+		if len(values) > 0 && len(values[0]) > 0 {
+			fieldFoundOnce = true
+			break
+		}
+	}
+	if !fieldFoundOnce {
+		return nil, fmt.Errorf("couldn't find any field with path %q in the list of objects", field)
 	}
 
 	sorter := NewRuntimeSort(field, objs)
@@ -260,7 +269,7 @@ func (r *RuntimeSort) Less(i, j int) bool {
 	iObj := r.objs[i]
 	jObj := r.objs[j]
 
-	parser := jsonpath.New("sorting")
+	parser := jsonpath.New("sorting").AllowMissingKeys(true)
 	parser.Parse(r.field)
 
 	var iValues [][]reflect.Value
@@ -285,12 +294,18 @@ func (r *RuntimeSort) Less(i, j int) bool {
 		glog.Fatalf("Failed to get j values for %#v using %s (%v)", jObj, r.field, err)
 	}
 
+	if len(iValues) == 0 || len(iValues[0]) == 0 {
+		return true
+	}
+	if len(jValues) == 0 || len(jValues[0]) == 0 {
+		return false
+	}
 	iField := iValues[0][0]
 	jField := jValues[0][0]
 
 	less, err := isLess(iField, jField)
 	if err != nil {
-		glog.Fatalf("Field %s in %v is an unsortable type: %s, err: %v", r.field, iObj, iField.Kind().String(), err)
+		glog.Fatalf("Field %s in %T is an unsortable type: %s, err: %v", r.field, iObj, iField.Kind().String(), err)
 	}
 	return less
 }

pkg/kubectl/sorting_printer_test.go

@@ -18,9 +18,11 @@ package kubectl
 
 import (
 	"reflect"
+	"strings"
 	"testing"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	internal "k8s.io/kubernetes/pkg/api"
 	api "k8s.io/kubernetes/pkg/api/v1"
@@ -56,10 +58,11 @@ func TestSortingPrinter(t *testing.T) {
 	}
 
 	tests := []struct {
-		obj   runtime.Object
-		sort  runtime.Object
-		field string
-		name  string
+		obj         runtime.Object
+		sort        runtime.Object
+		field       string
+		name        string
+		expectedErr string
 	}{
 		{
 			name: "in-order-already",
@@ -265,12 +268,140 @@ func TestSortingPrinter(t *testing.T) {
 			},
 			field: "{.metadata.name}",
 		},
+		{
+			name: "some-missing-fields",
+			obj: &unstructured.UnstructuredList{
+				Object: map[string]interface{}{
+					"kind":       "List",
+					"apiVersion": "v1",
+				},
+				Items: []*unstructured.Unstructured{
+					{
+						Object: map[string]interface{}{
+							"kind":       "ReplicationController",
+							"apiVersion": "v1",
+							"status": map[string]interface{}{
+								"availableReplicas": 2,
+							},
+						},
+					},
+					{
+						Object: map[string]interface{}{
+							"kind":       "ReplicationController",
+							"apiVersion": "v1",
+							"status":     map[string]interface{}{},
+						},
+					},
+					{
+						Object: map[string]interface{}{
+							"kind":       "ReplicationController",
+							"apiVersion": "v1",
+							"status": map[string]interface{}{
+								"availableReplicas": 1,
+							},
+						},
+					},
+				},
+			},
+			sort: &unstructured.UnstructuredList{
+				Object: map[string]interface{}{
+					"kind":       "List",
+					"apiVersion": "v1",
+				},
+				Items: []*unstructured.Unstructured{
+					{
+						Object: map[string]interface{}{
+							"kind":       "ReplicationController",
+							"apiVersion": "v1",
+							"status":     map[string]interface{}{},
+						},
+					},
+					{
+						Object: map[string]interface{}{
+							"kind":       "ReplicationController",
+							"apiVersion": "v1",
+							"status": map[string]interface{}{
+								"availableReplicas": 1,
+							},
+						},
+					},
+					{
+						Object: map[string]interface{}{
+							"kind":       "ReplicationController",
+							"apiVersion": "v1",
+							"status": map[string]interface{}{
+								"availableReplicas": 2,
+							},
+						},
+					},
+				},
+			},
+			field: "{.status.availableReplicas}",
+		},
+		{
+			name: "all-missing-fields",
+			obj: &unstructured.UnstructuredList{
+				Object: map[string]interface{}{
+					"kind":       "List",
+					"apiVersion": "v1",
+				},
+				Items: []*unstructured.Unstructured{
+					{
+						Object: map[string]interface{}{
+							"kind":       "ReplicationController",
+							"apiVersion": "v1",
+							"status": map[string]interface{}{
+								"replicas": 0,
+							},
+						},
+					},
+					{
+						Object: map[string]interface{}{
+							"kind":       "ReplicationController",
+							"apiVersion": "v1",
+							"status": map[string]interface{}{
+								"replicas": 0,
+							},
+						},
+					},
+				},
+			},
+			field:       "{.status.availableReplicas}",
+			expectedErr: "couldn't find any field with path \"{.status.availableReplicas}\" in the list of objects",
+		},
+		{
+			name: "model-invalid-fields",
+			obj: &api.ReplicationControllerList{
+				Items: []api.ReplicationController{
+					{
+						Status: api.ReplicationControllerStatus{},
+					},
+					{
+						Status: api.ReplicationControllerStatus{},
+					},
+					{
+						Status: api.ReplicationControllerStatus{},
+					},
+				},
+			},
+			field:       "{.invalid}",
+			expectedErr: "couldn't find any field with path \"{.invalid}\" in the list of objects",
+		},
 	}
 	for _, test := range tests {
 		sort := &SortingPrinter{SortField: test.field, Decoder: internal.Codecs.UniversalDecoder()}
-		if err := sort.sortObj(test.obj); err != nil {
-			t.Errorf("unexpected error: %v (%s)", err, test.name)
-			continue
+		err := sort.sortObj(test.obj)
+		if err != nil {
+			if len(test.expectedErr) > 0 {
+				if strings.Contains(err.Error(), test.expectedErr) {
+					continue
+				}
+				t.Fatalf("%s: expected error containing: %q, got: \"%v\"", test.name, test.expectedErr, err)
+			}
+			t.Fatalf("%s: unexpected error: %v", test.name, err)
+		}
+		if len(test.expectedErr) > 0 {
+			t.Fatalf("%s: expected error containing: %q, got none", test.name, test.expectedErr)
 		}
 		if !reflect.DeepEqual(test.obj, test.sort) {
 			t.Errorf("[%s]\nexpected:\n%v\nsaw:\n%v", test.name, test.sort, test.obj)