add --match-server-url flag to improve openapi server url matching #10
Conversation
It will probably be useful to have the full URL in the HAR type. It's defined as Node's builtin URL class. We will se if this bites us in the future!
This flag allows one to specify how strict the open api server matching should be. None means that it should ignore the host, protocol etc and only match paths. This is useful if e.g. your tests target localhost but you specify a proper url in the open api. Full means that the request has to fully match what is stated in the specification. The logic is somewhat convoluted and can be improved. However, this covers the cases that I tested. We can improve as we further understand how this tool will be used
|
This PR add the --match-server-url flag. For now it has two options: "none", and "full". (It defaults to none) If you set it to none then it ignores protocol, host etc. And only matches on subpaths. If you set it to full, everything has to match. None is useful if you e.g. have tests that target localhost, but your specification only has proper urls. The logic is probably somewhat convoluted since I'm not entirerly sure how this will be used. However a somewhat extensive test suite for the url matching function was written that covers the cases I'm currently interested in. As we understand the tool better we can extend this test suite and then refactor the url matching function! |
dachrillz
changed the title
There was a problem hiding this comment.
Some URL corner case nitpicking below, but generally speaking looks good. However, your formatter seems to be more opinionated than prettier and has performed some whitespace changes. It would be good if you could tell your formatter to behave or include config to make everyone else's formatter to behave.
Also, it is clear that we will at some point introduce an option --spec-source-url to be able to implement the spec:s strategy for making relative URLs absolute by prefixing the URL from whence the OpenAPI spec was supposedly served.
| @@ -1,4 +1,4 @@ | |||
| import { findPath } from "."; | |||
| import { findPathNodeFromPath } from "."; | |||
There was a problem hiding this comment.
You may want to rename the file as well?
| // If the relative server ends with a trailing slash we remove to simplify logic | ||
| if (server.endsWith("/")) { | ||
| serverToMatch = serverToMatch.slice(0, serverToMatch.length - 2); | ||
| } |
There was a problem hiding this comment.
This is a violation of RFC3986. /foo/bar and /foo//bar are not the same URL. The OpenAPI spec explicitly states "he path is appended (no relative URL resolution) to the expanded URL from the Server Object's url field in order to construct the full URL." If we want to address this case, I think it would be better to warn on (or refuse) a spec with a url ending in a "non-path-abempty" path.
| serverToMatch = serverToMatch.slice(0, serverToMatch.length - 2); | ||
| } | ||
|
|
||
| if (pathToMatch.indexOf(serverToMatch) === 0) { |
There was a problem hiding this comment.
Isn't this pathToMatch.startsWith(serverToMatch)? I think that would be easier to read.
| } | ||
|
|
||
| if (pathToMatch.indexOf(serverToMatch) === 0) { | ||
| return pathToMatch.slice(serverToMatch.length, pathToMatch.length); |
There was a problem hiding this comment.
"foo".slice(1, 1) will return "". There seems to be no test case where this happen. Can it happen?
| ); | ||
| }; | ||
|
|
||
| export const removeServer = ( |
There was a problem hiding this comment.
This function is now two different non-overlapping execution paths depending on whether the server url is absolute or not. I think it would be better if the big if statement was made into a separate function.
| @@ -7,17 +7,20 @@ import { Result } from "../rule/model"; | |||
|
|
|||
| export const run = async ( | |||
| openApiPath: string, | |||
| requestResponsePaths: string[] | |||
| requestResponsePaths: string[], | |||
| match_server_url: string | |||
There was a problem hiding this comment.
I think we can save ourselves some work by introducing an options object from the start.
| @@ -12,6 +12,7 @@ export type response = { | |||
|
|
|||
| export type request = { | |||
| method: "post" | "get" | "put" | "patch" | "delete"; | |||
| url: URL; | |||
| path: string; | |||
There was a problem hiding this comment.
It seems odd that we keep the path?
It will probably be useful to have the full URL in the HAR type. It's
defined as Node's builtin URL class. We will se if this bites us in the
future!