Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure default AppProvider is top of list and keep mime type order from config #2138

Merged
merged 5 commits into from
Oct 8, 2021
Merged

Ensure default AppProvider is top of list and keep mime type order from config #2138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4f4b281
Default AppProvider always on top of the list when asking for mime ty…
gmgigi96 Oct 7, 2021
e2a9062
Add tests
gmgigi96 Oct 7, 2021
df3b0d5
Add changelog
gmgigi96 Oct 7, 2021
ad330b9
Fix AppRegistry tests
gmgigi96 Oct 7, 2021
39d4e4d
Simplified code
gmgigi96 Oct 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions changelog/unreleased/app-registry-order.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
Enhancement: Default AppProvider on top of the providers list
for each mime type

Now for each mime type, when asking for the list of mime types,
the default AppProvider, set both using the config and the
SetDefaultProviderForMimeType method, is always in the top of the
list of AppProviders.
The config for the Providers and Mime Types for the AppRegistry changed,
using a list instead of a map. In fact the list of mime types returned by
ListSupportedMimeTypes is now ordered according the config.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When looking at a single entry in the list app providers response the provider infos do not let clients find out if they are dealing with the default provider or not. When listing them in the UI the list might be stored in a map, losing the order again.

We should expose the 'default' provider with a dedicated property in https://cs3org.github.io/cs3apis/#cs3.app.registry.v1beta1.ProviderInfo

A priority property might be more flexible.

Anyway, for humans, relying on the order of a list is good enough when dealing with checklists on paper, but for machines it is not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is true, but the order is important as well from a UI perspective - to make sure the interface is stable and does not get reshuffled.

Now given the "rush" to get the first deployment really out, I suggested to go ahead with just the reordering, to avoid another round of CS3APIs (likely breaking) change. But I agree we could make this fully explicit as a property.


