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.

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

[WIP] Keep file ids when moving between storages #13385

Closed
wants to merge 2 commits into from

Conversation

PVince81
Copy link
Contributor

  • keep file id when moving file between storages
  • keep file id when moving folders between storages
  • test for possible side effects
  • unit tests

@icewind1991 note, this is hacky and I'm afraid the change to the DB "Cache" class could cause some other side effects

Tentative fix for #13310

@PVince81 PVince81 added this to the 8.1-next milestone Jan 15, 2015
@PVince81 PVince81 mentioned this pull request Jan 15, 2015
3 tasks
@icewind1991
Copy link
Contributor

I would rather have proper cross storage rename for the cache instead of fixing the remote-update approach since it would be cleaner and saves having to do a rescan of the target.

That said I'm not sure if there is a clean way to do it

@PVince81
Copy link
Contributor Author

So I guess it could be a follow up on your other PR #13360 but for the caches.

I already spent quite some time on the file id part so I don't think we'll have a solution for 8.0, so let's leave this for 8.1.

I'll keep this PR just in case.

@PVince81
Copy link
Contributor Author

For the record, the idea I had in mind (with the approach from this PR here) was to provide an alternative "renameToStorage()" method that would be called instead of "copy()" when moving folders.
The method "renamteToStorage()" would work in a similar manner, but instead of doing "copy all" then delete all at the end", it would do a "move every file one by one then adjust their cache entry + file id", using a similar approach like for single files.

@scrutinizer-notifier
Copy link

The inspection completed: 4 new issues, 4 updated code elements

@PVince81
Copy link
Contributor Author

@icewind1991 I looked at #13360 but still don't see how to fit in the file id change there.
Even with moveFromStorage in some cases (like external storages) the move will still be a stream copy, and yet we do need to keep the file id even if the move from one cache instance to another.

Did you have any particular idea how to solve this ?

Do we need to provide new cache methods called "moveFromStorage" so we can do $targetStorage->getCache()->moveFromStorage($sourceStorage) ?

Even if we do, the logic will probably not be too different than in this PR:

  1. get old fileid from source cache
  2. remove entry from source cache
  3. insert entry into target cache using the old file id
    regardless where this logic is, whether in the cache updater or in the cache implementation itself.

If we don't want to do the remove+insert, then we'll need specialized code that knows how to move a cache entry from one storage to another and update the parents (which is partially done by scanFile() + oldFileId in this PR).

The only advantage of having it in the cache implementation is probably to be able to detect whether cross-cache changes are done on the same implementations (similar to cross-storage moving from #13360)

@icewind1991
Copy link
Contributor

Do we need to provide new cache methods called "moveFromStorage" so we can do $targetStorage->getCache()->moveFromStorage($sourceStorage) ?

Something like that yes.

Even if we do, the logic will probably not be too different than in this PR:
[snip]
regardless where this logic is, whether in the cache updater or in the cache implementation itself.

We can do something like

UPDATE oc_filecache SET storage = $targetStorage->getId() AND path = $targetInternalPath

If we don't want to do the remove+insert, then we'll need specialized code that knows how to move a cache entry from one storage to another and update the parents (which is partially done by scanFile() + oldFileId in this PR).

The same propagator logic that we currently have should work correctly

@PVince81
Copy link
Contributor Author

It's a bit more tricky for the update: we also need to update the parent field and also the path_hash.

And even more tricky when moving folders: we need to do this for every file/folder inside it.

@PVince81
Copy link
Contributor Author

Hmm... and permissions might need updating too (which was done by scanFile()) .
So if we do that operation seperately, we need to do it in a way that reuses the logic from scanFile() somehow.

@PVince81
Copy link
Contributor Author

PVince81 commented Apr 9, 2015

Closing in favor of #15360

@PVince81 PVince81 closed this Apr 9, 2015
@PVince81 PVince81 deleted the files-keepfileidonmovebetweenstorages branch April 9, 2015 12:44
@lock lock bot locked as resolved and limited conversation to collaborators Aug 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants