Merge pull request #1542 from opencloud-eu/non-blocking-collaborations-service

introduce AppURLs helper for atomic backgroud updates

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),

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.

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

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).

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
 	}

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
 	}

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
 			}
 		}

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),
 		)
 	})