Refactor sharing #1685
Refactor sharing #1685
Conversation
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
C0rby
force-pushed
the
refactor-sharing
branch
5 times, most recently
from
a9cf4b6
to
eeede49
Compare
|
I don't quite get why fail... Also @ishank011 or @labkode, I don't have access to the cs3org in snyk so I can't see what test is failing there. Could you tell me what I need to fix? |
I think I found the issue. |
|
Listing a public link after renaming it fails for some reason.... |
|
sadly the logs of reva are cut off in drone |
|
I rebased on master here #1690 let's see |
In owncloud drone CI we put a flush of the file-system and a 30-second wait at the end of acceptance test runs. That improved the situation - pipeline step output seemed to get from the drone agent to drone server more often! I am not sure what happens currently in cs3org/reva CI. |
|
@individual-it, my refactoring is causing the issue. I just haven't found the cause yet... |
|
Arrgh I found it... When we are doing a stat request on a share like here we eventually end up here reva/pkg/storage/fs/owncloud/owncloud.go Line 746 in 28500a8 In that line we get the file path using the OpaqueId as the key. When we rename a file, that path will be wrong because it doesn't get updated on a file rename. Because of that the stat request fails with a not found error.
Back in the But if you look closely you see that it doesn't check the status code of the stat response but of the listPublicShare response. My refactoring changed that to checking the stat response and that's why it doesn't return a share after the shared file gets renamed... |
|
This is the underlying issue. #1693 Will work on it on monday |
|
Waiting for this fix to be merged: #1696 |
C0rby
force-pushed
the
refactor-sharing
branch
3 times, most recently
from
e7b817e
to
aa9780f
Compare
|
@ishank011 can you review this one? |
| } | ||
|
|
||
| share := getShareRes.Share | ||
| share := res.Share | ||
| if share == nil { | ||
| panic("gateway: error updating a received share: the share is nil") | ||
| } | ||
| createRefStatus, err := s.createReference(ctx, share.Share.ResourceId) |
There was a problem hiding this comment.
Can you add checks for createRefStatus and err?
| additionalInfoTemplate *template.Template | ||
| userIdentifierCache *ttlcache.Cache | ||
| resourceInfoCache gcache.Cache | ||
| resourceInfoCacheTTL time.Duration | ||
| useResourceInfoCache bool |
There was a problem hiding this comment.
Should we use it in the user-configurable struct? If a user sets useResourceInfoCache to true but doesn't set resourceInfoCacheTTL assuming a default value, we won't use the cache. I'd suggest removing this and checking the value of resourceInfoCacheTTL in all places
There was a problem hiding this comment.
Okay, no it's not user configurable. But I'd still vote for removing the extra variable. Doesn't really add any value
There was a problem hiding this comment.
It does add a bit of readability but I have no hard opinion on this, I can remove it again.
Also the responses to accepting or rejecting a share return the updated share now.