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

Remove Content-Type header check in Files POST requests #16176

Merged
merged 7 commits into from
Nov 1, 2022

Conversation

azrikahar
Copy link
Contributor

@azrikahar azrikahar commented Oct 26, 2022

Description

Fixes #16140

#11347 moved the content type header check from multipartHandler to the files POST request controller, however it was wrongly assumed that POST request must always include multipart file(s). This prevented the creation of file entry without actual file which was supported prior to that PR. Hence this PR removes the check altogether.

Type of Change

  • Bugfix
  • Improvement
  • New Feature
  • Refactor / codestyle updates
  • Other, please describe:

Requirements Checklist

  • New / updated tests are included
  • All tests are passing locally
  • Performed a self-review of the submitted code

If adding a new feature:

  • Documentation was added/updated. PR:

Copy link
Member

@br41nslug br41nslug left a comment

Choose a reason for hiding this comment

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

Do we want to document the option of sending JSON to the /files endpoint? The current docs explicitly say you need to use multipart/form-data (https://docs.directus.io/reference/files.html#upload-a-file).
I think we should make type a required property when creating an empty file. The current test payload { title: 'Test File', storage: 'local', filename_download: 'test_file' } gets a success response and is saved to the database but is not rendered in the App.

firefox_p2glGt8dGa.mp4

@azrikahar
Copy link
Contributor Author

Do we want to document the option of sending JSON to the /files endpoint? The current docs explicitly say you need to use multipart/form-data (https://docs.directus.io/reference/files.html#upload-a-file).

Agreed 👍

I think we should make type a required property when creating an empty file. The current test payload { title: 'Test File', storage: 'local', filename_download: 'test_file' } gets a success response and is saved to the database but is not rendered in the App.

That's a great point. Though I do wonder if it's intentional (and there's an existing assumption) that file type may be null? 🤔 It seems to be filtered here:

{
type: {
_nnull: true,
},
},

FWIW I do personally still wonder what is/was the utility of being able to make the POST request without a file.

@rijkvanzanten thoughts on these points/questions? Here's a quick summary:

  • Should type be a required property?

  • Currently the app filters out files without type when fetching them, is there a specific reasoning?

  • The files POST endpoint allows creation of files with only JSON payload, is there a specific use case? As Tim pointed out, the docs does note that it must be multipart/data instead of JSON, so there might be so disparity between how the API should/actually work. 🤔

@rijkvanzanten
Copy link
Member

The only real use case is so you can associate metadata with a file that already exists on disk

@azrikahar
Copy link
Contributor Author

The only real use case is so you can associate metadata with a file that already exists on disk

That does makes sense! In that case, I assume type and filesize should also be included by the user themselves when making such association? Not sure should this use case be documented somehow 🤔

I think we should make type a required property when creating an empty file. The current test payload { title: 'Test File', storage: 'local', filename_download: 'test_file' } gets a success response and is saved to the database but is not rendered in the App.

@br41nslug upon further inspection, I've noticed this block of code setting the default type for uploaded files in case there's no type:

if (!payload.type) {
payload.type = 'application/octet-stream';
}

Perhaps we can do the same for JSON payload? It does at least show up in the App after I tested to put such default in place.

@br41nslug
Copy link
Member

Perhaps we can do the same for JSON payload? It does at least show up in the App after I tested to put such default in place.

We definitely could but as Rijk said the usecase for this is to associate meta with already existing files i think requiring people to assign the correct type for that may be the better way to go.

@azrikahar
Copy link
Contributor Author

We definitely could but as Rijk said the usecase for this is to associate meta with already existing files i think requiring people to assign the correct type for that may be the better way to go.

Makes sense! Was partially wondering would it be too strict in the case where the user themselves aren't sure about the type, but it does feel like that should fall under the responsibility of users going with such use case to be begin with. Added a check for "type" in FileService's createOne so now it is required in this scenario 👍

@br41nslug br41nslug self-requested a review November 1, 2022 12:01
Copy link
Member

@br41nslug br41nslug left a comment

Choose a reason for hiding this comment

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

Looking good 😄

@rijkvanzanten rijkvanzanten self-assigned this Nov 1, 2022
@rijkvanzanten rijkvanzanten added this to the Next Release milestone Nov 1, 2022
@rijkvanzanten rijkvanzanten merged commit 17c0ef9 into main Nov 1, 2022
@rijkvanzanten rijkvanzanten deleted the issue/16140 branch November 1, 2022 16:06
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

useless conditional statement
3 participants