From ac2f0944efdaa7a756c640f8fdcec8e7a5f883c8 Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Wed, 12 Aug 2026 16:51:57 +0200 Subject: [PATCH] fix(search): pass order_by through the grpc service and cache key The grpc layer rebuilt the searcher request field by field and dropped order_by; the response cache also ignored it, so differently sorted searches collided on the same cache entry. (cherry picked from commit 97626bb26bd4fe8cdb74c19b519b2230c492681c) --- .../search/pkg/service/grpc/v0/service.go | 21 ++-- .../pkg/service/grpc/v0/service_test.go | 111 ++++++++++++++++++ 2 files changed, 122 insertions(+), 10 deletions(-) create mode 100644 services/search/pkg/service/grpc/v0/service_test.go diff --git a/services/search/pkg/service/grpc/v0/service.go b/services/search/pkg/service/grpc/v0/service.go index 72f02ffa95..2e49e1551f 100644 --- a/services/search/pkg/service/grpc/v0/service.go +++ b/services/search/pkg/service/grpc/v0/service.go @@ -95,7 +95,7 @@ func (s Service) Search(ctx context.Context, in *searchsvc.SearchRequest, out *s } ctx = revactx.ContextSetUser(ctx, u) - key := cacheKey(in.Query, in.PageSize, in.Ref, u, in.Aggregations) + key := cacheKey(in.Query, in.PageSize, in.Ref, u, in.Aggregations, in.OrderBy) res, ok := s.FromCache(key) if !ok { var err error @@ -104,6 +104,7 @@ func (s Service) Search(ctx context.Context, in *searchsvc.SearchRequest, out *s PageSize: in.PageSize, Ref: in.Ref, Aggregations: in.Aggregations, + OrderBy: in.OrderBy, }) if err != nil { switch err.(type) { @@ -246,17 +247,17 @@ func (s Service) Cache(key string, res *searchsvc.SearchResponse) { } // cacheKey builds the cache identity for a search. Every result-affecting field -// must be in the key, including aggregations (serialised via deterministic proto -// marshalling). If the aggregation proto ever gains a map field, determinism -// requires all writers to set Deterministic=true. -func cacheKey(query string, pagesize int32, ref *v0.Reference, user *user.User, aggs []*searchsvc.AggregationOption) string { - aggPart := "" - if len(aggs) > 0 { - b, _ := proto.MarshalOptions{Deterministic: true}.Marshal(&searchsvc.SearchRequest{Aggregations: aggs}) - aggPart = string(b) +// must be in the key, including aggregations and order_by (serialised via +// deterministic proto marshalling). If those protos ever gain a map field, +// determinism requires all writers to set Deterministic=true. +func cacheKey(query string, pagesize int32, ref *v0.Reference, user *user.User, aggs []*searchsvc.AggregationOption, orderBy []*searchsvc.SortProperty) string { + protoPart := "" + if len(aggs) > 0 || len(orderBy) > 0 { + b, _ := proto.MarshalOptions{Deterministic: true}.Marshal(&searchsvc.SearchRequest{Aggregations: aggs, OrderBy: orderBy}) + protoPart = string(b) } return fmt.Sprintf("%s|%d|%s$%s!%s/%s|%s|%s", query, pagesize, ref.GetResourceId().GetStorageId(), ref.GetResourceId().GetSpaceId(), ref.GetResourceId().GetOpaqueId(), - ref.GetPath(), user.GetId().GetOpaqueId(), aggPart) + ref.GetPath(), user.GetId().GetOpaqueId(), protoPart) } diff --git a/services/search/pkg/service/grpc/v0/service_test.go b/services/search/pkg/service/grpc/v0/service_test.go new file mode 100644 index 0000000000..7116bf6677 --- /dev/null +++ b/services/search/pkg/service/grpc/v0/service_test.go @@ -0,0 +1,111 @@ +package service + +import ( + "context" + "testing" + "time" + + user "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" + "github.com/jellydator/ttlcache/v2" + "github.com/opencloud-eu/reva/v2/pkg/auth/scope" + revactx "github.com/opencloud-eu/reva/v2/pkg/ctx" + "github.com/opencloud-eu/reva/v2/pkg/token/manager/jwt" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + "go-micro.dev/v4/metadata" + + "github.com/opencloud-eu/opencloud/pkg/log" + searchmsg "github.com/opencloud-eu/opencloud/protogen/gen/opencloud/messages/search/v0" + searchsvc "github.com/opencloud-eu/opencloud/protogen/gen/opencloud/services/search/v0" + searchmocks "github.com/opencloud-eu/opencloud/services/search/pkg/search/mocks" +) + +func newTestService(t *testing.T, searcher *searchmocks.Searcher) (Service, context.Context) { + t.Helper() + + tm, err := jwt.New(map[string]interface{}{"secret": "test-secret"}) + require.NoError(t, err) + + u := &user.User{Id: &user.UserId{OpaqueId: "test-user", Idp: "idp"}, Username: "test"} + scopes, err := scope.AddOwnerScope(nil) + require.NoError(t, err) + tok, err := tm.MintToken(context.Background(), u, scopes) + require.NoError(t, err) + ctx := metadata.Set(context.Background(), revactx.TokenHeader, tok) + + cache := ttlcache.NewCache() + require.NoError(t, cache.SetTTL(30*time.Second)) + + logger := log.NopLogger() + return Service{ + log: &logger, + searcher: searcher, + cache: cache, + tokenManager: tm, + }, ctx +} + +func TestServiceSearchForwardsOrderBy(t *testing.T) { + searcher := searchmocks.NewSearcher(t) + svc, ctx := newTestService(t, searcher) + + var captured *searchsvc.SearchRequest + searcher.EXPECT(). + Search(mock.Anything, mock.Anything). + Run(func(_ context.Context, req *searchsvc.SearchRequest) { + captured = req + }). + Return(&searchsvc.SearchResponse{}, nil). + Once() + + err := svc.Search(ctx, &searchsvc.SearchRequest{ + Query: "mediatype:image", + OrderBy: []*searchsvc.SortProperty{ + {Name: "photo.takenDateTime", IsDescending: true}, + }, + }, &searchsvc.SearchResponse{}) + require.NoError(t, err) + require.NotNil(t, captured) + require.Len(t, captured.OrderBy, 1) + require.Equal(t, "photo.takenDateTime", captured.OrderBy[0].Name) + require.True(t, captured.OrderBy[0].IsDescending) +} + +func TestServiceSearchCacheDistinguishesOrderBy(t *testing.T) { + searcher := searchmocks.NewSearcher(t) + svc, ctx := newTestService(t, searcher) + + responseFor := func(name string) *searchsvc.SearchResponse { + return &searchsvc.SearchResponse{ + TotalMatches: 1, + Matches: []*searchmsg.Match{{Entity: &searchmsg.Entity{Name: name}}}, + } + } + searcher.EXPECT(). + Search(mock.Anything, mock.MatchedBy(func(req *searchsvc.SearchRequest) bool { + return len(req.OrderBy) > 0 && req.OrderBy[0].IsDescending + })). + Return(responseFor("newest.jpg"), nil). + Once() + searcher.EXPECT(). + Search(mock.Anything, mock.MatchedBy(func(req *searchsvc.SearchRequest) bool { + return len(req.OrderBy) > 0 && !req.OrderBy[0].IsDescending + })). + Return(responseFor("oldest.jpg"), nil). + Once() + + descOut := &searchsvc.SearchResponse{} + require.NoError(t, svc.Search(ctx, &searchsvc.SearchRequest{ + Query: "mediatype:image", + OrderBy: []*searchsvc.SortProperty{{Name: "photo.takenDateTime", IsDescending: true}}, + }, descOut)) + + ascOut := &searchsvc.SearchResponse{} + require.NoError(t, svc.Search(ctx, &searchsvc.SearchRequest{ + Query: "mediatype:image", + OrderBy: []*searchsvc.SortProperty{{Name: "photo.takenDateTime"}}, + }, ascOut)) + + require.Equal(t, "newest.jpg", descOut.Matches[0].Entity.Name) + require.Equal(t, "oldest.jpg", ascOut.Matches[0].Entity.Name) +}