LemmyNet / lemmy

🐀 A link aggregator and forum for the fediverse
https://join-lemmy.org
GNU Affero General Public License v3.0
12.95k stars 859 forks source link

Rework image tables to add foreign key references #4714

Open dessalines opened 1 month ago

dessalines commented 1 month ago

Requirements

Is your proposal related to a problem?

Currently there are 3 image tables: remote_image, local_image, and image_details . None of these tables share SQL references, and each of them isolated.

Additionally, the post_view.thumbnail column (and comment text) uses a 3rd link, either identical to a remote image link, a proxied one, or a local one built from the local_image pictrs alias.

Describe the solution you'd like.

We should create an image table, which stores the link alone, and possibly a local column.

The local_image table shouldn't use a pictrs alias, but the actual local url. It can then be foreign-key referenced to the image table.

The image_details table can then also be foreign-key referenced to the image_table.


I recommend to do this, we should:

Describe alternatives you've considered.

NA

Additional context

4704

cc @Nutomic @dulla

Nutomic commented 1 month ago

This will probably get a bit awkward when deleting or purging an image, because that requires the alias. So then we need to store the alias in a separate, nullable column or extract it manually from the url. Btw we could also put the data from image_details directly into nullable columns in image, instead of a separate table.

Anyway its best if you can make these changes as part of #4704 so that we can include them in 0.19.4. Otherwise there will be unnecessary breaking changes.

cc @dullbananas @asonix

asonix commented 1 month ago

Making it harder to access an alias probably isn't great, since the alias the the identifier that pict-rs cares about. If you need to include URLs for indexing reasons that's fine, but make sure the alias is trivially available when needed

dullbananas commented 1 month ago

link should instead be called thumbnail_link where appropiate.

Nutomic commented 1 month ago

We could store the url, as well as alias for local images. Then if alias is not null, that would indicate its a local image (so no need for separate local column).

And on second thought, lets leave this for 0.19.5, not good to make major changes right before the release.

dessalines commented 1 month ago

We could store the url, as well as alias for local images. Then if alias is not null, that would indicate its a local image (so no need for separate local column).

My recommendation above is to keep the local_image table, since it has some important extra columns too, like the pictrs_delete_token. I can keep its existing pictrs_alias column tho. I'd prefer keeping it separate rather than adding a bunch of nullable columns.

To clarify, the local_image table will have : {link, pictrs_alias, pictrs_delete_token}, with the new primary key link being to the new image table.

I'll put my current PR in draft till I can get that done.

dessalines commented 1 month ago

And on second thought, lets leave this for 0.19.5, not good to make major changes right before the release.

That's fine, but I'll need to extract the proxy fixes into its own PR.