Adding a stable way of adding whitelisted "orphaned" uploads in a plugin

There are some situations in which people want to have uploads kept in Discourse that are not strictly associated with existing Discourse models and properties that support uploads, i.e. “whitelisted” orphaned uploads.

For example some people want to use the Custom Wizard plugin to upload content associated with a topic (not a post per se).

The issue is that the clean_up_uploads job removes all orphaned uploads. It’s exceptions are limited to urls in specific site settings.

You could of course turn off clean_up_uploads, or increase the clean_orphan_uploads_grace_period_hours, which is what I’m sometimes forced to do. But this is not ideal.

Maybe there’s another way of “whitelisting” orphaned uploads from a plugin that I’m not seeing, but if not, it’d be great to add some mechanism of doing that into the upload cleanup process.

I’d be happy to prepare a PR along these lines, but I’m wondering if this is something others see as an issue.

8 Likes

This is so clunky

For reviewables @eviltrout introduced a general mechanism where arbitrary objects could be linked to a reviewable.

Moving to a similar system for uploads would be like moving multiple mountains. It would be a nice mountain to move though cause this current system is pretty fragile.

Easiest workaround you have for now is to create a hidden site setting with a data_type for :upload. Then put it in the site settings table.

5 Likes

Yeah, I was thinking this. I didn’t love it as a solution as I’d need to create different a setting for each upload, which feels like an abuse of the site_settings table.

As an interim solution, before the mountain is moved, would you mind if made a PR to add an extra exception to the big “exceptions” query that uses plugin_store_row with a plugin_name like whitelisted_orphan_upload_id?

3 Likes

This query is already SOOOO slow, I worry that another join here is going to make it very painful.

I am not sure but maybe we start by preparing the new table in core for UploadRefrence(object_id, object_type, upload_id) and joining into that, at least it is cheap and then we can move stuff to it. There are quite a few easy moves, only post uploads would be tricky…er.

I think this is work I would prefer the team here to take, it is very fiddly.

Curious to know what @zogstrip and @eviltrout think

4 Likes

Are you talking about Rails’ single table inheritance? It’s a really good fit for reviewables because there is core logic to a reviewable that they all share, but then extra fields.

In this case though, it seems like the problem is we’re putting upload ids all over the place, and not deleting the uploads when they’re cleared out?

Is it unsafe to delete an upload if a user removes it from their profile, because the same upload sha1 could be used elsewhere in the app? If that’s the case I think @sam’s suggestion of an UploadReference table is a good solution.

If uploads are never shared this way, I think a better solution would be to delete when those fields are NULLd out.

2 Likes

That’s exactly why we need the UploadReference table :+1:

I actually think that’s a very good task to learn more about Discourse internals. Might be a good task for @CvX, starting early October. We’ll do some pair programming to get things started and properly split into several “small” tasks.

9 Likes