-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Optimise getFileInfo - for mounts, dont fetch again filecache, but reuse sourceRootInfo #27403
Conversation
mrow4a
commented
Mar 16, 2017
•
edited
Loading
edited
- Optimization
- Unit tests
…use sourceRootInfo
@mrow4a, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @PVince81 and @tanghus to be potential reviewers. |
Ok, I see it wont be that easy looking on unit tests |
a3a4551
to
1e1095f
Compare
@@ -261,7 +261,7 @@ public function getChildren() { | |||
// the caller believe that the collection itself does not exist | |||
throw new Forbidden('No read permissions'); | |||
} | |||
$folderContent = $this->fileView->getDirectoryContent($this->path); | |||
$folderContent = $this->fileView->getDirectoryContent($this->path, '', true); |
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.
''
doesn't seems to be supported as possible value
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.
This is default value for this function
namespace OCA\Files_Sharing; | ||
|
||
|
||
interface IShareCache { |
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.
I'm not sure about adding this new interface.
First, the name seems wrong. There is already a cache interface available, so when I implement this IShareCache
I expect to implement the methods for a cache (get, set, remove, etc). Right now we only have a getMetadata
method which doesn't fit being a cache by itself.
Second, the getMetadata
method usage seems bound to the cache. Does it make sense to implement an isolated getMetadata
method? I don't think so.
I think the cleanest option is to create a new interface (ICacheMetadata
or something similar) in the OCP namespace that inherit from the public ICache
interface (I assume there is such interface) and includes the new getMetadata
method. The problem with this solution is that we'll likely need to rewrite these caches.
Another option could be forget this interface and just add the new method.
@@ -1365,7 +1365,11 @@ public function getFileInfo($path, $includeMountPoints = true) { | |||
continue; | |||
} | |||
$subCache = $subStorage->getCache(''); | |||
$rootEntry = $subCache->get(''); | |||
if ($parentReadOnly && $subCache instanceof \OCA\Files_Sharing\IShareCache) { |
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.
🚫 core shouldn't have any knowledge about specific apps.
$subCache = $subStorage->getCache(''); | ||
if ($subCache instanceof \OCA\Files_Sharing\IShareCache) { |
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.
Same as above
@jvillafanez This PR is purely experimental, I do not try to revolutionize anything (since the target is for OC10, not OC11), but optimize using existing structure. Problem there is that https://github.com/owncloud/core/blob/master/lib/private/Files/View.php#L1431 The other problem is that https://github.com/owncloud/core/blob/master/lib/private/Files/View.php#L1433 is generaly a free ride, this just extends Cache. The check you mentioned The flag for this PR is Developing, but any feedback is generally most needed, thanks! |
* @return array | ||
*/ | ||
public function getMetadata() { | ||
return $this->formatCacheEntry($this->sourceRootInfo); |
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.
not sure what you're trying to achieve but something that comes to mind is getSourceCache()->get('')
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |