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

Issue in findBestHttpContentByMediaType due to inconsistent behaviour between accepts and type-is libraries #1152

Closed
mpretty-cyro opened this issue May 5, 2020 · 11 comments · Fixed by #1159
Labels

Comments

@mpretty-cyro
Copy link

Describe the bug
Our API provides versioning information in the Accept header with this structure application/vnd.archa.api+json; version=1 but trying to use this structure as the content type seems to result in one of two errors depending on how it's specified:

If we specify a Content-Type of application/vnd.archa.api+json; version=1 we get an error saying Violation: response The received media type "application/vnd.archa.api+json; version=1" does not match the one specified in the current response: application/vnd.archa.api+json; version=1 (though the request returns).

Or, if we specify a Content-Type of application/vnd.archa.api+json we get an error saying Request terminated with error: https://stoplight.io/prism/errors#NOT_ACCEPTABLE: The server cannot produce a representation for your accept header.

  • This seems to be because the "accepts" library seems to do an exact comparison of the media type

To Reproduce

  1. Given this OpenAPI document
openapi: 3.0.0
info:
  title: httpbin
  version: '1.0'
paths:
  /get:
    get:
      parameters:
        in: header
        name: Accept
        required: true
        schema:
          type: string
        example: application/vnd.archa.api+json; version=1
      responses:
        '200':
          description: Created
          content:
            application/vnd.archa.api+json:
              schema:
                required:
                  - value
                properties:
                  value:
                    type: string
                    example: "hello"
  1. Run this CLI command
    prism mock api.yaml

  2. See error
    After sending a Get request with the correct Accept header (see below) you receive the Request terminated with error: https://stoplight.io/prism/errors#NOT_ACCEPTABLE: The server cannot produce a representation for your accept header error.

curl --location --request GET 'http://127.0.0.1:8001/get' \
--header 'Accept: application/vnd.archa.api+json; version=1'

Expected behavior
Providing an Accept header of application/vnd.archa.api+json; version=1 should find a response with a Content-Type of application/vnd.archa.api+json.

Environment (remove any that are not applicable):

  • Library version: 3.3.4
  • OS: OSX 10.15.2

Question
Which is the expected Content-Type in this case? (application/vnd.archa.api+json; version=1 or application/vnd.archa.api+json)


Potential Fix 1
I've done something wrong and change my spec 😛

Potential Fix 2
One work-around I've found is to update the findBestHttpContentByMediaType method to use the "type-is" library, this way it matches the application/vnd.archa.api+json; version=1 Accept header to the application/vnd.archa.api+json Content-Type.

function findBestHttpContentByMediaType(response, mediaType) {
    const req = {
        headers: {
            ['content-type']: mediaType.join(',')
        }
    };
    return pipeable_1.pipe(response.contents, Array_1.findFirst(content => typeIs.is(req, [content.mediaType])));
}

Potential Fix 3
An option which seems pretty hacky to me would be to remove anything after a ; in the media type so we can continue to use the "accepts" library.

function findBestHttpContentByMediaType(response, mediaType) {
    const bestType = accepts({
        headers: {
            accept: mediaType.map(m => m.split(";")[0]).join(','),
        },
    }).type(response.contents.map(c => c.mediaType));
    
    return pipeable_1.pipe(response.contents, Array_1.findFirst(content => content.mediaType === bestType));
}
@mpretty-cyro
Copy link
Author

TS code for Potential Fix 2 (which we are using internally) is:

export function findBestHttpContentByMediaType(
  response: PickRequired<IHttpOperationResponse, 'contents'>,
  mediaType: string[]
): Option<IMediaTypeContent> {
    return pipe(
      response.contents,
      findFirst(content => typeIs.is(mediaType.join(','), [content.mediaType]) !== false)
    );
}

@XVincentX
Copy link
Contributor

Hey @mpretty-cyro

thanks a lot for the super-detailed report. I'm going to have a look into this but we can probably go with the fix 2 you have proposed.

We've migrated most of the parts to use type-is, but as you can see we might have forgot some parts.

@mpretty-cyro
Copy link
Author

@XVincentX No worries, sorry I'd submit a PR myself but am pretty slammed at work at the moment. Just to note with the fix 2 - I ran into some problems with empty responses and when debugging found that a change to the findEmptyResponse in the NegotiatorHelper fixed it (changing findIndex(ct => ct.includes('*/*')) to findIndex(ct => !ct.includes('*/*')) - I'm not too familiar with the fp-ts library so can't read the behaviour well but it looks like it had been previously changed from a !includes in 824c6c5)

@XVincentX
Copy link
Contributor

XVincentX commented May 8, 2020

Hey,

I've checked your alternative (and other combinations) that won't solve the issue. The accept package is kind of fundamental because it accounts also for the quality priorities that's key factor for us, and it's the one unfortunately returning false, blowing the negotiation process.

Kind of the same with the type-is (although it seems like there's a reasonable workaround we can use: jshttp/type-is#44 (comment)

This seems to be because the "accepts" library seems to do an exact comparison of the media type

To me the behaviour looks correct. The accept header has no version parameter; so that's why is not returning anything. Let's say you have version:1version:2;version:3.

What would no version mean? Version 1? Latest version? I am going to check out the RFC to see if there's a standard behaviour about that but if not, I would say the accept library (and thous Prism) is behaving correctly since there's no way to know which version you'd like to return

@dougwilson
Copy link

I tried reading through this issue, and AFAIK accepts works just fine with what is in the OP, at least my reading of it. Perhaps someone can distill this down into a reproduction case that uses accepts directly I can copy and paste and run to see what the accepts issue is?

@XVincentX
Copy link
Contributor

XVincentX commented May 8, 2020

@dougwilson I'm sorry I have pinged you (it was not my intention).

I've just clarified my comment, I also agree that the library is behaving correctly. Here's the excerpt:

To me the behaviour looks correct. The accept header has no version parameter; so that's why is not returning anything. Let's say you have version:1version:2;version:3.

What would no version mean? Version 1? Latest version? I am going to check out the RFC to see if there's a standard behaviour about that but if not, I would say the accept library (and thous Prism) is behaving correctly since there's no way to know which version you'd like to return

@XVincentX
Copy link
Contributor

@mpretty-cyro I've verified and effectively the only "kind of standardised" parameter is the quality. There's nothing about version and so we can't really assume anything about it.

@mpretty-cyro
Copy link
Author

@mpretty-cyro I've verified and effectively the only "kind of standardised" parameter is the quality. There's nothing about version and so we can't really assume anything about it.

@XVincentX So I'm less concerned about be able to version responses (while that would be nice I accept that it's not really part of the standard); the main issue I was having is that I would have expected the accepts library to just ignore the version part of the Accept header and match on the Content-Type of application/vnd.archa.api+json but it can't find a match.

@XVincentX
Copy link
Contributor

@mpretty-cyro Can you try the branch linked to the PR #1159 ? I should have been able to overcome both the limitations there.

@mpretty-cyro
Copy link
Author

@XVincentX Sorry can't seem to get the branch building correctly - I tried updating the code in the InternalHelper and NegotiatorHelper to match the changes you have and it seems to correctly find the response now but I'm getting exceptions before the data gets returned (likely because I've missed some changes from the branch).

@XVincentX
Copy link
Contributor

That's weird @mpretty-cyro, what problems are you facing when trying the branch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants