Skip to content
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

FileMiddleware will provide correct mime type for non lowercase file extensions #661

Merged

Conversation

mredig
Copy link
Contributor

@mredig mredig commented Jan 18, 2025

I added a test for FileMiddleware to test handling file extensions that aren't homogeneous lowercase and it failed.

I then updated FileMiddlware to normalize as lowercase the file extensions it extracts from filenames (to match the existing convention on MediaType) prior to passing them to MediaType.getMediaType(forExtension:).

The test passes now.

@mredig
Copy link
Contributor Author

mredig commented Jan 25, 2025

Is it worth adding a custom ExpressibleByDictionaryLiteral type for this dictionary use case? That way we can represent it in, what I believe, would be a clearer API surface. ref

I have the beginnings of this specific idea right here:

extension MediaType {
    public struct WellknownExtensions: RawRepresentable, Sendable, ExpressibleByDictionaryLiteral {
        public private(set) var rawValue: [FileExtension: MediaType]

        public init(rawValue: [MediaType.FileExtension : MediaType]) {
            self.rawValue = rawValue
        }

        public init(dictionaryLiteral elements: (FileExtension, MediaType)...) {
            self.init(rawValue: elements.reduce(into: .init(), {
                $0[$1.0] = $1.1
            }))
        }
    }
}

but it occurred to me that between the changes I already made by adding FileExtension and making it case-insensitive equatable, ExpressibleByStringLiteral and then naturally being able to use the existing interface with a dictionary literal argument, I can't think of any value this would bring.

I personally think that using a type safe wrapper for file extensions is also a cleaner interface for case insensitive comparison rather than something like WellknownExtensions.compare("jpg", "JPG")

Should I continue forward with this, or do you agree what I already have is better?

@mredig mredig requested a review from Joannis January 25, 2025 07:09
@mredig
Copy link
Contributor Author

mredig commented Jan 25, 2025

Re - the api breaking changes...

I wasn't aware that the last PR had been released yet, so I wasn't concerning myself with thinking about that, but I can make sure there are String args complementing the FileExtension ones, if we want to proceed with this.

@adam-fowler
Copy link
Member

@mredig ah that was my bad.

@mredig mredig force-pushed the case-insensitive-file-extension branch from 4927a94 to 3ff663d Compare January 27, 2025 02:00
@mredig
Copy link
Contributor Author

mredig commented Jan 27, 2025

I am considering this currently as unapproved since I made significant changes since the approval, however unless you guys see any shortcomings, I am considering it complete and ready for real PR approval.

I just want to make sure that there's not a limbo of me waiting for you while you are waiting for something from me :)

@mredig
Copy link
Contributor Author

mredig commented Jan 27, 2025

(the test failures are from certificates that expired hours prior to the tests running - in the Tests/HummingbirdHTTP2Tests/Certificates.swift file.)

Copy link
Member

@adam-fowler adam-fowler left a comment

Choose a reason for hiding this comment

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

Couple of minor things, but in general looks good. I'm not sure it is necessary to include all the extensions as static members of MediaType.FileExtension but I'm not opposed either.

Note #665 includes updated certificates, so tests should work once that is merged

.mailmap Show resolved Hide resolved
Tests/HummingbirdTests/FileMiddlewareTests.swift Outdated Show resolved Hide resolved
@adam-fowler
Copy link
Member

thanks Michael

@adam-fowler adam-fowler merged commit a426cda into hummingbird-project:main Jan 29, 2025
7 of 8 checks passed
bcbod2002 pushed a commit to bcbod2002/hummingbird that referenced this pull request Feb 6, 2025
…extensions (hummingbird-project#661)

* FileMiddleware will provide correct mime type for non lowercase file extensions

* update mailmap

* fix url init for swift 5.x on linux

* revert file extension lowercase in file middleware

* create MediaType.WellknownExtension for type safety and comparison of file extensions

* added test case for customized mime type/extension case insensitivity

* rename to FileExtension

* maintain existing api

* refactor reduce into merging for media type file extensions

* update contributors

* update test to only start server once, then loop assertions

* doc comments for the MediaType.FileExtension type

---------

Co-authored-by: Adam Fowler <[email protected]>
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.

3 participants