Override provider if was previously registered (#2168)

* Override the provider in the list of mime apps when re-registering

* Add unit test for overriding an existent provider

* Add changelog

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

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

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: &registrypb.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: &registrypb.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: &registrypb.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 {