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

Fixed a problem with file names when moving from external storage to nextcloud (with ObjectStore S3 enabled) #43660

Closed

Conversation

hopleus
Copy link
Contributor

@hopleus hopleus commented Feb 19, 2024

Summary

Fixed a problem with file names when moving from external storage to nextcloud (with ObjectStore S3 enabled)

Use case

The file is copied from the external storage to the local one (extDisk: test.docx => localDisk: test.docx.part)
An entry is created about a new file (test.docx.part) in the database, the oc_filecache table. The new file contains a file id equal to the one that is saved as the filename in S3
Renames the file in the local storage from test.docx.part to test.docx
The information in the database is updated (new path, new storage ID) to record the source file (the one that we move from the external storage)
, the record in the database about the new (temporary) file (test.docx.part) is deleted.
Result: The old file id remains in the database, but with new information (path, name, storage). And S3 stores a file with a temporary ID that has not been changed after all the manipulations.

Checklist

…nextcloud (with ObjectStore S3 enabled)

Signed-off-by: hopleus <hopleus@gmail.com>
@hopleus
Copy link
Contributor Author

hopleus commented Feb 19, 2024

/backport stable28

@hopleus
Copy link
Contributor Author

hopleus commented Feb 19, 2024

/backport stable27

@hopleus
Copy link
Contributor Author

hopleus commented Feb 19, 2024

/backport stable26

@hopleus
Copy link
Contributor Author

hopleus commented Feb 29, 2024

@joshtrichards Hi. Could you run the tests again?

P.S. Last time cypress crashed with 503 errors and I didn't understand why

Copy link
Member

@icewind1991 icewind1991 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the correct approach to solve the issue, it ends up creating a cache entry for the .part file that isn't used, it also impacts copies, where it will write to the wrong urn. And the method of passing the fileid to writeStream is very fragile to other changes.

The best I can think of is having ObjectStoreStorage overwrite moveFromStorage where it can then write the files to the correct urn without making new cache entries for them and instead updating the existing source cache entries to move them to the new storage.

@icewind1991
Copy link
Member

Great job with figuring out the root cause of the issue though!

@hopleus
Copy link
Contributor Author

hopleus commented Mar 19, 2024

I don't think this is the correct approach to solve the issue, it ends up creating a cache entry for the .part file that isn't used, it also impacts copies, where it will write to the wrong urn. And the method of passing the fileid to writeStream is very fragile to other changes.

The best I can think of is having ObjectStoreStorage overwrite moveFromStorage where it can then write the files to the correct urn without making new cache entries for them and instead updating the existing source cache entries to move them to the new storage.

I've changed the logic, check when the opportunity arises

@susnux susnux added this to the Nextcloud 30 milestone Apr 18, 2024
@icewind1991
Copy link
Member

I can confirm that this solves the problem but the extra copy added will cause issues.

I've created an alternative approach that forgoes the default copyFromStorage logic to ensure it writes directly to the correct urn: #46013

Please verify that it resolves the issue.

This was referenced Jul 30, 2024
This was referenced Aug 5, 2024
@skjnldsv skjnldsv mentioned this pull request Aug 13, 2024
@skjnldsv skjnldsv modified the milestones: Nextcloud 30, Nextcloud 31 Aug 14, 2024
Copy link
Member

@juliushaertl juliushaertl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the alternative from @icewind1991 looks a bit safer, could you double check if that also resolves your issue?

@kesselb
Copy link
Contributor

kesselb commented Aug 26, 2024

Thank you very much @hopleus 🙏

The alternative approach was just merged.

Thanks again for bringing it to our attention and your thorough analysis of the root cause.

@kesselb kesselb closed this Aug 26, 2024
@AndyScherzinger AndyScherzinger removed this from the Nextcloud 31 milestone Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants