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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PHP] handle properly multiple accept headers #13844

Merged
merged 2 commits into from Oct 31, 2022

Conversation

thomasphansen
Copy link
Contributor

@thomasphansen thomasphansen commented Oct 27, 2022

This fixes #12926 and addresses #728 for the specific PHP client.

When handling Accept headers, the current behavior of the php OpenAPIGenerator is:

  • If there is none in the api description, don't send one.
  • If one of them matches a "json-like" RE, send only "application/json" and ignore the others
  • otherwise, concatenate all them and send it.

The first and third behaviors are fine, but the second one has some issues:

  1. if the API description includes parameters for the "json-like" Accept header, they will be simply disregarded. This can be a problem if those parameters are mandatory for that API.
  2. scenarios like the one described in [BUG][PHP] Accept headers are discarding content types in favour of application/json聽#12926 cannot be solved without using a middleware in the http client, to change the Accept header just before sending the request. This is not a good experience for the user, IMO, since the proper Accept headers are already described in the OpenAPI description - we should just use them properly.

In order to solve this, both mentioned issues suggest using Quality Values (which I'll abbreviate as QV) to define the priority of the Accept headers. QV's are described in IETF RFC 9110, item 12.4.2, and their usage is described in the same RFC, item 12.5.1. Link to the RFC: https://www.rfc-editor.org/rfc/rfc9110.html#section-12.4.2

This PR implements a solution for a more convenient way of handling the Accept headers, using the following rules:

  1. if there is no Accept header, send none (same as before)
  2. if there is only one Accept header, send it unchanged
  3. if there are multiple Accept headers, but none are "json-like", send them all (same as before)
  4. otherwise, use QV's (weights) to give the highest priority to "application/json"-like headers, followed by other "json-like" headers, followed by all other headers.

This last rule can be a bit tricky, especially if the given Accept headers already have QV's. To handle this properly, this PR recalculates all quality codes, by splitting the headers in three categories ('application/json'-like, 'json'-like, 'non-json'-like), ordering each one by QV's and then attributing new QV's for all them, respecting the categories order (json-related first) and the internal order for each category.

But calculating the new values for the QV's is also a bit tricky: they vary from 0.001 to 1 (not having a QV implicitly means a value of 1), and most of the time get values like "q=0.9", "q=0.8" and so on. To create a sequence that emulates a "human" usage (and avoid values like "q=0.999", "q=0.998" etc), I used a logarithmic expression that generates a series like 1000, 900, 800, ... 100, 90, 80, ... 10, 9, 8 ... 2, 1, resulting in QV's like "q=1", "q=0.9", "q=0.8", ... "q=0.1", "q=0.09", "q=0.08" etc.

Since the series is limited, it will work perfectly for up to 28 headers with QV's - for more than that (which is extremely unlikely to happen, I'd say), the QV's will fallback to the poor "q=0.999"-like behavior - which is still valid according to the RFC.

More details can be found in the comments, directly in the code. New tests are also present, both for the headers selection and for the QV's generation.

Hope you guys like it! 馃槃

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master (6.1.0) (minor release - breaking changes with fallbacks), 7.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the [technical committee]
    @jebentier, @dkarlovi, @mandrean, @jfastnacht, @ybelenko, @renepardon

@wing328
Copy link
Member

wing328 commented Oct 31, 2022

@wing328 wing328 merged commit d6de9c1 into OpenAPITools:master Oct 31, 2022
@Pittiplatsch
Copy link

@thomasphansen Thanks for fixing 馃憤

@thomasphansen thomasphansen deleted the th_php_fix_accept_header branch November 7, 2022 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG][PHP] Accept headers are discarding content types in favour of application/json
3 participants