diff --git a/pkg/app/server/grpcapi/api.go b/pkg/app/server/grpcapi/api.go index e590c3c2de..43eb14dd26 100644 --- a/pkg/app/server/grpcapi/api.go +++ b/pkg/app/server/grpcapi/api.go @@ -305,22 +305,27 @@ func (a *API) ListApplications(ctx context.Context, req *apiservice.ListApplicat return nil, gRPCStoreError(err, "failed to list applications") } + // Filter applications based on labels and deleted status. + // NOTE: Filtering is done application-side to avoid requiring new composite indexes. labels := req.Labels - if len(req.Labels) == 0 { - return &apiservice.ListApplicationsResponse{ - Applications: apps, - Cursor: cursor, - }, nil + filterApp := func(app *model.Application) bool { + if app.Deleted { + return false + } + if len(labels) > 0 && !app.ContainLabels(labels) { + return false + } + return true } - // NOTE: Filtering by labels is done by the application-side because we need to create composite indexes for every combination in the filter. filtered := make([]*model.Application, 0, len(apps)) - for _, a := range apps { - if a.ContainLabels(req.Labels) { - filtered = append(filtered, a) + for _, app := range apps { + if filterApp(app) { + filtered = append(filtered, app) } } - // Stop running additional queries for more data, and return filtered deployments immediately with + + // Stop running additional queries for more data, and return filtered applications immediately with // current cursor if the size before filtering is already less than the page size. if len(apps) < limit { return &apiservice.ListApplicationsResponse{ @@ -328,10 +333,12 @@ func (a *API) ListApplications(ctx context.Context, req *apiservice.ListApplicat Cursor: cursor, }, nil } - // Repeat the query until the number of filtered deployments reaches the page size, - // or until it finishes scanning to page_min_updated_at. + // Repeat the query until the number of filtered applications reaches the page size, + // or until it finishes scanning all pages. for len(filtered) < limit { options.Cursor = cursor + // Fetch only the remaining number of applications to keep the response within the requested limit. + options.Limit = limit - len(filtered) apps, cursor, err = a.applicationStore.List(ctx, options) if err != nil { a.logger.Error("failed to get applications", zap.Error(err)) @@ -340,9 +347,9 @@ func (a *API) ListApplications(ctx context.Context, req *apiservice.ListApplicat if len(apps) == 0 { break } - for _, d := range apps { - if d.ContainLabels(labels) { - filtered = append(filtered, d) + for _, app := range apps { + if filterApp(app) { + filtered = append(filtered, app) } } } diff --git a/pkg/app/server/grpcapi/api_test.go b/pkg/app/server/grpcapi/api_test.go index e45016e851..31288f02dc 100644 --- a/pkg/app/server/grpcapi/api_test.go +++ b/pkg/app/server/grpcapi/api_test.go @@ -166,3 +166,112 @@ func TestListApplicationsCursor(t *testing.T) { }) } } + +func TestListApplicationsFilterDeleted(t *testing.T) { + testcases := []struct { + name string + req *apiservice.ListApplicationsRequest + pages [][]*model.Application + expectedAppIDs []string + }{ + { + name: "deleted applications are filtered out", + req: &apiservice.ListApplicationsRequest{Limit: 10}, + pages: [][]*model.Application{ + { + {Id: "app-1", ProjectId: "project-id", Deleted: false}, + {Id: "app-2", ProjectId: "project-id", Deleted: true}, + {Id: "app-3", ProjectId: "project-id", Deleted: false}, + }, + }, + expectedAppIDs: []string{"app-1", "app-3"}, + }, + { + name: "pagination continues when page contains only deleted apps", + req: &apiservice.ListApplicationsRequest{Limit: 2}, + pages: [][]*model.Application{ + // First page: all deleted + { + {Id: "app-1", ProjectId: "project-id", Deleted: true}, + {Id: "app-2", ProjectId: "project-id", Deleted: true}, + }, + // Second page: mix of deleted and non-deleted + { + {Id: "app-3", ProjectId: "project-id", Deleted: false}, + {Id: "app-4", ProjectId: "project-id", Deleted: true}, + }, + // Third page: non-deleted + { + {Id: "app-5", ProjectId: "project-id", Deleted: false}, + }, + }, + expectedAppIDs: []string{"app-3", "app-5"}, + }, + { + name: "the number of returned applications does not exceed the limit", + req: &apiservice.ListApplicationsRequest{Limit: 3}, + pages: [][]*model.Application{ + // First page: one of them is deleted, so an additional query is needed. + { + {Id: "app-1", ProjectId: "project-id", Deleted: false}, + {Id: "app-2", ProjectId: "project-id", Deleted: true}, + {Id: "app-3", ProjectId: "project-id", Deleted: false}, + }, + // Second page: only the remaining one should be taken. + { + {Id: "app-4", ProjectId: "project-id", Deleted: false}, + {Id: "app-5", ProjectId: "project-id", Deleted: false}, + {Id: "app-6", ProjectId: "project-id", Deleted: false}, + }, + }, + expectedAppIDs: []string{"app-1", "app-3", "app-4"}, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + pageIndex := 0 + store := datastoretest.NewMockApplicationStore(ctrl) + store.EXPECT(). + List(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, opts datastore.ListOptions) ([]*model.Application, string, error) { + if pageIndex >= len(tc.pages) { + return nil, "", nil + } + apps := tc.pages[pageIndex] + pageIndex++ + // Mimic the datastore behavior of returning at most the requested number of rows. + if opts.Limit > 0 && len(apps) > opts.Limit { + apps = apps[:opts.Limit] + } + cursor := "" + if pageIndex < len(tc.pages) { + cursor = "next-cursor" + } + return apps, cursor, nil + }). + AnyTimes() + + api := &API{ + applicationStore: store, + logger: zap.NewNop(), + } + ctx := rpcauth.ContextWithAPIKey(context.TODO(), &model.APIKey{ + ProjectId: "project-id", + Role: model.APIKey_READ_ONLY, + }) + + resp, err := api.ListApplications(ctx, tc.req) + assert.NoError(t, err) + + gotIDs := make([]string, len(resp.Applications)) + for i, app := range resp.Applications { + gotIDs[i] = app.Id + } + assert.ElementsMatch(t, tc.expectedAppIDs, gotIDs) + }) + } +} diff --git a/pkg/app/server/grpcapi/web_api.go b/pkg/app/server/grpcapi/web_api.go index 4fdbedbe71..0ac75e7c4b 100644 --- a/pkg/app/server/grpcapi/web_api.go +++ b/pkg/app/server/grpcapi/web_api.go @@ -685,18 +685,21 @@ func (a *WebAPI) ListApplications(ctx context.Context, req *webservice.ListAppli return nil, gRPCStoreError(err, "list applications") } - if len(req.Options.Labels) == 0 { - return &webservice.ListApplicationsResponse{ - Applications: apps, - }, nil + // Filter applications based on labels and deleted status. + // NOTE: Filtering is done application-side to avoid requiring new composite indexes. + var labels map[string]string + if o := req.Options; o != nil { + labels = o.Labels } - - // NOTE: Filtering by labels is done by the application-side because we need to create composite indexes for every combination in the filter. filtered := make([]*model.Application, 0, len(apps)) - for _, a := range apps { - if a.ContainLabels(req.Options.Labels) { - filtered = append(filtered, a) + for _, app := range apps { + if app.Deleted { + continue + } + if len(labels) > 0 && !app.ContainLabels(labels) { + continue } + filtered = append(filtered, app) } return &webservice.ListApplicationsResponse{ Applications: filtered,