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

Content negotation - "content-type" is being skipped during conversion #361

Closed
thim81 opened this issue Apr 21, 2021 · 12 comments
Closed

Content negotation - "content-type" is being skipped during conversion #361

thim81 opened this issue Apr 21, 2021 · 12 comments

Comments

@thim81
Copy link
Contributor

thim81 commented Apr 21, 2021

In an OpenAPI, users will explicitly define the "content-type", since users can document multiple "content-type"s in the requests to facilitate content negotiation.

Example OpenAPI

  '/portal/v1/accounts/{accountId}':
    parameters:
         in: header
            name: content-type
            required: true
            schema:
              type: string
              example: application/json
              default: application/json
            description: Define the file type and format for the response object.

But since the latest release, the content-types gets stripped out, because of this addition in the schemaUtils.js

 // adding headers to request from reqParam
    _.forEach(reqParams.header, (header) => {
      if (!_.includes(IMPLICIT_HEADERS, _.toLower(_.get(header, 'name')))) {
        item.request.addHeader(this.convertToPmHeader(header, REQUEST_TYPE.ROOT, PARAMETER_SOURCE.REQUEST,
          components, options, schemaCache));
      }
    });

where IMPLICIT_HEADERS refers to:

// These headers are to be validated explicitly
  // As these are not defined under usual parameters object and need special handling
  IMPLICIT_HEADERS = [
    'content-type', // 'content-type' is defined based on content/media-type of req/res body,
    'accept',
    'authorization'
  ],

Can you give some insights why you want to strip out any explicitly defined content-types during the conversion from OpenAPI to Postman? Since by stripping them out, requests made to an API that explicitly validates them start to report errors.

thim81 pushed a commit to thim81/openapi-to-postman that referenced this issue Apr 21, 2021
@VShingala
Copy link
Member

@thim81 As defined in OpenAPI specification here, these headers should be ignored as part of the parameter definition. Currently Content-type header is generated from what's mentioned under the request body (i.e. media type of request body content).

@thim81
Copy link
Contributor Author

thim81 commented Apr 28, 2021

@VShingala The problem lies with the requests without any body, since then there is no content-type header set in Postman, which for APIs with content negotiation is required, otherwise they return HTML. This is why we define the content type explicitly in the openapi spec, via an Enum with a default set to application/json

thim81 pushed a commit to thim81/openapi-to-postman that referenced this issue Apr 28, 2021
Patch related to the stripping of "content-type" defined in OpenAPI postmanlabs#361
@VShingala
Copy link
Member

VShingala commented Apr 28, 2021

@thim81 Got it and it makes sense, Thanks for pointing it out. So would you say adding content-type header is good idea for only operations with no content? Cause previously there were duplications and same content-type header was present twice, one from header and one coming from content of request body.

Also, this change was initiated because OpenAPI specification specifically added to ignore such definition. But what you pointed out makes sense. Is there any reference that states this?

Alternatively, you can just add Content-type header at the collection level and that would work for your use case.

@thim81
Copy link
Contributor Author

thim81 commented Apr 28, 2021

I would make the OpenAPI document leading, during the generation

  1. If the openApi has a content-type header specified, set this as a header on the Postman request (skip the request body detection)
  2. If there is no content-type header specified, keep the existing behaviour by using the content-type detection of the request body

with regards to: https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.3.md#fixed-fields-10
If in is "header" and the name field is "Accept", "Content-Type" or "Authorization", the parameter definition SHALL be ignored.

It is not clear what they mean with "SHALL be ignored"? Ignore for what? For display in documentation or for SDK generation?

@VShingala
Copy link
Member

with regards to: https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.3.md#fixed-fields-10
If in is "header" and the name field is "Accept", "Content-Type" or "Authorization", the parameter definition SHALL be ignored.

It is not clear what they mean with "SHALL be ignored"? Ignore for what? For display in documentation or for SDK generation?

I think what they mean by that is corresponding parameter definition should be discarded entirely. Meaning we can ignore it as a parameter.

@thim81
Copy link
Contributor Author

thim81 commented Jun 29, 2021

@VShingala since they are now skipped, means that we have manually re-add the required "Content-Type" and "Authorization" headers. Is it not possible to keep these headers based on a setting?

I created a PR #383 to have the option to manage the implicit headers

@VShingala
Copy link
Member

@thim81 Thanks for the contribution! I Will take a look at PR later on.

@thim81
Copy link
Contributor Author

thim81 commented Jul 22, 2021

@VShingala Did you had some more comments or remarks on the PR #383 ?

@VShingala
Copy link
Member

Hey @thim81, there are a couple of more changes that might be needed from validateTransaction() so validation also works. I am working on it but may take up to a week due to other issues.

@thim81
Copy link
Contributor Author

thim81 commented Aug 17, 2021

@VShingala Did you had some time to look into the PR? And did you have any remarks or comments, I should work on?

@thim81
Copy link
Contributor Author

thim81 commented Sep 28, 2021

Closing this item, since it solved in the 2.11.0 release, which includes the PR #383

@thim81 thim81 closed this as completed Sep 28, 2021
@thim81
Copy link
Contributor Author

thim81 commented Sep 28, 2021

Thanks @VShingala for the review and all the tips to make this land in the release 🙇

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

No branches or pull requests

2 participants