kotovalexarian-likes-github
/
moby--moby
mirror of https://github.com/moby/moby.git
1
0
Fork 0
Code Releases Activity

Merge pull request #43312 from thaJeztah/search_fixes 

API: fix status codes for search, and some refactoring for splitting  out
This commit is contained in:
Sebastiaan van Stijn 2022-03-18 18:30:50 +01:00 committed by GitHub
parent 8dfecb96d0 64e50ce86a
commit df32377b65
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
21 changed files with 112 additions and 81 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
api/server/router/container/container_routes.go
View File

@ -721,7 +721,7 @@ func (s *containerRouter) postContainersPrune(ctx context.Context, w http.Respon
pruneFilters, err := filters.FromJSON(r.Form.Get("filters"))
if err != nil {
return errdefs.InvalidParameter(err)
return err
}
pruneReport, err := s.backend.ContainersPrune(ctx, pruneFilters)

2
api/server/router/image/backend.go
View File

@ -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)
}

19
api/server/router/image/image_routes.go
View File

@ -16,7 +16,6 @@ import (
"github.com/docker/docker/errdefs"
"github.com/docker/docker/pkg/ioutils"
"github.com/docker/docker/pkg/streamformatter"
"github.com/docker/docker/registry"
specs "github.com/opencontainers/image-spec/specs-go/v1"
"github.com/pkg/errors"
)
@ -290,15 +289,21 @@ func (s *imageRouter) getImagesSearch(ctx context.Context, w http.ResponseWriter
headers[k] = v
}
}
limit := registry.DefaultSearchLimit
var limit int
if r.Form.Get("limit") != "" {
limitValue, err := strconv.Atoi(r.Form.Get("limit"))
if err != nil {
return err
var err error
limit, err = strconv.Atoi(r.Form.Get("limit"))
if err != nil || limit < 0 {
return errdefs.InvalidParameter(errors.Wrap(err, "invalid limit specified"))
}
limit = limitValue
}
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
}

2
api/server/router/network/network_routes.go
View File

@ -30,7 +30,7 @@ func (n *networkRouter) getNetworksList(ctx context.Context, w http.ResponseWrit
}
if err := network.ValidateFilters(filter); err != nil {
return errdefs.InvalidParameter(err)
return err
}
var list []types.NetworkResource

2
api/server/router/swarm/cluster_routes.go
View File

@ -164,7 +164,7 @@ func (sr *swarmRouter) getServices(ctx context.Context, w http.ResponseWriter, r
}
filter, err := filters.FromJSON(r.Form.Get("filters"))
if err != nil {
return errdefs.InvalidParameter(err)
return err
}
// the status query parameter is only support in API versions >= 1.41. If

2
api/server/router/volume/volume_routes.go
View File

