Merge pull request #3862 from aduffeck/search-stale-facets

Remove facet metadata the indexer no longer extracts

services/search/pkg/search/service.go

@@ -46,6 +46,10 @@ const (
 	_spaceTypeProject    = "project"
 	_spaceTypeGrant      = "grant"
 	_slowQueryDuration   = 500 * time.Millisecond
+
+	// facetKeyPrefix is the arbitrary metadata namespace the facets are stored
+	// under. The indexer owns all of it and removes keys it no longer writes.
+	facetKeyPrefix = "libre.graph."
 )
 
 // Searcher is the interface to the SearchService
@@ -720,34 +724,57 @@ func (s *Service) doUpsertItem(ctx context.Context, ref *provider.Reference, bat
 
 	// determine if metadata needs to be stored in storage as well
 	metadata := map[string]string{}
-	facetToMetadata(metadata, doc.Audio, "libre.graph.audio.")
-	facetToMetadata(metadata, doc.Image, "libre.graph.image.")
-	facetToMetadata(metadata, doc.Location, "libre.graph.location.")
-	facetToMetadata(metadata, doc.Photo, "libre.graph.photo.")
-	facetToMetadata(metadata, doc.Video, "libre.graph.video.")
-	facetToMetadata(metadata, doc.MotionPhoto, "libre.graph.motionPhoto.")
-	facetToMetadata(metadata, doc.LivePhoto, "libre.graph.livePhoto.")
-	if len(metadata) == 0 {
+	facetToMetadata(metadata, doc.Audio, facetKeyPrefix+"audio.")
+	facetToMetadata(metadata, doc.Image, facetKeyPrefix+"image.")
+	facetToMetadata(metadata, doc.Location, facetKeyPrefix+"location.")
+	facetToMetadata(metadata, doc.Photo, facetKeyPrefix+"photo.")
+	facetToMetadata(metadata, doc.Video, facetKeyPrefix+"video.")
+	facetToMetadata(metadata, doc.MotionPhoto, facetKeyPrefix+"motionPhoto.")
+	facetToMetadata(metadata, doc.LivePhoto, facetKeyPrefix+"livePhoto.")
+
+	// the facet keys are ours, so the ones this extraction did not produce
+	// describe content the resource no longer has
+	var stale []string
+	for k := range stat.GetInfo().GetArbitraryMetadata().GetMetadata() {
+		if _, ok := metadata[k]; !ok && strings.HasPrefix(k, facetKeyPrefix) {
+			stale = append(stale, k)
+		}
+	}
+	if len(metadata) == 0 && len(stale) == 0 {
 		return
 	}
 
-	s.logger.Trace().Str("name", doc.Name).Interface("metadata", metadata).Msg("Storing metadata")
-
 	gatewayClient, err := s.gatewaySelector.Next()
 	if err != nil {
 		s.logger.Error().Err(err).Msg("could not retrieve client to store metadata")
 		return
 	}
 
-	resp, err := gatewayClient.SetArbitraryMetadata(ctx, &provider.SetArbitraryMetadataRequest{
-		Ref: ref,
-		ArbitraryMetadata: &provider.ArbitraryMetadata{
-			Metadata: metadata,
-		},
-	})
-	if err != nil || resp.GetStatus().GetCode() != rpc.Code_CODE_OK {
-		s.logger.Error().Err(err).Int32("status", int32(resp.GetStatus().GetCode())).Msg("error storing metadata")
-		return
+	if len(metadata) > 0 {
+		s.logger.Trace().Str("name", doc.Name).Interface("metadata", metadata).Msg("Storing metadata")
+
+		resp, err := gatewayClient.SetArbitraryMetadata(ctx, &provider.SetArbitraryMetadataRequest{
+			Ref: ref,
+			ArbitraryMetadata: &provider.ArbitraryMetadata{
+				Metadata: metadata,
+			},
+		})
+		if err != nil || resp.GetStatus().GetCode() != rpc.Code_CODE_OK {
+			s.logger.Error().Err(err).Int32("status", int32(resp.GetStatus().GetCode())).Msg("error storing metadata")
+			return
+		}
+	}
+
+	if len(stale) > 0 {
+		s.logger.Trace().Str("name", doc.Name).Strs("keys", stale).Msg("Removing stale metadata")
+
+		resp, err := gatewayClient.UnsetArbitraryMetadata(ctx, &provider.UnsetArbitraryMetadataRequest{
+			Ref:                   ref,
+			ArbitraryMetadataKeys: stale,
+		})
+		if err != nil || resp.GetStatus().GetCode() != rpc.Code_CODE_OK {
+			s.logger.Error().Err(err).Int32("status", int32(resp.GetStatus().GetCode())).Msg("error removing stale metadata")
+		}
 	}
 }
 

services/search/pkg/search/service_test.go

@@ -10,6 +10,7 @@ import (
 	typesv1beta1 "github.com/cs3org/go-cs3apis/cs3/types/v1beta1"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	libregraph "github.com/opencloud-eu/libre-graph-api-go"
 	revactx "github.com/opencloud-eu/reva/v2/pkg/ctx"
 	"github.com/opencloud-eu/reva/v2/pkg/rgrpc/status"
 	"github.com/opencloud-eu/reva/v2/pkg/rgrpc/todo/pool"
@@ -175,6 +176,92 @@ var _ = Describe("Searchprovider", func() {
 			Entry("./", "./", false),
 			Entry("/", "/", false),
 		)
+
+		Context("with facets stored on the resource", func() {
+			var (
+				info  *sprovider.ResourceInfo
+				calls []string
+				set   map[string]string
+				unset []string
+			)
+
+			BeforeEach(func() {
+				calls, set, unset = nil, nil, nil
+				info = &sprovider.ResourceInfo{
+					Id:       &sprovider.ResourceId{StorageId: "storageid", SpaceId: "spaceid", OpaqueId: "photo-opaqueid"},
+					ParentId: &sprovider.ResourceId{StorageId: "storageid", OpaqueId: "parentopaqueid"},
+					Path:     "./photo.jpg",
+					ArbitraryMetadata: &sprovider.ArbitraryMetadata{
+						Metadata: map[string]string{
+							"libre.graph.photo.cameraMake":    "Canon",
+							"libre.graph.photo.iso":           "100",
+							"libre.graph.location.altitude":   "12",
+							"http://owncloud.org/ns/favorite": "1",
+						},
+					},
+				}
+				gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(&sprovider.StatResponse{
+					Status: status.NewOK(context.Background()),
+					Info:   info,
+				}, nil)
+				gatewayClient.On("GetPath", mock.Anything, mock.MatchedBy(func(req *sprovider.GetPathRequest) bool {
+					return req.GetResourceId().GetOpaqueId() == info.GetId().GetOpaqueId()
+				})).Return(&sprovider.GetPathResponse{
+					Status: status.NewOK(context.Background()),
+					Path:   info.GetPath(),
+				}, nil)
+				indexClient.On("Upsert", mock.Anything, mock.Anything).Return(nil)
+				gatewayClient.On("SetArbitraryMetadata", mock.Anything, mock.Anything).Run(func(args mock.Arguments) {
+					calls = append(calls, "set")
+					set = args.Get(1).(*sprovider.SetArbitraryMetadataRequest).GetArbitraryMetadata().GetMetadata()
+				}).Return(&sprovider.SetArbitraryMetadataResponse{Status: status.NewOK(context.Background())}, nil)
+				gatewayClient.On("UnsetArbitraryMetadata", mock.Anything, mock.Anything).Run(func(args mock.Arguments) {
+					calls = append(calls, "unset")
+					unset = args.Get(1).(*sprovider.UnsetArbitraryMetadataRequest).GetArbitraryMetadataKeys()
+				}).Return(&sprovider.UnsetArbitraryMetadataResponse{Status: status.NewOK(context.Background())}, nil)
+			})
+
+			It("removes the facets the extraction no longer produces after storing the new ones", func() {
+				extractor.On("Extract", mock.Anything, mock.Anything, mock.Anything).Return(content.Document{
+					Photo: &libregraph.Photo{CameraMake: libregraph.PtrString("Nikon")},
+				}, nil)
+
+				s.UpsertItem(&sprovider.Reference{ResourceId: info.GetId()})
+				Expect(calls).To(Equal([]string{"set", "unset"}))
+				Expect(set).To(Equal(map[string]string{"libre.graph.photo.cameraMake": "Nikon"}))
+				Expect(unset).To(ConsistOf("libre.graph.photo.iso", "libre.graph.location.altitude"))
+			})
+
+			It("removes all facets when the extraction produces none", func() {
+				extractor.On("Extract", mock.Anything, mock.Anything, mock.Anything).Return(content.Document{}, nil)
+
+				s.UpsertItem(&sprovider.Reference{ResourceId: info.GetId()})
+				Expect(calls).To(Equal([]string{"unset"}))
+				Expect(unset).To(ConsistOf("libre.graph.photo.cameraMake", "libre.graph.photo.iso", "libre.graph.location.altitude"))
+			})
+
+			It("removes keys of facets the indexer no longer knows", func() {
+				info.ArbitraryMetadata.Metadata["libre.graph.retiredFacet.field"] = "value"
+				extractor.On("Extract", mock.Anything, mock.Anything, mock.Anything).Return(content.Document{
+					Photo:    &libregraph.Photo{CameraMake: libregraph.PtrString("Canon"), Iso: libregraph.PtrInt32(100)},
+					Location: &libregraph.GeoCoordinates{Altitude: libregraph.PtrFloat64(12)},
+				}, nil)
+
+				s.UpsertItem(&sprovider.Reference{ResourceId: info.GetId()})
+				Expect(calls).To(Equal([]string{"set", "unset"}))
+				Expect(unset).To(ConsistOf("libre.graph.retiredFacet.field"))
+			})
+
+			It("removes nothing when the extraction still produces every stored facet", func() {
+				extractor.On("Extract", mock.Anything, mock.Anything, mock.Anything).Return(content.Document{
+					Photo:    &libregraph.Photo{CameraMake: libregraph.PtrString("Canon"), Iso: libregraph.PtrInt32(100)},
+					Location: &libregraph.GeoCoordinates{Altitude: libregraph.PtrFloat64(12)},
+				}, nil)
+
+				s.UpsertItem(&sprovider.Reference{ResourceId: info.GetId()})
+				Expect(calls).To(Equal([]string{"set"}))
+			})
+		})
 	})
 
 	DescribeTable("ExpandQueryTemplate",