From 507e5ee4dacac428c535832a7cd7a5c238f42b6b Mon Sep 17 00:00:00 2001 From: Gianmaria Del Monte Date: Wed, 13 Oct 2021 16:31:28 +0200 Subject: [PATCH 1/3] Override the provider in the list of mime apps when re-registering --- pkg/app/registry/static/static.go | 39 +++++++++++++++++++++++-------- 1 file changed, 29 insertions(+), 10 deletions(-) diff --git a/pkg/app/registry/static/static.go b/pkg/app/registry/static/static.go index 2380f45e9c..dcce6e2bdc 100644 --- a/pkg/app/registry/static/static.go +++ b/pkg/app/registry/static/static.go @@ -21,7 +21,6 @@ package static import ( "context" "fmt" - "reflect" "strings" "sync" @@ -118,7 +117,20 @@ func New(m map[string]interface{}) (app.Registry, error) { return &newManager, nil } +// remove a provider from the provider list in a mime type +// it's a no-op if the provider is not in the list of providers in the mime type +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:]...) + } +} + func registerProvider(p *registrypb.ProviderInfo, mime *mimeTypeConfig) { + // the app provider could be previously registered to the same mime type list + // so we will remove it + unregisterProvider(p, mime) + if providerIsDefaultForMimeType(p, mime) { mime.apps = prependProvider(p, mime.apps) } else { @@ -161,14 +173,25 @@ func (m *manager) AddProvider(ctx context.Context, p *registrypb.ProviderInfo) e m.Lock() defer m.Unlock() - m.providers[p.Address] = p + // check if the provider was already registered + // if it's the case, we have to unregister it + // from all the old mime types + if oldP, ok := m.providers[p.Address]; ok { + oldMimeTypes := oldP.MimeTypes + for _, mimeName := range oldMimeTypes { + mimeIf, ok := m.mimetypes.Get(mimeName) + if !ok { + continue + } + mime := mimeIf.(*mimeTypeConfig) + unregisterProvider(p, mime) + } + } - // log := appctx.GetLogger(ctx) + m.providers[p.Address] = p for _, mime := range p.MimeTypes { if mimeTypeInterface, ok := m.mimetypes.Get(mime); ok { - // TODO (gdelmont): don't add to the list of apps an AppProvider - // that was already registered mimeType := mimeTypeInterface.(*mimeTypeConfig) registerProvider(p, mimeType) } else { @@ -288,11 +311,7 @@ func (m *manager) GetDefaultProviderForMimeType(ctx context.Context, mimeType st } func equalsProviderInfo(p1, p2 *registrypb.ProviderInfo) bool { - return p1.Address == p2.Address && - p1.Name == p2.Name && - reflect.DeepEqual(p1.MimeTypes, p2.MimeTypes) && - p1.Description == p2.Description && - p1.DesktopOnly == p2.DesktopOnly + return p1.Name == p2.Name } // check that all providers in the two lists are equals From 872dcee680c6e6d59831fa726bf2695eb04d0d62 Mon Sep 17 00:00:00 2001 From: Gianmaria Del Monte Date: Wed, 13 Oct 2021 16:34:15 +0200 Subject: [PATCH 2/3] Add unit test for overriding an existent provider --- pkg/app/registry/static/static_test.go | 151 +++++++++++++++++++++++++ 1 file changed, 151 insertions(+) diff --git a/pkg/app/registry/static/static_test.go b/pkg/app/registry/static/static_test.go index dcab5fc66a..450b88aff6 100644 --- a/pkg/app/registry/static/static_test.go +++ b/pkg/app/registry/static/static_test.go @@ -306,6 +306,157 @@ func TestAddProvider(t *testing.T) { }, }, }, + { + name: "register a provider already registered", + mimeTypes: []*mimeTypeConfig{ + { + MimeType: "text/json", + Extension: "json", + Name: "JSON File", + Icon: "https://example.org/icons&file=json.png", + DefaultApp: "provider2", + }, + }, + initProviders: []*registrypb.ProviderInfo{ + { + MimeTypes: []string{"text/json"}, + Address: "ip-provider1", + Name: "provider1", + }, + { + MimeTypes: []string{"text/json"}, + Address: "ip-provider2", + Name: "provider2", + }, + }, + newProvider: ®istrypb.ProviderInfo{ + MimeTypes: []string{"text/json"}, + Address: "ip-provider1", + Name: "provider1", + }, + expectedProviders: map[string][]*registrypb.ProviderInfo{ + "text/json": { + { + MimeTypes: []string{"text/json"}, + Address: "ip-provider2", + Name: "provider2", + }, + { + MimeTypes: []string{"text/json"}, + Address: "ip-provider1", + Name: "provider1", + }, + }, + }, + }, + { + name: "register a provider already registered supporting more 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", + }, + }, + initProviders: []*registrypb.ProviderInfo{ + { + MimeTypes: []string{"text/json"}, + Address: "ip-provider1", + Name: "provider1", + }, + { + MimeTypes: []string{"text/json"}, + Address: "ip-provider2", + Name: "provider2", + }, + }, + newProvider: ®istrypb.ProviderInfo{ + MimeTypes: []string{"text/json", "text/xml"}, + Address: "ip-provider1", + Name: "provider1", + }, + expectedProviders: map[string][]*registrypb.ProviderInfo{ + "text/json": { + { + MimeTypes: []string{"text/json"}, + Address: "ip-provider2", + Name: "provider2", + }, + { + MimeTypes: []string{"text/json", "text/xml"}, + Address: "ip-provider1", + Name: "provider1", + }, + }, + "text/xml": { + { + MimeTypes: []string{"text/json", "text/xml"}, + Address: "ip-provider1", + Name: "provider1", + }, + }, + }, + }, + { + name: "register a provider already registered supporting less 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", + }, + }, + initProviders: []*registrypb.ProviderInfo{ + { + MimeTypes: []string{"text/json", "text/xml"}, + Address: "ip-provider1", + Name: "provider1", + }, + { + MimeTypes: []string{"text/json"}, + Address: "ip-provider2", + Name: "provider2", + }, + }, + newProvider: ®istrypb.ProviderInfo{ + MimeTypes: []string{"text/json"}, + Address: "ip-provider1", + Name: "provider1", + }, + expectedProviders: map[string][]*registrypb.ProviderInfo{ + "text/json": { + { + MimeTypes: []string{"text/json"}, + Address: "ip-provider2", + Name: "provider2", + }, + { + MimeTypes: []string{"text/json"}, + Address: "ip-provider1", + Name: "provider1", + }, + }, + "text/xml": {}, + }, + }, } for _, tt := range testCases { From 01ff5d2c35a609f3e4b53cd77ef467a87e43b637 Mon Sep 17 00:00:00 2001 From: Gianmaria Del Monte Date: Wed, 13 Oct 2021 17:17:14 +0200 Subject: [PATCH 3/3] Add changelog --- changelog/unreleased/appregistry-register-once.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 changelog/unreleased/appregistry-register-once.md diff --git a/changelog/unreleased/appregistry-register-once.md b/changelog/unreleased/appregistry-register-once.md new file mode 100644 index 0000000000..45b4e4e036 --- /dev/null +++ b/changelog/unreleased/appregistry-register-once.md @@ -0,0 +1,8 @@ +Bugfix: Override provider if was previously registered + +Previously if an AppProvider registered himself two times, for example +after a failure, the mime types supported by the provider contained +multiple times the same provider. +Now this has been fixed, overriding the previous one. + +https://github.com/cs3org/reva/pull/2168