Pluggable transfers for invenio-records-resources

[mesemus]

❤️ Thank you for your contribution!

Description

This pull request discusses changing the transfer types to be extensible with new transfers

Checklist

Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:

• I'm aware of the code of conduct.
• I've created logical separate commits and followed the commit message format.
• I've added relevant test cases.
• I've added relevant documentation.
• I've marked translation strings (for relevant code).
• I've followed the CSS/JS and React guidelines (for relevant code).
• I've followed the web accessibility guidelines (for relevant code).
• I've followed the user interface guidelines (for relevant code).
• I've identified the copyright holder(s) and updated copyright headers for touched files (>15 lines contributions).
• I've NOT included third-party code (copy/pasted source code or new dependencies).

Third-party code

If you've added third-party code (copy/pasted or new dependencies), please reach out to an architect.

Reminder

By using GitHub, you have already agreed to the GitHub’s Terms of Service including that:

1. You license your contribution under the same terms as the current repository’s license.
2. You agree that you have the right to license your contribution under the current repository’s license.

[slint]

Thanks a lot for the implementation and the nice write-up of the questions! I've answered most, but left some that might need a 3rd-party opinion. I browsed through the PRs as well, but I'd prefer to thoroughly review after seeing all the questions/comments here closed.

[slint]

👍 We should really make it clear that transfer_type is the canonical term, both in terms of user input in the Marshmallow schema, but also throughout the API and data model. I think that also means that we should store transfer_type in the DraftFileRecord.json or even a new DraftFileRecord.transfer_type column... My feeling is that storage_class was "overloaded"/multi-purposed to avoid having to store the transfer type in a new field (which is not an ideal solution given the semantics).

This rename is a backward-incompatible change, but given the low usage of the feature (and the fact that we'll probably go with a major version bump) I think it's reasonable to get it right now.

---

storage_class is something that the user shouldn't be able to control or know about (though IIRC there were discussions in the workshop about having a choice for it in the upload UI).

[lnielsen]

Original intention of storage class was to be able to record if e.g. a file was online/offline so you could use it to make decisions if it should be fetched, or check for integrity etc.....was never really used however.

[slint]

Agree on not exposing uri, and keeping links.content pointing to /api/records/{id}/files/{filename}/content to be able to keep track of usage statistics (cc @tmorrell I've updated the File Linking API discussion to take usage statistics into account).

Some things to keep in mind:

• For FETCH, the uri is stored to know the location from where to fetch the file, that will later be stored as a regular file and have a FileInstance.uri of a path "on disk".
• For REMOTE, the uri is actually passed all the way through and stored in the FileInstance.uri (see also the PoC PR for RemoteTransfer).

[mesemus]

+1, will hide it

[slint]

can_review is a bit convoluted since it also represents community review request curators who have edit rights to the unpublished draft (e.g., for fixing metadata, or possibly correcting file formats from XLS to CSV, etc.)

[slint]

As mentioned above, agree on hiding uri. There's still a question about if we should also be completely removing the uri after the fetch, since one might want to know the original provenance of the fetched file.

But I think this is out of scope for this RFC (unless we see it heavily affecting the implementation).

[slint]

Agree, the definition of "Locality" here is flawed/confusing and was probably labeled as such to be able to enforce via permissions the FETCH workflow, i.e. that while fetching the file, only the system (via the background Celery task) can set the contents of the draft file.

I think the more logical and explicit way to deal with permissions, would be to have some sort of "switch"-style permissions generator for each transfer type, e.g. something like:

can_draft_set_content_files = [
    IfTransferType({
        TransferType.LOCAL: can_review,
        TransferType.MULTIPART: [Disabled()],
        TransferType.FETCH: [SystemProcess()],
        TransferType.REMOTE: [Disabled()],
    })
]

I see some problems though:

• In OOP, having a switch on an enum is a bit of a code smell and hints towards actually moving the code implementation on each class...
  • On the other hand, permissions should be configurable by each instance, so if we moved the permissions logic to the BaseTransfer implementation for each transfer type, end-users wouldn't be able to customize them easily.
• When adding a new transfer type or modifying permissions for an existing type, one has to do a lot of work to update all the files-related permissions, without a good solution for reusing/extending existing values.

An elegant solution might be to have some sort of helper class that auto-generates and bundles permissions for each transfer type and action, and can be easily extended/overridden.

So to move forward, a possible path would be:

1. Go with the explicit IfTransferType generator proposed above, which solves the problem
2. See how it feels for someone implementing a new transfer type in the future
3. Re-evaluate if we need something fancier

[mesemus]

Done for now, see https://github.com/mesemus/invenio-rdm-records/pull/1/files#diff-d2ac50f4b70b7a64fda723336e09162ad3b8e627c41a3189ceaa47b04f7d2f53R160

[slint]

The semantics/naming of MultipartStorageExt look very much bound to the init/content/commit terminology, so they would look somewhat out of place in invenio-files-rest. Thus I'm leaning more towards keeping a "Protocol"-style coupling.

Another solution, might be to move the relevant invenio-s3 parts that implement the MultipartStorageExt interface in this module under "contrib" or similar and do some try/except around imports to do feature detection.

[mesemus]

Ok, let's keep the protocol style, I've made it more readable in the code.

[slint]

question: Invenio-Files-REST comes with some existing models for storing multipart-related info, like e.g. MultipartObject and Part. It also implements the REST API for working with this, but it's very much bound to the Angular.js client-side library we picked at the time. For Zenodo (and all Invenio v3-based services at CERN) we also never used this implementation, but because of performance issues for multipart on our storage cluster at CERN.

I see that you've used ObjectVersionTags for organizing the parts; was there a specific reason for not reusing the MultipartObject/Part classes? In any case, what matters is if it works, but I could also see it as an opportunity to consider purging all the multipart models from files-rest :)

[mesemus]

The primary reason for not using the MultipartObject was that the uploadId is uuid type, which is not the case for S3 - there it is a long string. From the S3 perspective, only the "MultipartObject" is relevant as parts are managed directly by calls to S3 and Invenio does not know about them at all.

The implementation of MultipartObject/Part also merges "model" and "service" level stuff - the service level is the "generic" implementation for multipart unaware backends, that I've got, for example, at https://github.com/mesemus/invenio-records-resources/pull/3/files#diff-ab7d7ef448eaae0f109622e167d9168d7f0ad56dbba383d6d681b9cc17b62bcfR184 - there is this "if contract, call storage, else generic impl" pattern.

So if the ObjectVersionTag solution is ok for you, I'd be happy to keep it like that.

Btw, having looked at expired upload task at https://github.com/inveniosoftware/invenio-files-rest/blob/master/invenio_files_rest/tasks.py#L272 - do you think that it is necessary to clean the uploads that have not been touched for a time? Just now I handle only delete - if a file is deleted, it's multipart upload (if pending) is aborted as well.

[mesemus]

I've just extracted the global code to previously unused interface (going the "protocol" way), imho looks better. https://github.com/mesemus/invenio-records-resources/blob/multipart-transfers/invenio_records_resources/services/files/transfer/providers/multipart.py