Improve json marshalling of share protobuf messages (#1655)

.github/config.yml

@@ -9,6 +9,9 @@ updateDocsWhiteList:
   - tests-only
   - Tests-Only
   - Build-deps
+  - docs-only
+  - Docs-only
+  - Docs-Only
 
 updateDocsTargetFiles:
   - changelog/unreleased/

changelog/unreleased/improve-share-json-marshal.md

@@ -0,0 +1,9 @@
+Enhancement: Improve json marshalling of share protobuf messages
+
+Protobuf oneof fields cannot be properly handled by the native json marshaller,
+and the protojson package can only handle proto messages. Previously, we were
+using a workaround of storing these oneof fields separately, which made the code
+inelegant. Now we marshal these messages as strings before marshalling them via
+the native json package.
+
+https://github.com/cs3org/reva/pull/1655

examples/ocmd/ocmd-server-1.toml

@@ -1,16 +1,16 @@
 [shared]
 gatewaysvc = "localhost:19000"
 
-[registry]
-driver = "static"
-
-[registry.static]
-services = ["authprovider","userprovider"]
-
-[registry.static.authprovider]
-bearer = ["localhost:0123"]
-basic = ["localhost:1234"]
-publiclink = ["localhost:9876"]
+# [registry]
+# driver = "static"
+#
+# [registry.static]
+# services = ["authprovider","userprovider"]
+#
+# [registry.static.authprovider]
+# bearer = ["localhost:0123"]
+# basic = ["localhost:1234"]
+# publiclink = ["localhost:9876"]
 
 [grpc]
 address = "0.0.0.0:19000"

internal/grpc/services/gateway/gateway.go

@@ -82,9 +82,6 @@ func (c *config) init() {
 
 	// if services address are not specified we used the shared conf
 	// for the gatewaysvc to have dev setups very quickly.
-
-	// we're commenting this line to showcase the fact that now we don't want to point to an ip address but rather
-	// resolve an ip address from a name.
 	c.AuthRegistryEndpoint = sharedconf.GetGatewaySVC(c.AuthRegistryEndpoint)
 	c.StorageRegistryEndpoint = sharedconf.GetGatewaySVC(c.StorageRegistryEndpoint)
 	c.AppRegistryEndpoint = sharedconf.GetGatewaySVC(c.AppRegistryEndpoint)

pkg/cbox/user/rest/rest.go

@@ -123,7 +123,7 @@ func New(m map[string]interface{}) (user.Manager, error) {
 
 func (m *manager) getUserByParam(ctx context.Context, param, val string) (map[string]interface{}, error) {
 	url := fmt.Sprintf("%s/Identity?filter=%s:%s&field=upn&field=primaryAccountEmail&field=displayName&field=uid&field=gid&field=type",
-		m.conf.APIBaseURL, param, val)
+		m.conf.APIBaseURL, param, url.QueryEscape(val))
 	responseData, err := m.apiTokenManager.SendAPIGetRequest(ctx, url, false)
 	if err != nil {
 		return nil, err
@@ -225,7 +225,6 @@ func (m *manager) GetUser(ctx context.Context, uid *userpb.UserId) (*userpb.User
 }
 
 func (m *manager) GetUserByClaim(ctx context.Context, claim, value string) (*userpb.User, error) {
-	value = url.QueryEscape(value)
 	opaqueID, err := m.fetchCachedParam(claim, value)
 	if err == nil {
 		return m.GetUser(ctx, &userpb.UserId{OpaqueId: opaqueID})

pkg/share/manager/json/json.go

@@ -26,8 +26,6 @@ import (
 	"sync"
 	"time"
 
-	grouppb "github.com/cs3org/go-cs3apis/cs3/identity/group/v1beta1"
-	userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1"
 	collaboration "github.com/cs3org/go-cs3apis/cs3/sharing/collaboration/v1beta1"
 	provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
 	typespb "github.com/cs3org/go-cs3apis/cs3/types/v1beta1"
@@ -91,23 +89,20 @@ func loadOrCreate(file string) (*shareModel, error) {
 		return nil, err
 	}
 
-	m := &shareModel{}
-	if err := json.Unmarshal(data, m); err != nil {
-		err = errors.Wrap(err, "error decoding data to json")
+	j := &jsonEncoding{}
+	if err := json.Unmarshal(data, j); err != nil {
+		err = errors.Wrap(err, "error decoding data from json")
 		return nil, err
 	}
 
-	// There are discrepancies in the marshalling of oneof fields, so these need
-	// to be stored separately
-	for i := range m.Grantees {
-		id := m.Grantees[i].(map[string]interface{})
-		if m.Shares[i].Grantee.Type == provider.GranteeType_GRANTEE_TYPE_USER {
-			m.Shares[i].Grantee.Id = &provider.Grantee_UserId{UserId: &userpb.UserId{OpaqueId: id["opaque_id"].(string), Idp: id["idp"].(string)}}
-		} else if m.Shares[i].Grantee.Type == provider.GranteeType_GRANTEE_TYPE_GROUP {
-			m.Shares[i].Grantee.Id = &provider.Grantee_GroupId{GroupId: &grouppb.GroupId{OpaqueId: id["opaque_id"].(string), Idp: id["idp"].(string)}}
+	m := &shareModel{State: j.State}
+	for _, s := range j.Shares {
+		var decShare collaboration.Share
+		if err = utils.UnmarshalJSONToProtoV1([]byte(s), &decShare); err != nil {
+			return nil, errors.Wrap(err, "error decoding share from json")
 		}
+		m.Shares = append(m.Shares, &decShare)
 	}
-	m.Grantees = nil
 
 	if m.State == nil {
 		m.State = map[string]map[string]collaboration.ShareState{}
@@ -118,28 +113,27 @@ func loadOrCreate(file string) (*shareModel, error) {
 }
 
 type shareModel struct {
-	file     string
-	State    map[string]map[string]collaboration.ShareState `json:"state"` // map[username]map[share_id]ShareState
-	Shares   []*collaboration.Share                         `json:"shares"`
-	Grantees []interface{}                                  `json:"grantees"`
+	file   string
+	State  map[string]map[string]collaboration.ShareState `json:"state"` // map[username]map[share_id]ShareState
+	Shares []*collaboration.Share                         `json:"shares"`
+}
+
+type jsonEncoding struct {
+	State  map[string]map[string]collaboration.ShareState `json:"state"` // map[username]map[share_id]ShareState
+	Shares []string                                       `json:"shares"`
 }
 
 func (m *shareModel) Save() error {
-	temp := *m
-	temp.Grantees = []interface{}{}
-	temp.Shares = []*collaboration.Share{}
-	for i := range m.Shares {
-		s := *m.Shares[i]
-		if s.Grantee.Type == provider.GranteeType_GRANTEE_TYPE_USER {
-			temp.Grantees = append(temp.Grantees, s.Grantee.GetUserId())
-		} else if s.Grantee.Type == provider.GranteeType_GRANTEE_TYPE_GROUP {
-			temp.Grantees = append(temp.Grantees, s.Grantee.GetGroupId())
+	j := &jsonEncoding{State: m.State}
+	for _, s := range m.Shares {
+		encShare, err := utils.MarshalProtoV1ToJSON(s)
+		if err != nil {
+			return errors.Wrap(err, "error encoding to json")
 		}
-		s.Grantee = &provider.Grantee{Type: s.Grantee.Type}
-		temp.Shares = append(temp.Shares, &s)
+		j.Shares = append(j.Shares, string(encShare))
 	}
 
-	data, err := json.Marshal(temp)
+	data, err := json.Marshal(j)
 	if err != nil {
 		err = errors.Wrap(err, "error encoding to json")
 		return err

tools/check-changelog/main.go

@@ -32,7 +32,7 @@ import (
 )
 
 // Case-insensitive list of PRs for which changelog enforcement needs to be skipped
-var skipTags = []string{"[tests-only]", "[build-deps]"}
+var skipTags = []string{"[tests-only]", "[build-deps]", "[docs-only]"}
 
 func skipPR(prID int) bool {
 	ctx := context.Background()