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

Fix for issue #726 #1589

Merged
merged 1 commit into from
Mar 10, 2017
Merged

Fix for issue #726 #1589

merged 1 commit into from
Mar 10, 2017

Conversation

inclooder
Copy link
Contributor

This changes should fix issue described in #726.
Default format is used when there is no Content-type.
When Content-type is provided but it's not on the list then the error is thrown.
Normal behavior otherwise.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

This is great, thanks. Just update CHANGELOG please. Feel free to squash commits too or I'll squash merge.

CHANGELOG.md Outdated
@@ -18,6 +18,7 @@
* [#1564](https://github.com/ruby-grape/grape/pull/1564): Fix declared params bug with nested namespaces - [@bmarini](https://github.com/bmarini).
* [#1567](https://github.com/ruby-grape/grape/pull/1567): Fix values validator when value is empty array and apply except to input array - [@jlfaber](https://github.com/jlfaber).
* [#1569](https://github.com/ruby-grape/grape/pull/1569), [#1511](https://github.com/ruby-grape/grape/issues/1511): Upgrade mustermann-grape to 1.0.0 - [@namusyaka](https://github.com/namusyaka).
* [#1589](https://github.com/ruby-grape/grape/pull/1589): Fix for issue #726 - [@inclooder](https://github.com/inclooder).
Copy link
Member

Choose a reason for hiding this comment

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

Describe what the problem is, otherwise users have click on issues in changelog to figure out what the deal is.

…ia format and fallback to default_format only if content-type is not specified.
@inclooder
Copy link
Contributor Author

@dblock Should be OK

@AndrewRayCode
Copy link

How do you get logging around this? Lost a few days trying to track down why bumping grape blocked a pubsub system, and it's because it's coming in as text/plain. Makes sense it errors, however I can't find anything in the logs except a 406 returned from the API, I had to hack into the guts of the gem to figure out it was coming from here. I also installed the grape logging middleware, and tried adding my own, but was unable to get useful logs to show up.

@dblock
Copy link
Member

dblock commented May 2, 2018

The detail is in the HTTP response, but logging the body seems like a bad idea generally, no? Feel free to open a new issue or suggest what we can do here.

@AndrewRayCode
Copy link

Am I correct in my analysis that this changes discards anything set by ?format= in the URL?

https://github.com/ruby-grape/grape/blob/master/lib/grape/middleware/formatter.rb#L120-L127

@dblock
Copy link
Member

dblock commented May 3, 2018

@AndrewRayCode Why do you think that? If anything was dropped it wasn't a spec-ed behavior.

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