From 1b91d9828b820bd86a381ed3ab7d269958190ca0 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Mon, 21 Oct 2024 10:47:32 +0200 Subject: [PATCH] Revert "Adding an image_details table to store image dimensions. (#4704)" This reverts commit 6d8d23130d4b70d52437cce7152d39ab3e37d7eb. --- api_tests/.eslintrc.json | 42 ++++++++++ api_tests/eslint.config.mjs | 56 ------------- api_tests/package.json | 4 +- api_tests/pnpm-lock.yaml | 10 +-- api_tests/prepare-drone-federation-test.sh | 2 +- api_tests/src/image.spec.ts | 4 - api_tests/src/post.spec.ts | 2 +- crates/api_common/src/request.rs | 58 +------------- crates/api_common/src/utils.rs | 32 +------- crates/db_schema/src/impls/images.rs | 78 ++++--------------- crates/db_schema/src/schema.rs | 13 +--- crates/db_schema/src/source/images.rs | 33 ++------ crates/routes/src/images.rs | 8 +- .../down.sql | 7 -- .../up.sql | 15 ---- 15 files changed, 82 insertions(+), 282 deletions(-) create mode 100644 api_tests/.eslintrc.json delete mode 100644 api_tests/eslint.config.mjs delete mode 100644 migrations/2024-05-05-162540_add_image_detail_table/down.sql delete mode 100644 migrations/2024-05-05-162540_add_image_detail_table/up.sql diff --git a/api_tests/.eslintrc.json b/api_tests/.eslintrc.json new file mode 100644 index 000000000..75b1706aa --- /dev/null +++ b/api_tests/.eslintrc.json @@ -0,0 +1,42 @@ +{ + "root": true, + "env": { + "browser": true + }, + "plugins": ["@typescript-eslint"], + "extends": ["eslint:recommended", "plugin:@typescript-eslint/recommended"], + "parser": "@typescript-eslint/parser", + "parserOptions": { + "project": "./tsconfig.json", + "warnOnUnsupportedTypeScriptVersion": false + }, + "rules": { + "@typescript-eslint/ban-ts-comment": 0, + "@typescript-eslint/no-explicit-any": 0, + "@typescript-eslint/explicit-module-boundary-types": 0, + "@typescript-eslint/no-var-requires": 0, + "arrow-body-style": 0, + "curly": 0, + "eol-last": 0, + "eqeqeq": 0, + "func-style": 0, + "import/no-duplicates": 0, + "max-statements": 0, + "max-params": 0, + "new-cap": 0, + "no-console": 0, + "no-duplicate-imports": 0, + "no-extra-parens": 0, + "no-return-assign": 0, + "no-throw-literal": 0, + "no-trailing-spaces": 0, + "no-unused-expressions": 0, + "no-useless-constructor": 0, + "no-useless-escape": 0, + "no-var": 0, + "prefer-const": 0, + "prefer-rest-params": 0, + "quote-props": 0, + "unicorn/filename-case": 0 + } +} diff --git a/api_tests/eslint.config.mjs b/api_tests/eslint.config.mjs deleted file mode 100644 index cf2c426d0..000000000 --- a/api_tests/eslint.config.mjs +++ /dev/null @@ -1,56 +0,0 @@ -import pluginJs from "@eslint/js"; -import tseslint from "typescript-eslint"; - -export default [ - pluginJs.configs.recommended, - ...tseslint.configs.recommended, - { - languageOptions: { - parser: tseslint.parser, - }, - }, - // For some reason this has to be in its own block - { - ignores: [ - "putTypesInIndex.js", - "dist/*", - "docs/*", - ".yalc", - "jest.config.js", - ], - }, - { - files: ["src/**/*"], - rules: { - "@typescript-eslint/no-empty-interface": 0, - "@typescript-eslint/no-empty-function": 0, - "@typescript-eslint/ban-ts-comment": 0, - "@typescript-eslint/no-explicit-any": 0, - "@typescript-eslint/explicit-module-boundary-types": 0, - "@typescript-eslint/no-var-requires": 0, - "arrow-body-style": 0, - curly: 0, - "eol-last": 0, - eqeqeq: 0, - "func-style": 0, - "import/no-duplicates": 0, - "max-statements": 0, - "max-params": 0, - "new-cap": 0, - "no-console": 0, - "no-duplicate-imports": 0, - "no-extra-parens": 0, - "no-return-assign": 0, - "no-throw-literal": 0, - "no-trailing-spaces": 0, - "no-unused-expressions": 0, - "no-useless-constructor": 0, - "no-useless-escape": 0, - "no-var": 0, - "prefer-const": 0, - "prefer-rest-params": 0, - "quote-props": 0, - "unicorn/filename-case": 0, - }, - }, -]; diff --git a/api_tests/package.json b/api_tests/package.json index bc2d3ec2d..6af067797 100644 --- a/api_tests/package.json +++ b/api_tests/package.json @@ -8,7 +8,7 @@ "license": "AGPL-3.0", "packageManager": "pnpm@9.9.0", "scripts": { - "lint": "tsc --noEmit && eslint --report-unused-disable-directives && prettier --check 'src/**/*.ts'", + "lint": "tsc --noEmit && eslint --report-unused-disable-directives --ext .js,.ts,.tsx src && prettier --check 'src/**/*.ts'", "fix": "prettier --write src && eslint --fix src", "api-test": "jest -i follow.spec.ts && jest -i image.spec.ts && jest -i user.spec.ts && jest -i private_message.spec.ts && jest -i community.spec.ts && jest -i post.spec.ts && jest -i comment.spec.ts ", "api-test-follow": "jest -i follow.spec.ts", @@ -27,7 +27,7 @@ "eslint": "^9.8.0", "eslint-plugin-prettier": "^5.1.3", "jest": "^29.5.0", - "lemmy-js-client": "0.19.5-alpha.1", + "lemmy-js-client": "0.19.4", "prettier": "^3.2.5", "ts-jest": "^29.1.0", "typescript": "^5.5.4", diff --git a/api_tests/pnpm-lock.yaml b/api_tests/pnpm-lock.yaml index 3b1970681..d2a2c630c 100644 --- a/api_tests/pnpm-lock.yaml +++ b/api_tests/pnpm-lock.yaml @@ -30,8 +30,8 @@ importers: specifier: ^29.5.0 version: 29.7.0(@types/node@22.5.1) lemmy-js-client: - specifier: 0.19.5-alpha.1 - version: 0.19.5-alpha.1 + specifier: 0.19.4 + version: 0.19.4 prettier: specifier: ^3.2.5 version: 3.3.3 @@ -1179,8 +1179,8 @@ packages: resolution: {integrity: sha512-eTIzlVOSUR+JxdDFepEYcBMtZ9Qqdef+rnzWdRZuMbOywu5tO2w2N7rqjoANZ5k9vywhL6Br1VRjUIgTQx4E8w==} engines: {node: '>=6'} - lemmy-js-client@0.19.5-alpha.1: - resolution: {integrity: sha512-GOhaiTQzrpwdmc3DFYemT2SmNmpuQJe2BWUms9QOzdYlkA1WZ0uu7axPE3s+T5OOxfy7K9Q2gsLe72dcVSlffw==} + lemmy-js-client@0.19.4: + resolution: {integrity: sha512-k3d+YRDj3+JuuEP+nuEg27efR/e4m8oMk2BoC8jq9AnMrwSAKfsN2F2vG70Zke0amXtOclDZrCSHkIpNw99ikg==} leven@3.1.0: resolution: {integrity: sha512-qsda+H8jTaUaN/x5vzW2rzc+8Rw4TAQ/4KjB46IwK5VH+IlVeeeje/EoZRpiXvIqjFgK84QffqPztGI3VBLG1A==} @@ -3108,7 +3108,7 @@ snapshots: kleur@3.0.3: {} - lemmy-js-client@0.19.5-alpha.1: {} + lemmy-js-client@0.19.4: {} leven@3.1.0: {} diff --git a/api_tests/prepare-drone-federation-test.sh b/api_tests/prepare-drone-federation-test.sh index 65c4827d9..31eb111c2 100755 --- a/api_tests/prepare-drone-federation-test.sh +++ b/api_tests/prepare-drone-federation-test.sh @@ -15,7 +15,7 @@ export LEMMY_TEST_FAST_FEDERATION=1 # by default, the persistent federation queu # pictrs setup if [ ! -f "api_tests/pict-rs" ]; then - curl "https://git.asonix.dog/asonix/pict-rs/releases/download/v0.5.16/pict-rs-linux-amd64" -o api_tests/pict-rs + curl "https://git.asonix.dog/asonix/pict-rs/releases/download/v0.5.13/pict-rs-linux-amd64" -o api_tests/pict-rs chmod +x api_tests/pict-rs fi ./api_tests/pict-rs \ diff --git a/api_tests/src/image.spec.ts b/api_tests/src/image.spec.ts index ed96451a2..56226256b 100644 --- a/api_tests/src/image.spec.ts +++ b/api_tests/src/image.spec.ts @@ -164,7 +164,6 @@ test("Purge post, linked image removed", async () => { upload.url, ); expect(post.post_view.post.url).toBe(upload.url); - expect(post.post_view.image_details).toBeDefined(); // purge post const purgeForm: PurgePost = { @@ -190,9 +189,6 @@ test("Images in remote image post are proxied if setting enabled", async () => { const post = postRes.post_view.post; expect(post).toBeDefined(); - // Make sure it fetched the image details - expect(postRes.post_view.image_details).toBeDefined(); - // remote image gets proxied after upload expect( post.thumbnail_url?.startsWith( diff --git a/api_tests/src/post.spec.ts b/api_tests/src/post.spec.ts index 6b5c8d812..75df80775 100644 --- a/api_tests/src/post.spec.ts +++ b/api_tests/src/post.spec.ts @@ -512,7 +512,7 @@ test("Enforce site ban federation for local user", async () => { } let newAlphaUserJwt = await loginUser(alpha, alphaUserPerson.name); alphaUserHttp.setHeaders({ - Authorization: "Bearer " + newAlphaUserJwt.jwt, + Authorization: "Bearer " + newAlphaUserJwt.jwt ?? "", }); // alpha makes new post in beta community, it federates let postRes2 = await createPost(alphaUserHttp, betaCommunity!.community.id); diff --git a/crates/api_common/src/request.rs b/crates/api_common/src/request.rs index de6ba4f39..15b690bfc 100644 --- a/crates/api_common/src/request.rs +++ b/crates/api_common/src/request.rs @@ -12,7 +12,7 @@ use futures::StreamExt; use lemmy_db_schema::{ newtypes::DbUrl, source::{ - images::{ImageDetailsForm, LocalImage, LocalImageForm}, + images::{LocalImage, LocalImageForm}, local_site::LocalSite, post::{Post, PostUpdateForm}, }, @@ -263,19 +263,6 @@ pub struct PictrsFileDetails { pub created_at: DateTime, } -impl PictrsFileDetails { - /// Builds the image form. This should always use the thumbnail_url, - /// Because the post_view joins to it - pub fn build_image_details_form(&self, thumbnail_url: &Url) -> ImageDetailsForm { - ImageDetailsForm { - link: thumbnail_url.clone().into(), - width: self.width.into(), - height: self.height.into(), - content_type: self.content_type.clone(), - } - } -} - #[derive(Deserialize, Serialize, Debug)] struct PictrsPurgeResponse { msg: String, @@ -383,52 +370,11 @@ async fn generate_pictrs_thumbnail(image_url: &Url, context: &LemmyContext) -> L let protocol_and_hostname = context.settings().get_protocol_and_hostname(); let thumbnail_url = image.thumbnail_url(&protocol_and_hostname)?; - // Also store the details for the image - let details_form = image.details.build_image_details_form(&thumbnail_url); - LocalImage::create(&mut context.pool(), &form, &details_form).await?; + LocalImage::create(&mut context.pool(), &form).await?; Ok(thumbnail_url) } -/// Fetches the image details for pictrs proxied images -/// -/// We don't need to check for image mode, as that's already been done -#[tracing::instrument(skip_all)] -pub async fn fetch_pictrs_proxied_image_details( - image_url: &Url, - context: &LemmyContext, -) -> LemmyResult { - let pictrs_url = context.settings().pictrs_config()?.url; - let encoded_image_url = encode(image_url.as_str()); - - // Pictrs needs you to fetch the proxied image before you can fetch the details - let proxy_url = format!("{pictrs_url}image/original?proxy={encoded_image_url}"); - - let res = context - .client() - .get(&proxy_url) - .timeout(REQWEST_TIMEOUT) - .send() - .await? - .status(); - if !res.is_success() { - Err(LemmyErrorType::NotAnImageType)? - } - - let details_url = format!("{pictrs_url}image/details/original?proxy={encoded_image_url}"); - - let res = context - .client() - .get(&details_url) - .timeout(REQWEST_TIMEOUT) - .send() - .await? - .json() - .await?; - - Ok(res) -} - // TODO: get rid of this by reading content type from db #[tracing::instrument(skip_all)] async fn is_image_content_type(client: &ClientWithMiddleware, url: &Url) -> LemmyResult<()> { diff --git a/crates/api_common/src/utils.rs b/crates/api_common/src/utils.rs index 64cd41135..5cfc397e1 100644 --- a/crates/api_common/src/utils.rs +++ b/crates/api_common/src/utils.rs @@ -1,10 +1,6 @@ use crate::{ context::LemmyContext, - request::{ - delete_image_from_pictrs, - fetch_pictrs_proxied_image_details, - purge_image_from_pictrs, - }, + request::{delete_image_from_pictrs, purge_image_from_pictrs}, site::{FederatedInstances, InstanceWithFederationState}, }; use chrono::{DateTime, Days, Local, TimeZone, Utc}; @@ -954,18 +950,7 @@ pub async fn process_markdown( if context.settings().pictrs_config()?.image_mode() == PictrsImageMode::ProxyAllImages { let (text, links) = markdown_rewrite_image_links(text); - - // Create images and image detail rows - for link in links { - // Insert image details for the remote image - let details_res = fetch_pictrs_proxied_image_details(&link, context).await; - if let Ok(details) = details_res { - let proxied = - build_proxied_image_url(&link, &context.settings().get_protocol_and_hostname())?; - let details_form = details.build_image_details_form(&proxied); - RemoteImage::create(&mut context.pool(), &details_form).await?; - } - } + RemoteImage::create(&mut context.pool(), links).await?; Ok(text) } else { Ok(text) @@ -1000,14 +985,8 @@ async fn proxy_image_link_internal( Ok(link.into()) } else if image_mode == PictrsImageMode::ProxyAllImages { let proxied = build_proxied_image_url(&link, &context.settings().get_protocol_and_hostname())?; - // This should fail softly, since pictrs might not even be running - let details_res = fetch_pictrs_proxied_image_details(&link, context).await; - - if let Ok(details) = details_res { - let details_form = details.build_image_details_form(&proxied); - RemoteImage::create(&mut context.pool(), &details_form).await?; - }; + RemoteImage::create(&mut context.pool(), vec![link]).await?; Ok(proxied.into()) } else { Ok(link.into()) @@ -1145,13 +1124,10 @@ mod tests { "https://lemmy-alpha/api/v3/image_proxy?url=http%3A%2F%2Flemmy-beta%2Fimage.png", proxied.as_str() ); - - // This fails, because the details can't be fetched without pictrs running, - // And a remote image won't be inserted. assert!( RemoteImage::validate(&mut context.pool(), remote_image.into()) .await - .is_err() + .is_ok() ); } } diff --git a/crates/db_schema/src/impls/images.rs b/crates/db_schema/src/impls/images.rs index 547bfc4e2..9589aeee3 100644 --- a/crates/db_schema/src/impls/images.rs +++ b/crates/db_schema/src/impls/images.rs @@ -1,14 +1,7 @@ use crate::{ newtypes::DbUrl, - schema::{image_details, local_image, remote_image}, - source::images::{ - ImageDetails, - ImageDetailsForm, - LocalImage, - LocalImageForm, - RemoteImage, - RemoteImageForm, - }, + schema::{local_image, remote_image}, + source::images::{LocalImage, LocalImageForm, RemoteImage, RemoteImageForm}, utils::{get_conn, DbPool}, }; use diesel::{ @@ -20,29 +13,15 @@ use diesel::{ NotFound, QueryDsl, }; -use diesel_async::{AsyncPgConnection, RunQueryDsl}; +use diesel_async::RunQueryDsl; +use url::Url; impl LocalImage { - pub async fn create( - pool: &mut DbPool<'_>, - form: &LocalImageForm, - image_details_form: &ImageDetailsForm, - ) -> Result { + pub async fn create(pool: &mut DbPool<'_>, form: &LocalImageForm) -> Result { let conn = &mut get_conn(pool).await?; - conn - .build_transaction() - .run(|conn| { - Box::pin(async move { - let local_insert = insert_into(local_image::table) - .values(form) - .get_result::(conn) - .await; - - ImageDetails::create(conn, image_details_form).await?; - - local_insert - }) as _ - }) + insert_into(local_image::table) + .values(form) + .get_result::(conn) .await } @@ -60,26 +39,16 @@ impl LocalImage { } impl RemoteImage { - pub async fn create(pool: &mut DbPool<'_>, form: &ImageDetailsForm) -> Result { + pub async fn create(pool: &mut DbPool<'_>, links: Vec) -> Result { let conn = &mut get_conn(pool).await?; - conn - .build_transaction() - .run(|conn| { - Box::pin(async move { - let remote_image_form = RemoteImageForm { - link: form.link.clone(), - }; - let remote_insert = insert_into(remote_image::table) - .values(remote_image_form) - .on_conflict_do_nothing() - .execute(conn) - .await; - - ImageDetails::create(conn, form).await?; - - remote_insert - }) as _ - }) + let forms = links + .into_iter() + .map(|url| RemoteImageForm { link: url.into() }) + .collect::>(); + insert_into(remote_image::table) + .values(forms) + .on_conflict_do_nothing() + .execute(conn) .await } @@ -98,16 +67,3 @@ impl RemoteImage { } } } - -impl ImageDetails { - pub(crate) async fn create( - conn: &mut AsyncPgConnection, - form: &ImageDetailsForm, - ) -> Result { - insert_into(image_details::table) - .values(form) - .on_conflict_do_nothing() - .execute(conn) - .await - } -} diff --git a/crates/db_schema/src/schema.rs b/crates/db_schema/src/schema.rs index c3102b578..50bdac751 100644 --- a/crates/db_schema/src/schema.rs +++ b/crates/db_schema/src/schema.rs @@ -309,15 +309,6 @@ diesel::table! { } } -diesel::table! { - image_details (link) { - link -> Text, - width -> Int4, - height -> Int4, - content_type -> Text, - } -} - diesel::table! { instance (id) { id -> Int4, @@ -858,7 +849,8 @@ diesel::table! { } diesel::table! { - remote_image (link) { + remote_image (id) { + id -> Int4, link -> Text, published -> Timestamptz, } @@ -1063,7 +1055,6 @@ diesel::allow_tables_to_appear_in_same_query!( federation_allowlist, federation_blocklist, federation_queue_state, - image_details, instance, instance_block, language, diff --git a/crates/db_schema/src/source/images.rs b/crates/db_schema/src/source/images.rs index 0dea4b84f..9d48e011b 100644 --- a/crates/db_schema/src/source/images.rs +++ b/crates/db_schema/src/source/images.rs @@ -1,12 +1,13 @@ use crate::newtypes::{DbUrl, LocalUserId}; #[cfg(feature = "full")] -use crate::schema::{image_details, local_image, remote_image}; +use crate::schema::{local_image, remote_image}; use chrono::{DateTime, Utc}; use serde::{Deserialize, Serialize}; use serde_with::skip_serializing_none; use std::fmt::Debug; #[cfg(feature = "full")] use ts_rs::TS; +use typed_builder::TypedBuilder; #[skip_serializing_none] #[derive(Clone, PartialEq, Eq, Debug, Serialize, Deserialize)] @@ -29,7 +30,7 @@ pub struct LocalImage { pub published: DateTime, } -#[derive(Debug, Clone)] +#[derive(Debug, Clone, TypedBuilder)] #[cfg_attr(feature = "full", derive(Insertable, AsChangeset))] #[cfg_attr(feature = "full", diesel(table_name = local_image))] pub struct LocalImageForm { @@ -45,39 +46,15 @@ pub struct LocalImageForm { #[cfg_attr(feature = "full", derive(Queryable, Selectable, Identifiable))] #[cfg_attr(feature = "full", diesel(table_name = remote_image))] #[cfg_attr(feature = "full", diesel(check_for_backend(diesel::pg::Pg)))] -#[cfg_attr(feature = "full", diesel(primary_key(link)))] pub struct RemoteImage { + pub id: i32, pub link: DbUrl, pub published: DateTime, } -#[derive(Debug, Clone)] +#[derive(Debug, Clone, TypedBuilder)] #[cfg_attr(feature = "full", derive(Insertable, AsChangeset))] #[cfg_attr(feature = "full", diesel(table_name = remote_image))] pub struct RemoteImageForm { pub link: DbUrl, } - -#[skip_serializing_none] -#[derive(Clone, PartialEq, Eq, Debug, Serialize, Deserialize)] -#[cfg_attr(feature = "full", derive(Queryable, Selectable, Identifiable, TS))] -#[cfg_attr(feature = "full", ts(export))] -#[cfg_attr(feature = "full", diesel(table_name = image_details))] -#[cfg_attr(feature = "full", diesel(check_for_backend(diesel::pg::Pg)))] -#[cfg_attr(feature = "full", diesel(primary_key(link)))] -pub struct ImageDetails { - pub link: DbUrl, - pub width: i32, - pub height: i32, - pub content_type: String, -} - -#[derive(Debug, Clone)] -#[cfg_attr(feature = "full", derive(Insertable, AsChangeset))] -#[cfg_attr(feature = "full", diesel(table_name = image_details))] -pub struct ImageDetailsForm { - pub link: DbUrl, - pub width: i32, - pub height: i32, - pub content_type: String, -} diff --git a/crates/routes/src/images.rs b/crates/routes/src/images.rs index 307d0ab9f..166b9b12a 100644 --- a/crates/routes/src/images.rs +++ b/crates/routes/src/images.rs @@ -156,13 +156,7 @@ async fn upload( pictrs_alias: image.file.to_string(), pictrs_delete_token: image.delete_token.to_string(), }; - - let protocol_and_hostname = context.settings().get_protocol_and_hostname(); - let thumbnail_url = image.thumbnail_url(&protocol_and_hostname)?; - - // Also store the details for the image - let details_form = image.details.build_image_details_form(&thumbnail_url); - LocalImage::create(&mut context.pool(), &form, &details_form).await?; + LocalImage::create(&mut context.pool(), &form).await?; } } diff --git a/migrations/2024-05-05-162540_add_image_detail_table/down.sql b/migrations/2024-05-05-162540_add_image_detail_table/down.sql deleted file mode 100644 index 819277c7f..000000000 --- a/migrations/2024-05-05-162540_add_image_detail_table/down.sql +++ /dev/null @@ -1,7 +0,0 @@ -ALTER TABLE remote_image - ADD UNIQUE (link), - DROP CONSTRAINT remote_image_pkey, - ADD COLUMN id serial PRIMARY KEY; - -DROP TABLE image_details; - diff --git a/migrations/2024-05-05-162540_add_image_detail_table/up.sql b/migrations/2024-05-05-162540_add_image_detail_table/up.sql deleted file mode 100644 index 9b2ed9658..000000000 --- a/migrations/2024-05-05-162540_add_image_detail_table/up.sql +++ /dev/null @@ -1,15 +0,0 @@ --- Drop the id column from the remote_image table, just use link -ALTER TABLE remote_image - DROP COLUMN id, - ADD PRIMARY KEY (link), - DROP CONSTRAINT remote_image_link_key; - --- No good way to do references here unfortunately, unless we combine the images tables --- The link should be the URL, not the pictrs_alias, to allow joining from post.thumbnail_url -CREATE TABLE image_details ( - link text PRIMARY KEY, - width integer NOT NULL, - height integer NOT NULL, - content_type text NOT NULL -); -