-
Notifications
You must be signed in to change notification settings - Fork 26
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.
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
use a relative reference for the stat request in the cs3 api #74
Conversation
That's interesting, is that a feature or a bug? ;-) Jokes apart, if it is intended that the full path be exposed (including in public links and single-file shares), one might not need the special |
A feature, but I didn't knew about it before, because I intuitively always used references with all three (storage-id, opaque-id and path=".")
You'll never see paths outside of your permissions. Therefore the could be different for you and me. In a single file public share you should see something like |
OK, good to know about this feature indeed. But with that case, you really need the other patch such that
in all cases. Correct? |
I'm just thinking that we could use that form everywhere and that's it, i.e. dropping the first case from |
You'll only see
As far as I know, EOS doesn't yet support relative references!? |
Yes, except that once the PR is merged, also Reva master will (eventually) be exposed to those relative paths (assuming they are implemented in EOS - which is expected to happen, otherwise how are we going to support spaces?). What I mean is: at the moment the code path in this PR is used by Reva master and edge on the very first |
True... We could add a flag for the reference types: [cs3]
cs3api_reference_type = absolute # "absolute" for REVA master, "relative" for REVA edge |
I think we can get along in a better way than by a configuration flag... These days I'm off but for next week I propose the following, given that we have added the
|
a56a45e
to
5beda27
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That does not look really dangerous to me ;-)
Just to keep this up to date: we have not merged this yet because (as I suspected) we do have an issue in the eosfs storage provider when calling Stat() with a given |
dbcbcb7
to
535ae76
Compare
Please check owncloud/ocis#3693 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Interestingly, I'm not able to contribute to the branch, therefore I post here a patch I would include as well - which relates to owncloud/ocis#3693 indeed. Short summary is that yes it "looks good", but with the explicit caveats in the following comments:
|
@wkloucek can you integrate the patch? |
Co-authored-by: Giuseppe Lo Presti <[email protected]> Co-authored-by: Willy Kloucek <[email protected]>
@glpatcern thanks for the patch! I applied it and did some quick testing. Looks good from my side |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good, indeed it ought not to change anything semantically, and we're adapting the eosfs.go
storage provider to support such paths as well.
this is about this code path: https://github.com/cs3org/reva/blob/208ecee6d113e2b323c80e366a92eedb776200f9/pkg/storage/utils/decomposedfs/node/node.go#L512-L520
If we omit the
path='.'
we'll get a stat response containing the absolute path of the file (not the filename only)