search

discourse / mini_mime

minimal mime type library
MIT License
95 stars 34 forks source link

Make sure the DB file is seekable, use a StringIO wrapper if it's not #38

Closed ikaronen-relex closed 1 year ago

ikaronen-relex commented 3 years ago

This (hopefully) fixes https://github.com/discourse/mini_mime/issues/37

What has been tested:

What has not (yet) been tested:

While I'm 99% confident that the answer to both questions is yes, I'd still like to do a bit more testing. But I wanted to get this PR out for review before the weekend.

(Also, I'm not sure if there are any potential newline and/or UTF-8 conversion issues on some platforms that might require opening the DB files explicitly in binary mode. If there were, I suspect the existing code would already be buggy on such platforms, since it's already seeking around in a file opened in text mode, but I'm still slightly worried and would like to see this tested better.)

SamSaffron commented 3 years ago

Looks like an ok approach. @jeremy how do you feel about this? adds two tell calls and one seek, seems reasonable in the big scheme.

Can we get away with a slightly less paranoid implementation? (seek 1 (if that fails) not seekable, if tell gives me anything that is not 1 then it is not seekable)

Tests are failing so something is off.

@headius FYI

ikaronen-relex commented 3 years ago

Can we get away with a slightly less paranoid implementation? (seek 1 (if that fails) not seekable, if tell gives me anything that is not 1 then it is not seekable)

That was actually more or less what I started with, but I figured one extra tell wouldn't hurt. AIUI tell is supposed to be a fast operation anyway. And since we're reading the first line anyway, I could actually test a real backwards seek after read for basically free.

(Now, if I was actually paranoid, I'd readline again after the seek and check that I get the same line back as first time…)

Tests are failing so something is off.

😬 Not much to go on in the test logs, but let me do some local testing and try to see what's going wrong.

ikaronen-relex commented 3 years ago

In local testing, I'm getting a failure in test_full_parity_with_mime_types for the extension .webmanifest, which should return "application/manifest+json" according to MIME::Types. However, rake rebuild_db fixes the failure. Should I just include the rebuilt DB in the PR?

Fryguy commented 3 years ago

.webmanifest was merged in https://github.com/discourse/mini_mime/pull/36, so you probably just need a rebase.

jeremy commented 3 years ago

Agreed; reasonable.

On Sun, Aug 22, 2021 at 19:28 Sam @.***> wrote:

Looks like an ok approach. @jeremy https://github.com/jeremy how do you feel about this? adds two tell calls and one seek, seems reasonable in the big scheme.

Can we get away with a slightly less paranoid implementation? (seek 1 (if that fails) not seekable, if tell gives me anything that is not 1 then it is not seekable)

Tests are failing so something is off.

@headius https://github.com/headius FYI

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/discourse/mini_mime/pull/38#issuecomment-903395855, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAABR3TFJBDNUQFXLO54FLT6GW4PANCNFSM5CQKX3DA .

ikaronen-relex commented 2 years ago

It's been over a year. Could someone please review this PR? *nudge, nudge* We'd like to stop using a private fork of this gem but we still need this fix merged in order to do so. 🙄