-
Notifications
You must be signed in to change notification settings - Fork 388
feat(upload): auto-detect file content-type if not sent #5361
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
Merged
Merged
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
6fb186a
fix(api): enhance content type handling in bzzUploadHandler for direc…
akrem-chabchoub b405b73
fix(api): improve error handling in fileUploadHandler for EOF scenarios
akrem-chabchoub 1c6cb71
Merge branch 'master' of https://github.com/ethersphere/bee into fix/…
akrem-chabchoub 147ce4b
refactor: remove unused mime parsing and streamline content type hand…
akrem-chabchoub fe1e6a3
test: add content type handling tests for file uploads and downloads
akrem-chabchoub 10580df
docs: update content-type docs
akrem-chabchoub bfaf1e7
docs: refine Content-Type description and remove comments
akrem-chabchoub 8dbae20
refactor: improve Content-Type handling in upload handlers
akrem-chabchoub 7e687cb
refactor: streamline body reading and Content-Type detection in file …
akrem-chabchoub 3735903
Merge branch 'master' of https://github.com/ethersphere/bee into fix/…
akrem-chabchoub 1206113
refactor: enhance upload handler logic to differentiate between file …
akrem-chabchoub c544efa
test: add tests for handling missing ContentType and body
akrem-chabchoub 4ca4f22
refactor: clarify comment on media type parsing in dirUploadHandler
akrem-chabchoub File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before my changes the
dirUploadHandlerwas acceptingcontentTypeString(although it is present request).So I changed that here and did trim and set the new value in main handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that I agree with @martinconic that this is not really clear why this is needed. If you're already reading, cleaning, parsing and checking the content type header - it does not make any more sense to update the value in the
Headertype.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@acud @martinconic
Good point, i agree this can look a bit strange at first.
i set the header on purpose, so we clean
Content-Typeone time, and then all next steps read the same value.without this, one part can use the trimmed value, but another part (
fileUploadHandler) can still read the original raw header. this can cause small inconsistent behavior later, especially for multipart handling and metadata.if changing the header feels too implicit, i can refactor and pass the cleaned value explicitly to the next functions. i am fine with both styles, but i think we should keep one canonical content-type value in one place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @akrem-chabchoub.
The problem from my perspective is that you're already doing cleaning and checking of a value, mutating it, but then you're telling downstream function calls to potentially do the same, again, because you've removed the argument from the signature. This is from my perspective not necessary and potentially duplicates code.
This may work in middleware style calls where the middlewares are decoupled (and you want to be setting a header on one middleware that processes for example a user-login or something along those lines, providing you with extra details down the call stack), but I can't see how this is needed here (you're already at the last middleware in the API).
I won't block the PR on this, please choose how you'd like to proceed with this.