Add support for Server Variables and relevant tests in Chai OpenAPI Response Validator

[JonnySpruce]

I believe this PR resolves #89, by adding support for server variables. I have added all tests and code for the Chai plugin - the Jest plugin is still TODO at this stage, although now the code is written it should be very easy to port across.

@Nitwel - can you confirm this resolves the issue for you?

[codecov]

Codecov Report

Merging #161 into master will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #161   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           33        33           
  Lines          646       665   +19     
  Branches        47        47           
=========================================
+ Hits           646       665   +19     

Impacted Files	Coverage Δ
.../lib/openapi-validator/lib/classes/OpenApi3Spec.js	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 48eddb4...84b1e18. Read the comment docs.

[rwalle61]

Cheers, let's get the Chai code ready to merge, then port it to Jest.

I'll review this code this week, and it'll be great if Nitwel confirms it functions as requested

[rwalle61]

Thanks very much for this, great to see plenty of test cases. I think we can ignore server variables in the host, and we may need to add one more test case to cover an edge case, but otherwise the suggestions are minor!

[rwalle61]

I don't think a real req.path (e.g. from axios, superagent) contains the host (following this definition: scheme://host:port/path?query). You can confirm this yourself running our differentRequestModules.test.js tests with the debugger.

(Note that in the preceding test blocks, whenever req.path overlaps with a server URL from the OpenAPI spec, only the path overlaps)

This probably means your new replaceServerVarsInURLs function just needs to act on server variables in the path, ignoring those in the host.

(And I guess we need to add a test case with multiple server variables in the path, to make sure the final solution handles more than 1 variable in the path)

[JonnySpruce]

You're absolutely right, about to push changes to the tests which reflect real request structures. I have left the server variables in the spec to ensure that we work as expected when they are included. Agree we only need to change the variables after the paths - is detecting the start of the path as simple as looking for the first single / or are there some edge cases we would also need to be able to detect?

[rwalle61]

you may be able to make use of url.parse e.g. https://github.com/RuntimeTools/OpenAPIValidators/blob/48eddb49bdc5a18af4de01bfb1055d315335ff26/packages/chai-openapi-response-validator/lib/openapi-validator/lib/utils.js#L11

[rwalle61]

Since this PR was opened, we decided to list only what the user wrote in their spec. Here this PR lists all the possible combinations of servers, which may be inconsistent with the decision.

An alternative error message of '/nonExistentEndpointPath' matches servers 'https://test.com/{serverVariable1}' but no <server/endpointPath> combinations may be easier for the user to debug. (I don't think we need to list all the server variables out - they can check their spec).

(btw, master may already output this error message, which would mean no extra implementation for you here)

[JonnySpruce]

I think we might actually want to be a little more specific than this, and it goes hand in hand with the comment you made elsewhere about only changing the server variables in the path.

I.e. in my tests I have a server with a port variable, and a path variable: https://test.com:{portVariable}/{pathVariable}. When I make a request with the path /defaultPathVariable at the moment I would get back:

      '/defaultPathVariable' matches servers [
        'https://test.com:1234/defaultPathVariable',
        'https://test.com:5678/defaultPathVariable'
      ]

Obviously the multiple servers with different port numbers is excessive, however if we changed it to

      '/defaultPathVariable' matches servers [
        'https://test.com:{portVariable}/{pathVariable}'
      ]

Then it's actually too vague, since in reality the request only matches one of the pathVariable enums: defaultPathVariable, not all pathVariable enums.

Instead, if we do the work to only change the server variables in the path, then it should be possible to give the following error message, which I think is the preferred solution:

     '/defaultPathVariable' matches servers [
       'https://test.com:{portVariable}/defaultPathVariable'
     ]

[rwalle61]

well put, I agree!

[rwalle61]

Although the tests prove this function works, ideally I'd prefer a simpler, more intuitive function if possible (esp without recursion).

I'll re-review this function after we've resolved the other comments, to see if we can think of a way to write it so that a newcomer (or I) can understand it easily :)

[JonnySpruce]

Happy to work on this another way, but not exactly sure how else to approach it. If you have any suggestions I'd be happy to take another crack at it.

[JonnySpruce]

I've pushed another commit which resolves the minor comments you had with the code. Happy to also do the work for splitting the host and path parts of the server URL, but want to make sure I'm doing it correctly and not somehow breaking other things by splitting in the wrong place in certain scenarios.

[rwalle61]

More thoughts, having looked at this more closely:

Given:

1. our updated understanding that we don't need to worry about server variables before the path when finding a match for req.path,
2. the default value is required, so in our test openapi.yml we need server variables without enum.
3. also, only the default value is required, so we can simplify the server variables in our test openapi.yml
4. "If the enum is defined, the [default] value SHOULD exist in the enum's values.". The "SHOULD" means that although they recommend that the default value is one of the enum values, it doesn't have to be. I think this will be easy to implement.

This PR already contains most/all of the test cases we need. We could rename our test cases roughly like this to clarify exactly what we are testing:

'res.req.path matches a server with a server variable in the path (matches the default value)'

'res.req.path matches a server with a server variable in the path (matches a non-default value)'

'res.req.path matches a server with multiple server variables in the path'

'res.req.path matches a server with server variables only before the path'

'res.req.path matches a server with server variables before and after the path'

'res.req.path matches a server with server variables in the path, but matches no endpoint paths'

"res.req.path does not match the default server base path ('/') nor any servers"

Our test openapi.yml could be this?

openapi: 3.0.0
info:
  title: Example OpenApi 3 spec
  description: Defines servers using server variables
  version: 0.1.0
servers:
  - url: /{variableInPath}
    description: server url with 1 server variable in the path
    variables:
      variableInPath:
        default: defaultValueForVariableInPath
        enum:
          - enumValueForVariableInPath
  - url: /{firstVariableInPath}/{secondVariableInPath}
    description: server url with multiple server variables in the path
    variables:
      firstVariableInPath:
        default: defaultValueForFirstVariableInPath
      secondVariableInPath:
        default: defaultValueForSecondVariableInPath
  - url: https://{hostVariable}.com:{portVariable}/
    description: server url with server variables only before the path
    variables:
      hostVariable:
        default: defaultValueForHostVariable
      portVariable:
        default: '1234'
  - url: https://{hostVariable}.com:{portVariable}/{variableInDifferentPath}
    description: server url with server variables before and after the path
    variables:
      hostVariable:
        default: defaultValueForHostVariable
      portVariable:
        default: '1234'
      variableInDifferentPath:
        default: defaultValueForVariableInDifferentPath
paths:
  /endpointPath:
    get:
      responses:
        200:
          description: Response body should be a string
          content:
            application/json:
              schema:
                type: string

What do you think?

edit: updated with more thoughts, you can see a PoC branch at https://github.com/RuntimeTools/OpenAPIValidators/compare/89-add-server-variable-support?expand=1

[rwalle61]

Superceded by #222. Thanks @JonnySpruce for your work on this, you'll be credited in the release!