fs: Fix serving precompressed files without an extension #507
Conversation
| let mut decoder = GzDecoder::new(&body[..]); | ||
| let mut decompressed = String::new(); | ||
| decoder.read_to_string(&mut decompressed).unwrap(); | ||
| assert!(decompressed.starts_with("Content.")); |
There was a problem hiding this comment.
Why assert it starts with that? This seems like it should be an assert_eq! - maybe the string literal needs an extra \n in that case, or we could call decompressed.trim().
There was a problem hiding this comment.
I agree that this is a weird way to do this. I did it for consistency with the existing precompression tests, which do the same.
There was a problem hiding this comment.
Weird. No need to be consistent with that. I would welcome another PR to change the existing tests too, if you feel like it.
There was a problem hiding this comment.
I changed it to check the entire response body.
I'm afraid it will break on Windows if git is configured to change line endings to CRLF. This conversion will also only affect the uncompressed test files, which is annoying.
There was a problem hiding this comment.
I realized that I can just remove the trailing newline from the test file, so I did that. That makes it not a valid Unix text file, but there is no reason it should be.
|
I forgot to say as part of the review, but thanks a lot for the clear issue description and providing a fix complete with tests! |
jplatte
changed the title
|
It seems I managed to add a commit to the PR between you approving it and you merging it. I didn't know that's possible and it seems dangerous. |
|
I saw the new commit before pressing merge. I know this can be disabled, we do it at work, but I haven't seen this be a problem in all of the open source projects I work on. Especially because the GitHub web UI frequently craps out when you try to merge if a commit is pushed between opening the page and pressing merge 😂 |
Motivation
When trying to locate the precompressed version of a file that doesn't have a file extension (say
/downloads/foobar-cli), theServeDirservice passes the relevant extension with a leading dot toPathBuf::set_extension, which doesn't expect a leading dot. This means it will end up with an filename that looks like this:/downloads/foobar-cli..gz, which probably doesn't exist. Worse still, it also fails to reconstruct the original path when it doesn't find anything at the corrupted path, which leads to a 404 error.Solution
This change makes the function responsible for constructing the modified path directly modify the filename, instead of using the extension API, since an extension might not exist.
Fixes #504
I feel that #504 is clearly a bug, but someone might for some reason depend on the broken behavior somewhere.