From a73b194a134e1f415567699c594d29ab18ac147b Mon Sep 17 00:00:00 2001 From: Gianmaria Del Monte <39946305+gmgigi96@users.noreply.github.com> Date: Thu, 18 Nov 2021 13:49:36 +0100 Subject: [PATCH] Add priority to app providers (#2263) --- changelog/unreleased/app-registry-priority.md | 12 + .../grpc/services/appprovider/appprovider.go | 18 +- pkg/app/registry/static/static.go | 104 +++-- pkg/app/registry/static/static_test.go | 387 ++++++++++++++++-- 4 files changed, 453 insertions(+), 68 deletions(-) create mode 100644 changelog/unreleased/app-registry-priority.md diff --git a/changelog/unreleased/app-registry-priority.md b/changelog/unreleased/app-registry-priority.md new file mode 100644 index 0000000000..1b23a68bbb --- /dev/null +++ b/changelog/unreleased/app-registry-priority.md @@ -0,0 +1,12 @@ +Enhancement: Add priority to app providers + +Before the order of the list returned by the method FindProviders +of app providers depended from the order in which the app provider registered +themselves. +Now, it is possible to specify a priority for each app provider, and even if +an app provider re-register itself (for example after a restart), the order +is kept. + +https://github.com/cs3org/reva/pull/2230 +https://github.com/cs3org/cs3apis/pull/157 +https://github.com/cs3org/reva/pull/2263 \ No newline at end of file diff --git a/internal/grpc/services/appprovider/appprovider.go b/internal/grpc/services/appprovider/appprovider.go index 1c08b8899f..ca1b86b61a 100644 --- a/internal/grpc/services/appprovider/appprovider.go +++ b/internal/grpc/services/appprovider/appprovider.go @@ -22,11 +22,13 @@ import ( "context" "errors" "os" + "strconv" "time" providerpb "github.com/cs3org/go-cs3apis/cs3/app/provider/v1beta1" registrypb "github.com/cs3org/go-cs3apis/cs3/app/registry/v1beta1" rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" + types "github.com/cs3org/go-cs3apis/cs3/types/v1beta1" "github.com/cs3org/reva/pkg/app" "github.com/cs3org/reva/pkg/app/provider/registry" "github.com/cs3org/reva/pkg/errtypes" @@ -55,6 +57,7 @@ type config struct { AppProviderURL string `mapstructure:"app_provider_url"` GatewaySvc string `mapstructure:"gatewaysvc"` MimeTypes []string `mapstructure:"mime_types"` + Priority uint64 `mapstructure:"priority"` } func (c *config) init() { @@ -122,7 +125,20 @@ func (s *service) registerProvider() { log.Error().Err(err).Msgf("error registering app provider: could not get gateway client") return } - res, err := client.AddAppProvider(ctx, ®istrypb.AddAppProviderRequest{Provider: pInfo}) + req := ®istrypb.AddAppProviderRequest{Provider: pInfo} + + if s.conf.Priority != 0 { + req.Opaque = &types.Opaque{ + Map: map[string]*types.OpaqueEntry{ + "priority": { + Decoder: "plain", + Value: []byte(strconv.FormatUint(s.conf.Priority, 10)), + }, + }, + } + } + + res, err := client.AddAppProvider(ctx, req) if err != nil { log.Error().Err(err).Msgf("error registering app provider: error calling add app provider") return diff --git a/pkg/app/registry/static/static.go b/pkg/app/registry/static/static.go index 5c803e017a..255627b0b7 100644 --- a/pkg/app/registry/static/static.go +++ b/pkg/app/registry/static/static.go @@ -19,8 +19,10 @@ package static import ( + "container/heap" "context" "fmt" + "strconv" "strings" "sync" @@ -37,6 +39,8 @@ func init() { registry.Register("static", New) } +const defaultPriority = 0 + type mimeTypeConfig struct { MimeType string `mapstructure:"mime_type"` Extension string `mapstructure:"extension"` @@ -45,9 +49,7 @@ type mimeTypeConfig struct { Icon string `mapstructure:"icon"` DefaultApp string `mapstructure:"default_app"` AllowCreation bool `mapstructure:"allow_creation"` - // apps keeps the Providers able to open this mime type. - // the list will always keep the default AppProvider at the head - apps []*registrypb.ProviderInfo + apps providerHeap } type config struct { @@ -122,7 +124,7 @@ func New(m map[string]interface{}) (app.Registry, error) { func unregisterProvider(p *registrypb.ProviderInfo, mime *mimeTypeConfig) { if index, in := getIndex(mime.apps, p); in { // remove the provider from the list - mime.apps = append(mime.apps[:index], mime.apps[index+1:]...) + heap.Remove(&mime.apps, index) } } @@ -131,11 +133,21 @@ func registerProvider(p *registrypb.ProviderInfo, mime *mimeTypeConfig) { // so we will remove it unregisterProvider(p, mime) - if providerIsDefaultForMimeType(p, mime) { - mime.apps = prependProvider(p, mime.apps) - } else { - mime.apps = append(mime.apps, p) + heap.Push(&mime.apps, providerWithPriority{ + provider: p, + priority: getPriority(p), + }) +} + +func getPriority(p *registrypb.ProviderInfo) uint64 { + if p.Opaque != nil && len(p.Opaque.Map) != 0 { + if priority, ok := p.Opaque.Map["priority"]; ok { + if pr, err := strconv.ParseUint(string(priority.GetValue()), 10, 64); err == nil { + return pr + } + } } + return defaultPriority } func (m *manager) FindProviders(ctx context.Context, mimeType string) ([]*registrypb.ProviderInfo, error) { @@ -160,15 +172,11 @@ func (m *manager) FindProviders(ctx context.Context, mimeType string) ([]*regist mimeMatch := mimeInterface.(*mimeTypeConfig) var providers = make([]*registrypb.ProviderInfo, 0, len(mimeMatch.apps)) for _, p := range mimeMatch.apps { - providers = append(providers, m.providers[p.Address]) + providers = append(providers, m.providers[p.provider.Address]) } return providers, nil } -func providerIsDefaultForMimeType(p *registrypb.ProviderInfo, mime *mimeTypeConfig) bool { - return p.Address == mime.DefaultApp || p.Name == mime.DefaultApp -} - func (m *manager) AddProvider(ctx context.Context, p *registrypb.ProviderInfo) error { m.Lock() defer m.Unlock() @@ -232,7 +240,7 @@ func (m *manager) ListSupportedMimeTypes(ctx context.Context) ([]*registrypb.Mim Name: mime.Name, Description: mime.Description, Icon: mime.Icon, - AppProviders: mime.apps, + AppProviders: mime.apps.getOrderedProviderByPriority(), AllowCreation: mime.AllowCreation, DefaultApplication: mime.DefaultApp, }) @@ -242,17 +250,17 @@ func (m *manager) ListSupportedMimeTypes(ctx context.Context) ([]*registrypb.Mim return res, nil } -// prepend an AppProvider obj to the list -func prependProvider(n *registrypb.ProviderInfo, lst []*registrypb.ProviderInfo) []*registrypb.ProviderInfo { - lst = append(lst, ®istrypb.ProviderInfo{}) - copy(lst[1:], lst) - lst[0] = n - return lst +func (h providerHeap) getOrderedProviderByPriority() []*registrypb.ProviderInfo { + providers := make([]*registrypb.ProviderInfo, 0, h.Len()) + for _, pp := range h { + providers = append(providers, pp.provider) + } + return providers } -func getIndex(lst []*registrypb.ProviderInfo, s *registrypb.ProviderInfo) (int, bool) { - for i, e := range lst { - if equalsProviderInfo(e, s) { +func getIndex(h providerHeap, s *registrypb.ProviderInfo) (int, bool) { + for i, e := range h { + if equalsProviderInfo(e.provider, s) { return i, true } } @@ -268,15 +276,7 @@ func (m *manager) SetDefaultProviderForMimeType(ctx context.Context, mimeType st mime := mimeInterface.(*mimeTypeConfig) mime.DefaultApp = p.Address - if index, in := getIndex(mime.apps, p); in { - // the element is in the list, we will remove it - // TODO (gdelmont): not the best way to remove an element from a slice - // but maybe we want to keep the order? - mime.apps = append(mime.apps[:index], mime.apps[index+1:]...) - } - // prepend it to the front of the list - mime.apps = prependProvider(p, mime.apps) - + registerProvider(p, mime) } else { // the mime type should be already registered as config in the AppRegistry // we will create a new entry fot the mimetype, but leaving a warning for @@ -288,9 +288,17 @@ func (m *manager) SetDefaultProviderForMimeType(ctx context.Context, mimeType st } func dummyMimeType(m string, apps []*registrypb.ProviderInfo) *mimeTypeConfig { + appsHeap := providerHeap{} + for _, p := range apps { + heap.Push(&appsHeap, providerWithPriority{ + provider: p, + priority: getPriority(p), + }) + } + return &mimeTypeConfig{ MimeType: m, - apps: apps, + apps: appsHeap, //Extension: "", // there is no meaningful general extension, so omit it //Name: "", // there is no meaningful general name, so omit it //Description: "", // there is no meaningful general description, so omit it @@ -337,3 +345,33 @@ func providersEquals(l1, l2 []*registrypb.ProviderInfo) bool { } return true } + +type providerWithPriority struct { + provider *registrypb.ProviderInfo + priority uint64 +} + +type providerHeap []providerWithPriority + +func (h providerHeap) Len() int { + return len(h) +} + +func (h providerHeap) Less(i, j int) bool { + return h[i].priority > h[j].priority +} + +func (h providerHeap) Swap(i, j int) { + h[i], h[j] = h[j], h[i] +} + +func (h *providerHeap) Push(x interface{}) { + *h = append(*h, x.(providerWithPriority)) +} + +func (h *providerHeap) Pop() interface{} { + last := len(*h) - 1 + x := (*h)[last] + *h = (*h)[:last] + return x +} diff --git a/pkg/app/registry/static/static_test.go b/pkg/app/registry/static/static_test.go index 450b88aff6..f23d4005c5 100644 --- a/pkg/app/registry/static/static_test.go +++ b/pkg/app/registry/static/static_test.go @@ -24,6 +24,7 @@ import ( "testing" registrypb "github.com/cs3org/go-cs3apis/cs3/app/registry/v1beta1" + typesv1beta1 "github.com/cs3org/go-cs3apis/cs3/types/v1beta1" "github.com/cs3org/reva/pkg/errtypes" ) @@ -101,6 +102,213 @@ func TestFindProviders(t *testing.T) { }, mimeType: "text/json", expectedErr: nil, + expectedRes: []*registrypb.ProviderInfo{ + { + MimeTypes: []string{"text/json"}, + Address: "ip-provider1", + Name: "provider1", + }, + { + MimeTypes: []string{"text/json"}, + Address: "ip-provider2", + Name: "provider2", + }, + { + MimeTypes: []string{"text/json"}, + Address: "ip-provider3", + Name: "provider3", + }, + }, + }, + { + name: "more providers registered for different mime types", + mimeTypes: []*mimeTypeConfig{ + { + MimeType: "text/json", + Extension: "json", + Name: "JSON File", + Icon: "https://example.org/icons&file=json.png", + DefaultApp: "provider2", + }, + { + MimeType: "text/xml", + Extension: "xml", + Name: "XML File", + Icon: "https://example.org/icons&file=xml.png", + DefaultApp: "provider1", + }, + }, + regProviders: []*registrypb.ProviderInfo{ + { + MimeTypes: []string{"text/json", "text/xml"}, + Address: "ip-provider1", + Name: "provider1", + }, + { + MimeTypes: []string{"text/json"}, + Address: "ip-provider2", + Name: "provider2", + }, + { + MimeTypes: []string{"text/xml"}, + Address: "ip-provider3", + Name: "provider3", + }, + }, + mimeType: "text/json", + expectedErr: nil, + expectedRes: []*registrypb.ProviderInfo{ + { + MimeTypes: []string{"text/json", "text/xml"}, + Address: "ip-provider1", + Name: "provider1", + }, + { + MimeTypes: []string{"text/json"}, + Address: "ip-provider2", + Name: "provider2", + }, + }, + }, + } + + for _, tt := range testCases { + + t.Run(tt.name, func(t *testing.T) { + + ctx := context.TODO() + + registry, err := New(map[string]interface{}{ + "mime_types": tt.mimeTypes, + "providers": tt.regProviders, + }) + if err != nil { + t.Error("unexpected error creating the registry:", err) + } + + providers, err := registry.FindProviders(ctx, tt.mimeType) + + // check that the error returned by FindProviders is the same as the expected + if tt.expectedErr != err { + t.Errorf("different error returned: got=%v expected=%v", err, tt.expectedErr) + } + + if !providersEquals(providers, tt.expectedRes) { + t.Errorf("providers list different from expected: \n\tgot=%v\n\texp=%v", providers, tt.expectedRes) + } + + }) + + } + +} + +func TestFindProvidersWithPriority(t *testing.T) { + + testCases := []struct { + name string + mimeTypes []*mimeTypeConfig + regProviders []*registrypb.ProviderInfo + mimeType string + expectedRes []*registrypb.ProviderInfo + expectedErr error + }{ + { + name: "no mime types registered", + mimeTypes: []*mimeTypeConfig{}, + mimeType: "SOMETHING", + expectedErr: errtypes.NotFound("application provider not found for mime type SOMETHING"), + }, + { + name: "one provider registered for one mime type", + mimeTypes: []*mimeTypeConfig{ + { + MimeType: "text/json", + Extension: "json", + Name: "JSON File", + Icon: "https://example.org/icons&file=json.png", + DefaultApp: "some Address", + }, + }, + regProviders: []*registrypb.ProviderInfo{ + { + MimeTypes: []string{"text/json"}, + Address: "some Address", + Name: "some Name", + Opaque: &typesv1beta1.Opaque{ + Map: map[string]*typesv1beta1.OpaqueEntry{ + "priority": { + Decoder: "plain", + Value: []byte("100"), + }, + }, + }, + }, + }, + mimeType: "text/json", + expectedErr: nil, + expectedRes: []*registrypb.ProviderInfo{ + { + MimeTypes: []string{"text/json"}, + Address: "some Address", + Name: "some Name", + }, + }, + }, + { + name: "more providers registered for one mime type", + mimeTypes: []*mimeTypeConfig{ + { + MimeType: "text/json", + Extension: "json", + Name: "JSON File", + Icon: "https://example.org/icons&file=json.png", + DefaultApp: "provider2", + }, + }, + regProviders: []*registrypb.ProviderInfo{ + { + MimeTypes: []string{"text/json"}, + Address: "ip-provider1", + Name: "provider1", + Opaque: &typesv1beta1.Opaque{ + Map: map[string]*typesv1beta1.OpaqueEntry{ + "priority": { + Decoder: "plain", + Value: []byte("10"), + }, + }, + }, + }, + { + MimeTypes: []string{"text/json"}, + Address: "ip-provider2", + Name: "provider2", + Opaque: &typesv1beta1.Opaque{ + Map: map[string]*typesv1beta1.OpaqueEntry{ + "priority": { + Decoder: "plain", + Value: []byte("20"), + }, + }, + }, + }, + { + MimeTypes: []string{"text/json"}, + Address: "ip-provider3", + Name: "provider3", + Opaque: &typesv1beta1.Opaque{ + Map: map[string]*typesv1beta1.OpaqueEntry{ + "priority": { + Decoder: "plain", + Value: []byte("5"), + }, + }, + }, + }, + }, + mimeType: "text/json", + expectedErr: nil, expectedRes: []*registrypb.ProviderInfo{ { MimeTypes: []string{"text/json"}, @@ -142,16 +350,40 @@ func TestFindProviders(t *testing.T) { MimeTypes: []string{"text/json", "text/xml"}, Address: "ip-provider1", Name: "provider1", + Opaque: &typesv1beta1.Opaque{ + Map: map[string]*typesv1beta1.OpaqueEntry{ + "priority": { + Decoder: "plain", + Value: []byte("5"), + }, + }, + }, }, { MimeTypes: []string{"text/json"}, Address: "ip-provider2", Name: "provider2", + Opaque: &typesv1beta1.Opaque{ + Map: map[string]*typesv1beta1.OpaqueEntry{ + "priority": { + Decoder: "plain", + Value: []byte("100"), + }, + }, + }, }, { MimeTypes: []string{"text/xml"}, Address: "ip-provider3", Name: "provider3", + Opaque: &typesv1beta1.Opaque{ + Map: map[string]*typesv1beta1.OpaqueEntry{ + "priority": { + Decoder: "plain", + Value: []byte("20"), + }, + }, + }, }, }, mimeType: "text/json", @@ -169,6 +401,80 @@ func TestFindProviders(t *testing.T) { }, }, }, + { + name: "more providers registered for different mime types2", + mimeTypes: []*mimeTypeConfig{ + { + MimeType: "text/json", + Extension: "json", + Name: "JSON File", + Icon: "https://example.org/icons&file=json.png", + DefaultApp: "provider2", + }, + { + MimeType: "text/xml", + Extension: "xml", + Name: "XML File", + Icon: "https://example.org/icons&file=xml.png", + DefaultApp: "provider1", + }, + }, + regProviders: []*registrypb.ProviderInfo{ + { + MimeTypes: []string{"text/json", "text/xml"}, + Address: "ip-provider1", + Name: "provider1", + Opaque: &typesv1beta1.Opaque{ + Map: map[string]*typesv1beta1.OpaqueEntry{ + "priority": { + Decoder: "plain", + Value: []byte("5"), + }, + }, + }, + }, + { + MimeTypes: []string{"text/json"}, + Address: "ip-provider2", + Name: "provider2", + Opaque: &typesv1beta1.Opaque{ + Map: map[string]*typesv1beta1.OpaqueEntry{ + "priority": { + Decoder: "plain", + Value: []byte("100"), + }, + }, + }, + }, + { + MimeTypes: []string{"text/xml"}, + Address: "ip-provider3", + Name: "provider3", + Opaque: &typesv1beta1.Opaque{ + Map: map[string]*typesv1beta1.OpaqueEntry{ + "priority": { + Decoder: "plain", + Value: []byte("20"), + }, + }, + }, + }, + }, + mimeType: "text/xml", + expectedErr: nil, + expectedRes: []*registrypb.ProviderInfo{ + { + MimeTypes: []string{"text/xml"}, + Address: "ip-provider3", + Name: "provider3", + }, + { + MimeTypes: []string{"text/json", "text/xml"}, + Address: "ip-provider1", + Name: "provider1", + }, + }, + }, } for _, tt := range testCases { @@ -179,12 +485,19 @@ func TestFindProviders(t *testing.T) { registry, err := New(map[string]interface{}{ "mime_types": tt.mimeTypes, - "providers": tt.regProviders, }) if err != nil { t.Error("unexpected error creating the registry:", err) } + // register all the providers + for _, p := range tt.regProviders { + err := registry.AddProvider(ctx, p) + if err != nil { + t.Error("unexpected error adding a new provider in the registry:", err) + } + } + providers, err := registry.FindProviders(ctx, tt.mimeType) // check that the error returned by FindProviders is the same as the expected @@ -257,13 +570,13 @@ func TestAddProvider(t *testing.T) { "text/json": { { MimeTypes: []string{"text/json"}, - Address: "ip-provider1", - Name: "provider1", + Address: "ip-provider2", + Name: "provider2", }, { MimeTypes: []string{"text/json"}, - Address: "ip-provider2", - Name: "provider2", + Address: "ip-provider1", + Name: "provider1", }, }, }, @@ -493,7 +806,7 @@ func TestAddProvider(t *testing.T) { for mime, expAddrs := range tt.expectedProviders { mimeConfInterface, _ := staticReg.mimetypes.Get(mime) - addrsReg := mimeConfInterface.(*mimeTypeConfig).apps + addrsReg := mimeConfInterface.(*mimeTypeConfig).apps.getOrderedProviderByPriority() if !reflect.DeepEqual(expAddrs, addrsReg) { t.Errorf("list of addresses different from expected: \n\tgot=%v\n\texp=%v", addrsReg, expAddrs) @@ -526,11 +839,12 @@ func TestListSupportedMimeTypes(t *testing.T) { newProviders: []*registrypb.ProviderInfo{}, expected: []*registrypb.MimeTypeInfo{ { - MimeType: "text/json", - Ext: "json", - AppProviders: []*registrypb.ProviderInfo{}, - Name: "JSON File", - Icon: "https://example.org/icons&file=json.png", + MimeType: "text/json", + Ext: "json", + AppProviders: []*registrypb.ProviderInfo{}, + Name: "JSON File", + Icon: "https://example.org/icons&file=json.png", + DefaultApplication: "provider2", }, }, }, @@ -563,8 +877,9 @@ func TestListSupportedMimeTypes(t *testing.T) { Name: "provider1", }, }, - Name: "JSON File", - Icon: "https://example.org/icons&file=json.png", + DefaultApplication: "provider1", + Name: "JSON File", + Icon: "https://example.org/icons&file=json.png", }, }, }, @@ -598,17 +913,18 @@ func TestListSupportedMimeTypes(t *testing.T) { AppProviders: []*registrypb.ProviderInfo{ { MimeTypes: []string{"text/json"}, - Address: "ip-provider1", - Name: "JSON_DEFAULT_PROVIDER", + Address: "ip-provider2", + Name: "NOT_DEFAULT_PROVIDER", }, { MimeTypes: []string{"text/json"}, - Address: "ip-provider2", - Name: "NOT_DEFAULT_PROVIDER", + Address: "ip-provider1", + Name: "JSON_DEFAULT_PROVIDER", }, }, - Name: "JSON File", - Icon: "https://example.org/icons&file=json.png", + DefaultApplication: "JSON_DEFAULT_PROVIDER", + Name: "JSON File", + Icon: "https://example.org/icons&file=json.png", }, }, }, @@ -657,34 +973,30 @@ func TestListSupportedMimeTypes(t *testing.T) { MimeType: "text/json", Ext: "json", AppProviders: []*registrypb.ProviderInfo{ - { - MimeTypes: []string{"text/xml", "text/json"}, - Address: "3", - Name: "JSON_DEFAULT_PROVIDER", - }, { MimeTypes: []string{"text/json", "text/xml"}, Address: "1", Name: "NOT_DEFAULT_PROVIDER2", }, + { + MimeTypes: []string{"text/xml", "text/json"}, + Address: "3", + Name: "JSON_DEFAULT_PROVIDER", + }, { MimeTypes: []string{"text/xml", "text/json"}, Address: "4", Name: "XML_DEFAULT_PROVIDER", }, }, - Name: "JSON File", - Icon: "https://example.org/icons&file=json.png", + DefaultApplication: "JSON_DEFAULT_PROVIDER", + Name: "JSON File", + Icon: "https://example.org/icons&file=json.png", }, { MimeType: "text/xml", Ext: "xml", AppProviders: []*registrypb.ProviderInfo{ - { - MimeTypes: []string{"text/xml", "text/json"}, - Address: "4", - Name: "XML_DEFAULT_PROVIDER", - }, { MimeTypes: []string{"text/json", "text/xml"}, Address: "1", @@ -700,9 +1012,15 @@ func TestListSupportedMimeTypes(t *testing.T) { Address: "3", Name: "JSON_DEFAULT_PROVIDER", }, + { + MimeTypes: []string{"text/xml", "text/json"}, + Address: "4", + Name: "XML_DEFAULT_PROVIDER", + }, }, - Name: "XML File", - Icon: "https://example.org/icons&file=xml.png", + DefaultApplication: "XML_DEFAULT_PROVIDER", + Name: "XML File", + Icon: "https://example.org/icons&file=xml.png", }, }, }, @@ -896,5 +1214,6 @@ func equalsMimeTypeInfo(m1, m2 *registrypb.MimeTypeInfo) bool { providersEquals(m1.AppProviders, m2.AppProviders) && m1.Ext == m2.Ext && m1.MimeType == m2.MimeType && - m1.Name == m2.Name + m1.Name == m2.Name && + m1.DefaultApplication == m2.DefaultApplication }