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

Add MIME registry #5765

Merged
merged 2 commits into from Nov 13, 2018

Conversation

@straight-shoota
Copy link
Member

straight-shoota commented Mar 3, 2018

This adds a MIME type which works as registry for mime types and allows to look up the mime type registered for each file name extension.

By default, a limited set of mime types is always registered, including those that have previously been used in HTTP::StaticFileHandler. Additionally, there is an implementation in Crystal::System::MIME that tries do load a mime database from the operating system. On unix systems the following paths are looked up:

/etc/mime.types
/etc/apache2/mime.types
/etc/apache/mime.types
# only freebsd
/usr/local/etc/mime.types
# only openbsd
/usr/share/misc/mime.types

For windows this is not yet implemented because we're missing access to the registry.

Applications (or shards) can also register custom mime types and extensions.

Closes #4632

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/mime branch 2 times, most recently from bbea07c to c8ad4eb Mar 3, 2018

Show resolved Hide resolved src/mime.cr
src/mime.cr Outdated
#
# ```
# require "mime"
# MIME[".html"] # => "text/html"

This comment has been minimized.

@Sija

Sija Mar 4, 2018

Contributor

Is MIME.[] implemented?

This comment has been minimized.

@straight-shoota

straight-shoota Mar 4, 2018

Author Member

Ah, no. I was unsure about that. Should we have MIME.[] and/or MIME.fetch? Or a different name?

This comment has been minimized.

@Sija

Sija Mar 4, 2018

Contributor

Personally, I'm finding Klass.[] aliased to Klass.fetch as a quite nice syntactic sugar, but of course there's the Crystal stand on aliasing...

This comment has been minimized.

@RX14

RX14 Mar 5, 2018

Member

No aliases please. Let's just name these methods normally and leave aside the unnecessary syntax sugar.

This comment has been minimized.

@straight-shoota

straight-shoota Mar 5, 2018

Author Member

Yeah, I figure it doesn't make much sense here anyway.

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/mime branch 2 times, most recently from 633ba09 to a4b5703 Mar 4, 2018

@straight-shoota

This comment has been minimized.

Copy link
Member Author

straight-shoota commented Mar 7, 2018

I've added .jpeg entry and removed the doc reference to MIME.[].

Anything else?

src/mime.cr Outdated
".htm" => "text/html; charset=utf-8",
".html" => "text/html; charset=utf-8",
".jpg" => "image/jpeg",
".jepg" => "image/jpeg",

This comment has been minimized.

@woodruffw

woodruffw Mar 7, 2018

Contributor

Small typo 🙂

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/mime branch from a4b5703 to d9da99a Mar 7, 2018

Show resolved Hide resolved src/crystal/system/mime.cr
]

{% if flag?(:freebsd) %}
MIME_SOURCES << "/usr/local/etc/mime.types"

This comment has been minimized.

@RX14

RX14 Mar 7, 2018

Member

macro indent.

mediatype = parse_media_type(type) || raise Error.new "invalid media type: #{type}"

@@types[extension] = type
@@types_lower[extension.downcase] = type

This comment has been minimized.

@RX14

RX14 Mar 7, 2018

Member

But you always downcase on line 99.

This comment has been minimized.

@straight-shoota

straight-shoota Mar 7, 2018

Author Member

Yeah, it shouldn't be downcased before.

This comment has been minimized.

@Sija

Sija Apr 6, 2018

Contributor

Why bother keeping two hashes, and not just downcase-d one?

This comment has been minimized.

@straight-shoota

straight-shoota Nov 8, 2018

Author Member

@Sija Resolution should be case-sensitive by default and only fall back to case-insensitive if there is no match.

src/mime.cr Outdated
@@types_lower[extension.downcase] = type

if mediatype
array = @@extensions.fetch(mediatype) do

This comment has been minimized.

@RX14

RX14 Mar 7, 2018

Member

@@extensions[mediatype] ||= [] of String?

@Sija

This comment has been minimized.

Copy link
Contributor

Sija commented Apr 5, 2018

Merge 🕦?

MIME.extensions("text/custom-type").should eq [".Custom-Type", ".custom-type2"]

MIME.register(".custom-type", "text/custom-type-lower")
MIME.fetch(".custom-type").should eq "text/custom-type-lower"

This comment has been minimized.

@RX14

RX14 Apr 6, 2018

Member

what happens when you fetch .Custom-Type here?

This comment has been minimized.

@straight-shoota

straight-shoota Apr 6, 2018

Author Member

The same as before. I can add a spec.

module Crystal::System::MIME
# Load MIME types from operating system source.
def self.load
# stub

This comment has been minimized.

@RX14

RX14 Apr 6, 2018

Member

Replace this with a comment saying that windows supplies no mime types.

This comment has been minimized.

@straight-shoota

