From 5eeef81edfd3a72b1911b59b142477a5189a7cb9 Mon Sep 17 00:00:00 2001 From: Fabiano Franz Date: Thu, 26 Jan 2017 17:26:00 -0200 Subject: [PATCH] Fix sorting printer with missing fields --- pkg/kubectl/BUILD | 1 + pkg/kubectl/sorting_printer.go | 59 ++++++----- pkg/kubectl/sorting_printer_test.go | 145 ++++++++++++++++++++++++++-- 3 files changed, 176 insertions(+), 29 deletions(-) diff --git a/pkg/kubectl/BUILD b/pkg/kubectl/BUILD index 1b7f7150f87..cc62e05f7f1 100644 --- a/pkg/kubectl/BUILD +++ b/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", diff --git a/pkg/kubectl/sorting_printer.go b/pkg/kubectl/sorting_printer.go index 2aa12bb66c3..381998c9f16 100644 --- a/pkg/kubectl/sorting_printer.go +++ b/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 } diff --git a/pkg/kubectl/sorting_printer_test.go b/pkg/kubectl/sorting_printer_test.go index 62608d2b42b..133dc3fc19d 100644 --- a/pkg/kubectl/sorting_printer_test.go +++ b/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)