@ -21,7 +21,7 @@ func (v *volumeRouter) getVolumesList(ctx context.Context, w http.ResponseWriter
filters, err := filters.FromJSON(r.Form.Get("filters"))
if err != nil {
return errdefs.InvalidParameter(errors.Wrap(err, "error reading volume filters"))
return errors.Wrap(err, "error reading volume filters")
}
volumes, warnings, err := v.backend.List(ctx, filters)
if err != nil {

9
api/types/filters/parse.go
View File

@ -9,6 +9,7 @@ import (
"strings"
"github.com/docker/docker/api/types/versions"
"github.com/pkg/errors"
)
// Args stores a mapping of keys to a set of multiple values.
@ -97,7 +98,7 @@ func FromJSON(p string) (Args, error) {
// Fallback to parsing arguments in the legacy slice format
deprecated := map[string][]string{}
if legacyErr := json.Unmarshal(raw, &deprecated); legacyErr != nil {
return args, err
return args, invalidFilter{errors.Wrap(err, "invalid filter")}
}
args.fields = deprecatedArgs(deprecated)
@ -247,10 +248,10 @@ func (args Args) Contains(field string) bool {
return ok
}
type invalidFilter string
type invalidFilter struct{ error }
func (e invalidFilter) Error() string {
return "Invalid filter '" + string(e) + "'"
return e.error.Error()
}
func (invalidFilter) InvalidParameter() {}
@ -260,7 +261,7 @@ func (invalidFilter) InvalidParameter() {}
func (args Args) Validate(accepted map[string]bool) error {
for name := range args.fields {
if !accepted[name] {
return invalidFilter(name)
return invalidFilter{errors.New("invalid filter '" + name + "'")}
}
}
return nil

14
api/types/filters/parse_test.go
View File

@ -69,9 +69,14 @@ func TestFromJSON(t *testing.T) {
}
for _, invalid := range invalids {
if _, err := FromJSON(invalid); err == nil {
_, err := FromJSON(invalid)
if err == nil {
t.Fatalf("Expected an error with %v, got nothing", invalid)
}
var invalidFilterError invalidFilter
if !errors.As(err, &invalidFilterError) {
t.Fatalf("Expected an invalidFilter error, got %T", err)
}
}
for expectedArgs, matchers := range valid {
@ -327,9 +332,14 @@ func TestValidate(t *testing.T) {
}
f.Add("bogus", "running")
if err := f.Validate(valid); err == nil {
err := f.Validate(valid)
if err == nil {
t.Fatal("Expected to return an error, got nil")
}
var invalidFilterError invalidFilter
if !errors.As(err, &invalidFilterError) {
t.Fatalf("Expected an invalidFilter error, got %T", err)
}
}
func TestWalkValues(t *testing.T) {

2
daemon/errors.go
View File

@ -115,7 +115,7 @@ type invalidFilter struct {
}
func (e invalidFilter) Error() string {
msg := "Invalid filter '" + e.filter
msg := "invalid filter '" + e.filter
if e.value != nil {
msg += fmt.Sprintf("=%s", e.value)
}

6
daemon/images/image_search.go
View File

@ -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
}

94
daemon/images/image_search_test.go
View File

@ -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 &registrytypes.SearchResults{
Query: s.term,
@ -32,45 +34,50 @@ 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: filters.NewArgs(filters.Arg("type", "custom")),
expectedError: "invalid filter 'type'",
},
{
filtersArgs: `{"type":{"custom":true}}`,
expectedError: "Invalid filter 'type'",
filtersArgs: filters.NewArgs(filters.Arg("is-automated", "invalid")),
expectedError: "invalid filter 'is-automated=[invalid]'",
},
{
filtersArgs: `{"is-automated":{"invalid":true}}`,
expectedError: "Invalid filter 'is-automated=[invalid]'",
filtersArgs: filters.NewArgs(
filters.Arg("is-automated", "true"),
filters.Arg("is-automated", "false"),
),
expectedError: "invalid filter 'is-automated",
},
{
filtersArgs: `{"is-automated":{"true":true,"false":true}}`,
expectedError: "Invalid filter 'is-automated",
filtersArgs: filters.NewArgs(filters.Arg("is-official", "invalid")),
expectedError: "invalid filter 'is-official=[invalid]'",
},
{
filtersArgs: `{"is-official":{"invalid":true}}`,
expectedError: "Invalid filter 'is-official=[invalid]'",
filtersArgs: filters.NewArgs(
filters.Arg("is-official", "true"),
filters.Arg("is-official", "false"),
),
expectedError: "invalid filter 'is-official",
},
{
filtersArgs: `{"is-official":{"true":true,"false":true}}`,
expectedError: "Invalid filter 'is-official",
filtersArgs: filters.NewArgs(filters.Arg("stars", "invalid")),
expectedError: "invalid filter 'stars=invalid'",
},
{
filtersArgs: `{"stars":{"invalid":true}}`,
expectedError: "Invalid filter 'stars=invalid'",
},
{
filtersArgs: `{"stars":{"1":true,"invalid":true}}`,
expectedError: "Invalid filter 'stars=invalid'",
filtersArgs: filters.NewArgs(
filters.Arg("stars", "1"),
filters.Arg("stars", "invalid"),
),
expectedError: "invalid filter 'stars=invalid'",
},
}
for index, e := range errorCases {
@ -79,30 +86,37 @@ func TestSearchRegistryForImagesErrors(t *testing.T) {
shouldReturnError: e.shouldReturnError,
},
}
_, err := daemon.SearchRegistryForImages(context.Background(), e.filtersArgs, "term", 25, nil, map[string][]string{})
_, err := daemon.SearchRegistryForImages(context.Background(), e.filtersArgs, "term", 0, nil, map[string][]string{})
if err == nil {
t.Errorf("%d: expected an error, got nothing", index)
}
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",
@ -326,7 +344,7 @@ func TestSearchRegistryForImages(t *testing.T) {
results: s.registryResults,
},
}
results, err := daemon.SearchRegistryForImages(context.Background(), s.filtersArgs, term, 25, nil, map[string][]string{})
results, err := daemon.SearchRegistryForImages(context.Background(), s.filtersArgs, term, 0, nil, map[string][]string{})
if err != nil {
t.Errorf("%d: %v", index, err)
}

2
daemon/images/locals.go
View File

@ -12,7 +12,7 @@ type invalidFilter struct {
}
func (e invalidFilter) Error() string {
msg := "Invalid filter '" + e.filter
msg := "invalid filter '" + e.filter
if e.value != nil {
msg += fmt.Sprintf("=%s", e.value)
}

2
daemon/list_test.go
View File

@ -91,7 +91,7 @@ func TestListInvalidFilter(t *testing.T) {
_, err = d.Containers(&types.ContainerListOptions{
Filters: f,
})
assert.Assert(t, is.Error(err, "Invalid filter 'invalid'"))
assert.Assert(t, is.Error(err, "invalid filter 'invalid'"))
}
func TestNameFilter(t *testing.T) {

4
integration-cli/docker_cli_images_test.go
View File

@ -68,7 +68,7 @@ func (s *DockerSuite) TestImagesOrderedByCreationDate(c *testing.T) {
func (s *DockerSuite) TestImagesErrorWithInvalidFilterNameTest(c *testing.T) {
out, _, err := dockerCmdWithError("images", "-f", "FOO=123")
assert.ErrorContains(c, err, "")
assert.Assert(c, strings.Contains(out, "Invalid filter"))
assert.Assert(c, strings.Contains(out, "invalid filter"))
}
func (s *DockerSuite) TestImagesFilterLabelMatch(c *testing.T) {
@ -252,7 +252,7 @@ func (s *DockerSuite) TestImagesEnsureDanglingImageOnlyListedOnce(c *testing.T)
func (s *DockerSuite) TestImagesWithIncorrectFilter(c *testing.T) {
out, _, err := dockerCmdWithError("images", "-f", "dangling=invalid")
assert.ErrorContains(c, err, "")
assert.Assert(c, strings.Contains(out, "Invalid filter"))
assert.Assert(c, strings.Contains(out, "invalid filter"))
}
func (s *DockerSuite) TestImagesEnsureOnlyHeadsImagesShown(c *testing.T) {

2
integration-cli/docker_cli_ps_test.go
View File

@ -203,7 +203,7 @@ func (s *DockerSuite) TestPsListContainersFilterStatus(c *testing.T) {
assert.Equal(c, RemoveOutputForExistingElements(containerOut, existingContainers), secondID)
result := cli.Docker(cli.Args("ps", "-a", "-q", "--filter=status=rubbish"), cli.WithTimeout(time.Second*60))
err := "Invalid filter 'status=rubbish'"
err := "invalid filter 'status=rubbish'"
if versions.LessThan(testEnv.DaemonAPIVersion(), "1.32") {
err = "Unrecognised filter value for status: rubbish"
}

10
integration-cli/docker_cli_search_test.go
View File

@ -17,19 +17,19 @@ func (s *DockerSuite) TestSearchOnCentralRegistry(c *testing.T) {
func (s *DockerSuite) TestSearchStarsOptionWithWrongParameter(c *testing.T) {
out, _, err := dockerCmdWithError("search", "--filter", "stars=a", "busybox")
assert.ErrorContains(c, err, "", out)
assert.Assert(c, strings.Contains(out, "Invalid filter"), "couldn't find the invalid filter warning")
assert.Assert(c, strings.Contains(out, "invalid filter"), "couldn't find the invalid filter warning")
out, _, err = dockerCmdWithError("search", "-f", "stars=a", "busybox")
assert.ErrorContains(c, err, "", out)
assert.Assert(c, strings.Contains(out, "Invalid filter"), "couldn't find the invalid filter warning")
assert.Assert(c, strings.Contains(out, "invalid filter"), "couldn't find the invalid filter warning")
out, _, err = dockerCmdWithError("search", "-f", "is-automated=a", "busybox")
assert.ErrorContains(c, err, "", out)
assert.Assert(c, strings.Contains(out, "Invalid filter"), "couldn't find the invalid filter warning")
assert.Assert(c, strings.Contains(out, "invalid filter"), "couldn't find the invalid filter warning")
out, _, err = dockerCmdWithError("search", "-f", "is-official=a", "busybox")
assert.ErrorContains(c, err, "", out)
assert.Assert(c, strings.Contains(out, "Invalid filter"), "couldn't find the invalid filter warning")
assert.Assert(c, strings.Contains(out, "invalid filter"), "couldn't find the invalid filter warning")
}
func (s *DockerSuite) TestSearchCmdOptions(c *testing.T) {
@ -73,7 +73,7 @@ func (s *DockerSuite) TestSearchWithLimit(c *testing.T) {
assert.Equal(c, len(outSlice), limit+2) // 1 header, 1 carriage return
}
for _, limit := range []int{-1, 0, 101} {
for _, limit := range []int{-1, 101} {
_, _, err := dockerCmdWithError("search", fmt.Sprintf("--limit=%d", limit), "docker")
assert.ErrorContains(c, err, "")
}

4
integration-cli/docker_cli_volume_test.go
View File

@ -166,13 +166,13 @@ func (s *DockerSuite) TestVolumeCLILsFilterDangling(c *testing.T) {
func (s *DockerSuite) TestVolumeCLILsErrorWithInvalidFilterName(c *testing.T) {
out, _, err := dockerCmdWithError("volume", "ls", "-f", "FOO=123")
assert.ErrorContains(c, err, "")
assert.Assert(c, strings.Contains(out, "Invalid filter"))
assert.Assert(c, strings.Contains(out, "invalid filter"))
}
func (s *DockerSuite) TestVolumeCLILsWithIncorrectFilterValue(c *testing.T) {
out, _, err := dockerCmdWithError("volume", "ls", "-f", "dangling=invalid")
assert.ErrorContains(c, err, "")
assert.Assert(c, strings.Contains(out, "Invalid filter"))
assert.Assert(c, strings.Contains(out, "invalid filter"))
}
func (s *DockerSuite) TestVolumeCLIRm(c *testing.T) {

2
plugin/errors.go
View File

@ -32,7 +32,7 @@ type invalidFilter struct {
}
func (e invalidFilter) Error() string {
msg := "Invalid filter '" + e.filter
msg := "invalid filter '" + e.filter
if len(e.value) > 0 {
msg += fmt.Sprintf("=%s", e.value)
}

5
registry/service.go
View File

@ -16,11 +16,6 @@ import (
"github.com/sirupsen/logrus"
)
const (
// DefaultSearchLimit is the default value for maximum number of returned search results.
DefaultSearchLimit = 25
)
// Service is the interface defining what a registry service should implement.
type Service interface {
Auth(ctx context.Context, authConfig *types.AuthConfig, userAgent string) (status, token string, err error)

6
registry/session.go
View File

@ -184,8 +184,14 @@ func newSession(client *http.Client, endpoint *v1Endpoint) *session {
}
}
// defaultSearchLimit is the default value for maximum number of returned search results.
const defaultSearchLimit = 25
// searchRepositories performs a search against the remote repository
func (r *session) searchRepositories(term string, limit int) (*registry.SearchResults, error) {
if limit == 0 {
limit = defaultSearchLimit
}
if limit < 1 || limit > 100 {
return nil, invalidParamf("limit %d is outside the range of [1, 100]", limit)
}

2
volume/service/errors.go
View File

@ -101,7 +101,7 @@ type invalidFilter struct {
}
func (e invalidFilter) Error() string {
msg := "Invalid filter '" + e.filter
msg := "invalid filter '" + e.filter
if e.value != nil {
msg += fmt.Sprintf("=%s", e.value)
}