search

pusher / pusher-http-ruby

Ruby library for Pusher Channels HTTP API
https://pusher.com/channels
MIT License
664 stars 124 forks source link

uses media_type for MIME type in webhooks #181

Closed yoLotus closed 1 year ago

yoLotus commented 2 years ago

Description

As describe in this issue #179, Rails displays a warning when reading the MIME type with content_type from the request object. I suggest to prioritize the media_type when existing.

CHANGELOG

fbenevides commented 2 years ago

Hi @yoLotus, thanks for the contribution.

It still needs a spec to cover both scenarios. Could you please add it, so we can merge it?

yoLotus commented 2 years ago

Hello @fbenevides , sorry for the late response and thanks you a lot for having taken a look.

I'm really not sure of my solution for the test, wondering if my mock it's acceptable or not.

It seems that ActionDispatch::Request use the Rack::Request under the hood so I use a delegator to be as close as possible of the Rails object. What do you think?

EDIT: the deprecation is here

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you'd like this issue to stay open please leave a comment indicating how this issue is affecting you. Thank you.

sonologico commented 1 year ago

I think we could simply replace content_type with media_type. media_type has been available for enough time that I don't think we need a fallback to content_type.

r7kamura commented 1 year ago

I too have encountered this problem and would be happy if this Pull Request is merged as soon as possible.

The reason for this Pull Request and the related Issue is to address the ActionDispatch::Request#content_type warning, but that warning is not the only problem.

In Rails apps that have been rails new since Rails 7.0 or have switched to load_defaults 7.0, the behavior of ActionDispatch::Request#content_type has already changed and it will return a value like "application/json; charset=utf-8", so the following case-when condition no longer works as intended:

https://github.com/pusher/pusher-http-ruby/blob/18109ec781501c673fb1853869cf89f8ed27296d/lib/pusher/webhook.rb#L86-L91

This is also a problem if you are simply using Rack, not Rails, because it is also not working as intended since Rack::Request#content_type will return a value like "Content-Type: application/json; charset=utf-8" as well.

I think it would also be better to have a test case for Content-Type: application/json; charset=utf-8, which would fail before this change.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you'd like this issue to stay open please leave a comment indicating how this issue is affecting you. Thank you.