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

Callbacks #716

Merged
merged 43 commits into from
Nov 6, 2019
Merged

Callbacks #716

merged 43 commits into from
Nov 6, 2019

Conversation

karol-maciaszek
Copy link
Contributor

@karol-maciaszek karol-maciaszek commented Oct 18, 2019

Closes #331

Checklist

  • Tests have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce?

feature

What is the current behavior? What is the new behavior?

This PR introduces Callback support in Prism (issue #331).

Screen Shot 2019-11-06 at 15 55 28

Does this PR introduce a breaking change?

No

Dependencies (needed to be merged first)

  1. feat: callbacks support types#54
  2. feat: oas3 callbacks support http-spec#31

Copy link
Contributor

@XVincentX XVincentX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a quick look into. Good start :)

I've explored a little bit the possibility of using an ABNF interpreter but effectively it's not worth the hassle. We'll keep the source code specific.

packages/http/src/util/runtimeExpression.ts Outdated Show resolved Hide resolved
packages/http/src/util/runtimeExpression.ts Outdated Show resolved Hide resolved
packages/http/src/util/runtimeExpression.ts Outdated Show resolved Hide resolved
packages/http/src/util/runtimeExpression.ts Outdated Show resolved Hide resolved
packages/http/src/util/runtimeExpression.ts Outdated Show resolved Hide resolved
@karol-maciaszek
Copy link
Contributor Author

Had a quick look into. Good start :)

I've explored a little bit the possibility of using an ABNF interpreter but effectively it's not worth the hassle. We'll keep the source code specific.

Yep, I've been also looking into some more automatic way of interpreting the syntax. Especially that the ABNF syntax was given. However, I ended feeling that it is too much to carry the whole engine just for resolving such simple syntax.

@XVincentX
Copy link
Contributor

However, I ended feeling that it is too much to carry the whole engine just for resolving such simple syntax.

Yeah that's totally legit. I'm jumping on this right now!

Copy link
Contributor

@XVincentX XVincentX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good stuff. The general direction is great and the implementation is basically almost the same I'd have written by myself.

I've dropped a first pass of comments but I had to stop because of TypeScript complaining about the missing updated packages that we need to sort out.

Very good stuff @StefanDywersant.

packages/http/src/forwarder/index.ts Show resolved Hide resolved
packages/http/src/forwarder/index.ts Outdated Show resolved Hide resolved
packages/http/src/util/response.ts Outdated Show resolved Hide resolved
packages/http/src/util/response.ts Outdated Show resolved Hide resolved
packages/http/src/util/__tests__/response.spec.ts Outdated Show resolved Hide resolved
packages/http/src/mocker/index.ts Outdated Show resolved Hide resolved
packages/http/src/mocker/index.ts Outdated Show resolved Hide resolved
packages/http/src/mocker/callback/callbacks.ts Outdated Show resolved Hide resolved
packages/http/src/mocker/callback/callbacks.ts Outdated Show resolved Hide resolved
packages/http/src/mocker/callback/callbacks.ts Outdated Show resolved Hide resolved
@karol-maciaszek karol-maciaszek changed the title WebHooks Callbacks Oct 31, 2019
Copy link
Contributor

@XVincentX XVincentX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropped another bunch of comments!

examples/callbacks/README.md Outdated Show resolved Hide resolved
packages/http/src/mocker/callback/callbacks.ts Outdated Show resolved Hide resolved
examples/callbacks/README.md Outdated Show resolved Hide resolved
examples/callbacks/README.md Outdated Show resolved Hide resolved
examples/callbacks/README.md Outdated Show resolved Hide resolved
packages/http/src/utils/__tests__/logger.spec.ts Outdated Show resolved Hide resolved
packages/http/src/utils/__tests__/parseResponse.spec.ts Outdated Show resolved Hide resolved
packages/http/src/utils/__tests__/parseResponse.spec.ts Outdated Show resolved Hide resolved
packages/http/src/utils/parseResponse.ts Outdated Show resolved Hide resolved
packages/http/src/utils/runtimeExpression.ts Show resolved Hide resolved
Copy link
Contributor

@XVincentX XVincentX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are other changes to do but it's mostly about code organisation and stuff — the feature looks good to me. I'm now going to pull the branch down and test the hooks in general.

@philsturgeon I think it's time for you as well to test down this and see if this is working in the way you want.

Very good job @StefanDywersant — we're about to ship a very good and unique feature in the mock servers space.

I've also marked this as ready to review since I've reviewed it at least 3 times :trollface:

