Skip to content

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
ericproulx added a commit to ericproulx/grape that referenced this pull request Jun 22, 2025
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