straight-shoota Apr 6, 2018

Author Member

Well it does, You'll just need to be able to access the registry to get them. But I will clarify that and add a TODO.

src/mime.cr Outdated
#
# A case sensitive search is tried first, if this yields not result, it it
# matched case-insensitive. Returns *default* if *extension* is not registered.
def self.fetch(extension : String, default) : String

This comment has been minimized.

@RX14

RX14 Apr 6, 2018

Member

Perhaps rename fetch to from_extension? Or make extension required? MIME.fetch(extension: ".foo"). Having a MIME.from_filename("index.html") or a MIME.fetch(filename: "index.html") could be really nice (it just calls extname for you).

This comment has been minimized.

@Sija

Sija Apr 6, 2018

Contributor

Comment could be improved too: if this yields not result, it it matched case-insensitive.

This comment has been minimized.

@straight-shoota

straight-shoota Apr 6, 2018

Author Member

The problem with a required named argument is that the default argument would be weird. And I don't think named argument should be used to distinguish whether the method receives an extension or filename argument.

This comment has been minimized.

@straight-shoota

straight-shoota Apr 6, 2018

Author Member

from_filename and from_extension sounds good.

Although I really like fetch because it's short ;)

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/mime branch from daa809c to 49c1543 Apr 6, 2018

src/mime.cr Outdated
#
# *extension* must start with a dot (`.`) and must not contain any null bytes.
def self.register(extension : String, type : String) : Nil
raise ArgumentError.new("extension does not start with a dot: #{extension.inspect}") unless extension.starts_with?('.')

This comment has been minimized.

@Sija

Sija Apr 6, 2018

Contributor

Please use Sentence case for exception messages.
ditto all below.

@RX14

RX14 approved these changes Apr 7, 2018

Copy link
Member

RX14 left a comment

Just fix the specs and we're good to go.

@RX14

RX14 approved these changes Apr 7, 2018

@jhass

jhass approved these changes Apr 9, 2018

@ysbaddaden
Copy link
Member

ysbaddaden left a comment

I believe a few things could be done better. Especially the MIME <-> Crystal::System::MIME relationship.

Last but not leat: I'm really sick of seeing return nil. I have goosebumps every single time I see these, and I really want to burn them with a f****** flamethrower.

src/mime.cr Outdated
private TSPECIAL_CHARACTERS = {'(', ')', '<', '>', '@', ',', ';', ':', '\\', '"', '/', '[', ']', '?', '='}

# :nodoc:
def self.parse_media_type(type : String) : String?

This comment has been minimized.

@ysbaddaden

ysbaddaden Apr 9, 2018

Member

Must be private. It's only public for testing purposes, but internal implementation detail musn't be tested directly. Test MIME.register with various cases instead.

This comment has been minimized.

@straight-shoota

straight-shoota Apr 9, 2018

Author Member

I'd want to avoid speccing this through MIME.register for it's side effects. It's easier to just spec the method directly.

This comment has been minimized.

@ysbaddaden

ysbaddaden Apr 9, 2018

Member

Easier != what should be done.

I don't deny the tests were helpful to design the feature, but either #parse_media_type as a value by itself, and thus should be public & tested, or it's an implementation detail and associated tests can be dropped.

MIME_SOURCES.each do |path|
next unless ::File.exists?(path)
::File.open(path) do |file|
::MIME.load_mime_database file

This comment has been minimized.

@ysbaddaden

ysbaddaden Apr 9, 2018

Member

That should work the other way around: Crystal::System::MIME should provide a method that returns (or yields) a mime.types file that MIME would try to parse.

That would avoid the circular dependency that MIME depends on Crystal::System::MIME which itself depends on MIME.

This comment has been minimized.

@straight-shoota

straight-shoota Apr 9, 2018

Author Member

I don't like the circular dependency either. But the fact that Crystal::System::MIME.load utilizes MIME.load_mime_database is already a platform specific implementation.

It could certainly be reversed if load yields if it needs to load a file. On windows it just wouldn't yield, but load the database differently. But that's baking platform-specifics into the abstraction API.

That's how I chose to implement it and I think it should stay this way. But I won't hurt about changing if requested.

This comment has been minimized.

@straight-shoota

straight-shoota Apr 9, 2018

Author Member

It's similar with Crystal::System::Time.load_localtime btw., the Unix implementation calls the non-platform-specific implementation Time::Location.read_zoneinfo.

This comment has been minimized.

@ysbaddaden

ysbaddaden Apr 9, 2018

Member

I think what makes me wary is that something from Crystal::System is initializing an external namespace; and ::MIME should initialize itself using Crystal::System. I wish we could find a better way.

This comment has been minimized.

@straight-shoota

straight-shoota Apr 9, 2018

Author Member

We could pull the platform-dependent initialization implementations out of Crystal::System into the main namespace. But that doesn't change anything, really.

