-
Notifications
You must be signed in to change notification settings - Fork 83
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 Webhooks model APIs #612
Conversation
Realised that |
I've now updated the list of annotations in the spec. |
Rebased on |
*/ | ||
@Target({}) | ||
@Retention(RetentionPolicy.RUNTIME) | ||
public @interface Webhook { |
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.
Should this also include parameters
(applicable to all operations) and servers
[1] since this is effectively a path item? I wonder also if it should have a $ref
for the same reason.
[1] https://spec.openapis.org/oas/v3.1.0.html#fixed-fields-6
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.
Should this also include
parameters
andservers
It could, but I'm not sure whether we should or not. Path items appear in four places: under paths
, callbacks
, webhooks
and components
(not yet added).
By far the most common is under paths
and we don't currently have any way to add parameters
or servers
there with annotations since there's nothing in Jakarta REST that really maps to path item and so there's nowhere to put an annotation.
@Callback
also represents a path item, so if we were to add parameters
and servers
to @Webhook
, we should probably add them to @Callback
as well.
I wonder also if it should have a
$ref
for the same reason.
Yes possibly. I was going to think about that under #437.
Another thought occurred to me: if we were to add a new annotation for #437, it would probably have identical fields to @Webhook
(since both represent one entry in a Map<String, PathItem>
). We could use one annotation (named @PathItem
) for both instead.
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 design this alongside #437.
I'll remove the annotations from this PR so we can get the model parts merged.
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.
Annotations removed.
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.
Looks good
OpenAPI.webhooks
add@Webhook
annotation and@OpenAPIDefinition.webhooks
add TCKs using the@Webhook
annotationFor #583