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

Skip unnecessary share retrieval #4454

Merged
merged 5 commits into from
Jan 18, 2024
Merged

Skip unnecessary share retrieval #4454

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7af5547
Skip unnecessary share retrieval
butonic Jan 9, 2024
9ae6877
drop getReceivedShareHideFlagFromShareID
butonic Jan 10, 2024
8c58b38
pull error handling up to public method
butonic Jan 10, 2024
b52ae1f
drop error from CS3Share2ShareData response
butonic Jan 10, 2024
5a60236
fix updating other received shares
butonic Jan 10, 2024
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
3 changes: 3 additions & 0 deletions changelog/unreleased/skip-unnecessary-received-share-retrieval.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Bugfix: Skip unnecessary share retrieval

https://github.com/cs3org/reva/pull/4454
109 changes: 47 additions & 62 deletions internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/pending.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,11 @@ func (h *Handler) AcceptReceivedShare(w http.ResponseWriter, r *http.Request) {
return
}

sharedResource, ocsResponse := getSharedResource(ctx, client, rs.Share.Share.ResourceId)
rs.State = collaboration.ShareState_SHARE_STATE_ACCEPTED

// now we also need to determine the mount point by evaluating all other mounted shares

sharedResource, ocsResponse := getSharedResource(ctx, client, rs.Share.ResourceId)
if ocsResponse != nil {
response.WriteOCSResponse(w, r, *ocsResponse, nil)
return
Expand All @@ -82,20 +86,19 @@ func (h *Handler) AcceptReceivedShare(w http.ResponseWriter, r *http.Request) {
}

// we need to sort the received shares by mount point in order to make things easier to evaluate.
base := path.Base(sharedResource.GetInfo().GetPath())
base := path.Base(sharedResource.GetInfo().GetPath()) // TODO should use name
mount := base
var mountedShares []*collaboration.ReceivedShare
sharesToAccept := map[string]bool{shareID: true}
for _, s := range lrs.Shares {
if utils.ResourceIDEqual(s.Share.ResourceId, rs.Share.Share.GetResourceId()) {
if utils.ResourceIDEqual(s.Share.ResourceId, rs.Share.GetResourceId()) {
if s.State == collaboration.ShareState_SHARE_STATE_ACCEPTED {
mount = s.MountPoint.Path
} else {
sharesToAccept[s.Share.Id.OpaqueId] = true
}
} else {
if s.State == collaboration.ShareState_SHARE_STATE_ACCEPTED {
s.Hidden = h.getReceivedShareHideFlagFromShareID(r.Context(), shareID)
butonic marked this conversation as resolved.
Show resolved Hide resolved
mountedShares = append(mountedShares, s)
}
}
Expand Down Expand Up @@ -130,24 +133,28 @@ func (h *Handler) AcceptReceivedShare(w http.ResponseWriter, r *http.Request) {
// TODO we could delete shares here if the stat returns code NOT FOUND ... but listening for file deletes would be better
}
}

// we need to add a path to the share
receivedShare := &collaboration.ReceivedShare{
Share: &collaboration.Share{
Id: &collaboration.ShareId{OpaqueId: shareID},
},
State: collaboration.ShareState_SHARE_STATE_ACCEPTED,
Hidden: h.getReceivedShareHideFlagFromShareID(r.Context(), shareID),
MountPoint: &provider.Reference{
Path: mount,
},
rs.MountPoint = &provider.Reference{
Path: mount,
}
updateMask := &fieldmaskpb.FieldMask{Paths: []string{"state", "hidden", "mount_point"}}
updateMask := &fieldmaskpb.FieldMask{Paths: []string{"state", "mount_point"}}

for id := range sharesToAccept {
data := h.updateReceivedShare(w, r, receivedShare, updateMask)
// only render the data for the changed share
if id == shareID && data != nil {
response.WriteOCSSuccess(w, r, []*conversions.ShareData{data})
data, meta, err := h.updateReceivedShare(r.Context(), rs, updateMask)
if err != nil {
// we log an error for affected shares, for the actual share we return an error
if id == shareID {
response.WriteOCSData(w, r, meta, nil, err)
continue
} else {
appctx.GetLogger(ctx).Error().Err(err).Str("received_share", shareID).Str("affected_share", id).Msg("could not update affected received share")
}
} else {
// only render the data for the changed share
if id == shareID {
response.WriteOCSSuccess(w, r, []*conversions.ShareData{data})
}
}
}
}
Expand All @@ -166,15 +173,15 @@ func (h *Handler) RejectReceivedShare(w http.ResponseWriter, r *http.Request) {
Share: &collaboration.Share{
Id: &collaboration.ShareId{OpaqueId: shareID},
},
State: collaboration.ShareState_SHARE_STATE_REJECTED,
Hidden: h.getReceivedShareHideFlagFromShareID(r.Context(), shareID),
State: collaboration.ShareState_SHARE_STATE_REJECTED,
}
updateMask := &fieldmaskpb.FieldMask{Paths: []string{"state", "hidden"}}
updateMask := &fieldmaskpb.FieldMask{Paths: []string{"state"}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we need to make sure the share managers respect the field mask!

Copy link
Contributor Author

@butonic butonic Jan 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

jsonsc3 handles it, memory handles it, owncloudsql handles it (but does not know anything about the hidden flag), json handles it (but does not know anything about the hidden flag), cs3 also has it but does not know anything about the hidden flag). good to merge


data := h.updateReceivedShare(w, r, receivedShare, updateMask)
if data != nil {
response.WriteOCSSuccess(w, r, []*conversions.ShareData{data})
data, meta, err := h.updateReceivedShare(r.Context(), receivedShare, updateMask)
if err != nil {
response.WriteOCSData(w, r, meta, nil, err)
}
response.WriteOCSSuccess(w, r, []*conversions.ShareData{data})
}

func (h *Handler) UpdateReceivedShare(w http.ResponseWriter, r *http.Request) {
Expand All @@ -199,18 +206,17 @@ func (h *Handler) UpdateReceivedShare(w http.ResponseWriter, r *http.Request) {

rs, _ := getReceivedShareFromID(r.Context(), client, shareID)
if rs != nil && rs.Share != nil {
receivedShare.State = rs.Share.State
receivedShare.State = rs.State
}

data := h.updateReceivedShare(w, r, receivedShare, updateMask)
if data != nil {
response.WriteOCSSuccess(w, r, []*conversions.ShareData{data})
data, meta, err := h.updateReceivedShare(r.Context(), receivedShare, updateMask)
if err != nil {
response.WriteOCSData(w, r, meta, nil, err)
}
// TODO: do we need error handling here?
response.WriteOCSSuccess(w, r, []*conversions.ShareData{data})
}

func (h *Handler) updateReceivedShare(w http.ResponseWriter, r *http.Request, receivedShare *collaboration.ReceivedShare, fieldMask *fieldmaskpb.FieldMask) *conversions.ShareData {
ctx := r.Context()
func (h *Handler) updateReceivedShare(ctx context.Context, receivedShare *collaboration.ReceivedShare, fieldMask *fieldmaskpb.FieldMask) (*conversions.ShareData, response.Meta, error) {
logger := appctx.GetLogger(ctx)

updateShareRequest := &collaboration.UpdateReceivedShareRequest{
Expand All @@ -220,51 +226,43 @@ func (h *Handler) updateReceivedShare(w http.ResponseWriter, r *http.Request, re

client, err := h.getClient()
if err != nil {
response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error getting grpc gateway client", err)
return nil
return nil, response.MetaServerError, errors.Wrap(err, "error getting grpc gateway client")
}

shareRes, err := client.UpdateReceivedShare(ctx, updateShareRequest)
if err != nil {
response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "grpc update received share request failed", err)
return nil
return nil, response.MetaServerError, errors.Wrap(err, "grpc update received share request failed")
}

if shareRes.Status.Code != rpc.Code_CODE_OK {
if shareRes.Status.Code == rpc.Code_CODE_NOT_FOUND {
response.WriteOCSError(w, r, response.MetaNotFound.StatusCode, "not found", nil)
return nil
return nil, response.MetaNotFound, errors.New(shareRes.Status.Message)
}
response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "grpc update received share request failed", errors.Errorf("code: %d, message: %s", shareRes.Status.Code, shareRes.Status.Message))
return nil
return nil, response.MetaServerError, errors.Errorf("grpc update received share request failed: code: %d, message: %s", shareRes.Status.Code, shareRes.Status.Message)
}

rs := shareRes.GetShare()

info, status, err := h.getResourceInfoByID(ctx, client, rs.Share.ResourceId)
if err != nil || status.Code != rpc.Code_CODE_OK {
h.logProblems(logger, status, err, "could not stat, skipping")
response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "grpc get resource info failed", errors.Errorf("code: %d, message: %s", status.Code, status.Message))
return nil
return nil, response.MetaServerError, errors.Errorf("grpc get resource info failed: code: %d, message: %s", status.Code, status.Message)
}

data, err := conversions.CS3Share2ShareData(r.Context(), rs.Share)
if err != nil {
logger.Debug().Interface("share", rs.Share).Interface("shareData", data).Err(err).Msg("could not CS3Share2ShareData, skipping")
}
data := conversions.CS3Share2ShareData(ctx, rs.Share)

data.State = mapState(rs.GetState())
data.Hidden = rs.GetHidden()

h.addFileInfo(ctx, data, info)
h.mapUserIds(r.Context(), client, data)
h.mapUserIds(ctx, client, data)

if data.State == ocsStateAccepted {
// Needed because received shares can be jailed in a folder in the users home
data.Path = path.Join(h.sharePrefix, path.Base(info.Path))
}

return data
return data, response.MetaOK, nil
}

func (h *Handler) updateReceivedFederatedShare(w http.ResponseWriter, r *http.Request, shareID string, rejectShare bool) {
Expand Down Expand Up @@ -337,21 +335,8 @@ func (h *Handler) updateReceivedFederatedShare(w http.ResponseWriter, r *http.Re
response.WriteOCSSuccess(w, r, []*conversions.ShareData{data})
}

// getReceivedShareHideFlagFromShareId returns the hide flag of a received share based on its ID.
func (h *Handler) getReceivedShareHideFlagFromShareID(ctx context.Context, shareID string) bool {
client, err := h.getClient()
if err != nil {
return false
}
rs, _ := getReceivedShareFromID(ctx, client, shareID)
if rs != nil {
return rs.GetShare().GetHidden()
}
return false
}

// getReceivedShareFromID uses a client to the gateway to fetch a share based on its ID.
func getReceivedShareFromID(ctx context.Context, client gateway.GatewayAPIClient, shareID string) (*collaboration.GetReceivedShareResponse, *response.Response) {
func getReceivedShareFromID(ctx context.Context, client gateway.GatewayAPIClient, shareID string) (*collaboration.ReceivedShare, *response.Response) {
s, err := client.GetReceivedShare(ctx, &collaboration.GetReceivedShareRequest{
Ref: &collaboration.ShareReference{
Spec: &collaboration.ShareReference_Id{
Expand All @@ -376,7 +361,7 @@ func getReceivedShareFromID(ctx context.Context, client gateway.GatewayAPIClient
return nil, arbitraryOcsResponse(response.MetaBadRequest.StatusCode, e.Error())
}

return s, nil
return s.Share, nil
}

// getSharedResource attempts to get a shared resource from the storage from the resource reference.
Expand Down
30 changes: 5 additions & 25 deletions internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,11 +303,7 @@ func (h *Handler) CreateShare(w http.ResponseWriter, r *http.Request) {
return
}

s, err := conversions.CS3Share2ShareData(ctx, share)
if err != nil {
response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error mapping share data", err)
return
}
s := conversions.CS3Share2ShareData(ctx, share)

h.addFileInfo(ctx, s, statRes.Info)

Expand Down Expand Up @@ -595,12 +591,8 @@ func (h *Handler) GetShare(w http.ResponseWriter, r *http.Request) {
})
if err == nil && uRes.GetShare() != nil {
resourceID = uRes.Share.Share.ResourceId
share, err = conversions.CS3Share2ShareData(ctx, uRes.Share.Share)
share = conversions.CS3Share2ShareData(ctx, uRes.Share.Share)
share.Hidden = uRes.Share.Hidden
if err != nil {
response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error mapping share data", err)
return
}
}
}

Expand Down Expand Up @@ -635,11 +627,7 @@ func (h *Handler) GetShare(w http.ResponseWriter, r *http.Request) {

if err == nil && uRes.GetShare() != nil {
resourceID = uRes.Share.ResourceId
share, err = conversions.CS3Share2ShareData(ctx, uRes.Share)
if err != nil {
response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error mapping share data", err)
return
}
share = conversions.CS3Share2ShareData(ctx, uRes.Share)
}
}

Expand Down Expand Up @@ -874,11 +862,7 @@ func (h *Handler) updateShare(w http.ResponseWriter, r *http.Request, share *col
h.statCache.RemoveStat(currentUser.Id, share.ResourceId)
}

resultshare, err := conversions.CS3Share2ShareData(ctx, uRes.Share)
if err != nil {
response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error mapping share data", err)
return
}
resultshare := conversions.CS3Share2ShareData(ctx, uRes.Share)

statReq := provider.StatRequest{Ref: &provider.Reference{
ResourceId: uRes.Share.ResourceId,
Expand Down Expand Up @@ -1072,11 +1056,7 @@ func (h *Handler) listSharesWithMe(w http.ResponseWriter, r *http.Request) {
}
}

data, err := conversions.CS3Share2ShareData(r.Context(), rs.Share)
if err != nil {
sublog.Debug().Interface("share", rs.Share).Interface("shareData", data).Err(err).Msg("could not CS3Share2ShareData, skipping")
continue
}
data := conversions.CS3Share2ShareData(r.Context(), rs.Share)

data.State = mapState(rs.GetState())
data.Hidden = rs.Hidden
Expand Down
12 changes: 2 additions & 10 deletions internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/user.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,11 +281,7 @@ func (h *Handler) removeUserShare(w http.ResponseWriter, r *http.Request, share
},
}

data, err := conversions.CS3Share2ShareData(ctx, share)
if err != nil {
response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "deleting share failed", err)
return
}
data := conversions.CS3Share2ShareData(ctx, share)
// A deleted share should not have an ID.
data.ID = ""

Expand Down Expand Up @@ -337,11 +333,7 @@ func (h *Handler) listUserShares(r *http.Request, filters []*collaboration.Filte

// build OCS response payload
for _, s := range lsUserSharesResponse.Shares {
data, err := conversions.CS3Share2ShareData(ctx, s)
if err != nil {
log.Debug().Interface("share", s).Interface("shareData", data).Err(err).Msg("could not CS3Share2ShareData, skipping")
continue
}
data := conversions.CS3Share2ShareData(ctx, s)

info, status, err := h.getResourceInfoByID(ctx, client, s.ResourceId)
if err != nil || status.Code != rpc.Code_CODE_OK {
Expand Down
4 changes: 2 additions & 2 deletions pkg/conversions/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ type MatchValueData struct {
}

// CS3Share2ShareData converts a cs3api user share into shareData data model
func CS3Share2ShareData(ctx context.Context, share *collaboration.Share) (*ShareData, error) {
func CS3Share2ShareData(ctx context.Context, share *collaboration.Share) *ShareData {
sd := &ShareData{
// share.permissions are mapped below
// Displaynames are added later
Expand Down Expand Up @@ -269,7 +269,7 @@ func CS3Share2ShareData(ctx context.Context, share *collaboration.Share) (*Share
sd.Expiration = expiration.Format(_iso8601)
}

return sd, nil
return sd
}

// PublicShare2ShareData converts a cs3api public share into shareData data model
Expand Down
Loading