end

# :nodoc:
def self.load_mime_database(io : IO) : Nil

This comment has been minimized.

@ysbaddaden

ysbaddaden Apr 9, 2018

Member

Either :

  • make it public —so we can load whatever mime.types file we want, which would be quite useful (especially on Windows);
  • make it private —and have Crystal::System::MIME merely tell where the mime.types files are located;
  • move it to Crystal::System::MIME that will return/yield mime types that MIME can register (compatible with Windows' registry, no circular dependency).

This comment has been minimized.

@straight-shoota

straight-shoota Apr 9, 2018

Author Member

Yes, it should be public.

src/mime.cr Outdated
array << extension
end

nil

This comment has been minimized.

@ysbaddaden

ysbaddaden Apr 9, 2018

Member

Redundant noise: the method's signature is already Nil.

@Sija

This comment has been minimized.

Copy link
Contributor

Sija commented Apr 9, 2018

I'm really sick of seeing return nil. I have goosebumps every single time I see these, and I really want to burn them with a f****** flamethrower.

@ysbaddaden wow, you just become my personal holy warrior of code style nit-picking 😲

@straight-shoota

This comment has been minimized.

Copy link
Member Author

straight-shoota commented Apr 26, 2018

@ysbaddaden Are your concerns satisfied?

adamdecaf added a commit to adamdecaf/crystal-image-host that referenced this pull request May 3, 2018

routes/upload: figure out how to serve images
However, browsers prompt for download since they're not coming down
with a relevant mimetype (they have application/octet-stream).

See: crystal-lang/crystal#5765
@straight-shoota

This comment has been minimized.

Copy link
Member Author

straight-shoota commented Jun 5, 2018

Can this be merged?

@RX14

RX14 approved these changes Jun 6, 2018

@RX14 RX14 requested review from ysbaddaden and bcardiff Jun 6, 2018

@bcardiff
Copy link
Member

bcardiff left a comment

initialize_types can be just init if init is guarded with a return if @@initalized. Since it's wrong to call it twice, I think that is a good thing to do.

But is not mandatory.

Also I would've called reset_initialized just reset. Again, not mandatory. 👍on my side.

@straight-shoota

This comment has been minimized.

Copy link
Member Author

straight-shoota commented Nov 8, 2018

It's not actually wrong. init can be called multiple times. I intended to mention that in the docs but forgot it.

But I'll rename reset_initialized.

@straight-shoota

This comment has been minimized.

Copy link
Member Author

straight-shoota commented Nov 8, 2018

@Sija: Named arguments for what?

@Sija

This comment has been minimized.

Copy link
Contributor

Sija commented Nov 8, 2018

@straight-shoota MIME.init(false)

@RX14

RX14 approved these changes Nov 10, 2018

@RX14

This comment has been minimized.

Copy link
Member

RX14 commented Nov 10, 2018

I think a squash + rebase to 2 commits and we're done

@RX14 RX14 added this to the 0.27.1 milestone Nov 10, 2018

src/mime.cr Outdated
".html" => "text/html; charset=utf-8",
".jpg" => "image/jpeg",
".jpeg" => "image/jpeg",
".js" => "application/x-javascript",

This comment has been minimized.

@Sija

Sija Nov 10, 2018

Contributor

Adding ; charset=utf-8 IMO would be a good failsafe (same as in .css).

This comment has been minimized.

@straight-shoota

straight-shoota Nov 10, 2018

Author Member

The charset parameter is only relevant for text/* types.

This comment has been minimized.

@Sija

Sija Nov 10, 2018

Contributor

Nope, see this or this for instance. Btw, official type for javascript atm is application/javascript (without x- prefix).

This comment has been minimized.

@straight-shoota

straight-shoota Nov 11, 2018

Author Member

application/javascript is the official MIME type for JavaScript as per RFC 4329, but application/x-javascript is still considered a de-factor standard.
This list was originally taken from the Go mime package but they've switched to application/javascript since this PR was created and MDN suggest using that as well. So I guess it's fine to change this.

The RFC also specifies a charset parameter for application/javascript, so then it would be okay to use charset=utf8 as well.

straight-shoota added some commits Mar 3, 2018

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/mime branch from adb6608 to e4a128f Nov 11, 2018

@straight-shoota

This comment has been minimized.

Copy link
Member Author

straight-shoota commented Nov 11, 2018

Changed application/x-javascript to application/javascript; charset=utf-8.

Rebased & squashed.

Show resolved Hide resolved src/mime.cr
@RX14

RX14 approved these changes Nov 13, 2018

@RX14 RX14 merged commit 5387248 into crystal-lang:master Nov 13, 2018

4 checks passed

ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@RX14

This comment has been minimized.

Copy link
Member

RX14 commented Nov 13, 2018

🎉

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