fix: Refactor filters parsing (runfinch#181)

fix:Rrefactor filters parsing

Signed-off-by: Arjun Raja Yogidas <[email protected]>

Author: coderbirju

api/handlers/container/list.go

@@ -4,7 +4,6 @@
 package container
 
 import (
-	"encoding/json"
 	"fmt"
 	"net/http"
 	"net/url"
@@ -14,6 +13,7 @@ import (
 	ncTypes "github.com/containerd/nerdctl/v2/pkg/api/types"
 
 	"github.com/runfinch/finch-daemon/api/response"
+	"github.com/runfinch/finch-daemon/api/types"
 )
 
 const (
@@ -45,7 +45,7 @@ func (h *handler) list(w http.ResponseWriter, r *http.Request) {
 		response.JSON(w, http.StatusBadRequest, response.NewErrorFromMsg(fmt.Sprintf("invalid query parameter \"size\": %s", err)))
 		return
 	}
-	filters, err := parseFiltersQP(q)
+	filters, err := NerdctlFiltersFromAPIFilters(q)
 	if err != nil {
 		response.JSON(w, http.StatusBadRequest, response.NewErrorFromMsg(fmt.Sprintf("invalid query parameter \"filters\": %s", err)))
 		return
@@ -59,7 +59,7 @@ func (h *handler) list(w http.ResponseWriter, r *http.Request) {
 		LastN:    limit,
 		Truncate: true,
 		Size:     size,
-		Filters:  nerdctlFiltersFromAPIFilters(filters),
+		Filters:  filters,
 	}
 	containers, err := h.service.List(ctx, listOpts)
 	if err != nil {
@@ -69,16 +69,6 @@ func (h *handler) list(w http.ResponseWriter, r *http.Request) {
 	response.JSON(w, http.StatusOK, containers)
 }
 
-func nerdctlFiltersFromAPIFilters(filters map[string][]string) []string {
-	var ncFilters []string
-	for filterType, filterList := range filters {
-		for _, f := range filterList {
-			ncFilters = append(ncFilters, fmt.Sprintf("%s=%s", filterType, f))
-		}
-	}
-	return ncFilters
-}
-
 func parseBoolQP(q url.Values, key string, defaultV bool) (bool, error) {
 	v := q.Get(key)
 	if v == "" {
@@ -105,14 +95,17 @@ func parseIntQP(q url.Values, key string, defaultV int) (int, error) {
 	}
 }
 
-func parseFiltersQP(q url.Values) (map[string][]string, error) {
-	filters := make(map[string][]string)
-	filterQuery := q.Get(filtersKey)
-	if filterQuery != "" {
-		err := json.Unmarshal([]byte(filterQuery), &filters)
-		if err != nil {
-			return nil, err
+func NerdctlFiltersFromAPIFilters(query url.Values) ([]string, error) {
+	filters, err := types.ParseFilterArgs(query)
+	if err != nil {
+		return nil, err
+	}
+
+	var ncFilters []string
+	for filterType, filterList := range filters.ToLegacyFormat() {
+		for _, f := range filterList {
+			ncFilters = append(ncFilters, fmt.Sprintf("%s=%s", filterType, f))
 		}
 	}
-	return filters, nil
+	return ncFilters, nil
 }

api/handlers/container/list_test.go

@@ -58,6 +58,7 @@ var _ = Describe("Container List API", func() {
 		It("should return containers and 200 status code upon success with all query parameters", func() {
 			req, err := http.NewRequest(http.MethodGet, "/containers/json?all=true&limit=1&size=true&filters={\"status\": [\"paused\"]}", nil)
 			Expect(err).Should(BeNil())
+
 			listOpts := ncTypes.ContainerListOptions{
 				GOptions: globalOpts,
 				All:      true,
@@ -119,7 +120,7 @@ var _ = Describe("Container List API", func() {
 		It("should return 400 status code when there is error parsing filters", func() {
 			req, err := http.NewRequest(http.MethodGet, "/containers/json?filters=invalid", nil)
 			Expect(err).Should(BeNil())
-			errorMsg := fmt.Sprintf("invalid query parameter \\\"filters\\\": %s", fmt.Errorf("invalid character 'i' looking for beginning of value"))
+			errorMsg := fmt.Sprintf("invalid query parameter \\\"filters\\\": %s", fmt.Errorf("error parsing filters: invalid character 'i' looking for beginning of value"))
 
 			h.list(rr, req)
 			Expect(rr.Body).Should(MatchJSON(`{"message": "` + errorMsg + `"}`))

api/handlers/system/events.go

@@ -10,20 +10,17 @@ import (
 
 	eventtype "github.com/runfinch/finch-daemon/api/events"
 	"github.com/runfinch/finch-daemon/api/response"
+	"github.com/runfinch/finch-daemon/api/types"
 )
 
 func (h *handler) events(w http.ResponseWriter, r *http.Request) {
-	filters := make(map[string][]string)
-	filterQuery := r.URL.Query().Get("filters")
-	if filterQuery != "" {
-		err := json.Unmarshal([]byte(filterQuery), &filters)
-		if err != nil {
-			response.JSON(w, http.StatusBadRequest, response.NewErrorFromMsg(fmt.Sprintf("invalid filter: %s", err)))
-			return
-		}
+	filters, err := types.ParseFilterArgs(r.URL.Query())
+	if err != nil {
+		response.JSON(w, http.StatusBadRequest, response.NewErrorFromMsg(fmt.Sprintf("invalid filter: %s", err)))
+		return
 	}
 
-	eventCh, errCh := h.service.SubscribeEvents(r.Context(), filters)
+	eventCh, errCh := h.service.SubscribeEvents(r.Context(), filters.ToLegacyFormat())
 
 	encoder := json.NewEncoder(w)
 

api/handlers/system/events_test.go

@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"sync"
 	"time"
 
@@ -56,6 +57,7 @@ var _ = Describe("Events API", func() {
 		}
 		mockEventJson, _ = json.Marshal(mockEvent)
 	})
+
 	It("should return 200 and stream events on success", func() {
 		// in order to add events to the channels while the events handler is running, we need to either run the handler
 		// or the channel publisher in a separate goroutine as the events handler will block until it returns. because all
@@ -104,14 +106,15 @@ var _ = Describe("Events API", func() {
 		Expect(err).Should(BeNil())
 		Expect(line).Should(MatchJSON(mockEventJson))
 	})
+
 	It("should return 400 if filters are not in the correct format", func() {
 		req, _ := http.NewRequest(http.MethodGet, "/events?filters=bad", nil)
-		req = mux.SetURLVars(req, map[string]string{"filters": "bad"})
 
 		h.events(rr, req)
 		Expect(rr).Should(HaveHTTPStatus(http.StatusBadRequest))
-		Expect(rr.Body).Should(MatchJSON(`{"message": "invalid filter: invalid character 'b' looking for beginning of value"}`))
+		Expect(rr.Body).Should(MatchJSON(`{"message": "invalid filter: error parsing filters: invalid character 'b' looking for beginning of value"}`))
 	})
+
 	It("should forward filters to the service", func() {
 		var waitGroup sync.WaitGroup
 
@@ -138,6 +141,7 @@ var _ = Describe("Events API", func() {
 
 		Expect(rr).Should(HaveHTTPStatus(http.StatusOK))
 	})
+
 	It("should stop streaming if an error is received", func() {
 		var waitGroup sync.WaitGroup
 
@@ -165,4 +169,40 @@ var _ = Describe("Events API", func() {
 		close(mockEventCh)
 		close(mockErrCh)
 	})
+
+	It("should handle multiple filters", func() {
+		var waitGroup sync.WaitGroup
+		mockEventCh = make(chan *events.Event)
+		mockErrCh = make(chan error)
+
+		filters := map[string]map[string]bool{
+			"type":   {"container": true},
+			"status": {"start": true, "stop": true},
+		}
+		filtersJSON, err := json.Marshal(filters)
+		Expect(err).Should(BeNil())
+
+		url := fmt.Sprintf("/events?filters=%s", url.QueryEscape(string(filtersJSON)))
+		req, err := http.NewRequest(http.MethodGet, url, nil)
+		Expect(err).Should(BeNil())
+
+		s.EXPECT().SubscribeEvents(req.Context(), map[string][]string{
+			"type":   {"container"},
+			"status": {"start", "stop"},
+		}).Return(mockEventCh, mockErrCh)
+		logger.EXPECT().Debugf("received error, exiting: %s", gomock.Any())
+
+		waitGroup.Add(1)
+		go func() {
+			defer waitGroup.Done()
+			h.events(rr, req)
+		}()
+
+		close(mockEventCh)
+		close(mockErrCh)
+
+		waitGroup.Wait()
+
+		Expect(rr).Should(HaveHTTPStatus(http.StatusOK))
+	})
 })

api/types/filter_types.go

@@ -0,0 +1,123 @@
+// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package types
+
+import (
+	"encoding/json"
+	"fmt"
+	"net/url"
+)
+
+// Filters represents a collection of filter types and their values.
+type Filters struct {
+	filterSet map[string]map[string]bool
+}
+
+// FilterKeyVal represents a single key-value pair for a filter.
+type FilterKeyVal struct {
+	Key   string
+	Value string
+}
+
+// newFilters creates a new Filters instance with the provided filter key-value pairs.
+func newFilters(filtersList ...FilterKeyVal) Filters {
+	filters := Filters{filterSet: map[string]map[string]bool{}}
+	for _, arg := range filtersList {
+		filters.add(arg.Key, arg.Value)
+	}
+	return filters
+}
+
+// getFiltersKeys returns all values for a given filter key.
+func (filters Filters) getFiltersKeys(key string) []string {
+	values := filters.filterSet[key]
+	if values == nil {
+		return make([]string, 0)
+	}
+	slice := make([]string, 0, len(values))
+	for key := range values {
+		slice = append(slice, key)
+	}
+	return slice
+}
+
+// add inserts a new key-value pair into the filter set.
+func (filters Filters) add(key, value string) {
+	if _, ok := filters.filterSet[key]; ok {
+		filters.filterSet[key][value] = true
+	} else {
+		filters.filterSet[key] = map[string]bool{value: true}
+	}
+}
+
+// getFilterFromLegacy converts the legacy filter format (map[string][]string)
+// to the new filter format (map[string]map[string]bool),
+// legacy filter format is currently the default format in the API spec.
+func getFilterFromLegacy(d map[string][]string) map[string]map[string]bool {
+	m := map[string]map[string]bool{}
+	for k, v := range d {
+		values := map[string]bool{}
+		for _, vv := range v {
+			values[vv] = true
+		}
+		m[k] = values
+	}
+	return m
+}
+
+// getFilterJSON parses a JSON string into a Filters struct.
+func getFilterJSON(p string) (Filters, error) {
+	filters := newFilters()
+
+	if p == "" {
+		return filters, nil
+	}
+
+	raw := []byte(p)
+	err := json.Unmarshal(raw, &filters)
+	if err == nil {
+		return filters, nil
+	}
+
+	// Fallback to parsing arguments in the legacy slice format
+	deprecated := map[string][]string{}
+	if legacyErr := json.Unmarshal(raw, &deprecated); legacyErr != nil {
+		return filters, legacyErr
+	}
+
+	filters.filterSet = getFilterFromLegacy(deprecated)
+	return filters, nil
+}
+
+// UnmarshalJSON implements the json.Unmarshaler interface for Filters.
+func (filters Filters) UnmarshalJSON(raw []byte) error {
+	return json.Unmarshal(raw, &filters.filterSet)
+}
+
+// ToLegacyFormat converts the Filters struct to the legacy format (map[string][]string).
+func (filters Filters) ToLegacyFormat() map[string][]string {
+	result := make(map[string][]string)
+	for key := range filters.filterSet {
+		values := filters.getFiltersKeys(key)
+		if len(values) > 0 {
+			result[key] = values
+		}
+	}
+	return result
+}
+
+// ParseFilterArgs extracts and parses filters from URL query parameters.
+func ParseFilterArgs(query url.Values) (Filters, error) {
+	filterQuery := query.Get("filters")
+	if filterQuery == "" {
+		return newFilters(), nil
+	}
+
+	filters, err := getFilterJSON(filterQuery)
+	if err != nil {
+		return Filters{}, fmt.Errorf("error parsing filters: %v", err)
+	}
+
+	return filters, nil
+}

e2e/tests/container_list.go

@@ -226,9 +226,32 @@ func ContainerList(opt *option.Option) {
 			body, err := io.ReadAll(res.Body)
 			Expect(err).Should(BeNil())
 			defer res.Body.Close()
-			errorMsg := fmt.Sprintf("invalid query parameter \\\"filters\\\": %s", fmt.Errorf("invalid character 'i' looking for beginning of value"))
+			errorMsg := fmt.Sprintf("invalid query parameter \\\"filters\\\": %s", fmt.Errorf("error parsing filters: invalid character 'i' looking for beginning of value"))
 			Expect(body).Should(MatchJSON(`{"message": "` + errorMsg + `"}`))
 		})
+		It("should list the running containers with new filter format", func() {
+			id := command.StdoutStr(opt, "run", "-d", "--name", testContainerName, "--label", "com.example.foo=bar", defaultImage, "sleep", "infinity")
+			want := []types.ContainerListItem{
+				{
+					Id:    id[:12],
+					Names: []string{wantContainerName},
+				},
+			}
+
+			// Using the new filter format
+			newFormatFilter := `{"label":{"com.example.foo=bar":true}}`
+			res, err := uClient.Get(client.ConvertToFinchUrl(version, "/containers/json?all=true&filters="+newFormatFilter))
+			Expect(err).Should(BeNil())
+			Expect(res.StatusCode).Should(Equal(http.StatusOK))
+
+			var got []types.ContainerListItem
+			err = json.NewDecoder(res.Body).Decode(&got)
+			Expect(err).Should(BeNil())
+			Expect(len(got)).Should(Equal(1))
+
+			got = filterContainerList(got)
+			Expect(got).Should(ContainElements(want))
+		})
 	})
 }