From 314390f302dc1f29144e5a6d4c11c1a0262721b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Tue, 23 Sep 2025 12:54:05 +0200 Subject: [PATCH] introduce AppURLs helper for atomic backgroud updates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- services/collaboration/pkg/command/server.go | 22 +- .../collaboration/pkg/helpers/discovery.go | 62 +++++ .../pkg/helpers/discovery_test.go | 259 ++++++++++++++++++ .../collaboration/pkg/helpers/registration.go | 20 +- .../collaboration/pkg/server/grpc/option.go | 5 +- .../pkg/service/grpc/v0/option.go | 5 +- .../pkg/service/grpc/v0/service.go | 26 +- .../pkg/service/grpc/v0/service_test.go | 30 +- 8 files changed, 371 insertions(+), 58 deletions(-) diff --git a/services/collaboration/pkg/command/server.go b/services/collaboration/pkg/command/server.go index f5ebcce29b..36f660a256 100644 --- a/services/collaboration/pkg/command/server.go +++ b/services/collaboration/pkg/command/server.go @@ -69,16 +69,26 @@ func Server(cfg *config.Config) *cli.Command { return err } - appUrls, err := helpers.GetAppURLs(cfg, logger) - if err != nil { - return err - } + // use the AppURLs helper (an atomic pointer) to fetch and store the app URLs + // this is required as the app URLs are fetched periodically in the background + // and read when handling requests + appURLs := helpers.NewAppURLs() ticker := time.NewTicker(cfg.CS3Api.APPRegistrationInterval) defer ticker.Stop() go func() { for ; true; <-ticker.C { - if err := helpers.RegisterAppProvider(ctx, cfg, logger, gatewaySelector, appUrls); err != nil { + // fetch and store the app URLs + v, err := helpers.GetAppURLs(cfg, logger) + if err != nil { + logger.Warn().Err(err).Msg("Failed to get app URLs") + // empty map to clear previous URLs + v = make(map[string]map[string]string) + } + appURLs.Store(v) + + // register the app provider + if err := helpers.RegisterAppProvider(ctx, cfg, logger, gatewaySelector, appURLs); err != nil { logger.Warn().Err(err).Msg("Failed to register app provider") } } @@ -97,7 +107,7 @@ func Server(cfg *config.Config) *cli.Command { // start GRPC server grpcServer, teardown, err := grpc.Server( - grpc.AppURLs(appUrls), + grpc.AppURLs(appURLs), grpc.Config(cfg), grpc.Logger(logger), grpc.TraceProvider(traceProvider), diff --git a/services/collaboration/pkg/helpers/discovery.go b/services/collaboration/pkg/helpers/discovery.go index b7d3a93592..1432f7525b 100644 --- a/services/collaboration/pkg/helpers/discovery.go +++ b/services/collaboration/pkg/helpers/discovery.go @@ -6,13 +6,75 @@ import ( "net/http" "net/url" "strings" + "sync/atomic" "github.com/beevik/etree" "github.com/opencloud-eu/opencloud/pkg/log" "github.com/opencloud-eu/opencloud/services/collaboration/pkg/config" + "github.com/opencloud-eu/reva/v2/pkg/mime" "github.com/pkg/errors" ) +// AppURLs holds the app urls fetched from the WOPI app discovery endpoint +// It is a type safe wrapper around an atomic pointer to a map +type AppURLs struct { + urls atomic.Pointer[map[string]map[string]string] +} + +func NewAppURLs() *AppURLs { + a := &AppURLs{} + a.urls.Store(&map[string]map[string]string{}) + return a +} + +func (a *AppURLs) Store(urls map[string]map[string]string) { + a.urls.Store(&urls) +} + +func (a *AppURLs) GetMimeTypes() []string { + currentURLs := a.urls.Load() + if currentURLs == nil { + return []string{} + } + + mimeTypesMap := make(map[string]bool) + for _, extensions := range *currentURLs { + for ext := range extensions { + m := mime.Detect(false, ext) + // skip the default + if m == "application/octet-stream" { + continue + } + mimeTypesMap[m] = true + } + } + + // Convert map to slice + mimeTypes := make([]string, 0, len(mimeTypesMap)) + for mimeType := range mimeTypesMap { + mimeTypes = append(mimeTypes, mimeType) + } + + return mimeTypes +} + +// GetAppURLFor gets the appURL from the list of appURLs based on the +// action and file extension provided. If there is no match, an empty +// string will be returned. +func (a *AppURLs) GetAppURLFor(action, fileExt string) string { + currentURLs := a.urls.Load() + if currentURLs == nil { + return "" + } + + if actionURL, ok := (*currentURLs)[action]; ok { + if actionExtensionURL, ok := actionURL[fileExt]; ok { + return actionExtensionURL + } + } + return "" +} + // GetAppURLs gets the edit and view urls for different file types from the // target WOPI app (onlyoffice, collabora, etc) via their "/hosting/discovery" // endpoint. diff --git a/services/collaboration/pkg/helpers/discovery_test.go b/services/collaboration/pkg/helpers/discovery_test.go index 2db298527f..6891323f35 100644 --- a/services/collaboration/pkg/helpers/discovery_test.go +++ b/services/collaboration/pkg/helpers/discovery_test.go @@ -12,6 +12,265 @@ import ( "github.com/opencloud-eu/opencloud/services/collaboration/pkg/helpers" ) +var _ = Describe("AppURLs", func() { + var appURLs *helpers.AppURLs + + BeforeEach(func() { + appURLs = helpers.NewAppURLs() + }) + + Describe("NewAppURLs", func() { + It("should create a new AppURLs instance with empty map", func() { + Expect(appURLs).NotTo(BeNil()) + Expect(appURLs.GetMimeTypes()).To(BeEmpty()) + Expect(appURLs.GetAppURLFor("view", ".pdf")).To(BeEmpty()) + }) + }) + + Describe("Store and GetAppURLFor", func() { + It("should store and retrieve app URLs correctly", func() { + testURLs := map[string]map[string]string{ + "view": { + ".pdf": "https://example.com/view/pdf", + ".docx": "https://example.com/view/docx", + ".xlsx": "https://example.com/view/xlsx", + }, + "edit": { + ".docx": "https://example.com/edit/docx", + ".xlsx": "https://example.com/edit/xlsx", + }, + } + + appURLs.Store(testURLs) + + // Test successful lookups + Expect(appURLs.GetAppURLFor("view", ".pdf")).To(Equal("https://example.com/view/pdf")) + Expect(appURLs.GetAppURLFor("view", ".docx")).To(Equal("https://example.com/view/docx")) + Expect(appURLs.GetAppURLFor("edit", ".docx")).To(Equal("https://example.com/edit/docx")) + }) + + It("should return empty string for non-existent action", func() { + testURLs := map[string]map[string]string{ + "view": {".pdf": "https://example.com/view/pdf"}, + } + + appURLs.Store(testURLs) + + Expect(appURLs.GetAppURLFor("nonexistent", ".pdf")).To(BeEmpty()) + }) + + It("should return empty string for non-existent extension", func() { + testURLs := map[string]map[string]string{ + "view": {".pdf": "https://example.com/view/pdf"}, + } + + appURLs.Store(testURLs) + + Expect(appURLs.GetAppURLFor("view", ".nonexistent")).To(BeEmpty()) + }) + + It("should handle empty maps gracefully", func() { + emptyURLs := map[string]map[string]string{} + appURLs.Store(emptyURLs) + + Expect(appURLs.GetAppURLFor("view", ".pdf")).To(BeEmpty()) + }) + + It("should handle nil action maps gracefully", func() { + testURLs := map[string]map[string]string{ + "view": nil, + } + + appURLs.Store(testURLs) + + Expect(appURLs.GetAppURLFor("view", ".pdf")).To(BeEmpty()) + }) + }) + + Describe("GetMimeTypes", func() { + It("should return empty slice for empty AppURLs", func() { + mimeTypes := appURLs.GetMimeTypes() + Expect(mimeTypes).To(BeEmpty()) + }) + + It("should return correct mime types for known extensions", func() { + testURLs := map[string]map[string]string{ + "view": { + ".pdf": "https://example.com/view/pdf", + ".docx": "https://example.com/view/docx", + ".xlsx": "https://example.com/view/xlsx", + ".pptx": "https://example.com/view/pptx", + }, + "edit": { + ".txt": "https://example.com/edit/txt", + ".html": "https://example.com/edit/html", + }, + } + + appURLs.Store(testURLs) + + mimeTypes := appURLs.GetMimeTypes() + + // Should contain expected mime types (order doesn't matter) + Expect(mimeTypes).To(ContainElements( + "application/pdf", + "application/vnd.openxmlformats-officedocument.wordprocessingml.document", + "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet", + "application/vnd.openxmlformats-officedocument.presentationml.presentation", + "text/plain", + "text/html", + )) + + // Should not contain application/octet-stream (filtered out) + Expect(mimeTypes).NotTo(ContainElement("application/octet-stream")) + }) + + It("should deduplicate mime types across actions", func() { + testURLs := map[string]map[string]string{ + "view": { + ".pdf": "https://example.com/view/pdf", + ".txt": "https://example.com/view/txt", + }, + "edit": { + ".pdf": "https://example.com/edit/pdf", // Same extension as view + ".txt": "https://example.com/edit/txt", // Same extension as view + }, + } + + appURLs.Store(testURLs) + + mimeTypes := appURLs.GetMimeTypes() + + // Should only have unique mime types + Expect(mimeTypes).To(ContainElements("application/pdf", "text/plain")) + Expect(len(mimeTypes)).To(Equal(2)) // No duplicates + }) + + It("should filter out application/octet-stream mime types", func() { + testURLs := map[string]map[string]string{ + "view": { + ".pdf": "https://example.com/view/pdf", + ".unknown": "https://example.com/view/unknown", // This might return application/octet-stream + ".fake-ext": "https://example.com/view/fake", // This might return application/octet-stream + }, + } + + appURLs.Store(testURLs) + + mimeTypes := appURLs.GetMimeTypes() + + // Should contain PDF but not octet-stream + Expect(mimeTypes).To(ContainElement("application/pdf")) + Expect(mimeTypes).NotTo(ContainElement("application/octet-stream")) + }) + + It("should handle empty extension maps", func() { + testURLs := map[string]map[string]string{ + "view": {}, + "edit": {}, + } + + appURLs.Store(testURLs) + + mimeTypes := appURLs.GetMimeTypes() + Expect(mimeTypes).To(BeEmpty()) + }) + + It("should handle nil extension maps", func() { + testURLs := map[string]map[string]string{ + "view": nil, + "edit": nil, + } + + appURLs.Store(testURLs) + + mimeTypes := appURLs.GetMimeTypes() + Expect(mimeTypes).To(BeEmpty()) + }) + }) + + Describe("Concurrent Access", func() { + It("should handle concurrent reads and writes safely", func() { + // This is a basic smoke test for concurrent access + // In practice, you'd want more sophisticated race testing + + initialURLs := map[string]map[string]string{ + "view": {".pdf": "https://example.com/view/pdf"}, + } + appURLs.Store(initialURLs) + + done := make(chan bool, 10) + + // Start multiple readers + for i := 0; i < 5; i++ { + go func() { + defer GinkgoRecover() + for j := 0; j < 100; j++ { + _ = appURLs.GetAppURLFor("view", ".pdf") + _ = appURLs.GetMimeTypes() + } + done <- true + }() + } + + // Start multiple writers + for i := 0; i < 5; i++ { + go func(id int) { + defer GinkgoRecover() + for j := 0; j < 100; j++ { + newURLs := map[string]map[string]string{ + "view": {".pdf": "https://example.com/updated/pdf"}, + } + appURLs.Store(newURLs) + } + done <- true + }(i) + } + + // Wait for all goroutines to complete + for i := 0; i < 10; i++ { + <-done + } + + // Should still be functional after concurrent access + Expect(appURLs.GetAppURLFor("view", ".pdf")).NotTo(BeEmpty()) + }) + }) + + Describe("Real-world scenarios", func() { + It("should handle realistic WOPI discovery data", func() { + // Based on the test data from the discovery tests + realisticURLs := map[string]map[string]string{ + "view": { + ".pdf": "https://cloud.opencloud.test/hosting/wopi/word/view", + ".djvu": "https://cloud.opencloud.test/hosting/wopi/word/view", + ".docx": "https://cloud.opencloud.test/hosting/wopi/word/view", + ".xls": "https://cloud.opencloud.test/hosting/wopi/cell/view", + ".xlsb": "https://cloud.opencloud.test/hosting/wopi/cell/view", + }, + "edit": { + ".docx": "https://cloud.opencloud.test/hosting/wopi/word/edit", + }, + } + + appURLs.Store(realisticURLs) + + // Test specific lookups + Expect(appURLs.GetAppURLFor("view", ".pdf")).To(Equal("https://cloud.opencloud.test/hosting/wopi/word/view")) + Expect(appURLs.GetAppURLFor("edit", ".docx")).To(Equal("https://cloud.opencloud.test/hosting/wopi/word/edit")) + Expect(appURLs.GetAppURLFor("edit", ".pdf")).To(BeEmpty()) // No edit for PDF + + // Test mime types + mimeTypes := appURLs.GetMimeTypes() + Expect(mimeTypes).To(ContainElements( + "application/pdf", + "application/vnd.openxmlformats-officedocument.wordprocessingml.document", + "application/vnd.ms-excel", + )) + }) + }) +}) + var _ = Describe("Discovery", func() { var ( discoveryContent1 string diff --git a/services/collaboration/pkg/helpers/registration.go b/services/collaboration/pkg/helpers/registration.go index fff73df152..2a28708cf7 100644 --- a/services/collaboration/pkg/helpers/registration.go +++ b/services/collaboration/pkg/helpers/registration.go @@ -7,7 +7,6 @@ import ( registryv1beta1 "github.com/cs3org/go-cs3apis/cs3/app/registry/v1beta1" gatewayv1beta1 "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" rpcv1beta1 "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" - "github.com/opencloud-eu/reva/v2/pkg/mime" "github.com/opencloud-eu/reva/v2/pkg/rgrpc/todo/pool" "github.com/opencloud-eu/opencloud/pkg/log" @@ -35,24 +34,9 @@ func RegisterAppProvider( cfg *config.Config, logger log.Logger, gws pool.Selectable[gatewayv1beta1.GatewayAPIClient], - appUrls map[string]map[string]string, + appUrls *AppURLs, ) error { - mimeTypesMap := make(map[string]bool) - for _, extensions := range appUrls { - for ext := range extensions { - m := mime.Detect(false, ext) - // skip the default - if m == "application/octet-stream" { - continue - } - mimeTypesMap[m] = true - } - } - - mimeTypes := make([]string, 0, len(mimeTypesMap)) - for m := range mimeTypesMap { - mimeTypes = append(mimeTypes, m) - } + mimeTypes := appUrls.GetMimeTypes() logger.Debug(). Str("AppName", cfg.App.Name). diff --git a/services/collaboration/pkg/server/grpc/option.go b/services/collaboration/pkg/server/grpc/option.go index 0ff72051f3..2cf3dc0e52 100644 --- a/services/collaboration/pkg/server/grpc/option.go +++ b/services/collaboration/pkg/server/grpc/option.go @@ -5,6 +5,7 @@ import ( "github.com/opencloud-eu/opencloud/pkg/log" "github.com/opencloud-eu/opencloud/services/collaboration/pkg/config" + "github.com/opencloud-eu/opencloud/services/collaboration/pkg/helpers" microstore "go-micro.dev/v4/store" "go.opentelemetry.io/otel/trace" ) @@ -14,7 +15,7 @@ type Option func(o *Options) // Options defines the available options for this package. type Options struct { - AppURLs map[string]map[string]string + AppURLs *helpers.AppURLs Name string Logger log.Logger Context context.Context @@ -35,7 +36,7 @@ func newOptions(opts ...Option) Options { } // AppURLs provides app urls based on mimetypes. -func AppURLs(val map[string]map[string]string) Option { +func AppURLs(val *helpers.AppURLs) Option { return func(o *Options) { o.AppURLs = val } diff --git a/services/collaboration/pkg/service/grpc/v0/option.go b/services/collaboration/pkg/service/grpc/v0/option.go index f031f62853..2bc1a8c232 100644 --- a/services/collaboration/pkg/service/grpc/v0/option.go +++ b/services/collaboration/pkg/service/grpc/v0/option.go @@ -7,6 +7,7 @@ import ( "github.com/opencloud-eu/opencloud/pkg/log" "github.com/opencloud-eu/opencloud/services/collaboration/pkg/config" + "github.com/opencloud-eu/opencloud/services/collaboration/pkg/helpers" ) // Option defines a single option function. @@ -16,7 +17,7 @@ type Option func(o *Options) type Options struct { Logger log.Logger Config *config.Config - AppURLs map[string]map[string]string + AppURLs *helpers.AppURLs GatewaySelector pool.Selectable[gatewayv1beta1.GatewayAPIClient] Store microstore.Store } @@ -47,7 +48,7 @@ func Config(val *config.Config) Option { } // AppURLs provides a function to set the AppURLs option. -func AppURLs(val map[string]map[string]string) Option { +func AppURLs(val *helpers.AppURLs) Option { return func(o *Options) { o.AppURLs = val } diff --git a/services/collaboration/pkg/service/grpc/v0/service.go b/services/collaboration/pkg/service/grpc/v0/service.go index 33a95a2f9d..e64ff20bca 100644 --- a/services/collaboration/pkg/service/grpc/v0/service.go +++ b/services/collaboration/pkg/service/grpc/v0/service.go @@ -42,6 +42,10 @@ func NewHandler(opts ...Option) (*Service, func(), error) { } } + if options.AppURLs == nil { + return nil, teardown, errors.New("AppURLs option is required") + } + return &Service{ id: options.Config.GRPC.Namespace + "." + options.Config.Service.Name + "." + options.Config.App.Name, appURLs: options.AppURLs, @@ -55,7 +59,7 @@ func NewHandler(opts ...Option) (*Service, func(), error) { // Service implements the OpenInApp interface type Service struct { id string - appURLs map[string]map[string]string + appURLs *helpers.AppURLs logger log.Logger config *config.Config gatewaySelector pool.Selectable[gatewayv1beta1.GatewayAPIClient] @@ -173,18 +177,6 @@ func (s *Service) OpenInApp( }, nil } -// getAppUrlFor gets the appURL from the list of appURLs based on the -// action and file extension provided. If there is no match, an empty -// string will be returned. -func (s *Service) getAppUrlFor(action, fileExt string) string { - if actionURL, ok := s.appURLs[action]; ok { - if actionExtensionURL, ok := actionURL[fileExt]; ok { - return actionExtensionURL - } - } - return "" -} - // getAppUrl will get the appURL that should be used based on the extension // and the provided view mode. // "view" urls will be chosen first, then if the view mode is "read/write", @@ -192,17 +184,17 @@ func (s *Service) getAppUrlFor(action, fileExt string) string { // "read/write" view mode if no "edit" url is found. func (s *Service) getAppUrl(fileExt string, viewMode appproviderv1beta1.ViewMode) string { // prioritize view action if possible - appURL := s.getAppUrlFor("view", fileExt) + appURL := s.appURLs.GetAppURLFor("view", fileExt) if strings.ToLower(s.config.App.Product) == "collabora" { // collabora provides only one action per extension. usual options // are "view" (checked above), "edit" or "view_comment" (this last one // is exclusive of collabora) if appURL == "" { - if editURL := s.getAppUrlFor("edit", fileExt); editURL != "" { + if editURL := s.appURLs.GetAppURLFor("edit", fileExt); editURL != "" { return editURL } - if commentURL := s.getAppUrlFor("view_comment", fileExt); commentURL != "" { + if commentURL := s.appURLs.GetAppURLFor("view_comment", fileExt); commentURL != "" { return commentURL } } @@ -210,7 +202,7 @@ func (s *Service) getAppUrl(fileExt string, viewMode appproviderv1beta1.ViewMode // If not collabora, there might be an edit action for the extension. // If read/write mode has been requested, prioritize edit action. if viewMode == appproviderv1beta1.ViewMode_VIEW_MODE_READ_WRITE { - if editAppURL := s.getAppUrlFor("edit", fileExt); editAppURL != "" { + if editAppURL := s.appURLs.GetAppURLFor("edit", fileExt); editAppURL != "" { appURL = editAppURL } } diff --git a/services/collaboration/pkg/service/grpc/v0/service_test.go b/services/collaboration/pkg/service/grpc/v0/service_test.go index 3a392aec73..22707752e5 100644 --- a/services/collaboration/pkg/service/grpc/v0/service_test.go +++ b/services/collaboration/pkg/service/grpc/v0/service_test.go @@ -23,6 +23,7 @@ import ( "github.com/opencloud-eu/opencloud/pkg/log" "github.com/opencloud-eu/opencloud/services/collaboration/mocks" "github.com/opencloud-eu/opencloud/services/collaboration/pkg/config" + "github.com/opencloud-eu/opencloud/services/collaboration/pkg/helpers" service "github.com/opencloud-eu/opencloud/services/collaboration/pkg/service/grpc/v0" ) @@ -80,22 +81,25 @@ var _ = Describe("Discovery", func() { gatewaySelector := mocks.NewSelectable[gatewayv1beta1.GatewayAPIClient](GinkgoT()) gatewaySelector.On("Next").Return(gatewayClient, nil) + appURLs := helpers.NewAppURLs() + appURLs.Store(map[string]map[string]string{ + "view": { + ".pdf": "https://cloud.opencloud.test/hosting/wopi/word/view", + ".djvu": "https://cloud.opencloud.test/hosting/wopi/word/view", + ".docx": "https://cloud.opencloud.test/hosting/wopi/word/view", + ".xls": "https://cloud.opencloud.test/hosting/wopi/cell/view", + ".xlsb": "https://cloud.opencloud.test/hosting/wopi/cell/view", + }, + "edit": { + ".docx": "https://cloud.opencloud.test/hosting/wopi/word/edit", + ".invalid": "://cloud.opencloud.test/hosting/wopi/cell/edit", + }, + }) + srv, srvTear, _ = service.NewHandler( service.Logger(log.NopLogger()), service.Config(cfg), - service.AppURLs(map[string]map[string]string{ - "view": { - ".pdf": "https://cloud.opencloud.test/hosting/wopi/word/view", - ".djvu": "https://cloud.opencloud.test/hosting/wopi/word/view", - ".docx": "https://cloud.opencloud.test/hosting/wopi/word/view", - ".xls": "https://cloud.opencloud.test/hosting/wopi/cell/view", - ".xlsb": "https://cloud.opencloud.test/hosting/wopi/cell/view", - }, - "edit": { - ".docx": "https://cloud.opencloud.test/hosting/wopi/word/edit", - ".invalid": "://cloud.opencloud.test/hosting/wopi/cell/edit", - }, - }), + service.AppURLs(appURLs), service.GatewaySelector(gatewaySelector), ) })