Skip to content

solution#191

Open
OleksandrY0 wants to merge 4 commits intomate-academy:masterfrom
OleksandrY0:master
Open

solution#191
OleksandrY0 wants to merge 4 commits intomate-academy:masterfrom
OleksandrY0:master

Conversation

@OleksandrY0
Copy link

No description provided.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Review Summary

I am requesting changes because there are a few critical points that will likely cause the task’s tests to fail.

  1. Filename extension mapping (critical blocker): In src/createServer.js, you currently derive the output extension directly from compressionType (e.g. .gzip, .deflate). The requirements specify exact extensions: gzip -> .gz, deflate -> .dfl, br -> .br. Please introduce a small mapping (e.g. an object or switch) that converts compressionType to these extensions before building the Content-Disposition header, so the response filename matches the spec.
  2. Field name validation (critical blocker): In the bb.on('file', fieldname, fileStream, ...) handler in src/createServer.js, any file field is accepted. The checklist requires the uploaded file field to be named exactly file. Add a check like if (fieldname !== 'file') and either resume/discard that stream and mark the request invalid (respond with 400 on finish), or immediately send a 400, so malformed forms do not pass as valid.
  3. Unconditional server start (likely test blocker): At the bottom of src/createServer.js, createServer().listen(3006) runs unconditionally. Test harnesses usually import createServer and control listen themselves, so this can lead to port conflicts or hanging tests. Wrap the listen call in if (require.main === module) { ... } or remove it from the module so only the tests or a separate entry file start the server.

Once you fix these points, your streaming logic and error handling structure look solid for this exercise; the changes are focused and will make your implementation align well with the task requirements.


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

}

// ВИПРАВЛЕННЯ: Використовуємо повне ім'я, як вимагають тести.
const fileExt = compressionType;

Choose a reason for hiding this comment

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

The output extension is set directly from compressionType (const fileExt = compressionType;) which will produce names like file.txt.gzip or file.txt.deflate. The task requires specific extensions: gzip -> .gz, deflate -> .dfl, br -> .br. Map compressionType to these exact extensions before constructing the Content-Disposition header.

let fileInfo = null;

