From 35ed9c4e39306ddbeebdf0445149be72c1cc5284 Mon Sep 17 00:00:00 2001 From: Tom Clegg Date: Thu, 21 Nov 2019 12:34:45 -0500 Subject: [PATCH] 15848: Fix incorrect order in federation query response. Arvados-DCO-1.1-Signed-off-by: Tom Clegg --- lib/controller/federation/generated.go | 35 +++++++++++++++++++++++--- lib/controller/federation/list.go | 18 +++++++++++-- lib/controller/federation/list_test.go | 4 +++ sdk/go/arvados/container.go | 5 +++- sdk/go/arvados/specimen.go | 5 ++-- 5 files changed, 57 insertions(+), 10 deletions(-) diff --git a/lib/controller/federation/generated.go b/lib/controller/federation/generated.go index b34b9b165a..9d5a962887 100755 --- a/lib/controller/federation/generated.go +++ b/lib/controller/federation/generated.go @@ -8,6 +8,7 @@ import ( "context" "sort" "sync" + "sync/atomic" "git.curoverse.com/arvados.git/sdk/go/arvados" ) @@ -19,6 +20,8 @@ import ( func (conn *Conn) ContainerList(ctx context.Context, options arvados.ListOptions) (arvados.ContainerList, error) { var mtx sync.Mutex var merged arvados.ContainerList + var needSort atomic.Value + needSort.Store(false) err := conn.splitListRequest(ctx, options, func(ctx context.Context, _ string, backend arvados.API, options arvados.ListOptions) ([]string, error) { cl, err := backend.ContainerList(ctx, options) if err != nil { @@ -28,8 +31,9 @@ func (conn *Conn) ContainerList(ctx context.Context, options arvados.ListOptions defer mtx.Unlock() if len(merged.Items) == 0 { merged = cl - } else { + } else if len(cl.Items) > 0 { merged.Items = append(merged.Items, cl.Items...) + needSort.Store(true) } uuids := make([]string, 0, len(cl.Items)) for _, item := range cl.Items { @@ -37,13 +41,25 @@ func (conn *Conn) ContainerList(ctx context.Context, options arvados.ListOptions } return uuids, nil }) - sort.Slice(merged.Items, func(i, j int) bool { return merged.Items[i].UUID < merged.Items[j].UUID }) + if needSort.Load().(bool) { + // Apply the default/implied order, "modified_at desc" + sort.Slice(merged.Items, func(i, j int) bool { + mi, mj := merged.Items[i].ModifiedAt, merged.Items[j].ModifiedAt + if mi == nil || mj == nil { + return false + } else { + return mj.Before(*mi) + } + }) + } return merged, err } func (conn *Conn) SpecimenList(ctx context.Context, options arvados.ListOptions) (arvados.SpecimenList, error) { var mtx sync.Mutex var merged arvados.SpecimenList + var needSort atomic.Value + needSort.Store(false) err := conn.splitListRequest(ctx, options, func(ctx context.Context, _ string, backend arvados.API, options arvados.ListOptions) ([]string, error) { cl, err := backend.SpecimenList(ctx, options) if err != nil { @@ -53,8 +69,9 @@ func (conn *Conn) SpecimenList(ctx context.Context, options arvados.ListOptions) defer mtx.Unlock() if len(merged.Items) == 0 { merged = cl - } else { + } else if len(cl.Items) > 0 { merged.Items = append(merged.Items, cl.Items...) + needSort.Store(true) } uuids := make([]string, 0, len(cl.Items)) for _, item := range cl.Items { @@ -62,6 +79,16 @@ func (conn *Conn) SpecimenList(ctx context.Context, options arvados.ListOptions) } return uuids, nil }) - sort.Slice(merged.Items, func(i, j int) bool { return merged.Items[i].UUID < merged.Items[j].UUID }) + if needSort.Load().(bool) { + // Apply the default/implied order, "modified_at desc" + sort.Slice(merged.Items, func(i, j int) bool { + mi, mj := merged.Items[i].ModifiedAt, merged.Items[j].ModifiedAt + if mi == nil || mj == nil { + return false + } else { + return mj.Before(*mi) + } + }) + } return merged, err } diff --git a/lib/controller/federation/list.go b/lib/controller/federation/list.go index 6ba184c471..c215a4d03b 100644 --- a/lib/controller/federation/list.go +++ b/lib/controller/federation/list.go @@ -10,6 +10,7 @@ import ( "net/http" "sort" "sync" + "sync/atomic" "git.curoverse.com/arvados.git/sdk/go/arvados" "git.curoverse.com/arvados.git/sdk/go/httpserver" @@ -23,6 +24,8 @@ import ( func (conn *Conn) CollectionList(ctx context.Context, options arvados.ListOptions) (arvados.CollectionList, error) { var mtx sync.Mutex var merged arvados.CollectionList + var needSort atomic.Value + needSort.Store(false) err := conn.splitListRequest(ctx, options, func(ctx context.Context, _ string, backend arvados.API, options arvados.ListOptions) ([]string, error) { cl, err := backend.CollectionList(ctx, options) if err != nil { @@ -32,8 +35,9 @@ func (conn *Conn) CollectionList(ctx context.Context, options arvados.ListOption defer mtx.Unlock() if len(merged.Items) == 0 { merged = cl - } else { + } else if len(cl.Items) > 0 { merged.Items = append(merged.Items, cl.Items...) + needSort.Store(true) } uuids := make([]string, 0, len(cl.Items)) for _, item := range cl.Items { @@ -41,7 +45,17 @@ func (conn *Conn) CollectionList(ctx context.Context, options arvados.ListOption } return uuids, nil }) - sort.Slice(merged.Items, func(i, j int) bool { return merged.Items[i].UUID < merged.Items[j].UUID }) + if needSort.Load().(bool) { + // Apply the default/implied order, "modified_at desc" + sort.Slice(merged.Items, func(i, j int) bool { + mi, mj := merged.Items[i].ModifiedAt, merged.Items[j].ModifiedAt + if mi == nil || mj == nil { + return false + } else { + return mj.Before(*mi) + } + }) + } return merged, err } diff --git a/lib/controller/federation/list_test.go b/lib/controller/federation/list_test.go index c9b981fc15..8f198a23ca 100644 --- a/lib/controller/federation/list_test.go +++ b/lib/controller/federation/list_test.go @@ -10,6 +10,7 @@ import ( "net/http" "net/url" "os" + "sort" "testing" "git.curoverse.com/arvados.git/lib/controller/router" @@ -430,6 +431,9 @@ func (s *CollectionListSuite) test(c *check.C, trial listTrial) { for _, uuid := range trial.expectUUIDs { expectItems = append(expectItems, arvados.Collection{UUID: uuid}) } + // expectItems is sorted by UUID, so sort resp.Items + // by UUID before checking DeepEquals. + sort.Slice(resp.Items, func(i, j int) bool { return resp.Items[i].UUID < resp.Items[j].UUID }) c.Check(resp, check.DeepEquals, arvados.CollectionList{ Items: expectItems, }) diff --git a/sdk/go/arvados/container.go b/sdk/go/arvados/container.go index fb095481bb..0a75b236e8 100644 --- a/sdk/go/arvados/container.go +++ b/sdk/go/arvados/container.go @@ -9,7 +9,10 @@ import "time" // Container is an arvados#container resource. type Container struct { UUID string `json:"uuid"` - CreatedAt time.Time `json:"created_at"` + CreatedAt *time.Time `json:"created_at"` + ModifiedByClientUUID string `json:"modified_by_client_uuid"` + ModifiedByUserUUID string `json:"modified_by_user_uuid"` + ModifiedAt *time.Time `json:"modified_at"` Command []string `json:"command"` ContainerImage string `json:"container_image"` Cwd string `json:"cwd"` diff --git a/sdk/go/arvados/specimen.go b/sdk/go/arvados/specimen.go index e320ca2c33..3b3b1629fe 100644 --- a/sdk/go/arvados/specimen.go +++ b/sdk/go/arvados/specimen.go @@ -9,9 +9,8 @@ import "time" type Specimen struct { UUID string `json:"uuid"` OwnerUUID string `json:"owner_uuid"` - CreatedAt time.Time `json:"created_at"` - ModifiedAt time.Time `json:"modified_at"` - UpdatedAt time.Time `json:"updated_at"` + CreatedAt *time.Time `json:"created_at"` + ModifiedAt *time.Time `json:"modified_at"` Properties map[string]interface{} `json:"properties"` } -- 2.30.2