Add getOwner to FileInfo #20224
Add getOwner to FileInfo #20224
Conversation
1c040e5 to
81a60c7
Compare
| @@ -1248,7 +1248,8 @@ public function getFileInfo($path, $includeMountPoints = true) { | |||
| $data['permissions'] |= \OCP\Constants::PERMISSION_DELETE; | |||
| } | |||
|
|
|||
| return new FileInfo($path, $storage, $internalPath, $data, $mount); | |||
| $owner = \OC::$server->getUserManager()->get($storage->getOwner($internalPath)); | |||
There was a problem hiding this comment.
I'm a bit worried that it will become expensive to resolve all users. Imagine a folder where each subfolder is a shared mount. I'd rather we still keep it as the user id and let the API consumer decide whether to retrieve the user object for it or not.
There was a problem hiding this comment.
(I do agree that in an object-oriented view it makes sense, but we don't want additional performance impact, especially considering that the user manager might be LDAP)
There was a problem hiding this comment.
I believe we already resolve all users anyway and the usermanager will cache them
There was a problem hiding this comment.
@blizzz can you confirm this belief ?
There was a problem hiding this comment.
Not so much into this part of the code, so I dived a bit into it. As LDAP user I shared a file to a local one, and logged in with the local on, while having a debug session. What I already find surprising is that while $mount is a Shared_Mount the storage it returns is a Files_Trashbin/Storage. Should it be like this? The $internalPath will be empty, $data however is set correctly and the owner is correct as well.
Anyway, the LDAP owner is already cached at this point of time.
There was a problem hiding this comment.
@blizzz what you observed is the storage wrappers. Basically Wrapper A wraps Wrapper B wraps Wrapper C wraps Storage, where each wrapper can be "trashbin wrapper", "encryption wrapper", etc.
The part I'd like to double check is whether the user object gets resolved for every new user that is queried.
Basically if I call getUserManager()->get('user1') then same with user2, then user3, then user4, whether it will make 4 different queries or whether these 4 users were already cached somewhere.
Also the second part to check: if I am receiving 4 shares from each of these users respectively, whether these users have already been pre-resolved through SharedStorage. If they have, then calling getUserManager()->get() should be fine. However if the users are not pre-resolved, then this will cause an additional needless performance impact: because not everyone might need the $owner from FileInfo, so we should always resolve it.
@icewind1991 can you help clarifying this or do you want me to dive into the code to check it ?
There was a problem hiding this comment.
@icewind1991 show me a blackfire diff before/after this PR 😄
81a60c7 to
20cad09
Compare
|
Added tests and set default value to |
|
👍 |
|
@MorrisJobke @rullzer please review |
|
👍 |
Expose file owner in fileinfo