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

Include detected file mime type in validate_mime_type_inclusion errors #608

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Include detected file mime type in validate_mime_type_inclusion errors #608

wants to merge 1 commit into from

Conversation

janklimo
Copy link

@janklimo janklimo commented Nov 23, 2022 •
edited
Loading

Problem

The default error message of validate_mime_type_inclusion can be confusing to the end user.

To give a specific recent example, we validate mime type to be text/csv. When a user uploads a CSV file that looks perfectly fine but includes an <html> tag, Marcel will identify the file as text/html.

To replicate:

string = StringIO.new("a,b,c,d,e,<html>data</html>")
Marcel::MimeType.for(string, name: "test.csv")
# => "text/html"

Without providing feedback as to what mime type was detected, the user will be unaware what's wrong with their data.csv file when the error they see is File type must be one of: text/csv.

There is currently no way to provide a custom error message with the detected mime type other than writing a custom plugin.

Changes

Include the detected mime type in the default validation error message. This way, the user can review the file and clean it up. In the example above, any HTML in the CSV is undesirable and will cause issues downstream.

Error before: type must be one of: text/csv
Error after:

  • type must be one of: text/csv, (text/html content type detected)
  • type must be one of: text/csv, (no content type detected)

Considerations

validate_mime_type_exclusion could be updated the same way. However, the message makes more sense in this case (e.g. the file can't be image/jpeg but somehow it is image/jpeg.

Some documentation could be added about defining a custom message (expecting 2 arguments, not 1) but I wanted to collect initial feedback first.

@janklimo janklimo force-pushed the descriptive-mime-type-inclusion-error branch 2 times, most recently from 1c5cff0 to b90abf7 Compare November 23, 2022 12:24
@janklimo janklimo force-pushed the descriptive-mime-type-inclusion-error branch from b90abf7 to 1b3d4c2 Compare November 23, 2022 12:27
@janklimo
Copy link
Author

@janko feel free to close if the PR isn't relevant anymore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@janklimo