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.

Sign up for GitHub

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

Jump to bottom

Improved callback examples #2048

Merged
merged 2 commits into from
Dec 8, 2019
Merged

Improved callback examples #2048

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
77e0e39
Improved callback examples
Oct 30, 2019
6f23ae5
Update versions/3.0.3.md
Nov 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 21 additions & 5 deletions versions/3.0.3.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1891,13 +1891,13 @@ $request.body#/successUrls/2 | http://clientdomain.com/medium
$response.header.Location | http://example.org/subscription/1


##### Callback Object Example
##### Callback Object Examples

The following example shows a callback to the URL specified by the `id` and `email` property in the request body.
The following example uses the user provided `queryUrl` query string parameter to define the callback URL. This is an example of how to use a callback object to describe a WebHook callback that goes with the subscription operation to enable registering for the WebHook.

```yaml
myWebhook:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

here we were conflating callbacks and webhooks, which could be confusing with Webhooks getting their own proposal: #1974.

Copy link
Member

Choose a reason for hiding this comment

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

Hence, why I am not a fan of calling the new thing WebHooks. Callbacks were designed to support Webhooks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's save discussions for naming of other things in the PRs for other things. This is callbacks and we should talk about callbacks and whether callbacks support webhooks nicely or not, we can be a bit less confusing here by sticking to the term callback consistently.

'http://notificationServer.com?transactionId={$request.body#/id}&email={$request.body#/email}':
myCallback:
'{$request.query.queryUrl}':
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Common use case, in fact this one was used above but the example was dropped on the floor for another less common example. Let's have both!

Copy link
Member

Choose a reason for hiding this comment

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

Are you suggesting that for something to be called a webhook the consumer needs to provide the entire URL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR exists solely because the Callbacks section is so confusing to me and the team that we struggled to build tooling around the callback feature. We got there in the end stoplightio/prism#716 but it could have been easier. This PR makes it easier.

I am not talking about webhooks here at all. Note that the only mention of webhook in the PR is in red, not in green. This PR is about examples, and one example is using an entire URL, another example is using parameters to construct a URL.

This example was elluded to earlier in the doc then just dropped on the floor. I'm bringing it back here to visualize what that earlier example might look like.

Copy link
Member

Choose a reason for hiding this comment

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

Our original intent was that Callback is a generic term that includes WebHooks as a type of CallBack. I am concerned about trying to make a distinction between Callbacks and Webhooks. Having an API to subscribe to a webhook should be a normal scenario.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hear you but this PR does not make a distinction between callbacks and webooks. This seems like a conversation that should be had with the webook proposal, not on this PR, which is just improving examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want the specification to have two distinct concepts of Callbacks and Webhooks

That is why I removed the awkward mention of Webhooks that was just slid into an example without any explanation. I am improving the situation you are describing in this comment!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not introducing any new meaning for Webhooks. That is being done in an already merged-to-master proposal which has nothing to do with this change.

Copy link
Member

Choose a reason for hiding this comment

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

The use of WebHooks in the callback example was intentional as a way of showing that you can model WebHooks with callback objects. I would do the same in examples of the Event Object to show you can model WebHooks that are registered out of band with an Event Object.

If I add text to explain that Callback Objects can model WebHooks, are you ok putting the WebHook example back in?

Copy link
Member

Choose a reason for hiding this comment

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

And you don't need to remind me that Lorna's proposal is "merged-to-master" as some badge of approval. I'm not falling for that :-P I'm still going to make the case for the rename to Events.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's not the intention of the reference, I am explaining that I am not giving Webhooks any new meaning, only that proposal is giving Webhooks new meaning. I am removing confusion not adding it, but you keep saying I'm adding confusion, and that is confusing me.

If you add a thing introducing that you can use callbacks for webooks then great, I'll take that, even if it needs to be resolved later on through some possible future movement on the webhooks proposal. That's not relevant to this PR so we can cross that bridge if we get to it I guess.

post:
requestBody:
description: Callback payload
Expand All @@ -1907,9 +1907,25 @@ myWebhook:
$ref: '#/components/schemas/SomePayload'
responses:
'200':
description: webhook successfully processed and no retries will be performed
description: callback successfully processed
```

The following example shows a callback where the server is hard-coded, but the query string parameters are populated from the `id` and `email` property in the request body.

```yaml
transactionCallback:
'http://notificationServer.com?transactionId={$request.body#/id}&email={$request.body#/email}':
post:
requestBody:
description: Callback payload
content:
'application/json':
schema:
$ref: '#/components/schemas/SomePayload'
responses:
'200':
description: callback successfully processed
```

#### <a name="exampleObject"></a>Example Object

Expand Down