fix(file_upload): validate mimetype as configured #1459
Conversation
qvalentin
force-pushed
the
fix/mimetype-validation-file-upload
branch
2 times, most recently
from
2e4689b
to
f094ebc
Compare
dosubot
bot
added
size:L
dokterbob
added
evaluate-with-priority
There was a problem hiding this comment.
Tests look very clean. Thanks this already great as it is, but I would prefer some slight changes to validation architecture -- separating HTTP logic from validation logic. Important as we'll add more such functionality going forward, good to have the API clean now.
backend/chainlit/server.py
Outdated
| file_response = await session.persist_file( | ||
| name=file.filename, content=content, mime=file.content_type | ||
| ) | ||
|
|
||
| return JSONResponse(content=file_response) | ||
|
|
||
|
|
||
| def validate_file_upload(file: UploadFile): | ||
| if config.features.spontaneous_file_upload is None: | ||
| return # TODO: if it is not configured what should happen? |
There was a problem hiding this comment.
The most elegant solution is to change the default value for config.features.spontaneous_file_upload to always be populated with SpontaneousFileUploadFeature then to have enabled default to False and move max_files and max_size_mb default values from the frontend to the backend config. This would remove the if ... is None in favour of more explicit code. But I get it if perhaps that's out of scope and left for a later PR.
The second best would be to follow the frontend in this and not allow uploads unless explicitly enabled. Definitely, we should try to avoid having a TODO in a PR. ;)
There was a problem hiding this comment.
The current behaviour for the e2e tests is that it is None. So rejecting upload when it is not configured will break the tests and might also break things for users.
I agree that the best would be to not have None as the default, but this indeed seems out of scope.
| file_response = await session.persist_file( | ||
| name=file.filename, content=content, mime=file.content_type | ||
| ) | ||
|
|
||
| return JSONResponse(content=file_response) | ||
|
|
||
|
|
||
| def validate_file_upload(file: UploadFile): |
There was a problem hiding this comment.
I would prefer to have validation functions raise a well-defined Exception and to have a docstring specifying it's behaviour.
For example, in pydantic this is a ValueError and in Django it's a ValidationError. It should definitely not be a HTTPException as the file validation code should not have anything to do with HTTP, it's just validating an UploadFile.
We can catch validation errors and then re-raise them in a HTTP request handler (upload_file).
dokterbob
added
the
blocked
Where would you put the validation logic? |
This is what I referred to in that comment: #1459 (comment) |
# Conflicts: # backend/tests/test_server.py
qvalentin
force-pushed
the
fix/mimetype-validation-file-upload
branch
from
d0f41f7
to
ddbe7ab
Compare
#1458