-
-
Notifications
You must be signed in to change notification settings - Fork 6
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
feat(fromOpenApi): support mapping and filtering operations #57
base: main
Are you sure you want to change the base?
feat(fromOpenApi): support mapping and filtering operations #57
Conversation
src/open-api/from-open-api.ts
Outdated
@@ -11,6 +11,12 @@ const supportedHttpMethods = Object.keys( | |||
http, | |||
) as unknown as SupportedHttpMethods | |||
|
|||
export type MapOperationFunction = ( | |||
url: string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend using object-as-argument pattern:
export type X = (args: {
url: string,
method: Y,
operation: Z
}) => T
We should've done that in fromTraffic
too, and that's something I'd like to do in the future versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am curious if we want to pass the PathItemObject instead and then let the user return the responses it wants to map?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weyert, I'm not sure whether that's beneficial. We may think more about how to decide which responses from the spec to pick. Right now, the library recommends using a search parameter but that means modifying the system's behavior, which I don't like.
You can use this map function instead... I'm thinking if that has any complications. For once, if you drastically transform the spec, you won't be able to know what is actually the source of truth for the network. It kind of becomes your map function, which is bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to rely on something like response scenarios name/ids. So you cannot modify them, maintaining the source of truth, but can reference/swap per your need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to object argument: c49d3c7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kettanaito Sorry, what do you mean with system's behaviour? Some other mocking solutions allow you to set a HTTP header to define which example of the responses should be returned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weyert, I meant the server you are describing in your OAS document. Basically, my main argument is to support filtering primarily, since the more your runtime modifies your spec, the least useful your spec becomes. If you need to modify an operation in your spec, that's an indicator your spec isn't covering all use cases (otherwise, why would you want to modify it?) I'd rather recommend developers extend their specs to cover the scenarios they want, and have Source provide them with the means to target specific scenarios in test (like we have with triggering particular response examples by the status code search parameter).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true. The idea of this PR is to limit the number of endpoints get mocked. If you have a specification with hundreds of endpoints then you might not need to mock all of them. So I like that the idea of this PR to only mock a subset of the endpoints. As you said I can also imagine that you might want to only mock a particular response example of an endpoint. Both are valid use cases but I agree with you that just remapping endpoints shouldn't be an end goal here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I do agree that it would be nice to cherry-pick certain responses (see #55).
src/open-api/from-open-api.ts
Outdated
} | ||
|
||
const operation = mapOperation | ||
? mapOperation(url, method, rowOperation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is properly referred to as path
, not url
, is it? We may consider renaming it across the implementation, including the new map function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea path is the open api spec concept. Renamed in the mapper function and also other places that touch open api spec:
If I missed url
to path
renaming somewhere else, let me know. I did not touch places such as isAbsoluteUrl
since that one has to do with url as per my understanding and not the open api spec path concept
src/open-api/from-open-api.ts
Outdated
@@ -11,6 +11,12 @@ const supportedHttpMethods = Object.keys( | |||
http, | |||
) as unknown as SupportedHttpMethods | |||
|
|||
export type MapOperationFunction = ( | |||
url: string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's expose the whole parsed document
of the spec as well. That way, developers can access any information anywhere to help them filter or map operations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | ||
const handlers = await fromOpenApi(openApiSpec, mapOperation) | ||
|
||
expect(await inspectHandlers(handlers)).toEqual<InspectedHandler[]>([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to add the request test for /orders
too, since now nothing will fail if the GET /orders
operation gets request handlers generated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm it will fail, no? Since toEqual
will actually check the whole structure of actual vs expected. So if GET /orders
gets generated, the handlers will contain not one handler anymore and the expect will not pass.
I added an extra expect for that to check the handlers length anyway: e3d2391
import { createOpenApiSpec } from '../support/create-open-api-spec.js' | ||
import { InspectedHandler, inspectHandlers } from '../support/inspect.js' | ||
|
||
it('creates handlers based on provided filter', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add another test case for mapping the operation. It may look like this:
- OAS with 1 operation defined.
- In the map function, we modify something about the operation (e.g. its path).
- The rest of the test as usual (request -> assertion).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great test! 45df894
I modified response, since operation itself does not contain path
and that one we are not able to modify in this mapping function
@kettanaito @weyert these new commits improve types for mapper function, so please re-review: Path in the mapper function will now be union of real paths instead of being just string |
Looks good. Not sure why all those |
This looks extremely good, @jauniusmentimeter! ❤️ Thank you so much for your hard work here. I skimmed through the discussions, and most seem to be resolved. Please let me also test this locally sometime this week, hopefully. Would love to get this merged. |
Hi, are there any plans to release this feature in the immediate future? Or are we waiting for it to be documented? If so, I can help you as well |
Add possibility to filter open api spec spec and not mock the whole spec.
The reason for introducing this is that it's quite common case that one has open api spec with lots of endpoint definitions, but for a specific tests we want to mock just specific endpoints. Due to that, it's good to be possible to do filtering on both url and method.