hanami / controller

Complete, fast and testable actions for Rack and Hanami
http://hanamirb.org
MIT License
246 stars 111 forks source link

Fix for accepting all content types #411

Closed pat closed 1 year ago

pat commented 1 year ago

I'm in the process of migrating an API within a Hanami app from Roda to hanami-router/hanami-controller. And while I do prefer to be strict about the content received by the API, I also don't entirely trust the third-parties using it will send through correct content types - and I don't want to be breaking their access via their current behaviour.

So, I've set format :all in the relevant action superclass… and my tests break. 😭

From what I can see: when we use :all as a format, we're comparing against the mime-type strings of */* and application/octet-stream. rack-test sends through a default content type of application/x-www-form-urlencoded. But it doesn't get matched against the catch-all */*, because the order of arguments to Rack::Mime.match? actually matters: if you want the wildcard behaviour to apply, the wildcards need to be in the second argument.

Now that I've flipped the arguments around, everything's happy.

All that said, I'm wading into unfamiliar code, so: have I misunderstood things? Should I be testing it in a different way? Should the other uses of Rack::Mime.match? be modified as well? (I feel like they should, but unsure how to write a breaking test to then fix with the switch of arguments)

jodosha commented 1 year ago

@pat

I'm in the process of migrating an API within a Hanami app from Roda to hanami-router/hanami-controller.

❤️ Thank you.

pat commented 1 year ago

Glad to know I got it right 😅 and thank you for merging :) (and all the Hanami work! 👏🏻)

alassek commented 1 year ago

I just ran into this myself, thanks for your work @pat!