Skip to content

Formatting from header acts like Versioning from header #2548

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

Merged
merged 7 commits into from
Mar 29, 2025

Conversation

ericproulx
Copy link
Contributor

@ericproulx ericproulx commented Mar 23, 2025

This PR changes the how the content_type is negotiated in Grape::Middleware::Formatter when looking in the ACCEPT header. Since #2389, Grape::Middleware::Versioner::Header is using Rack::Utils.best_q_match to find it but not Grape::Middleware::Formatter. It seems totally legit that both should act the same way.

This PR also optimize the negotiation regarding the extension found in a path lie /api/v2/test.json. Originally, it was using split without limit the number of occurrences that could be found and since its always looking at the end of string , it felts natural to just find the first dot starting from the end of the string.

Finally, Grape::Middleware::Formatter won't approximate the content-type. This test was expecting an approximation when using a vendor/type form

it 'is set to closest generic for custom vendored/versioned without registered type' do
   _, headers, = subject.call(Rack::PATH_INFO => '/info', Grape::Http::Headers::HTTP_ACCEPT => 'application/vnd.test+json')
   expect(headers[Rack::CONTENT_TYPE]).to eq('application/json')
end

IMO, it didn't make sense since user should use versioning through Grape::Middleware::Versioner in their API. It was made for this.

format_from_extension will use rindex instead of split
Add CHANGELOG.md
@ericproulx ericproulx requested a review from dblock March 23, 2025 15:32
@ericproulx ericproulx marked this pull request as ready for review March 23, 2025 15:32
@dblock
Copy link
Member

dblock commented Mar 23, 2025

Since there's definite change in behavior (even if that behavior was not desired, invalid, etc.) this needs a clear UPGRADING.md. Go through the tests being removed and try to use those as examples?

Add spec for unregistered content-type
@ericproulx
Copy link
Contributor Author

@dblock done

@ericproulx ericproulx requested a review from dblock March 29, 2025 09:52
@dblock dblock merged commit 2b35fed into ruby-grape:master Mar 29, 2025
63 checks passed
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.

2 participants