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.

Sign up for GitHub

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

Jump to bottom

Add 404 as valid response for /thumbnail and /download when mediaId does not exist #1122

Open
MadLittleMods opened this issue Jun 13, 2022 · 2 comments
Open

Add 404 as valid response for /thumbnail and /download when mediaId does not exist #1122

MadLittleMods opened this issue Jun 13, 2022 · 2 comments
Labels
A-Client-Server Issues affecting the CS API spec-omission implemented but not currently specified

Comments

@MadLittleMods
Copy link
Contributor

MadLittleMods commented Jun 13, 2022 •
edited
Loading

400 makes a lot of sense for all of the scenarios where the media repository can't provide a thumbnail for whatever reason when the media exists.

But 404 probably makes more sense when mediaId isn't valid and doesn't exist.

Current spec

https://spec.matrix.org/v1.1/client-server-api/#get_matrixmediav3thumbnailservernamemediaid

Responses

Status Description
200 A thumbnail of the requested content.
400 The request does not make sense to the server, or the server cannot thumbnail the content. For example, the client requested non-integer dimensions or asked for negatively-sized images.
413 The local content is too large for the server to thumbnail.
429 This request was rate-limited.
502 The remote content is too large for the server to thumbnail.
@MadLittleMods MadLittleMods mentioned this issue Jun 13, 2022
Closed
@turt2live
Copy link
Member

Related: #1095

@turt2live turt2live added A-Client-Server Issues affecting the CS API improvement An idea/future MSC for the spec labels Jun 13, 2022
@MadLittleMods MadLittleMods mentioned this issue Jun 14, 2022
Merged
4 tasks
MadLittleMods added a commit to matrix-org/synapse that referenced this issue Jul 15, 2022
@MadLittleMods
Provide more info why we don't have any thumbnails to serve (#13038)
7b67e93 
Fix #13016

## New error code and status

### Before

Previously, we returned a `404` for `/thumbnail` which isn't even in the spec.

```json
{
  "errcode": "M_NOT_FOUND",
  "error": "Not found [b'hs1', b'tefQeZhmVxoiBfuFQUKRzJxc']"
}
```

### After

What does the spec say?

> 400: The request does not make sense to the server, or the server cannot thumbnail the content. For example, the client requested non-integer dimensions or asked for negatively-sized images.
>
> *-- https://spec.matrix.org/v1.1/client-server-api/#get_matrixmediav3thumbnailservernamemediaid*

Now with this PR, we respond with a `400` when we don't have thumbnails to serve and we explain why we might not have any thumbnails.

```json
{
    "errcode": "M_UNKNOWN",
    "error": "Cannot find any thumbnails for the requested media ([b'example.com', b'12345']). This might mean the media is not a supported_media_format=(image/jpeg, image/jpg, image/webp, image/gif, image/png) or that thumbnailing failed for some other reason. (Dynamic thumbnails are disabled on this server.)",
}
```

> Cannot find any thumbnails for the requested media ([b'example.com', b'12345']). This might mean the media is not a supported_media_format=(image/jpeg, image/jpg, image/webp, image/gif, image/png) or that thumbnailing failed for some other reason. (Dynamic thumbnails are disabled on this server.)


---

We still respond with a 404 in many other places. But we can iterate on those later and maybe keep some in some specific places after spec updates/clarification: matrix-org/matrix-spec#1122

We can also iterate on the bugs where Synapse doesn't thumbnail when it should in other issues/PRs.
@turt2live turt2live changed the title Add 404 as valid response for /thumbnail when mediaId does not exist Add 404 as valid response for /thumbnail and /download when mediaId does not exist Nov 24, 2023
@turt2live turt2live mentioned this issue Nov 24, 2023
Closed
@turt2live
Copy link
Member

see also: #1678

@turt2live turt2live added spec-omission implemented but not currently specified and removed improvement An idea/future MSC for the spec labels Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-Client-Server Issues affecting the CS API spec-omission implemented but not currently specified
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@MadLittleMods @turt2live