Skip to content

New feature: Acceptable#791

Open
c960657 wants to merge 5 commits intohttprb:mainfrom
c960657:acceptable
Open

New feature: Acceptable#791
c960657 wants to merge 5 commits intohttprb:mainfrom
c960657:acceptable

Conversation

@c960657
Copy link
Contributor

@c960657 c960657 commented Oct 4, 2024

Some servers ignore the Accept header. It usually works fine for static files, but not dynamically generated responses.

This PR adds a new feature which compares the response Content-Type header to the request Accept header and generates a synthetic 406 Not Acceptable response if they don't match.

@ixti
Copy link
Member

ixti commented Nov 22, 2024

IMO in this case it's better raise UnacceptableError. Or even make that logic configurable:

HTTP.use(acceptable: { mime_type: "...", on_mismatch: :raise })
  • mime_type - if request does not have Accept header - it will inject one
  • on_mismatch
    • :raise - fails with UnacceptableError
      • UnacceptableError#response - will contain original response
    • :rewrite_status - rewrites response with new status

@c960657
Copy link
Contributor Author

c960657 commented Feb 6, 2025

Sorry about my late reply. I somehow missed your comments here.

I created this PR (about the Acceptable feature) and PR #792 (about the RaiseError feature) together. My idea was that two could be used together or separately, i.e. you could achieve your suggested mix of exceptions vs. rewrite by combining these two features rather than creating one feature with a lot of options.

This doesn't support a dedicated UnacceptableError exception as you suggest, though RaiseError could easily be extended to raise specified exceptions based on the HTTP status code (e.g. NotFoundError, ForbiddenError, InternalServerErrorError etc).

What do you think of that?

@ixti ixti added this to the v6.0.0 milestone Jun 30, 2025
@sferik sferik force-pushed the main branch 9 times, most recently from fa1fa1f to 2a1e5b3 Compare March 11, 2026 16:01
@sferik sferik modified the milestones: v6.0.0, v6.1.0 Mar 15, 2026
Copy link
Contributor

@sferik sferik left a comment

Choose a reason for hiding this comment

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

I just moved this from the v6.0.0 to the v6.1.0 milestone, but that implies compatibility with the forthcoming v6.0.0 release. Upon second thought, I’m not sure that returning a synthetic 406 Not Acceptable response that could raise an exception would actually be backward-compatible, so I think we need to have this debate now, before the v6.0.0 release.

I’m quite skeptical of generating a synthetic response that differs from the server’s response. In my view, adding this feature would violate several core engineering principles:

  • The Robustness Principle – The server is sending a valid 200 response with usable content and the client is rejecting it.
  • Principle of Least Surprise – Developers expect response.status to reflect what the server actually returned. A 406 that never crossed the wire is misleading, especially for debugging and logging.
  • Information loss – The original status code is discarded. The caller can no longer distinguish between a server that genuinely returned 406 and one that returned 200 with an unexpected content type.
  • Separation of Concerns – Content negotiation enforcement is application-layer logic. An HTTP client’s job is to faithfully transport HTTP messages. Deciding whether the response content type is "acceptable" is a judgment that belongs to the calling application, not the transport layer.
  • RFC 9110 §15.5.7 — 406 is defined as a server response meaning the server found no acceptable representation. Here the server explicitly did provide a representation. The client is fabricating a semantically incorrect status code that misattributes the decision.

For these reasons, I’d vote to close this PR, but I’m happy to be overruled by the other maintainers, @tarcieri and @ixti.

@c960657 Thank you for submitting this patch. I’m sorry it has taken me over a year to review it. I hope you’ve been able to find a suitable workaround for your use case in the mean time.

@sferik sferik mentioned this pull request Mar 15, 2026
@sferik sferik modified the milestones: v6.1.0, v6.0.0 Mar 15, 2026
@tarcieri
Copy link
Member

Closing it seems fine to me

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.

4 participants