core icon indicating copy to clipboard operation
core copied to clipboard

[WIP] Implement persistent major version workflow

Open JammingBen opened this issue 3 years ago • 2 comments

Description

Related Issue

  • https://github.com/owncloud/enterprise/issues/5286

Remaining To-Dos

  • [x] Clean Up
  • [x] Improve layout for current
  • [x] Adjust restore to create new version
  • [x] Bind functionally completely to the file_storage.save_version_author flag
  • [ ] Rename file_storage.save_version_author flag
  • [x] Fix expiry error logs
  • [ ] changelog

Types of changes

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [x] New feature (non-breaking change which adds functionality)
  • [ ] Database schema changes (next release will require increase of minor version instead of patch)
  • [ ] Breaking change (fix or feature that would cause existing functionality to change)
  • [ ] Technical debt
  • [ ] Tests only (no source changes)

JammingBen avatar Aug 26 '22 13:08 JammingBen

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

update-docs[bot] avatar Aug 26 '22 13:08 update-docs[bot]

:boom: Acceptance tests pipeline apiVersions-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/36722/88

ownclouders avatar Aug 31 '22 10:08 ownclouders

@JammingBen @hodyroff FYI, I unfortunetely would need to rollback the cleanup you did for "current version" handling. There are 2 main reasons:

  • I investigated and backwards compatibility with existing meta-files layout is very complicated
  • In your implementation, when file is created you automatically create additional version for it, doubling the file creation latency and storage.

I think I would take this work into separate PR and start it from scratch, using your code as a base for it. Otherwise it would take too long to rollback the changes you did as part of this breaking cleanup that you did.

mrow4a avatar Dec 02 '22 15:12 mrow4a

Sure, just take what you need 🙂 The PR is mainly a "proof of concept" - things like performance or backwards-compatibility were not taken into account here.

JammingBen avatar Dec 02 '22 16:12 JammingBen

@JammingBen I now completely follow on your decision to create a version file on each create or update of the file, additionally to just moving old file to versioned file..

Basically the way current files_version app works, is completely not compatible with the requirements of this FR (actually also with the requirements for previous PR with "editor of the version"). Version App was designed for non-conncurrent versions of the file, which were exposed via DAV Collection, not for handling current version of file (logic from files app). The logic added recently with "filename.current.json" feels really like a hack, looking at how MetaVersionCollection over DAV is implemented.

In fact, to make this proper we would need to refactor completely entire files_version app to cover both concurrent version of the file (one in user/files folder) and non-concurrent (one in user/file_versions folder). I agree this would not happen.

I now try to use DAV PROPFIND in order to return both metadata of meta versions root ("filename.current.json") and metadata of version collection ("filename.vX.json"), using PROPFIND with different depths. This approach seems the only one to fit into existing DAV protocol for versions, and that does not lead to both backwards compatibility and performance problems desribed in https://github.com/owncloud/core/pull/40319#issuecomment-1335435700 .

I will send/update this PR next days with the new approach using DAV.. otherwise if I fail we need to rollback to your approach, and deprecate the behaviour with "filename.current.json" (but it will be pretty ugly because we will loose metadata for 1 of the versions and customers may complain).. will be hard to justify it as a bug, because it would be intentional regression..

mrow4a avatar Dec 05 '22 07:12 mrow4a

@JammingBen just FYI

I found another bug, that increment algo is wrong. By just adding 0.1 you can have only 9 versions and then it goes to 1.0. It needs split into major and minor, and increment there.

Also found few bugs with restore, because when restoring we actally would mess-up counting. It should keep new version number and have a additional info that it was restored from past version number, otherwise it is super confusing in display.

mrow4a avatar Dec 09 '22 17:12 mrow4a

closing this PR in favour of https://github.com/owncloud/core/pull/40531 that rearchitects this feature for performance, backwards compatibility and correct handling of restore functionality

mrow4a avatar Dec 11 '22 21:12 mrow4a