Merge pull request #951 from meirf/selector-string-form

extend selector with EXISTS, new tests/some old tests use table-driven style

pkg/labels/labels.go

@@ -23,6 +23,9 @@ import (
 
 // Labels allows you to present labels independently from their storage.
 type Labels interface {
+	// Has returns whether the provided label exists.
+	Has(label string) (exists bool)
+
 	// Get returns the value for the provided label.
 	Get(label string) (value string)
 }
@@ -42,6 +45,12 @@ func (ls Set) String() string {
 	return strings.Join(selector, ",")
 }
 
+// Has returns whether the provided label exists in the map.
+func (ls Set) Has(label string) bool {
+	_, exists := ls[label]
+	return exists
+}
+
 // Get returns the value in the map for the provided label.
 func (ls Set) Get(label string) string {
 	return ls[label]

pkg/labels/labels_test.go

@@ -35,6 +35,23 @@ func TestSetString(t *testing.T) {
 	// with ",=!" characters in their names.
 }
 
+func TestLabelHas(t *testing.T) {
+	labelHasTests := []struct {
+		Ls  Labels
+		Key string
+		Has bool
+	}{
+		{Set{"x": "y"}, "x", true},
+		{Set{"x": ""}, "x", true},
+		{Set{"x": "y"}, "foo", false},
+	}
+	for _, lh := range labelHasTests {
+		if has := lh.Ls.Has(lh.Key); has != lh.Has {
+			t.Errorf("%#v.Has(%#v) => %v, expected %v", lh.Ls, lh.Key, has, lh.Has)
+		}
+	}
+}
+
 func TestLabelGet(t *testing.T) {
 	ls := Set{"x": "y"}
 	if ls.Get("x") != "y" {

pkg/labels/selector.go

@@ -17,6 +17,7 @@ limitations under the License.
 package labels
 
 import (
+	"bytes"
 	"fmt"
 	"sort"
 	"strings"
@@ -138,46 +139,159 @@ func (t andTerm) String() string {
 
 // Operator represents a key's relationship
 // to a set of values in a Requirement.
-// TODO: Should also represent key's existence.
 type Operator int
 
 const (
-	IN Operator = iota + 1
+	In Operator = iota + 1
-	NOT_IN
+	NotIn
+	Exists
 )
 
-// LabelSelector only not named 'Selector' due
+// LabelSelector contains a list of Requirements.
-// to name conflict until Selector is deprecated.
+// LabelSelector is set-based and is distinguished from exact
+// match-based selectors composed of key=value matching conjunctions.
+// TODO: Remove previous sentence when exact match-based
+// selectors are removed.
 type LabelSelector struct {
 	Requirements []Requirement
 }
 
+// Requirement is a selector that contains values, a key
+// and an operator that relates the key and values. The zero
+// value of Requirement is invalid. See the NewRequirement
+// constructor for creating a valid Requirement.
+// Requirement is set-based and is distinguished from exact
+// match-based selectors composed of key=value matching.
+// TODO: Remove previous sentence when exact match-based
+// selectors are removed.
 type Requirement struct {
 	key       string
 	operator  Operator
 	strValues util.StringSet
 }
 
-func (r *Requirement) Matches(ls Labels) bool {
+// NewRequirement is the constructor for a Requirement.
-	switch r.operator {
+// If either of these rules is violated, an error is returned:
-	case IN:
+// (1) The operator can only be In, NotIn or Exists.
-		return r.strValues.Has(ls.Get(r.key))
+// (2) If the operator is In or NotIn, the values set must
-	case NOT_IN:
+//     be non-empty.
-		return !r.strValues.Has(ls.Get(r.key))
+//
+// The empty string is a valid value in the input values set.
+func NewRequirement(key string, op Operator, vals util.StringSet) (*Requirement, error) {
+	switch op {
+	case In, NotIn:
+		if len(vals) == 0 {
+			return nil, fmt.Errorf("for In,NotIn operators, values set can't be empty")
+		}
+	case Exists:
 	default:
-		return false
+		return nil, fmt.Errorf("operator '%v' is not recognized", op)
+	}
+	return &Requirement{key: key, operator: op, strValues: vals}, nil
+}
+
+// Matches returns true if the Requirement matches the input Labels.
+// There is a match in the following cases:
+// (1) The operator is Exists and Labels has the Requirement's key.
+// (2) The operator is In, Labels has the Requirement's key and Labels'
+//     value for that key is in Requirement's value set.
+// (3) The operator is NotIn, Labels has the Requirement's key and
+//     Labels' value for that key is not in Requirement's value set.
+// (4) The operator is NotIn and Labels does not have the
+//     Requirement's key.
+//
+// If called on an invalid Requirement, an error is returned. See
+// NewRequirement for creating a valid Requirement.
+func (r *Requirement) Matches(ls Labels) (bool, error) {
+	switch r.operator {
+	case In:
+		if !ls.Has(r.key) {
+			return false, nil
+		}
+		return r.strValues.Has(ls.Get(r.key)), nil
+	case NotIn:
+		if !ls.Has(r.key) {
+			return true, nil
+		}
+		return !r.strValues.Has(ls.Get(r.key)), nil
+	case Exists:
+		return ls.Has(r.key), nil
+	default:
+		return false, fmt.Errorf("requirement is not set: %+v", r)
 	}
 }
 
-func (sg *LabelSelector) Matches(ls Labels) bool {
+// String returns a human-readable string that represents this
-	for _, req := range sg.Requirements {
+// Requirement. If called on an invalid Requirement, an error is
-		if !req.Matches(ls) {
+// returned. See NewRequirement for creating a valid Requirement.
-			return false
+func (r *Requirement) String() (string, error) {
+	var buffer bytes.Buffer
+	buffer.WriteString(r.key)
+
+	switch r.operator {
+	case In:
+		buffer.WriteString(" in ")
+	case NotIn:
+		buffer.WriteString(" not in ")
+	case Exists:
+		return buffer.String(), nil
+	default:
+		return "", fmt.Errorf("requirement is not set: %+v", r)
+	}
+
+	buffer.WriteString("(")
+	if len(r.strValues) == 1 {
+		buffer.WriteString(r.strValues.List()[0])
+	} else { // only > 1 since == 0 prohibited by NewRequirement
+		buffer.WriteString(strings.Join(r.strValues.List(), ","))
+	}
+	buffer.WriteString(")")
+	return buffer.String(), nil
+}
+
+// Matches for a LabelSelector returns true if all
+// its Requirements match the input Labels. If any
+// Requirement does not match, false is returned.
+// An error is returned if any match attempt between
+// a Requirement and the input Labels returns an error.
+func (lsel *LabelSelector) Matches(l Labels) (bool, error) {
+	for _, req := range lsel.Requirements {
+		if matches, err := req.Matches(l); err != nil {
+			return false, err
+		} else if !matches {
+			return false, nil
 		}
 	}
-	return true
+	return true, nil
 }
 
+// String returns a comma-separated string of all
+// the LabelSelector Requirements' human-readable strings.
+// An error is returned if any attempt to get a
+// Requirement's  human-readable string returns an error.
+func (lsel *LabelSelector) String() (string, error) {
+	var reqs []string
+	for _, req := range lsel.Requirements {
+		if str, err := req.String(); err != nil {
+			return "", err
+		} else {
+			reqs = append(reqs, str)
+		}
+	}
+	return strings.Join(reqs, ","), nil
+}
+
+// TODO: Parse takes a string representing a selector and returns
+// a selector, or an error. A well-formed input string follows
+// the syntax of that which is returned by LabelSelector.String
+// and therefore is largely controlled by that which is returned
+// by Requirement.String. The returned selector object's type
+// should be an interface implemented by LabelSelector. Note that
+// this parsing function is different than ParseSelector since
+// they parse different selectors with different syntaxes.
+// See comments above for LabelSelector and Requirement struct
+// definition for more details.
+
 func try(selectorPiece, op string) (lhs, rhs string, ok bool) {
 	pieces := strings.Split(selectorPiece, op)
 	if len(pieces) == 2 {

pkg/labels/selector_test.go

@@ -199,56 +199,100 @@ func TestRequiresExactMatch(t *testing.T) {
 	}
 }
 
-func expectMatchRequirement(t *testing.T, req Requirement, ls Set) {
+func TestRequirementConstructor(t *testing.T) {
-	if !req.Matches(ls) {
+	requirementConstructorTests := []struct {
-		t.Errorf("Wanted '%+v' to match '%s', but it did not.\n", req, ls)
+		Key     string
+		Op      Operator
+		Vals    util.StringSet
+		Success bool
+	}{
+		{"x", 8, util.NewStringSet("foo"), false},
+		{"x", In, nil, false},
+		{"x", NotIn, util.NewStringSet(), false},
+		{"x", In, util.NewStringSet("foo"), true},
+		{"x", NotIn, util.NewStringSet("foo"), true},
+		{"x", Exists, nil, true},
+	}
+	for _, rc := range requirementConstructorTests {
+		if _, err := NewRequirement(rc.Key, rc.Op, rc.Vals); err == nil && !rc.Success {
+			t.Errorf("expected error with key:%#v op:%v vals:%v, got no error", rc.Key, rc.Op, rc.Vals)
+		} else if err != nil && rc.Success {
+			t.Errorf("expected no error with key:%#v op:%v vals:%v, got:%v", rc.Key, rc.Op, rc.Vals, err)
+		}
 	}
 }
 
-func expectNoMatchRequirement(t *testing.T, req Requirement, ls Set) {
+func TestToString(t *testing.T) {
-	if req.Matches(ls) {
+	var req Requirement
-		t.Errorf("Wanted '%+v' to not match '%s', but it did.", req, ls)
+	toStringTests := []struct {
+		In    *LabelSelector
+		Out   string
+		Valid bool
+	}{
+		{&LabelSelector{Requirements: []Requirement{
+			getRequirement("x", In, util.NewStringSet("abc", "def"), t),
+			getRequirement("y", NotIn, util.NewStringSet("jkl"), t),
+			getRequirement("z", Exists, nil, t),
+		}}, "x in (abc,def),y not in (jkl),z", true},
+		{&LabelSelector{Requirements: []Requirement{
+			getRequirement("x", In, util.NewStringSet("abc", "def"), t),
+			req,
+		}}, "", false},
+		{&LabelSelector{Requirements: []Requirement{
+			getRequirement("x", NotIn, util.NewStringSet("abc"), t),
+			getRequirement("y", In, util.NewStringSet("jkl", "mno"), t),
+			getRequirement("z", NotIn, util.NewStringSet(""), t),
+		}}, "x not in (abc),y in (jkl,mno),z not in ()", true},
+	}
+	for _, ts := range toStringTests {
+		if out, err := ts.In.String(); err != nil && ts.Valid {
+			t.Errorf("%+v.String() => %v, expected no error", ts.In, err)
+		} else if out != ts.Out {
+			t.Errorf("%+v.String() => %v, want %v", ts.In, out, ts.Out)
+		}
 	}
 }
 
-func TestRequirementMatches(t *testing.T) {
+func TestRequirementLabelSelectorMatching(t *testing.T) {
-	s := Set{"x": "foo", "y": "baz"}
+	var req Requirement
-	a := Requirement{key: "x", operator: IN, strValues: util.NewStringSet("foo")}
+	labelSelectorMatchingTests := []struct {
-	b := Requirement{key: "x", operator: NOT_IN, strValues: util.NewStringSet("beta")}
+		Set   Set
-	c := Requirement{key: "y", operator: IN, strValues: nil}
+		Sel   *LabelSelector
-	d := Requirement{key: "y", strValues: util.NewStringSet("foo")}
+		Match bool
-	expectMatchRequirement(t, a, s)
+		Valid bool
-	expectMatchRequirement(t, b, s)
+	}{
-	expectNoMatchRequirement(t, c, s)
+		{Set{"x": "foo", "y": "baz"}, &LabelSelector{Requirements: []Requirement{
-	expectNoMatchRequirement(t, d, s)
+			req,
-}
+		}}, false, false},
-
+		{Set{"x": "foo", "y": "baz"}, &LabelSelector{Requirements: []Requirement{
-func expectMatchLabSelector(t *testing.T, lsel LabelSelector, s Set) {
+			getRequirement("x", In, util.NewStringSet("foo"), t),
-	if !lsel.Matches(s) {
+			getRequirement("y", NotIn, util.NewStringSet("alpha"), t),
-		t.Errorf("Wanted '%+v' to match '%s', but it did not.\n", lsel, s)
+		}}, true, true},
+		{Set{"x": "foo", "y": "baz"}, &LabelSelector{Requirements: []Requirement{
+			getRequirement("x", In, util.NewStringSet("foo"), t),
+			getRequirement("y", In, util.NewStringSet("alpha"), t),
+		}}, false, true},
+		{Set{"y": ""}, &LabelSelector{Requirements: []Requirement{
+			getRequirement("x", NotIn, util.NewStringSet(""), t),
+			getRequirement("y", Exists, nil, t),
+		}}, true, true},
+		{Set{"y": "baz"}, &LabelSelector{Requirements: []Requirement{
+			getRequirement("x", In, util.NewStringSet(""), t),
+		}}, false, true},
+	}
+	for _, lsm := range labelSelectorMatchingTests {
+		if match, err := lsm.Sel.Matches(lsm.Set); err != nil && lsm.Valid {
+			t.Errorf("%+v.Matches(%#v) => %v, expected no error", lsm.Sel, lsm.Set, err)
+		} else if match != lsm.Match {
+			t.Errorf("%+v.Matches(%#v) => %v, want %v", lsm.Sel, lsm.Set, match, lsm.Match)
+		}
 	}
 }
 
-func expectNoMatchLabSelector(t *testing.T, lsel LabelSelector, s Set) {
+func getRequirement(key string, op Operator, vals util.StringSet, t *testing.T) Requirement {
-	if lsel.Matches(s) {
+	req, err := NewRequirement(key, op, vals)
-		t.Errorf("Wanted '%+v' to not match '%s', but it did.\n", lsel, s)
+	if err != nil {
+		t.Errorf("NewRequirement(%v, %v, %v) resulted in error:%v", key, op, vals, err)
 	}
-}
+	return *req
-
-func TestLabelSelectorMatches(t *testing.T) {
-	s := Set{"x": "foo", "y": "baz"}
-	allMatch := LabelSelector{
-		Requirements: []Requirement{
-			{key: "x", operator: IN, strValues: util.NewStringSet("foo")},
-			{key: "y", operator: NOT_IN, strValues: util.NewStringSet("alpha")},
-		},
-	}
-	singleNonMatch := LabelSelector{
-		Requirements: []Requirement{
-			{key: "x", operator: IN, strValues: util.NewStringSet("foo")},
-			{key: "y", operator: IN, strValues: util.NewStringSet("alpha")},
-		},
-	}
-	expectMatchLabSelector(t, allMatch, s)
-	expectNoMatchLabSelector(t, singleNonMatch, s)
 }