From 64e50ce86a0a764166f9cd160c00a9972b7c8971 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 2 Mar 2022 16:03:38 +0100 Subject: [PATCH] search: remove parsing JSON filters out of the backend All other endpoints handle this in the API; given that the JSON format for filters is part of the API, it makes sense to handle it there, and not have that concept leak into further down the code. Signed-off-by: Sebastiaan van Stijn --- api/server/router/image/backend.go | 2 +- api/server/router/image/image_routes.go | 7 ++- daemon/images/image_search.go | 6 +- daemon/images/image_search_test.go | 76 +++++++++++++++---------- 4 files changed, 55 insertions(+), 36 deletions(-) diff --git a/api/server/router/image/backend.go b/api/server/router/image/backend.go index d54d2c0e6c..c67093950d 100644 --- a/api/server/router/image/backend.go +++ b/api/server/router/image/backend.go @@ -37,5 +37,5 @@ type importExportBackend interface { type registryBackend interface { PullImage(ctx context.Context, image, tag string, platform *specs.Platform, metaHeaders map[string][]string, authConfig *types.AuthConfig, outStream io.Writer) error PushImage(ctx context.Context, image, tag string, metaHeaders map[string][]string, authConfig *types.AuthConfig, outStream io.Writer) error - SearchRegistryForImages(ctx context.Context, filtersArgs string, term string, limit int, authConfig *types.AuthConfig, metaHeaders map[string][]string) (*registry.SearchResults, error) + SearchRegistryForImages(ctx context.Context, searchFilters filters.Args, term string, limit int, authConfig *types.AuthConfig, metaHeaders map[string][]string) (*registry.SearchResults, error) } diff --git a/api/server/router/image/image_routes.go b/api/server/router/image/image_routes.go index 09f812dedf..05a7d6bfbb 100644 --- a/api/server/router/image/image_routes.go +++ b/api/server/router/image/image_routes.go @@ -298,7 +298,12 @@ func (s *imageRouter) getImagesSearch(ctx context.Context, w http.ResponseWriter return errdefs.InvalidParameter(errors.Wrap(err, "invalid limit specified")) } } - query, err := s.backend.SearchRegistryForImages(ctx, r.Form.Get("filters"), r.Form.Get("term"), limit, config, headers) + searchFilters, err := filters.FromJSON(r.Form.Get("filters")) + if err != nil { + return err + } + + query, err := s.backend.SearchRegistryForImages(ctx, searchFilters, r.Form.Get("term"), limit, config, headers) if err != nil { return err } diff --git a/daemon/images/image_search.go b/daemon/images/image_search.go index 8b65ec709c..d991ce837c 100644 --- a/daemon/images/image_search.go +++ b/daemon/images/image_search.go @@ -21,14 +21,10 @@ var acceptedSearchFilterTags = map[string]bool{ // // TODO: this could be implemented in a registry service instead of the image // service. -func (i *ImageService) SearchRegistryForImages(ctx context.Context, filtersArgs string, term string, limit int, +func (i *ImageService) SearchRegistryForImages(ctx context.Context, searchFilters filters.Args, term string, limit int, authConfig *types.AuthConfig, headers map[string][]string) (*registrytypes.SearchResults, error) { - searchFilters, err := filters.FromJSON(filtersArgs) - if err != nil { - return nil, err - } if err := searchFilters.Validate(acceptedSearchFilterTags); err != nil { return nil, err } diff --git a/daemon/images/image_search_test.go b/daemon/images/image_search_test.go index c53f8c7f1b..112802496c 100644 --- a/daemon/images/image_search_test.go +++ b/daemon/images/image_search_test.go @@ -7,7 +7,9 @@ import ( "testing" "github.com/docker/docker/api/types" + "github.com/docker/docker/api/types/filters" registrytypes "github.com/docker/docker/api/types/registry" + "github.com/docker/docker/errdefs" "github.com/docker/docker/registry" ) @@ -21,7 +23,7 @@ type fakeService struct { func (s *fakeService) Search(ctx context.Context, term string, limit int, authConfig *types.AuthConfig, userAgent string, headers map[string][]string) (*registrytypes.SearchResults, error) { if s.shouldReturnError { - return nil, errors.New("Search unknown error") + return nil, errdefs.Unknown(errors.New("search unknown error")) } return ®istrytypes.SearchResults{ Query: s.term, @@ -32,44 +34,49 @@ func (s *fakeService) Search(ctx context.Context, term string, limit int, authCo func TestSearchRegistryForImagesErrors(t *testing.T) { errorCases := []struct { - filtersArgs string + filtersArgs filters.Args shouldReturnError bool expectedError string }{ { - expectedError: "Search unknown error", + expectedError: "search unknown error", shouldReturnError: true, }, { - filtersArgs: "invalid json", - expectedError: "invalid character 'i' looking for beginning of value", - }, - { - filtersArgs: `{"type":{"custom":true}}`, + filtersArgs: filters.NewArgs(filters.Arg("type", "custom")), expectedError: "invalid filter 'type'", }, { - filtersArgs: `{"is-automated":{"invalid":true}}`, + filtersArgs: filters.NewArgs(filters.Arg("is-automated", "invalid")), expectedError: "invalid filter 'is-automated=[invalid]'", }, { - filtersArgs: `{"is-automated":{"true":true,"false":true}}`, + filtersArgs: filters.NewArgs( + filters.Arg("is-automated", "true"), + filters.Arg("is-automated", "false"), + ), expectedError: "invalid filter 'is-automated", }, { - filtersArgs: `{"is-official":{"invalid":true}}`, + filtersArgs: filters.NewArgs(filters.Arg("is-official", "invalid")), expectedError: "invalid filter 'is-official=[invalid]'", }, { - filtersArgs: `{"is-official":{"true":true,"false":true}}`, + filtersArgs: filters.NewArgs( + filters.Arg("is-official", "true"), + filters.Arg("is-official", "false"), + ), expectedError: "invalid filter 'is-official", }, { - filtersArgs: `{"stars":{"invalid":true}}`, + filtersArgs: filters.NewArgs(filters.Arg("stars", "invalid")), expectedError: "invalid filter 'stars=invalid'", }, { - filtersArgs: `{"stars":{"1":true,"invalid":true}}`, + filtersArgs: filters.NewArgs( + filters.Arg("stars", "1"), + filters.Arg("stars", "invalid"), + ), expectedError: "invalid filter 'stars=invalid'", }, } @@ -86,23 +93,30 @@ func TestSearchRegistryForImagesErrors(t *testing.T) { if !strings.Contains(err.Error(), e.expectedError) { t.Errorf("%d: expected error to contain %s, got %s", index, e.expectedError, err.Error()) } + if e.shouldReturnError { + if !errdefs.IsUnknown(err) { + t.Errorf("%d: expected expected an errdefs.ErrUnknown, got: %T: %v", index, err, err) + } + continue + } + if !errdefs.IsInvalidParameter(err) { + t.Errorf("%d: expected expected an errdefs.ErrInvalidParameter, got: %T: %v", index, err, err) + } } } func TestSearchRegistryForImages(t *testing.T) { term := "term" successCases := []struct { - filtersArgs string + filtersArgs filters.Args registryResults []registrytypes.SearchResult expectedResults []registrytypes.SearchResult }{ { - filtersArgs: "", registryResults: []registrytypes.SearchResult{}, expectedResults: []registrytypes.SearchResult{}, }, { - filtersArgs: "", registryResults: []registrytypes.SearchResult{ { Name: "name", @@ -117,7 +131,7 @@ func TestSearchRegistryForImages(t *testing.T) { }, }, { - filtersArgs: `{"is-automated":{"true":true}}`, + filtersArgs: filters.NewArgs(filters.Arg("is-automated", "true")), registryResults: []registrytypes.SearchResult{ { Name: "name", @@ -127,7 +141,7 @@ func TestSearchRegistryForImages(t *testing.T) { expectedResults: []registrytypes.SearchResult{}, }, { - filtersArgs: `{"is-automated":{"true":true}}`, + filtersArgs: filters.NewArgs(filters.Arg("is-automated", "true")), registryResults: []registrytypes.SearchResult{ { Name: "name", @@ -144,7 +158,7 @@ func TestSearchRegistryForImages(t *testing.T) { }, }, { - filtersArgs: `{"is-automated":{"false":true}}`, + filtersArgs: filters.NewArgs(filters.Arg("is-automated", "false")), registryResults: []registrytypes.SearchResult{ { Name: "name", @@ -155,7 +169,7 @@ func TestSearchRegistryForImages(t *testing.T) { expectedResults: []registrytypes.SearchResult{}, }, { - filtersArgs: `{"is-automated":{"false":true}}`, + filtersArgs: filters.NewArgs(filters.Arg("is-automated", "false")), registryResults: []registrytypes.SearchResult{ { Name: "name", @@ -172,7 +186,7 @@ func TestSearchRegistryForImages(t *testing.T) { }, }, { - filtersArgs: `{"is-official":{"true":true}}`, + filtersArgs: filters.NewArgs(filters.Arg("is-official", "true")), registryResults: []registrytypes.SearchResult{ { Name: "name", @@ -182,7 +196,7 @@ func TestSearchRegistryForImages(t *testing.T) { expectedResults: []registrytypes.SearchResult{}, }, { - filtersArgs: `{"is-official":{"true":true}}`, + filtersArgs: filters.NewArgs(filters.Arg("is-official", "true")), registryResults: []registrytypes.SearchResult{ { Name: "name", @@ -199,7 +213,7 @@ func TestSearchRegistryForImages(t *testing.T) { }, }, { - filtersArgs: `{"is-official":{"false":true}}`, + filtersArgs: filters.NewArgs(filters.Arg("is-official", "false")), registryResults: []registrytypes.SearchResult{ { Name: "name", @@ -210,7 +224,7 @@ func TestSearchRegistryForImages(t *testing.T) { expectedResults: []registrytypes.SearchResult{}, }, { - filtersArgs: `{"is-official":{"false":true}}`, + filtersArgs: filters.NewArgs(filters.Arg("is-official", "false")), registryResults: []registrytypes.SearchResult{ { Name: "name", @@ -227,7 +241,7 @@ func TestSearchRegistryForImages(t *testing.T) { }, }, { - filtersArgs: `{"stars":{"0":true}}`, + filtersArgs: filters.NewArgs(filters.Arg("stars", "0")), registryResults: []registrytypes.SearchResult{ { Name: "name", @@ -244,7 +258,7 @@ func TestSearchRegistryForImages(t *testing.T) { }, }, { - filtersArgs: `{"stars":{"1":true}}`, + filtersArgs: filters.NewArgs(filters.Arg("stars", "1")), registryResults: []registrytypes.SearchResult{ { Name: "name", @@ -255,7 +269,7 @@ func TestSearchRegistryForImages(t *testing.T) { expectedResults: []registrytypes.SearchResult{}, }, { - filtersArgs: `{"stars":{"1":true}}`, + filtersArgs: filters.NewArgs(filters.Arg("stars", "1")), registryResults: []registrytypes.SearchResult{ { Name: "name0", @@ -277,7 +291,11 @@ func TestSearchRegistryForImages(t *testing.T) { }, }, { - filtersArgs: `{"stars":{"1":true}, "is-official":{"true":true}, "is-automated":{"true":true}}`, + filtersArgs: filters.NewArgs( + filters.Arg("stars", "1"), + filters.Arg("is-official", "true"), + filters.Arg("is-automated", "true"), + ), registryResults: []registrytypes.SearchResult{ { Name: "name0",