https://github.com/cs3org/reva/pull/2138
1 change: 1 addition & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ require (
github.com/thanhpk/randstr v1.0.4
github.com/tidwall/pretty v1.2.0 // indirect
github.com/tus/tusd v1.6.0
github.com/wk8/go-ordered-map v0.2.0
go.mongodb.org/mongo-driver v1.7.2 // indirect
go.opencensus.io v0.23.0
go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc v0.24.0
Expand Down
2 changes: 2 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,6 +465,8 @@ github.com/tus/tusd v1.6.0/go.mod h1:ygrT4B9ZSb27dx3uTnobX5nOFDnutBL6iWKLH4+KpA0
github.com/ugorji/go v1.1.7/go.mod h1:kZn38zHttfInRq0xu/PH0az30d+z6vm202qpg1oXVMw=
github.com/ugorji/go/codec v1.1.7/go.mod h1:Ax+UKWsSmolVDwsd+7N3ZtXu+yMGCf907BLYF3GoBXY=
github.com/vimeo/go-util v1.2.0/go.mod h1:s13SMDTSO7AjH1nbgp707mfN5JFIWUFDU5MDDuRRtKs=
github.com/wk8/go-ordered-map v0.2.0 h1:KlvGyHstD1kkGZkPtHCyCfRYS0cz84uk6rrW/Dnhdtk=
github.com/wk8/go-ordered-map v0.2.0/go.mod h1:9ZIbRunKbuvfPKyBP1SIKLcXNlv74YCOZ3t3VTS6gRk=
github.com/xdg-go/pbkdf2 v1.0.0/go.mod h1:jrpuAogTd400dnrH08LKmI/xc1MbPOebTwRqcT5RDeI=
github.com/xdg-go/scram v1.0.2/go.mod h1:1WAq6h33pAW+iRreB34OORO2Nf7qel3VV3fjBj+hCSs=
github.com/xdg-go/stringprep v1.0.2/go.mod h1:8F9zXuvzgwmyT5DUm4GUfZGDdT3W+LCvS6+da4O5kxM=
Expand Down
67 changes: 29 additions & 38 deletions internal/grpc/services/appregistry/appregistry_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,30 +39,32 @@ func (a ByAddress) Swap(i, j int) { a[i], a[j] = a[j], a[i] }
func Test_ListAppProviders(t *testing.T) {
tests := []struct {
name string
providers map[string]interface{}
mimeTypes map[string]map[string]string
providers []map[string]interface{}
mimeTypes []map[string]interface{}
want *registrypb.ListAppProvidersResponse
}{
{
name: "simple test",
providers: map[string]interface{}{
"some Address": map[string]interface{}{
providers: []map[string]interface{}{
{
"address": "some Address",
"mimetypes": []string{"text/json"},
},
"another address": map[string]interface{}{
{
"address": "another address",
"mimetypes": []string{"currently/ignored"},
},
},
mimeTypes: map[string]map[string]string{
"text/json": {
mimeTypes: []map[string]interface{}{
{
"mime_type": "text/json",
"extension": "json",
"name": "JSON File",
"icon": "https://example.org/icons&file=json.png",
"default_app": "some Address",
},
"currently/ignored": {
{
"mime_type": "currently/ignored",
"extension": "unknown",
"name": "Ignored file",
"icon": "https://example.org/icons&file=unknown.png",
Expand Down Expand Up @@ -103,8 +105,8 @@ func Test_ListAppProviders(t *testing.T) {
},
{
name: "empty providers",
providers: map[string]interface{}{},
mimeTypes: map[string]map[string]string{},
providers: []map[string]interface{}{},
mimeTypes: []map[string]interface{}{},

// only Status and Providers will be asserted in the tests
want: &registrypb.ListAppProvidersResponse{
Expand All @@ -116,23 +118,6 @@ func Test_ListAppProviders(t *testing.T) {
Providers: []*registrypb.ProviderInfo{},
},
},
{
name: "provider value is nil",
providers: map[string]interface{}{
"some Address": nil,
},
mimeTypes: map[string]map[string]string{},

// only Status and Providers will be asserted in the tests
want: &registrypb.ListAppProvidersResponse{
Status: &rpcv1beta1.Status{
Code: 1,
Trace: "00000000000000000000000000000000",
Message: "",
},
Providers: []*registrypb.ProviderInfo{nil},
},
},
}

for _, tt := range tests {
Expand Down Expand Up @@ -161,53 +146,59 @@ func Test_ListAppProviders(t *testing.T) {
}

func Test_GetAppProviders(t *testing.T) {
providers := map[string]interface{}{
"text appprovider addr": map[string]interface{}{
providers := []map[string]interface{}{
{
"address": "text appprovider addr",
"mimetypes": []string{"text/json", "text/xml"},
},
"image appprovider addr": map[string]interface{}{
{
"address": "image appprovider addr",
"mimetypes": []string{"image/bmp"},
},
"misc appprovider addr": map[string]interface{}{
{
"address": "misc appprovider addr",
"mimetypes": []string{"application/vnd.openxmlformats-officedocument.wordprocessingml.document", "application/vnd.oasis.opendocument.presentation", "application/vnd.apple.installer+xml"},
},
}

mimeTypes := map[string]map[string]string{
"text/json": {
mimeTypes := []map[string]string{
{
"mime_type": "text/json",
"extension": "json",
"name": "JSON File",
"icon": "https://example.org/icons&file=json.png",
"default_app": "some Address",
},
"text/xml": {
{
"mime_type": "text/xml",
"extension": "xml",
"name": "XML File",
"icon": "https://example.org/icons&file=xml.png",
"default_app": "some Address",
},
"application/vnd.openxmlformats-officedocument.wordprocessingml.document": {
{
"mime_type": "application/vnd.openxmlformats-officedocument.wordprocessingml.document",
"extension": "doc",
"name": "Word File",
"icon": "https://example.org/icons&file=doc.png",
"default_app": "some Address",
},
"application/vnd.oasis.opendocument.presentation": {
{
"mime_type": "application/vnd.oasis.opendocument.presentation",
"extension": "odf",
"name": "OpenDocument File",
"icon": "https://example.org/icons&file=odf.png",
"default_app": "some Address",
},
"application/vnd.apple.installer+xml": {
{
"mime_type": "application/vnd.apple.installer+xml",
"extension": "mpkg",
"name": "Mpkg File",
"icon": "https://example.org/icons&file=mpkg.png",
"default_app": "some Address",
},
"image/bmp": {
{
"mime_type": "image/bmp",
"extension": "bmp",
"name": "Image File",
"icon": "https://example.org/icons&file=bmp.png",
Expand Down
Loading