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

Dont pass accept-encoding header to pictrs (ref #1734) #1738

Merged
merged 3 commits into from
Sep 17, 2021
Merged

Conversation

Nutomic
Copy link
Member

@Nutomic Nutomic commented Aug 27, 2021

I didnt manage to get image upload working locally, but based on dbg!(&client_req.headers().keys()) it removes the header as expected.

@dessalines
Copy link
Member

I pry wont be able to test this until sunday

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 tried this, it didn't work. I also added println's to make sure the headers were okay. You also used CONTENT_ENCODING here, not ACCEPT_ENCODING, both of which I tried to remove, and didn't work.

If you want to test this on your own, all you need to do is have a locally running nginx that looks identical to the one in the ansible folder, but use 1235 for the lemmy_ui_port and 8536 for the lemmy_port. Then do any picture upload, say in a comment box.

Also verified that the main branch does currently work, just to make sure.

@Nutomic
Copy link
Member Author

Nutomic commented Sep 2, 2021

If you have such an nginx setup working, could you add it to the repo in docker/dev/?

@dessalines
Copy link
Member

Mmmk that's here: #1765

@Nutomic
Copy link
Member Author

Nutomic commented Sep 17, 2021

Updated. Image uploads are working fine now without brotli/gzip deps. I had to rework #1765 a bit.

nginx:
image: nginx:1-alpine
ports:
- "1236:1236"
Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to use port 1234 here for consistency, but that wasnt working because image uploads were sent to localhost:8536 in that case, so the jwt cookie wasnt included. This is because of this overly complicated code in lemmy-ui, which seems somewhat broken.

https://github.com/LemmyNet/lemmy-ui/blob/main/src/shared/env.ts

@@ -103,6 +106,7 @@ async fn image(
client: web::Data<Client>,
) -> Result<HttpResponse, Error> {
let mut client_req = client.request_from(url, req.head());
client_req.headers_mut().remove(ACCEPT_ENCODING);
Copy link
Member

Choose a reason for hiding this comment

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

I'll test this today. I coulda swore I tried and failed when doing this on my own system.

@dessalines
Copy link
Member

Works!

@dessalines dessalines merged commit e84e119 into main Sep 17, 2021
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.

2 participants