-
Notifications
You must be signed in to change notification settings - Fork 28
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
Alignement of event-subscription-template.yaml with CAMARA_common.yaml for the errors #251
Conversation
Co-authored-by: Pedro Díez García <pedro.diezgarcia@telefonica.com>
Co-authored-by: Pedro Díez García <pedro.diezgarcia@telefonica.com>
Co-authored-by: Pedro Díez García <pedro.diezgarcia@telefonica.com>
Thanks a lot Pedro for the review.
Thanks a lot @PedroDiez for the review. I've took into consideration all your comments. |
TokenMismatch: | ||
code: PERMISSION_DENIED | ||
message: Client does not have sufficient permissions to perform this action. | ||
GENERIC_403_SUBSCRIPTION_MISMATCH: |
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 SUBSCRIPTION_MISMATCH doesn't belong to Generic403, it's subscription specific
Can't we have only 1 403 type like SubscriptionPermissionDenied with 3 examples (permissiondenied, invalidtoken, subscription mismatch), and remove Generic403?
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.
@akoshunyadi I used CreateSubscriptionPermissionDenied403
only for the POST /subscriptions
and I have 403_INVALID_TOKEN_CONTEXT
specific for the POST
Generic403
is for the GET & DELETE and here we have the GENERIC_403_SUBSCRIPTION_MISMATCH
which applies only for GET & DELETE and not POST
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.
For me GenericXXX are types which we could put into/take from the common yaml and use for all APIs, because they don't contain any API specific hints. So I think we can have multiple 403 types here but if *Generic then it should only have generic examples.
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.
@bigludo7 I think GENERIC_403_SUBSCRIPTION_MISMATCH is to be used within CreateSubscriptionPermissionDenied403 (Line 896) and within "Generic 403" (Line 875) also applies GENERIC_403_INVALID_TOKEN_CONTEXT
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.
humm...hope i was able to follow you
I've defined SubscriptionPermissionDenied403
for all subscription related 403 for the operations and I kept Generic403
as response possible for the callback.
status: 415 | ||
code: UNSUPPORTED_MEDIA_TYPE | ||
message: The server refuses to accept the request because the payload format is in an unsupported format | ||
Generic422: |
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 it should be named specifically, since it contains subscription specific examples
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.
hummm so I not get it because when I read in the CAMARA_common.yam that we can have in Generic422
:
GENERIC_422_{{SPECIFIC_CODE}}:
description: Any semantic condition associated to business logic, specifically related to a field or data structure
value:
status: 422
code: { { SPECIFIC_CODE } }
message: { { SPECIFIC_CODE_MESSAGE } }
(lines 373) I was thinking this is a placeholder for specific code.
What is your understanding @akoshunyadi ?
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.
yes, but I still have the problem that we will have many e.g. Generic422 over all Camara APIs but there are not the same.
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've changed to CreateSubscriptionUnprocessableEntity422 ;)
Hope that fine for @PedroDiez
"409": | ||
$ref: "#/components/responses/Generic409" | ||
"415": | ||
$ref: "#/components/responses/Generic415" | ||
"422": | ||
$ref: "#/components/responses/CreateSubscription422" | ||
$ref: "#/components/responses/Generic422" | ||
"429": | ||
$ref: "#/components/responses/Generic429" | ||
"500": |
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.
How about other errorcodes in the guideline; e.g. 405,406,501 , I think those ones would make a sense 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.
I did not add them because they were not in the initial yaml.
Regarding 405 & 406 I'm fine to add them. @PedroDiez WDYT?
Regarding 501 with @PedroDiez we think that this API will "not work" if all operation are not implemented. Indeed, it you provide subscription you must provide POST/GET/DELETE
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 prefer not doing now. And it is fair point of discussion.
Because there is no common guideline about to mention explicitly HTTP codes that are usually managed at API-GW Infras. We need a guideline first to agree on which HTTP codes are traversal to be explicitly mention in every API and which not (as assumed that any implementation will manage it accordingly). If not we will again have misaligment among APIs.
To me is an issue for discussion for the next MetaRelease Spring 25
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.
Fine for me to discuss it later.
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.
LGTM
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.
LGTM. Points covered
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.
LGTM
Hello @rartych, @shilpa-padgaonkar |
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.
/LGTM
What type of PR is this?
Add one of the following kinds:
What this PR does / why we need it:
Alignement of artifacts/camara-cloudevents/event-subscription-template.yaml with artifacts/CAMARA_common.yaml for the errors
cc: @PedroDiez @shilpa-padgaonkar @akoshunyadi @maxl2287
Which issue(s) this PR fixes:
Fixes #250
Special notes for reviewers:
This is an urgent issue as required for this meta release for some of our API.
Changelog input
Additional documentation
This section can be blank.