Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pictrs delete token #5317

Merged
merged 59 commits into from
Jan 15, 2025
Merged

Pictrs delete token #5317

merged 59 commits into from
Jan 15, 2025

Conversation

Nutomic
Copy link
Member

@Nutomic Nutomic commented Jan 9, 2025

As noted by @Nothing4You there is actually no need to store any pictrs delete tokens. Instead we can use /internal/delete endpoint and allow deletion whenever it is done by the uploader or an admin (or community mod in case of icon/banner). Note that this wont work if the api key isnt set.

Requires #5260

@Nutomic
Copy link
Member Author

Nutomic commented Jan 10, 2025

@dullbananas test_schema_setup is failing with the following error. So the problem is that columns are in the wrong order after running down.sql (which restores the column that was deleted in up.sql). From what I found there is no easy way to change the column order in postgres. What do you think?

thread 'schema_setup::tests::test_schema_setup' panicked at crates/db_schema/src/schema_setup/diff_check.rs:39:5:
These changes need to be applied in migrations/2025-01-09-144233_no-image-token/down.sql:
--- 	2025-01-10 11:22:14.727699044 +0000
+++ 	2025-01-10 11:22:14.727803887 +0000
@@ -1094,8 +1094,8 @@
 CREATE TABLE public.local_image (
     local_user_id integer,
     pictrs_alias text NOT NULL,
-    published timestamp with time zone DEFAULT now() NOT NULL,
-    pictrs_delete_token text DEFAULT ''::text NOT NULL
+    pictrs_delete_token text NOT NULL,
+    published timestamp with time zone DEFAULT now() NOT NULL
 );

Edit: Maybe the solution is to sort these lines in alphabetical order, but not sure how to do that. Ive disabled the test for now.

@dullbananas
Copy link
Collaborator

#5204 implements a diff checker that ignores changes in column order. For now, instead of disabling the check, it would be better to change the column order in the add_image_upload migration.

@Nutomic
Copy link
Member Author

Nutomic commented Jan 13, 2025

Alright its a bit complicated but I did that now. One thing I noticed is that cargo run -- migration run only runs the very first migration, but it should automatically run everything (you need to do cargo run -- migration --all run for that).

@dessalines
Copy link
Member

dessalines commented Jan 13, 2025

I've had to do this a bunch recently so a tip to avoid all the conflicts due to squash merges to main:

Now that #5260 is merged, take this PR, and do

git fetch origin
git merge api-image-rework # Should work fine, because no squashes are done to feature branches.
git merge origin/main --ours # Favors our changes in conflicts, which should be fine bc the previous PR is already close to main

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just these 2 things, and the conflicts.

let pictrs_config = context.settings().pictrs()?;
let url = format!(
"{}image/delete/{}/{}",
pictrs_config.url, &data.token, &data.filename
"{}internal/delete?alias={}",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to build this again, you could just the delete_image_from_pictrs function you made above.

@dessalines dessalines merged commit 5bc3f0c into main Jan 15, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants