Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work setting up the server, implementing the required routes, and handling the error conditions! The overall structure is solid. However, this submission requires changes because it doesn't meet the core requirement of using streams for file processing.
The current implementation buffers the entire file into memory before compressing it. The goal is to process the data in chunks as it arrives. Here are the key areas to revise:
- Use Streaming for File Processing: Instead of waiting for the request's
'end'event to process a single large buffer, you should process the incoming request stream directly. - Use
zlibStreams: Please replace the synchronouszlibmethods (likegzipSync) with their streaming counterparts (e.g.,zlib.createGzip()). The correct approach is topipethe file stream from the request through the appropriatezlibcompression stream and then into the response stream.
Also, please update the file extensions to match the requirements: .gz for gzip and .dfl for deflate.
You're very close, and I'm confident you can make these adjustments!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
src/createServer.js
Outdated
| const body = []; | ||
|
|
||
| req.on('data', (chunk) => body.push(chunk)); | ||
|
|
||
| req.on('end', () => { | ||
| const buffer = Buffer.concat(body); |
There was a problem hiding this comment.
This implementation buffers the entire request body into memory before processing it. A core requirement of the task is to use streams, which involves processing data in chunks as it arrives without loading the entire file into memory. You should look into processing the request stream as it comes in rather than waiting for the 'end' event to process a single large buffer.
src/createServer.js
Outdated
| gzip: { method: zlib.gzipSync, ext: 'gzip' }, | ||
| deflate: { method: zlib.deflateSync, ext: 'deflate' }, | ||
| br: { method: zlib.brotliCompressSync, ext: 'br' }, |
There was a problem hiding this comment.
The task requires using streams, but synchronous zlib methods are used here. The zlib module provides stream-based APIs (e.g., zlib.createGzip(), zlib.createDeflate()) that should be used instead. The file content stream should be piped through one of these compression streams to the response.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job refactoring to use streams and pipeline! You're very close to a complete solution. However, changes are needed because the server isn't correctly handling the multipart/form-data request from the browser. There are two main issues to address: 1. Incorrect Data Stream for Compression: You are currently piping the entire req object to the compressor. A multipart/form-data request includes headers and boundaries along with the file data. You need to parse this request to isolate only the file's content stream and pipe that into the compression stream. Compressing the raw req results in a corrupted output file. 2. Hardcoded Filename and Missing Form Data: The output filename is hardcoded (e.g., file.gz). A core requirement is to name the output file based on the original (e.g., my-document.txt.gz). The original filename, as well as the compressionType from the form, can be extracted by parsing the multipart request. Once you correctly parse the incoming request to get the file stream and metadata, the rest of your logic should work perfectly. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
src/createServer.js
Outdated
| const urlParams = new URL(url, `http://${req.headers.host}`); | ||
| const compressionType = | ||
| urlParams.searchParams.get('compressionType') || | ||
| req.headers['x-compression-type'] || | ||
| 'gzip'; |
There was a problem hiding this comment.
When the form is submitted, the compressionType is sent in the request body, not in the URL. This logic won't be able to retrieve the selected value from the form. You'll need to parse the multipart/form-data body to get this value along with the file.
src/createServer.js
Outdated
|
|
||
| res.writeHead(200, { | ||
| 'Content-Type': 'application/octet-stream', | ||
| 'Content-Disposition': `attachment; filename="file.${config.ext}"`, |
There was a problem hiding this comment.
According to the requirements, the output filename should be based on the original uploaded file's name (e.g., original.txt.gz). Currently, it's hardcoded to file.<ext>. The original filename can be extracted from the Content-Disposition header of the file part within the multipart/form-data request.
src/createServer.js
Outdated
| 'Content-Disposition': `attachment; filename="file.${config.ext}"`, | ||
| }); | ||
|
|
||
| pipeline(req, config.create(), res, (err) => { |
There was a problem hiding this comment.
Excellent job using pipeline to stream the data! This correctly addresses the main feedback from the last review. However, the req object represents the entire incoming HTTP request stream, including multipart boundaries and other form data. You need to first parse this multipart/form-data stream to isolate only the file's content and then pipe that into the compressor. Piping req directly will compress the entire raw request, which is not the intended behavior.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job parsing the multipart form data to get the filename and compression type! I'm approving this submission as an educational step to help you move forward, but there are a few critical stream-handling issues to address.
Your current approach buffers the entire file in memory and stops processing data chunks too early, which can cause out-of-memory errors and result in corrupted files. The server also hangs on invalid form submissions (like a missing file) instead of sending the required 400 error. Lastly, the output filename is using the full compression type (e.g., gzip) instead of the correct extension (e.g., .gz), which you have available in config.ext.
The key is to process the request stream chunk-by-chunk, find the boundary where the file data begins, and then pipe only the file content as it arrives into your compression stream. You're very close to a robust solution!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| let bodyBuffer = Buffer.alloc(0); | ||
| let isStreamingStarted = false; | ||
|
|
||
| req.on('data', (chunk) => { |
There was a problem hiding this comment.
This implementation has a few issues. First, by buffering the entire request with Buffer.concat, it defeats the purpose of using streams for potentially large files, risking out-of-memory errors. Second, the isStreamingStarted check on the next line will cause your server to ignore all subsequent data chunks after the file headers are found, leading to truncated and corrupted files. The file's content needs to be piped from the request stream to the compression stream as it arrives.
| // Prepare response | ||
| res.writeHead(200, { | ||
| 'Content-Type': 'application/octet-stream', | ||
| 'Content-Disposition': `attachment; filename="${filename}.${compressionType}"`, |
There was a problem hiding this comment.
The task requires using the correct file extension for the compressed file (e.g., .gz for gzip). This line is using the full compression type name (gzip) instead of the extension. You have the correct extension available in config.ext which you defined on lines 65-67.
| } | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The server needs to handle cases where the form submission is invalid (e.g., no file is uploaded) and respond with a 400 status code as required. Currently, if the filePartHeaderMatch and typeMatch are never found, the request will hang until it times out. Consider adding a req.on('end', ...) event listener to check if streaming has started. If not, it means the form was invalid and you should send the 400 error.
2 participants
No description provided.