docs/guides/callbacks.md Show resolved Hide resolved
packages/http/src/mocker/callback/callbacks.ts Outdated Show resolved Hide resolved
packages/http/src/mocker/callback/callbacks.ts Outdated Show resolved Hide resolved
packages/http/src/mocker/index.ts Outdated Show resolved Hide resolved
packages/http/src/mocker/callback/callbacks.ts Outdated Show resolved Hide resolved
packages/http/src/mocker/generator/HttpParamGenerator.ts Outdated Show resolved Hide resolved
packages/http/src/utils/runtimeExpression.ts Show resolved Hide resolved
docs/guides/callbacks.md Show resolved Hide resolved
@XVincentX XVincentX marked this pull request as ready for review November 6, 2019 00:13
XVincentX and others added 3 commits November 6, 2019 10:23
@philsturgeon
Copy link
Contributor

This is so cool, amazing work @StefanDywersant.

One thing could do with a bit of polish: I was using https://webhook.site/ which is a cool request bin type thing which captures webhooks for testing. Apparently their API is not responding quite how Prism would like:

Screen Shot 2019-11-06 at 15 55 28

I guess it's because the callback has rather specific requirements for the callback:

      callbacks:
        actions:
          '{$request.body#/url}?token={$request.body#/token}':
            post:
              summary: 'Receive notification about certain invoice'
              requestBody:
                required: true
                content:
                  application/json:
                    schema:
                      $ref: '#/components/schemas/notification'
              responses:
                200:
                  description: 'Notification successfully processed'
                  content:
                    text/plain:
                      examples:
                        ok:
                          value: 'ok'

Here is what it responded with:

HTTP/1.1 200 OK
Cache-Control: no-cache, private
Content-Encoding: gzip
Content-Type: text/plain; charset=UTF-8
Date: Wed, 06 Nov 2019 15:02:21 GMT
Server: nginx/1.10.3
Transfer-Encoding: chunked
Vary: Accept-Encoding

Anyhow, the error message it gives is not as descriptive as it could be.

[VALIDATOR] ✖ error Violation: The received media type does not match the one specified in the document

Maaaybe that should actually be displayed as [CALLBACK]? Or if not, a super clear message:

Violation: The description document expects text/plan or application/json but instead it got application/nonsense

Beyond the error message, I suppose if somebody wanted to avoid this problem they would simply not specify a content right?

So instead of:

              responses:
                200:
                  description: 'Notification successfully processed'
                  content:
                    text/plain:
                      examples:
                        ok:
                          value: 'ok'

it would be:

              responses:
                200:
                  description: 'Notification successfully processed'

@karol-maciaszek
Copy link
Contributor Author

Beyond the error message, I suppose if somebody wanted to avoid this problem they would simply not specify a content right?

If you do not specify the content prism will also report an error that it couldn't match. Basically callbacks validation acts exactly the same as normal request validation.

That points to a possible problem which I also noticed doing more or less the same as you are doing with webhook.site: when contents are not defined, shouldn't prism accept whatever mediaType of response?

@philsturgeon @XVincentX

@XVincentX
Copy link
Contributor

@StefanDywersant You're correct, there might be a little bug in the mismatchMediaType function — which checks for something even though there's nothing to look for.

Let's keep this separate though. I'll investigate into this — in meantime I would just change the error message from a generic thing to something more like "I received THIS and instead this is what's available in the document"

@philsturgeon
Copy link
Contributor

@StefanDywersant ahh well this just shows that my next job of "play with proxy server and contract testing to make sure it works as expected" should have been done sooner. My bad.

If the API does not specify content, then anything which comes in is a bonus, maybe a warning or an info, but certainly not an error. It's the sort of thing we'd report to folks via Studio somehow (long way off) saying "Hey we found some content, looks like this is JSON, here is the schema. Wanna save that to your description?" etc.

How do I make a callback which says "I dont care what they respond with im just looking for a 200? A lot of callbacks work this way, they just want a "Yep!" and nothing more.

@karol-maciaszek
Copy link
Contributor Author

How do I make a callback which says "I dont care what they respond with im just looking for a 200? A lot of callbacks work this way, they just want a "Yep!" and nothing more.

@XVincentX just submitted an issue for that #774

@philsturgeon
Copy link
Contributor

@StefanDywersant sorry yeah, I didnt see his comment on this crap ferry wifi. Our comments passed each other like ships in the night. If you do the thing he suggested I've got a thumb ready and waiting!

Copy link
Contributor

@XVincentX XVincentX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@karol-maciaszek
Copy link
Contributor Author

@StefanDywersant sorry yeah, I didnt see his comment on this crap ferry wifi. Our comments passed each other like ships in the night. If you do the thing he suggested I've got a thumb ready and waiting!

Done in a22024a

@XVincentX XVincentX merged commit 6ecb3ee into master Nov 6, 2019
@XVincentX XVincentX deleted the feat/webhooks branch November 6, 2019 16:06
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

Successfully merging this pull request may close these issues.

OpenAPI v3 Callbacks
4 participants