From beb80a2d327fad51a7baa47f2d24368d6cfd70b3 Mon Sep 17 00:00:00 2001 From: Sophie Stadler Date: Tue, 8 Oct 2024 10:04:33 -0400 Subject: [PATCH] DEVPROD-11769: Improve waterfall handling for inactive versions (#8361) --- graphql/query_resolver.go | 50 ++++++++++++------- graphql/tests/query/waterfall/data.json | 38 +++++++++++++- .../queries/all_inactive_versions.graphql | 20 ++++++++ .../waterfall/queries/no_versions.graphql | 20 ++++++++ graphql/tests/query/waterfall/results.json | 48 ++++++++++++++++++ graphql/util.go | 6 +-- graphql/util_test.go | 24 +++------ model/waterfall.go | 19 ++++--- model/waterfall_test.go | 4 ++ 9 files changed, 184 insertions(+), 45 deletions(-) create mode 100644 graphql/tests/query/waterfall/queries/all_inactive_versions.graphql create mode 100644 graphql/tests/query/waterfall/queries/no_versions.graphql diff --git a/graphql/query_resolver.go b/graphql/query_resolver.go index 8c008cc9565..46038679130 100644 --- a/graphql/query_resolver.go +++ b/graphql/query_resolver.go @@ -1000,15 +1000,23 @@ func (r *queryResolver) Waterfall(ctx context.Context, options WaterfallOptions) // Since GetAllWaterfallVersions uses an inclusive order range ($gte instead of $gt), add 1 to our minimum range minVersionOrder := minOrderOpt + 1 - if minOrderOpt == 0 { + if minOrderOpt == 0 && len(activeVersions) != 0 { // Only use the last active version order number if no minOrder was provided. Using the activeVersions bounds may omit inactive versions between the min and the last active version found. minVersionOrder = activeVersions[len(activeVersions)-1].RevisionOrderNumber + } else if len(activeVersions) == 0 { + // If there are no active versions, use 0 to fetch all inactive versions + minVersionOrder = 0 } // Same as above, but subtract for max order maxVersionOrder := maxOrderOpt - 1 - if maxOrderOpt == 0 { - // Same as above: only use the first active version if no maxOrder was specified to avoid omitting inactive versions. + if len(activeVersions) == 0 { + maxVersionOrder = 0 + } else if maxOrderOpt == 0 && minOrderOpt == 0 { + // If no order options were specified, we're on the first page and should not put a limit on the first version returned so that we don't omit inactive versions + maxVersionOrder = 0 + } else if maxOrderOpt == 0 { + // If we're paginating backwards, use the newest active version as the upper bound maxVersionOrder = activeVersions[0].RevisionOrderNumber } @@ -1026,26 +1034,32 @@ func (r *queryResolver) Waterfall(ctx context.Context, options WaterfallOptions) activeVersionIds = append(activeVersionIds, v.Id) } - waterfallVersions := groupInactiveVersions(activeVersionIds, allVersions) + waterfallVersions := groupInactiveVersions(allVersions) + bv := []*model.WaterfallBuildVariant{} - buildVariants, err := model.GetWaterfallBuildVariants(ctx, activeVersionIds) - if err != nil { - return nil, InternalServerError.Send(ctx, fmt.Sprintf("getting waterfall build variants: %s", err.Error())) - } + if len(activeVersionIds) > 0 { + buildVariants, err := model.GetWaterfallBuildVariants(ctx, activeVersionIds) + if err != nil { + return nil, InternalServerError.Send(ctx, fmt.Sprintf("getting waterfall build variants: %s", err.Error())) + } - bv := []*model.WaterfallBuildVariant{} - for _, b := range buildVariants { - bCopy := b - bv = append(bv, &bCopy) + for _, b := range buildVariants { + bCopy := b + bv = append(bv, &bCopy) + } } - // Return the min and max orders returned to be used as parameters for navigating to the next page - prevPageOrder := allVersions[0].RevisionOrderNumber - nextPageOrder := allVersions[len(allVersions)-1].RevisionOrderNumber + prevPageOrder := 0 + nextPageOrder := 0 + if len(allVersions) > 0 { + // Return the min and max orders returned to be used as parameters for navigating to the next page + prevPageOrder = allVersions[0].RevisionOrderNumber + nextPageOrder = allVersions[len(allVersions)-1].RevisionOrderNumber - // If loading base page, there's no prev page to navigate to regardless of max order - if maxOrderOpt == 0 && minOrderOpt == 0 { - prevPageOrder = 0 + // If loading base page, there's no prev page to navigate to regardless of max order + if maxOrderOpt == 0 && minOrderOpt == 0 { + prevPageOrder = 0 + } } return &Waterfall{ diff --git a/graphql/tests/query/waterfall/data.json b/graphql/tests/query/waterfall/data.json index 0d53b85f6e4..dcc942ec3c6 100644 --- a/graphql/tests/query/waterfall/data.json +++ b/graphql/tests/query/waterfall/data.json @@ -89,6 +89,36 @@ "author": "mohamed.khelif", "status": "created", "order": 45 + }, + { + "_id": "inactive1", + "gitspec": "abc", + "identifier": "mci", + "r": "gitter_request", + "activated": false, + "author": "sophie.stadler", + "status": "inactive", + "order": 1 + }, + { + "_id": "inactive2", + "gitspec": "abc", + "identifier": "mci", + "r": "gitter_request", + "activated": false, + "author": "sophie.stadler", + "status": "inactive", + "order": 2 + }, + { + "_id": "inactive3", + "gitspec": "abc", + "identifier": "mci", + "r": "gitter_request", + "activated": false, + "author": "sophie.stadler", + "status": "inactive", + "order": 3 } ], "tasks": [ @@ -169,8 +199,12 @@ "identifier": "spruce-identifier" }, { - "_id": "evergreen", - "identifier": "evergreen" + "_id": "mci", + "identifier": "mci" + }, + { + "_id": "sandbox_project_id", + "identifier": "project_sandbox" } ], "builds": [ diff --git a/graphql/tests/query/waterfall/queries/all_inactive_versions.graphql b/graphql/tests/query/waterfall/queries/all_inactive_versions.graphql new file mode 100644 index 00000000000..4fadc6427c8 --- /dev/null +++ b/graphql/tests/query/waterfall/queries/all_inactive_versions.graphql @@ -0,0 +1,20 @@ +{ + waterfall(options: { projectIdentifier: "mci" }) { + nextPageOrder + prevPageOrder + versions { + version { + activated + author + id + order + } + inactiveVersions { + activated + author + id + order + } + } + } +} diff --git a/graphql/tests/query/waterfall/queries/no_versions.graphql b/graphql/tests/query/waterfall/queries/no_versions.graphql new file mode 100644 index 00000000000..f58d6e583e7 --- /dev/null +++ b/graphql/tests/query/waterfall/queries/no_versions.graphql @@ -0,0 +1,20 @@ +{ + waterfall(options: { projectIdentifier: "project_sandbox" }) { + nextPageOrder + prevPageOrder + versions { + version { + activated + author + id + order + } + inactiveVersions { + activated + author + id + order + } + } + } +} diff --git a/graphql/tests/query/waterfall/results.json b/graphql/tests/query/waterfall/results.json index 40f592a9d60..a76f19fe1df 100644 --- a/graphql/tests/query/waterfall/results.json +++ b/graphql/tests/query/waterfall/results.json @@ -252,6 +252,54 @@ } } } + }, + { + "query_file": "all_inactive_versions.graphql", + "result": { + "data": { + "waterfall": { + "nextPageOrder": 1, + "prevPageOrder": 0, + "versions": [ + { + "version": null, + "inactiveVersions": [ + { + "activated": false, + "author": "sophie.stadler", + "id": "inactive3", + "order": 3 + }, + { + "activated": false, + "author": "sophie.stadler", + "id": "inactive2", + "order": 2 + }, + { + "activated": false, + "author": "sophie.stadler", + "id": "inactive1", + "order": 1 + } + ] + } + ] + } + } + } + }, + { + "query_file": "no_versions.graphql", + "result": { + "data": { + "waterfall": { + "nextPageOrder": 0, + "prevPageOrder": 0, + "versions": [] + } + } + } } ] } diff --git a/graphql/util.go b/graphql/util.go index ff8f00d9805..0f7fe7a9dab 100644 --- a/graphql/util.go +++ b/graphql/util.go @@ -1320,11 +1320,11 @@ func annotationPermissionHelper(ctx context.Context, taskID string, execution *i } // groupInactiveVersions partitions a slice of versions into a slice where each entry is either an active version or slice of inactive versions (i.e. versions that don't match filters; they may be technically activated). -func groupInactiveVersions(activeVersionIds []string, versions []model.Version) []*WaterfallVersion { +func groupInactiveVersions(versions []model.Version) []*WaterfallVersion { waterfallVersions := []*WaterfallVersion{} i := 0 for i < len(versions) { - if utility.StringSliceContains(activeVersionIds, versions[i].Id) { + if utility.FromBoolPtr(versions[i].Activated) { apiVersion := restModel.APIVersion{} apiVersion.BuildFromService(versions[i]) waterfallVersions = append(waterfallVersions, &WaterfallVersion{ @@ -1334,7 +1334,7 @@ func groupInactiveVersions(activeVersionIds []string, versions []model.Version) i++ } else { inactiveGroup := []*restModel.APIVersion{} - for i < len(versions) && !utility.StringSliceContains(activeVersionIds, versions[i].Id) { + for i < len(versions) && !utility.FromBoolPtr(versions[i].Activated) { apiVersion := restModel.APIVersion{} apiVersion.BuildFromService(versions[i]) inactiveGroup = append(inactiveGroup, &apiVersion) diff --git a/graphql/util_test.go b/graphql/util_test.go index 27fba37190b..83dad83c964 100644 --- a/graphql/util_test.go +++ b/graphql/util_test.go @@ -515,22 +515,14 @@ func TestHasAnnotationPermission(t *testing.T) { } func TestGroupInactiveVersions(t *testing.T) { - assert.NoError(t, db.ClearCollections(model.VersionCollection)) - - v0 := model.Version{Id: "0"} - v1 := model.Version{Id: "1"} - assert.NoError(t, v1.Insert()) - v2 := model.Version{Id: "2"} - assert.NoError(t, v2.Insert()) - v3 := model.Version{Id: "3"} - assert.NoError(t, v3.Insert()) - v4 := model.Version{Id: "4"} - assert.NoError(t, v4.Insert()) - v5 := model.Version{Id: "5"} - assert.NoError(t, v5.Insert()) - - activeVersionIds := []string{v2.Id, v3.Id, v5.Id} - waterfallVersions := groupInactiveVersions(activeVersionIds, []model.Version{v0, v1, v2, v3, v4, v5}) + v0 := model.Version{Id: "0", Activated: utility.ToBoolPtr(false)} + v1 := model.Version{Id: "1", Activated: utility.ToBoolPtr(false)} + v2 := model.Version{Id: "2", Activated: utility.ToBoolPtr(true)} + v3 := model.Version{Id: "3", Activated: utility.ToBoolPtr(true)} + v4 := model.Version{Id: "4", Activated: utility.ToBoolPtr(false)} + v5 := model.Version{Id: "5", Activated: utility.ToBoolPtr(true)} + + waterfallVersions := groupInactiveVersions([]model.Version{v0, v1, v2, v3, v4, v5}) require.Len(t, waterfallVersions, 5) assert.Nil(t, waterfallVersions[0].Version) diff --git a/model/waterfall.go b/model/waterfall.go index 106a6ce7a72..dc1daf306ba 100644 --- a/model/waterfall.go +++ b/model/waterfall.go @@ -103,19 +103,26 @@ func GetActiveWaterfallVersions(ctx context.Context, projectId string, opts Wate // GetAllWaterfallVersions returns all of a project's versions within an inclusive range of orders. func GetAllWaterfallVersions(ctx context.Context, projectId string, minOrder int, maxOrder int) ([]Version, error) { - if minOrder >= maxOrder { + if minOrder != 0 && maxOrder != 0 && minOrder >= maxOrder { return nil, errors.New("minOrder must be less than maxOrder") } - match := bson.M{ VersionIdentifierKey: projectId, VersionRequesterKey: bson.M{ "$in": evergreen.SystemVersionRequesterTypes, }, - VersionRevisionOrderNumberKey: bson.M{ - "$gte": minOrder, - "$lte": maxOrder, - }, + } + + hasOrderFilters := maxOrder != 0 || minOrder != 0 + if hasOrderFilters { + revisionFilter := bson.M{} + if minOrder != 0 { + revisionFilter["$gte"] = minOrder + } + if maxOrder != 0 { + revisionFilter["$lte"] = maxOrder + } + match[VersionRevisionOrderNumberKey] = revisionFilter } pipeline := []bson.M{{"$match": match}} diff --git a/model/waterfall_test.go b/model/waterfall_test.go index e323df3506e..21eb94c2d08 100644 --- a/model/waterfall_test.go +++ b/model/waterfall_test.go @@ -191,6 +191,10 @@ func TestGetAllWaterfallVersions(t *testing.T) { assert.NoError(t, err) require.Len(t, versions, 1) assert.EqualValues(t, "v_1", versions[0].Id) + + versions, err = GetAllWaterfallVersions(ctx, p.Id, 0, 0) + assert.NoError(t, err) + require.Len(t, versions, 5) } func TestGetWaterfallBuildVariants(t *testing.T) {