diff --git a/services/search/pkg/content/cs3.go b/services/search/pkg/content/cs3.go index 56b504cc35..a458b68f48 100644 --- a/services/search/pkg/content/cs3.go +++ b/services/search/pkg/content/cs3.go @@ -93,64 +93,3 @@ func (s cs3) Retrieve(ctx context.Context, rID *provider.ResourceId) (io.ReadClo return cres.Body, nil } - -// RetrieveRange downloads length bytes starting at offset from a cs3 service. -// The caller MUST make sure to close the returned ReadCloser. -// It relies on HTTP range support of the download endpoint. If the endpoint -// ignores the Range header and returns the full file (200 instead of 206), the -// leading offset bytes are discarded so the returned reader is always positioned -// at offset. -func (s cs3) RetrieveRange(ctx context.Context, rID *provider.ResourceId, offset, length int64) (io.ReadCloser, error) { - if offset < 0 || length <= 0 { - return nil, fmt.Errorf("invalid range: offset %d, length %d", offset, length) - } - - ep, tt, at, err := s.initiateDownload(ctx, rID) - if err != nil { - return nil, err - } - - req, err := http.NewRequest(http.MethodGet, ep, nil) - if err != nil { - return nil, err - } - - req.Header.Set(revactx.TokenHeader, at) - req.Header.Set("X-Reva-Transfer", tt) - // A single range keeps the response a plain 206 with a Content-Range header, - // never a multipart/byteranges body. - req.Header.Set("Range", fmt.Sprintf("bytes=%d-%d", offset, offset+length-1)) - - cres, err := s.httpClient.Do(req) - if err != nil { - return nil, err - } - - switch cres.StatusCode { - case http.StatusPartialContent: - // Range honored: the body already starts at offset. - return capped(cres.Body, length), nil - case http.StatusOK: - // Range ignored: the body is the whole file. Skip to offset so the - // caller always reads from the requested position. - if _, err := io.CopyN(io.Discard, cres.Body, offset); err != nil { - _ = cres.Body.Close() - return nil, fmt.Errorf("could not skip to offset %d: %w", offset, err) - } - return capped(cres.Body, length), nil - default: - _ = cres.Body.Close() - return nil, fmt.Errorf("could not download range. Request returned with statuscode %d ", cres.StatusCode) - } -} - -type cappedReadCloser struct { - io.Reader - io.Closer -} - -// capped limits rc to length bytes, keeping the contract even when the server -// returns more than the requested range. -func capped(rc io.ReadCloser, length int64) io.ReadCloser { - return cappedReadCloser{io.LimitReader(rc, length), rc} -} diff --git a/services/search/pkg/content/mocks/retriever.go b/services/search/pkg/content/mocks/retriever.go index 4fb4b5607e..5060f071a3 100644 --- a/services/search/pkg/content/mocks/retriever.go +++ b/services/search/pkg/content/mocks/retriever.go @@ -106,83 +106,3 @@ func (_c *Retriever_Retrieve_Call) RunAndReturn(run func(ctx context.Context, rI _c.Call.Return(run) return _c } - -// RetrieveRange provides a mock function for the type Retriever -func (_mock *Retriever) RetrieveRange(ctx context.Context, rID *providerv1beta1.ResourceId, offset int64, length int64) (io.ReadCloser, error) { - ret := _mock.Called(ctx, rID, offset, length) - - if len(ret) == 0 { - panic("no return value specified for RetrieveRange") - } - - var r0 io.ReadCloser - var r1 error - if returnFunc, ok := ret.Get(0).(func(context.Context, *providerv1beta1.ResourceId, int64, int64) (io.ReadCloser, error)); ok { - return returnFunc(ctx, rID, offset, length) - } - if returnFunc, ok := ret.Get(0).(func(context.Context, *providerv1beta1.ResourceId, int64, int64) io.ReadCloser); ok { - r0 = returnFunc(ctx, rID, offset, length) - } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).(io.ReadCloser) - } - } - if returnFunc, ok := ret.Get(1).(func(context.Context, *providerv1beta1.ResourceId, int64, int64) error); ok { - r1 = returnFunc(ctx, rID, offset, length) - } else { - r1 = ret.Error(1) - } - return r0, r1 -} - -// Retriever_RetrieveRange_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'RetrieveRange' -type Retriever_RetrieveRange_Call struct { - *mock.Call -} - -// RetrieveRange is a helper method to define mock.On call -// - ctx context.Context -// - rID *providerv1beta1.ResourceId -// - offset int64 -// - length int64 -func (_e *Retriever_Expecter) RetrieveRange(ctx interface{}, rID interface{}, offset interface{}, length interface{}) *Retriever_RetrieveRange_Call { - return &Retriever_RetrieveRange_Call{Call: _e.mock.On("RetrieveRange", ctx, rID, offset, length)} -} - -func (_c *Retriever_RetrieveRange_Call) Run(run func(ctx context.Context, rID *providerv1beta1.ResourceId, offset int64, length int64)) *Retriever_RetrieveRange_Call { - _c.Call.Run(func(args mock.Arguments) { - var arg0 context.Context - if args[0] != nil { - arg0 = args[0].(context.Context) - } - var arg1 *providerv1beta1.ResourceId - if args[1] != nil { - arg1 = args[1].(*providerv1beta1.ResourceId) - } - var arg2 int64 - if args[2] != nil { - arg2 = args[2].(int64) - } - var arg3 int64 - if args[3] != nil { - arg3 = args[3].(int64) - } - run( - arg0, - arg1, - arg2, - arg3, - ) - }) - return _c -} - -func (_c *Retriever_RetrieveRange_Call) Return(readCloser io.ReadCloser, err error) *Retriever_RetrieveRange_Call { - _c.Call.Return(readCloser, err) - return _c -} - -func (_c *Retriever_RetrieveRange_Call) RunAndReturn(run func(ctx context.Context, rID *providerv1beta1.ResourceId, offset int64, length int64) (io.ReadCloser, error)) *Retriever_RetrieveRange_Call { - _c.Call.Return(run) - return _c -} diff --git a/services/search/pkg/content/retriever.go b/services/search/pkg/content/retriever.go index 586e42790f..bb64584d6f 100644 --- a/services/search/pkg/content/retriever.go +++ b/services/search/pkg/content/retriever.go @@ -12,10 +12,6 @@ import ( // It requests and then returns a resource from the underlying storage. type Retriever interface { Retrieve(ctx context.Context, rID *provider.ResourceId) (io.ReadCloser, error) - // RetrieveRange returns a reader positioned at offset for up to length bytes - // of the resource. Implementations must ensure the reader starts at offset - // even when the storage does not honor HTTP range requests. - RetrieveRange(ctx context.Context, rID *provider.ResourceId, offset, length int64) (io.ReadCloser, error) } func contextGet(ctx context.Context, k string) (string, bool) { diff --git a/services/search/pkg/content/tika.go b/services/search/pkg/content/tika.go index 89f368a831..7bcdae426e 100644 --- a/services/search/pkg/content/tika.go +++ b/services/search/pkg/content/tika.go @@ -82,6 +82,7 @@ func (t Tika) Extract(ctx context.Context, ri *provider.ResourceInfo) (Document, return doc, err } + var motionPhotoVideo bool for _, meta := range metas { title, err := getFirstValue(meta, "dc:title") if err != nil { @@ -118,11 +119,14 @@ func (t Tika) Extract(ctx context.Context, ri *provider.ResourceInfo) (Document, if v := t.getMotionPhoto(meta); v != nil { doc.MotionPhoto = v } + if isMotionPhotoVideo(meta) { + motionPhotoVideo = true + } } - // verify against the file itself: a shared motion photo can keep the XMP but - // lose the appended video, which would leave an unplayable facet. - if doc.MotionPhoto != nil && !t.motionPhotoHasVideo(ctx, ri, doc.MotionPhoto.GetVideoSize()) { + // the xmp alone does not prove the video is there, tika emitting it as an + // embedded attachment does + if !motionPhotoVideo { doc.MotionPhoto = nil } diff --git a/services/search/pkg/content/tika_motion_photo.go b/services/search/pkg/content/tika_motion_photo.go index b02a84e21f..5115f13a38 100644 --- a/services/search/pkg/content/tika_motion_photo.go +++ b/services/search/pkg/content/tika_motion_photo.go @@ -1,20 +1,16 @@ package content import ( - "context" - "io" + libregraph "github.com/opencloud-eu/libre-graph-api-go" "sort" "strconv" "strings" - - provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" - libregraph "github.com/opencloud-eu/libre-graph-api-go" ) -// motionPhotoVideoSignatureLen is the number of trailing bytes we read to confirm -// an actual video is present. Enough to cover the ISO base media (MP4) box size -// and the "ftyp" box type at bytes [4:8]. -const motionPhotoVideoSignatureLen = 12 +// motionPhotoVideoName is the resource name tika gives the appended video when +// it emits it as an embedded attachment. The extension follows the detected +// type and is absent for legacy MicroVideo, which declares no mime type. +const motionPhotoVideoName = "motion-photo" // getMotionPhoto reads Google Motion Photo XMP, which Tika exposes under the // canonical Camera/Container prefixes. It covers both the current MotionPhoto @@ -89,31 +85,15 @@ func motionPhotoVideoSize(meta map[string][]string) (int64, bool) { return 0, false } -// looksLikeMP4 reports whether buf begins with an ISO base media (MP4/QuickTime) -// "ftyp" box, which Google Motion Photo and legacy MicroVideo clips start with. -func looksLikeMP4(buf []byte) bool { - return len(buf) >= 8 && string(buf[4:8]) == "ftyp" -} - -// motionPhotoHasVideo confirms that the file actually contains the embedded video -// the XMP advertises. A photos.google.com share strips the appended video but -// keeps the XMP, which would otherwise make us expose an unplayable facet. The -// video is appended at the end, so it starts at fileSize-videoSize; we read a few -// bytes there and require an MP4 signature. -func (t Tika) motionPhotoHasVideo(ctx context.Context, ri *provider.ResourceInfo, videoSize int64) bool { - size := int64(ri.GetSize()) - if videoSize <= 0 || videoSize >= size { - return false - } - - rc, err := t.RetrieveRange(ctx, ri.GetId(), size-videoSize, motionPhotoVideoSignatureLen) +// isMotionPhotoVideo reports whether meta describes the video tika extracted +// from a motion photo. Tika only emits it when the bytes the xmp advertises are +// really there, so its presence is what confirms the facet: a shared motion +// photo can keep the xmp and lose the appended video. +func isMotionPhotoVideo(meta map[string][]string) bool { + // tika 4 renamed the meta prefix from X-TIKA: to tk: + name, err := getFirstValue(meta, "tk:resource-name", "X-TIKA:resource-name") if err != nil { - t.logger.Debug().Err(err).Interface("ResourceID", ri.GetId()).Msg("could not read motion photo video header, dropping facet") return false } - defer rc.Close() - - buf := make([]byte, motionPhotoVideoSignatureLen) - n, _ := io.ReadFull(rc, buf) - return looksLikeMP4(buf[:n]) + return name == motionPhotoVideoName || strings.HasPrefix(name, motionPhotoVideoName+".") } diff --git a/services/search/pkg/content/tika_motion_photo_test.go b/services/search/pkg/content/tika_motion_photo_test.go index 02dd8793ce..4cd300ca77 100644 --- a/services/search/pkg/content/tika_motion_photo_test.go +++ b/services/search/pkg/content/tika_motion_photo_test.go @@ -1,17 +1,9 @@ package content import ( - "context" - "errors" - "io" - "strings" - - provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" libregraph "github.com/opencloud-eu/libre-graph-api-go" - - "github.com/opencloud-eu/opencloud/pkg/log" ) var _ = Describe("getMotionPhoto", func() { @@ -85,73 +77,16 @@ var _ = Describe("getMotionPhoto", func() { }) }) -var _ = Describe("looksLikeMP4", func() { - It("recognizes an ftyp box and rejects everything else", func() { - Expect(looksLikeMP4([]byte{0, 0, 0, 24, 'f', 't', 'y', 'p', 'i', 's', 'o', 'm'})).To(BeTrue()) - Expect(looksLikeMP4([]byte("JFIF garbage"))).To(BeFalse()) - Expect(looksLikeMP4([]byte{0, 0, 0})).To(BeFalse()) - }) -}) - -// rangeStub serves RetrieveRange from a string and records the requested range. -type rangeStub struct { - data string - err error - gotOffset int64 - gotLength int64 - calls int -} - -func (r *rangeStub) Retrieve(context.Context, *provider.ResourceId) (io.ReadCloser, error) { - return nil, errors.New("unused") -} - -func (r *rangeStub) RetrieveRange(_ context.Context, _ *provider.ResourceId, offset, length int64) (io.ReadCloser, error) { - r.calls++ - r.gotOffset, r.gotLength = offset, length - if r.err != nil { - return nil, r.err - } - return io.NopCloser(strings.NewReader(r.data)), nil -} - -var _ = Describe("motionPhotoHasVideo", func() { - var ( - retriever *rangeStub - tika Tika - ri *provider.ResourceInfo +var _ = Describe("isMotionPhotoVideo", func() { + DescribeTable("recognizes the video tika emits as an embedded attachment", + func(meta map[string][]string, expected bool) { + Expect(isMotionPhotoVideo(meta)).To(Equal(expected)) + }, + Entry("named attachment", map[string][]string{"tk:resource-name": {"motion-photo.mp4"}}, true), + Entry("legacy tika prefix", map[string][]string{"X-TIKA:resource-name": {"motion-photo.mp4"}}, true), + Entry("no extension, as for MicroVideo", map[string][]string{"tk:resource-name": {"motion-photo"}}, true), + Entry("another attachment", map[string][]string{"tk:resource-name": {"cover.jpg"}}, false), + Entry("a name that only starts alike", map[string][]string{"tk:resource-name": {"motion-photography.mp4"}}, false), + Entry("the image itself", map[string][]string{"Camera:MotionPhoto": {"1"}}, false), ) - - BeforeEach(func() { - retriever = &rangeStub{} - basic, err := NewBasicExtractor(log.NewLogger()) - Expect(err).ToNot(HaveOccurred()) - tika = Tika{Basic: basic, Retriever: retriever} - ri = &provider.ResourceInfo{Size: 100} - }) - - It("confirms a video that starts with an ftyp box", func() { - retriever.data = "\x00\x00\x00\x18ftypisom" - Expect(tika.motionPhotoHasVideo(context.Background(), ri, 40)).To(BeTrue()) - Expect(retriever.gotOffset).To(Equal(int64(60)), "the video starts at size-videoSize") - Expect(retriever.gotLength).To(Equal(int64(motionPhotoVideoSignatureLen))) - }) - - It("rejects trailing bytes without an MP4 signature", func() { - retriever.data = "not a video " - Expect(tika.motionPhotoHasVideo(context.Background(), ri, 40)).To(BeFalse()) - }) - - It("rejects degenerate video sizes without reading", func() { - Expect(tika.motionPhotoHasVideo(context.Background(), ri, 0)).To(BeFalse()) - Expect(tika.motionPhotoHasVideo(context.Background(), ri, -1)).To(BeFalse()) - Expect(tika.motionPhotoHasVideo(context.Background(), ri, 100)).To(BeFalse()) - Expect(tika.motionPhotoHasVideo(context.Background(), ri, 101)).To(BeFalse()) - Expect(retriever.calls).To(BeZero()) - }) - - It("drops the facet when the range read fails", func() { - retriever.err = errors.New("nope") - Expect(tika.motionPhotoHasVideo(context.Background(), ri, 40)).To(BeFalse()) - }) }) diff --git a/services/search/pkg/content/tika_test.go b/services/search/pkg/content/tika_test.go index 1e4d64feef..154e4798e9 100644 --- a/services/search/pkg/content/tika_test.go +++ b/services/search/pkg/content/tika_test.go @@ -217,13 +217,8 @@ var _ = Describe("Tika", func() { Expect(doc.Content).To(Equal("one two")) }) - It("verifies the motion photo video against the file", func() { - fullResponse = `[{"Camera:MotionPhotoVersion": "1", "Container:Directory/Item[2]/Item:Semantic": "MotionPhoto", "Container:Directory/Item[2]/Item:Length": "40"}]` - retriever := &contentMocks.Retriever{} - retriever.On("Retrieve", mock.Anything, mock.Anything).Return(io.NopCloser(strings.NewReader("")), nil) - retriever.On("RetrieveRange", mock.Anything, mock.Anything, int64(60), mock.Anything). - Return(io.NopCloser(strings.NewReader("\x00\x00\x00\x18ftypisom")), nil) - tika.Retriever = retriever + It("keeps the motion photo facet when tika emits the video", func() { + fullResponse = `[{"Camera:MotionPhotoVersion": "1", "Container:Directory/Item[2]/Item:Semantic": "MotionPhoto", "Container:Directory/Item[2]/Item:Length": "40"}, {"tk:resource-name": "motion-photo.mp4", "Content-Type": "video/mp4"}]` doc, err := tika.Extract(context.TODO(), &provider.ResourceInfo{ Type: provider.ResourceType_RESOURCE_TYPE_FILE, @@ -236,11 +231,6 @@ var _ = Describe("Tika", func() { It("drops the motion photo facet when the advertised video is gone", func() { fullResponse = `[{"Camera:MotionPhotoVersion": "1", "Container:Directory/Item[2]/Item:Semantic": "MotionPhoto", "Container:Directory/Item[2]/Item:Length": "40"}]` - retriever := &contentMocks.Retriever{} - retriever.On("Retrieve", mock.Anything, mock.Anything).Return(io.NopCloser(strings.NewReader("")), nil) - retriever.On("RetrieveRange", mock.Anything, mock.Anything, int64(60), mock.Anything). - Return(io.NopCloser(strings.NewReader("JFIF leftovers")), nil) - tika.Retriever = retriever doc, err := tika.Extract(context.TODO(), &provider.ResourceInfo{ Type: provider.ResourceType_RESOURCE_TYPE_FILE,