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

Add support for WebHooks #115

Merged
merged 1 commit into from
Dec 21, 2021
Merged

Add support for WebHooks #115

merged 1 commit into from
Dec 21, 2021

Conversation

WyriHaximus
Copy link

No description provided.

@cebe
Copy link
Owner

cebe commented Oct 18, 2021

@WyriHaximus please rebase this on the openapi-31 branch. We'll add all 3.1 features to that branch and merge it into master when ready.

@WyriHaximus WyriHaximus changed the base branch from master to openapi-31 October 18, 2021 18:04
@WyriHaximus
Copy link
Author

@cebe Will rebase and push later tonight 👍

@WyriHaximus
Copy link
Author

@cebe Rebased it, is there any specific testing I should add reside from the existing change?

@WyriHaximus WyriHaximus changed the title [WIP] Add support for WebHooks Add support for WebHooks Oct 18, 2021
@WyriHaximus WyriHaximus marked this pull request as ready for review October 18, 2021 20:11
@cebe
Copy link
Owner

cebe commented Oct 18, 2021

Webhook property check should depend in the openapi Version. Only require webhook in 3.1 and keep old behavior in 3.0.

Also would be good to have a WebHookTest which checks basic webhook example

@WyriHaximus
Copy link
Author

WyriHaximus commented Oct 18, 2021

Webhook property check should depend in the openapi Version. Only require webhook in 3.1 and keep old behavior in 3.0.

Done that just now

Also would be good to have a WebHookTest which checks basic webhook example

Doing this tomorrow 👍

@WyriHaximus WyriHaximus force-pushed the webhooks branch 2 times, most recently from 2176808 to 0df8b5e Compare October 20, 2021 07:09
if ($this->getMajorVersion() === static::VERSION_3_0) {
$this->requireProperties(['openapi', 'info', 'paths']);
} else {
$this->requireProperties(['openapi', 'info'], ['paths', 'webhooks']);
Copy link
Contributor

Choose a reason for hiding this comment

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

I added the testcases for OpenAPI 3.1 here #129 and the OpenAPI Schema says, that only openapi and info is a required field in OpenAPI 3.1

// vendor/oai/openapi-specification-3.1/schemas/v3.1/schema.json
"required": [
    "openapi",
    "info"
  ]
// vendor/oai/openapi-specification-3.1/schemas/v3.0/schema.json
  "required": [
    "openapi",
    "info",
    "paths"
  ],

Copy link
Author

Choose a reason for hiding this comment

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

Interesting, I was expecting to be a oneOf for paths and webhooks: https://github.com/OAI/OpenAPI-Specification/blob/main/schemas/v3.1/schema.json#L56-L72

Copy link
Contributor

@marcelthole marcelthole Oct 20, 2021

Choose a reason for hiding this comment

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

yeah you are right. Thats my fault. Then components are also possible and missing in the atLeastOne argument.

Then should the examples with components, hooks and paths should work

Copy link
Author

@WyriHaximus WyriHaximus Oct 20, 2021

Choose a reason for hiding this comment

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

Subscribed to #129 I'll rebase this PR and fix any errors coming up when it gets merged

Copy link
Owner

Choose a reason for hiding this comment

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

here is the text representation of that from the specification:

https://spec.openapis.org/oas/latest.html#openapi-document
The OpenAPI document MUST contain at least one paths field, a components field or a webhooks field.

@WyriHaximus WyriHaximus force-pushed the webhooks branch 2 times, most recently from 48586f5 to f7ecbac Compare October 20, 2021 15:53
tests/spec/OpenApiTest.php Outdated Show resolved Hide resolved
@WyriHaximus
Copy link
Author

Rebased now that #129 has been merged.

Also would be good to have a WebHookTest which checks basic webhook example

Doing this tomorrow +1

@cebe Basic variant for this is in (been since nearly two weeks but details 😛 ), do you want me to expand it, or rely on the tests added from #129?

@@ -46,6 +47,7 @@ protected function attributes(): array
'info' => Info::class,
'servers' => [Server::class],
'paths' => Paths::class,
'webhooks' => WebHooks::class,
Copy link
Owner

Choose a reason for hiding this comment

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

is there a specific reason why you introduce the "WebHooks" class and not implement this as a map of "string"->"PathItem" as described in the specification?

... ... ...
webhooks Map[string, Path Item Object | Reference Object] ] The incoming webhooks that MAY be received as part of this API and that the API consumer MAY choose to implement. Closely related to the callbacks feature, this section describes requests initiated other than by an API call, for example by an out of band registration. The key name is a unique string to refer to each webhook, while the (optionally referenced) Path Item Object describes a request that may be initiated by the API provider and the expected responses. An example is available.

https://spec.openapis.org/oas/v3.1.0#openapi-object

Copy link
Author

Choose a reason for hiding this comment

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

The WebHooks class is a copy, with some things changed, of the Paths class. Would the alternative just be wrapping a Path::class in an array, like servers?

src/spec/Schema.php Outdated Show resolved Hide resolved
src/spec/Type.php Outdated Show resolved Hide resolved
@cebe
Copy link
Owner

cebe commented Nov 21, 2021

been since nearly two weeks but details

I'm not better in this, so not judging you ;-)

Tests are fine, I just wonder about implementation. I tried to follow the spec quite stricly in which Objects are described. There is a "Paths" object in the spec but no "Webhooks" object, which is not consistent. Need to decide if it's fine for the implementation to break with the strict dependence on the spec or not.

@WyriHaximus
Copy link
Author

I tried to follow the spec quite stricly in which Objects are described. There is a "Paths" object in the spec but no "Webhooks" object, which is not consistent. Need to decide if it's fine for the implementation to break with the strict dependence on the spec or not.

Just dropped the WebHooks class of in a new commit so you can compare both.

@cebe
Copy link
Owner

cebe commented Nov 29, 2021

Thanks, looks good.

@cebe
Copy link
Owner

cebe commented Nov 29, 2021

Tests are failing with Error: Class 'cebe\openapi\spec\WebHooks' not found

@WyriHaximus
Copy link
Author

Tests are failing with Error: Class 'cebe\openapi\spec\WebHooks' not found

Whoops, want me to squash both commits into one?

@WyriHaximus
Copy link
Author

Squashed both commits together

@cebe cebe merged commit 088af05 into cebe:openapi-31 Dec 21, 2021
@cebe
Copy link
Owner

cebe commented Dec 21, 2021

Thank you!

@WyriHaximus
Copy link
Author

No problem, looking forward to having full support in

@cebe cebe modified the milestones: 1.6.0, 1.8.0 Apr 20, 2022
@cebe cebe mentioned this pull request Apr 20, 2022
3 tasks
@WyriHaximus WyriHaximus deleted the webhooks branch January 12, 2023 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants