From 0e372174d255599738c91c830998a5869e9b81a3 Mon Sep 17 00:00:00 2001
From: k8s-infra-cherrypick-robot
 <90416843+k8s-infra-cherrypick-robot@users.noreply.github.com>
Date: Mon, 12 Jun 2023 07:27:50 -0700
Subject: [PATCH] =?UTF-8?q?[release-0.15]=20=F0=9F=90=9B=20hasLabels=20and?=
 =?UTF-8?q?=20matchingLabels=20step=20on=20each=20other=20(#2373)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* fix: hasLabels and matchingLabels step on each other

* remove testcase with invalid input

---------

Co-authored-by: Shanshan.Ying <shanshan.ying@apecloud.com>
---
 pkg/client/options.go      | 20 ++++++++++++-----
 pkg/client/options_test.go | 45 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 5 deletions(-)

diff --git a/pkg/client/options.go b/pkg/client/options.go
index 50a461f1cc..d81bf25de9 100644
--- a/pkg/client/options.go
+++ b/pkg/client/options.go
@@ -513,8 +513,15 @@ type MatchingLabels map[string]string
 // ApplyToList applies this configuration to the given list options.
 func (m MatchingLabels) ApplyToList(opts *ListOptions) {
 	// TODO(directxman12): can we avoid reserializing this over and over?
-	sel := labels.SelectorFromValidatedSet(map[string]string(m))
-	opts.LabelSelector = sel
+	if opts.LabelSelector == nil {
+		opts.LabelSelector = labels.NewSelector()
+	}
+	// If there's already a selector, we need to AND the two together.
+	noValidSel := labels.SelectorFromValidatedSet(map[string]string(m))
+	reqs, _ := noValidSel.Requirements()
+	for _, req := range reqs {
+		opts.LabelSelector = opts.LabelSelector.Add(req)
+	}
 }
 
 // ApplyToDeleteAllOf applies this configuration to the given an List options.
@@ -528,14 +535,17 @@ type HasLabels []string
 
 // ApplyToList applies this configuration to the given list options.
 func (m HasLabels) ApplyToList(opts *ListOptions) {
-	sel := labels.NewSelector()
+	if opts.LabelSelector == nil {
+		opts.LabelSelector = labels.NewSelector()
+	}
+	// TODO: ignore invalid labels will result in an empty selector.
+	// This is inconsistent to the that of MatchingLabels.
 	for _, label := range m {
 		r, err := labels.NewRequirement(label, selection.Exists, nil)
 		if err == nil {
-			sel = sel.Add(*r)
+			opts.LabelSelector = opts.LabelSelector.Add(*r)
 		}
 	}
-	opts.LabelSelector = sel
 }
 
 // ApplyToDeleteAllOf applies this configuration to the given an List options.
diff --git a/pkg/client/options_test.go b/pkg/client/options_test.go
index efba976c4f..a1208f1bfd 100644
--- a/pkg/client/options_test.go
+++ b/pkg/client/options_test.go
@@ -237,6 +237,19 @@ var _ = Describe("MatchingLabels", func() {
 		expectedErrMsg := `values[0][k]: Invalid value: "axahm2EJ8Phiephe2eixohbee9eGeiyees1thuozi1xoh0GiuH3diewi8iem7Nui": must be no more than 63 characters`
 		Expect(err.Error()).To(Equal(expectedErrMsg))
 	})
+
+	It("Should add matchingLabels to existing selector", func() {
+		listOpts := &client.ListOptions{}
+
+		matchingLabels := client.MatchingLabels(map[string]string{"k": "v"})
+		matchingLabels2 := client.MatchingLabels(map[string]string{"k2": "v2"})
+
+		matchingLabels.ApplyToList(listOpts)
+		Expect(listOpts.LabelSelector.String()).To(Equal("k=v"))
+
+		matchingLabels2.ApplyToList(listOpts)
+		Expect(listOpts.LabelSelector.String()).To(Equal("k=v,k2=v2"))
+	})
 })
 
 var _ = Describe("FieldOwner", func() {
@@ -292,3 +305,35 @@ var _ = Describe("ForceOwnership", func() {
 		Expect(*o.Force).To(Equal(true))
 	})
 })
+
+var _ = Describe("HasLabels", func() {
+	It("Should produce hasLabels in given order", func() {
+		listOpts := &client.ListOptions{}
+
+		hasLabels := client.HasLabels([]string{"labelApe", "labelFox"})
+		hasLabels.ApplyToList(listOpts)
+		Expect(listOpts.LabelSelector.String()).To(Equal("labelApe,labelFox"))
+	})
+
+	It("Should add hasLabels to existing hasLabels selector", func() {
+		listOpts := &client.ListOptions{}
+
+		hasLabel := client.HasLabels([]string{"labelApe"})
+		hasLabel.ApplyToList(listOpts)
+
+		hasOtherLabel := client.HasLabels([]string{"labelFox"})
+		hasOtherLabel.ApplyToList(listOpts)
+		Expect(listOpts.LabelSelector.String()).To(Equal("labelApe,labelFox"))
+	})
+
+	It("Should add hasLabels to existing matchingLabels", func() {
+		listOpts := &client.ListOptions{}
+
+		matchingLabels := client.MatchingLabels(map[string]string{"k": "v"})
+		matchingLabels.ApplyToList(listOpts)
+
+		hasLabel := client.HasLabels([]string{"labelApe"})
+		hasLabel.ApplyToList(listOpts)
+		Expect(listOpts.LabelSelector.String()).To(Equal("k=v,labelApe"))
+	})
+})