WIP: Refactor upload, get url from modules #504

Draft

ilja wants to merge 4 commits from ilja/akkoma:refactor_upload_get_url_from_modules into develop

ilja commented

2023-03-12 09:16:57 +00:00

Contributor

To get the url and base url, we had a function in upload.ex. This included a case to check on what module is used. This means that adding a new upload module implies changing code besides just adding a new module (I noticed this in a Pleroma PR https://git.pleroma.social/pleroma/pleroma/-/merge_requests/3654).

While trying to fix this, I decided to fix up some other parts of the upload flow. This doesn't add new functionality, but should make for a more consistent flow, and make it easier to add new uploaders in the future.

Because this touches several parts of the upload flow, and because there are several options, it's best to properly test this in production in different configurations. Some things I can think of:

Local vs S3
MediaProxy enabled vs disabled
MediaProxy deny/whitelisting (own media-domain should be allowed by default)
Downloading backup data ./lib/pleroma/web/pleroma_api/views/backup_view.ex: Pleroma.Upload.base_url() <> "/backups/" <> file_name
...

There are some things I keep out-of-scope:

I think we should stick to one representation of the upload once we're past the client-API. But we already have different representation in how it's stored in the database vs how we send it as AP. I can try to get as close as possible to an ideal situation, but don't want to do DB migrations at this point.
Media is stored in the objects table. For me, that table is for objects that show on the TL's directly and at least can be fetched over AP. Uploads aren't that. Storing it in the Objects table doesn't make sense to me, neither from a SQL nor from a NoSQL perspective. However, changing that would imply DB migrations, which I don't want to do rn.
This is the upload through the POST /api/v1/media endpoint. GET /api/v1/media/:id, PUT /api/v1/media/:id, and the flow when fetching objects who contain media as eg attachements are all out of scope.

To get the url and base url, we had a function in upload.ex. This included a `case` to check on what module is used. This means that adding a new upload module implies changing code besides just adding a new module (I noticed this in a Pleroma PR https://git.pleroma.social/pleroma/pleroma/-/merge_requests/3654). While trying to fix this, I decided to fix up some other parts of the upload flow. This doesn't add new functionality, but should make for a more consistent flow, and make it easier to add new uploaders in the future. Because this touches several parts of the upload flow, and because there are several options, it's best to properly test this in production in different configurations. Some things I can think of: * `Local` vs `S3` * MediaProxy enabled vs disabled * MediaProxy deny/whitelisting (own media-domain should be allowed by default) * Downloading backup data `./lib/pleroma/web/pleroma_api/views/backup_view.ex: Pleroma.Upload.base_url() <> "/backups/" <> file_name` * ... There are some things I keep out-of-scope: * I think we should stick to one representation of the upload once we're past the client-API. But we already have different representation in how it's stored in the database vs how we send it as AP. I can try to get as close as possible to an ideal situation, but don't want to do DB migrations at this point. * Media is stored in the objects table. For me, that table is for objects that show on the TL's directly and at least can be fetched over AP. Uploads aren't that. Storing it in the Objects table doesn't make sense to me, neither from a SQL nor from a NoSQL perspective. However, changing that would imply DB migrations, which I don't want to do rn. * This is the upload through the `POST /api/v1/media` endpoint. `GET /api/v1/media/:id`, `PUT /api/v1/media/:id`, and the flow when fetching objects who contain media as eg attachements are all out of scope.

ilja added 1 commit 2023-03-12 09:16:58 +00:00

ci/woodpecker/pr/woodpecker Pipeline is pending Details

053276ef6c Refactor upload, get url from modules

To get the url and base url, we had a function in upload.ex.
This included a `case` to check on what module is used.
This means that adding a new upload module implies changing code besides just adding a new module.

Here we try to get the logic properly into the respective modules.
This doesn't add new functionality, but should make it easier to add new uploaders in the future.

ilja added 1 commit 2023-03-19 11:03:39 +00:00

ci/woodpecker/pr/woodpecker Pipeline is pending Details

5db2e998c5 Make getting the uploader constistent

It was possible to get the uploader from opts.
But during the upload flow, a function was called who didn't use opts and just got it from settings.
This is inconsistent.

In practice, providing the uploader through opts was never used, so I removed it and now we always get it from settings.

ilja added 1 commit 2023-03-26 13:39:01 +00:00

ci/woodpecker/pr/woodpecker Pipeline is pending Details

2edb0944ba Return the complete `upload` from Pleroma.Uploaders.put_file/2

To move more logic to the Uploader modules, it's better to keep the same
`upload` and manipulate that while going through the upload pipeline, so
that's how it's now done.

Since we expect the `upload`, I removed the option for returning just `:ok`
from the uploaders.

It was possible to bypass the logic in the `get_url` function, but that was
unused. Since it makes even less sense when returning `upload`, I removed that
In a later stage I want to return the url from the uploader module, which
should obsolete this function.

I saw a warning from `Pleroma.Upload.Filter.MogrifyTest` which I now fixed.

ilja added 1 commit 2023-03-27 07:28:37 +00:00

ci/woodpecker/pr/woodpecker Pipeline is pending Details

9d8ab46cd6 Move building of url to modules

We got a path and there's a "base_url" function. With that the url was
build. But not all url's are nessecarely build like this. Building the
url should be the responsability of the Uploader itself, so it's now
moved to there.