// 1. Обробник 'file': ПРИЗУПИНЯЄМО потік і ЗБЕРІГАЄМО логіку
bb.on('file', (fieldname, fileStream, filename) => {

Choose a reason for hiding this comment

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

The bb.on('file', ...) handler does not validate the incoming field name. The form input must be named file; verify fieldname === 'file' and treat other file fields as invalid (resume and respond 400 or ignore) so malformed forms are rejected as required by the checklist.

return server;
}

createServer().listen(3006);

Choose a reason for hiding this comment

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

The module starts the server unconditionally with createServer().listen(3006). Tests often import createServer and control listening themselves. Consider removing or guarding this automatic listen so the test harness can manage server lifecycle.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Review Summary

I am requesting changes because there is a critical functional issue that will cause tests to fail and a couple of stream-handling problems that can leave uploads hanging. The main blocker is in src/createServer.js: your EXT_MAP currently maps to compression names like 'gzip', but the requirements expect exact file extensions gzip: 'gz', deflate: 'dfl', br: 'br', so the downloaded files must be named like file.txt.gz, file.txt.dfl, or file.txt.br. In the same file, when an unknown compressionType is detected, you call res.end(...) but never resume or destroy the incoming fileStream; you should explicitly call fileStream.resume() or fileStream.destroy() before ending the response so the upload stream is not left paused. Also in the finish handler there are two consecutive !compressionType checks: the second block (which resumes fileInfo.fileStream) is unreachable, so you should consolidate these into a single branch that both resumes/destroys the stream and returns a 400 with a clear message when compressionType is missing. As a smaller improvement, when setting Content-Disposition, wrap the filename in quotes, e.g. attachment; filename="${filename}.${fileExt}", to correctly handle spaces and special characters once EXT_MAP is fixed. Overall your implementation is very close—once you correct the extension mapping and clean up the stream handling branches, it should fully meet the task requirements.


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment on lines +10 to +14
const EXT_MAP = {
gzip: 'gzip',
deflate: 'deflate',
br: 'br',
};

Choose a reason for hiding this comment

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

EXT_MAP currently maps to the compression names (e.g. 'gzip') rather than the file extensions the tests require. Update the mapping so the stored extension values are exactly: gzip: 'gz', deflate: 'dfl', br: 'br' so the produced filename becomes file.txt.gz, file.txt.dfl, or file.txt.br as required.

Comment on lines 71 to 74
res.setHeader(
'Content-Disposition',
`attachment; filename=${filename}.${fileExt}`,
);

Choose a reason for hiding this comment

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

You set the Content-Disposition here. After changing EXT_MAP this will produce the correct extension. Also consider wrapping the filename in quotes to support spaces/special characters, e.g. attachment; filename="${filename}.${fileExt}".

Comment on lines 53 to 56
if (!EXT_MAP[compressionType]) {
res.statusCode = 400;

return res.end('Unknown compression type');

Choose a reason for hiding this comment

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

When detecting an unknown compression type you call res.end(...) but the incoming fileStream is still paused/unconsumed. Resume or destroy fileStream before ending the response (e.g. fileStream.resume() or fileStream.destroy()), otherwise the upload stream may be left hanging.

Comment on lines 110 to 119

if (!compressionType) {
return res.writeHead(400).end();
}

if (!compressionType) {
fileInfo.fileStream.resume();
res.statusCode = 400;

return res.end('Missing compressionType field');

Choose a reason for hiding this comment

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

There are two consecutive checks for !compressionType in the finish handler (one at lines 110–112 and another at 114–119). The second block (which calls fileInfo.fileStream.resume()) is unreachable — consolidate these into a single finish-branch that resumes/destroys the file stream and returns a 400 with a clear message when compressionType is missing.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Review Summary

I am approving your solution as an educational override so you can keep progressing while you refine a few remaining details.

You’ve done a solid job implementing the streaming server with busboy, zlib, and proper routing/validation for /compress, which covers the core requirements well. There are three specific improvements you should make next:

  1. In src/createServer.js, update your EXT_MAP so it maps to extensions, e.g. gzip: 'gz', deflate: 'dfl', br: 'br', so that outName becomes file.txt.gz, file.txt.dfl, or file.txt.br instead of using the algorithm names.
  2. Where you handle an unsupported compressionType (around the !EXT_MAP[compressionType] check), you currently pause the upload stream with file.pause(); before sending the 400 response, make sure to drain or destroy the file stream (for example, call file.resume() or file.destroy()) so the upload doesn’t hang.
  3. When setting Content-Disposition, wrap the filename in quotes, e.g. attachment; filename="${outName}", so filenames with spaces or special characters are handled correctly.

Overall, your structure and use of streams are on the right track—these refinements will bring your solution fully in line with typical production behavior and the assignment expectations. Keep going; you’re very close to a polished implementation.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

Comment on lines +10 to +14
const EXT_MAP = {
gzip: 'gzip',
deflate: 'deflate',
br: 'br',
};

Choose a reason for hiding this comment

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

(HIGH) EXT_MAP currently maps to compression names ('gzip','deflate','br'). The tests expect file extensions so the produced filename must be e.g. file.txt.gz, file.txt.dfl, file.txt.br. Change the mapping to the extension tokens (for example gzip: 'gz', deflate: 'dfl', br: 'br') so outName becomes correct.

Comment on lines +55 to +58
if (!EXT_MAP[compressionType]) {
res.statusCode = 400;

return res.end('Invalid compressionType');

Choose a reason for hiding this comment

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

(MEDIUM) When !EXT_MAP[compressionType] is detected you set res.statusCode = 400 and return res.end('Invalid compressionType'), but the incoming file stream was paused earlier (file.pause()) and is not resumed or destroyed. This can leave busboy/upload hanging. Resume or destroy the file stream (for example file.resume() or file.destroy()) before ending the response.

Comment on lines +72 to +74
res.setHeader(
'Content-Disposition',
`attachment; filename=${outName}`,

Choose a reason for hiding this comment

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

(LOW) Wrap the filename in quotes in the Content-Disposition header to handle spaces/special characters correctly. Use e.g. attachment; filename="${outName}" instead of attachment; filename=${outName}.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants