-
Notifications
You must be signed in to change notification settings - Fork 381
feat(rest): implement RFC 9457 problem details for errors #771
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
base: 1.0-dev
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -62,19 +62,86 @@ | |||||||||
| JSONParseError: 400, | ||||||||||
| InvalidRequestError: 400, | ||||||||||
| MethodNotFoundError: 404, | ||||||||||
| InvalidParamsError: 422, | ||||||||||
| InvalidParamsError: 400, | ||||||||||
| InternalError: 500, | ||||||||||
| JSONRPCInternalError: 500, | ||||||||||
| TaskNotFoundError: 404, | ||||||||||
| TaskNotCancelableError: 409, | ||||||||||
| PushNotificationNotSupportedError: 501, | ||||||||||
| UnsupportedOperationError: 501, | ||||||||||
| PushNotificationNotSupportedError: 400, | ||||||||||
| UnsupportedOperationError: 400, | ||||||||||
| ContentTypeNotSupportedError: 415, | ||||||||||
| InvalidAgentResponseError: 502, | ||||||||||
| AuthenticatedExtendedCardNotConfiguredError: 404, | ||||||||||
| AuthenticatedExtendedCardNotConfiguredError: 400, | ||||||||||
| } | ||||||||||
|
|
||||||||||
| A2AErrorToTypeURI: dict[_A2AErrorType, str] = { | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here and below: should be possible to only put domain errors inheriting This list actually reflects what should be handled, various |
||||||||||
| TaskNotFoundError: 'https://a2a-protocol.org/errors/task-not-found', | ||||||||||
| TaskNotCancelableError: 'https://a2a-protocol.org/errors/task-not-cancelable', | ||||||||||
| PushNotificationNotSupportedError: 'https://a2a-protocol.org/errors/push-notification-not-supported', | ||||||||||
| UnsupportedOperationError: 'https://a2a-protocol.org/errors/unsupported-operation', | ||||||||||
| ContentTypeNotSupportedError: 'https://a2a-protocol.org/errors/content-type-not-supported', | ||||||||||
| InvalidAgentResponseError: 'https://a2a-protocol.org/errors/invalid-agent-response', | ||||||||||
| AuthenticatedExtendedCardNotConfiguredError: 'https://a2a-protocol.org/errors/extended-agent-card-not-configured', | ||||||||||
| } | ||||||||||
|
|
||||||||||
| A2AErrorToTitle: dict[_A2AErrorType, str] = { | ||||||||||
| JSONRPCError: 'JSON RPC Error', | ||||||||||
| JSONParseError: 'JSON Parse Error', | ||||||||||
| InvalidRequestError: 'Invalid Request Error', | ||||||||||
| MethodNotFoundError: 'Method Not Found Error', | ||||||||||
| InvalidParamsError: 'Invalid Params Error', | ||||||||||
| InternalError: 'Internal Error', | ||||||||||
| JSONRPCInternalError: 'Internal Error', | ||||||||||
| TaskNotFoundError: 'Task Not Found', | ||||||||||
| TaskNotCancelableError: 'Task Not Cancelable', | ||||||||||
| PushNotificationNotSupportedError: 'Push Notification Not Supported', | ||||||||||
| UnsupportedOperationError: 'Unsupported Operation', | ||||||||||
| ContentTypeNotSupportedError: 'Content Type Not Supported', | ||||||||||
| InvalidAgentResponseError: 'Invalid Agent Response', | ||||||||||
| AuthenticatedExtendedCardNotConfiguredError: 'Extended Agent Card Not Configured', | ||||||||||
| } | ||||||||||
|
|
||||||||||
|
|
||||||||||
| def _build_problem_details_response(error: A2AError) -> JSONResponse: | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: maybe this one can return something typed shaped as a problem+json payload so that it can also handle non-A2A errors? This way the code below except Exception:
logger.exception('Unknown error occurred')
return JSONResponse(
content={
'type': 'about:blank',
'title': 'Internal Error',
'status': 500,
'detail': 'Unknown exception',
},
status_code=500,
media_type='application/problem+json',
)won't have to duplicate it |
||||||||||
| """Helper to convert exceptions to RFC 9457 Problem Details responses.""" | ||||||||||
| error_type = cast('_A2AErrorType', type(error)) | ||||||||||
| http_code = A2AErrorToHttpStatus.get(error_type, 500) | ||||||||||
| type_uri = A2AErrorToTypeURI.get(error_type, 'about:blank') | ||||||||||
| title = A2AErrorToTitle.get(error_type, error.__class__.__name__) | ||||||||||
|
|
||||||||||
| log_level = ( | ||||||||||
| logging.ERROR if isinstance(error, InternalError) else logging.WARNING | ||||||||||
| ) | ||||||||||
| logger.log( | ||||||||||
| log_level, | ||||||||||
| "Request error: Code=%s, Message='%s'%s", | ||||||||||
| getattr(error, 'code', 'N/A'), | ||||||||||
| getattr(error, 'message', str(error)), | ||||||||||
| ', Data=' + str(getattr(error, 'data', '')) | ||||||||||
| if getattr(error, 'data', None) | ||||||||||
| else '', | ||||||||||
|
Comment on lines
+120
to
+122
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current logic for logging the
Suggested change
|
||||||||||
| ) | ||||||||||
|
|
||||||||||
| payload = { | ||||||||||
| 'type': type_uri, | ||||||||||
| 'title': title, | ||||||||||
| 'status': http_code, | ||||||||||
| 'detail': getattr(error, 'message', str(error)), | ||||||||||
| } | ||||||||||
|
|
||||||||||
| data = getattr(error, 'data', None) | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe let's add an explicit property We may also put a comment explicitly stating that this data is going to be transmitted unaltered in a transport-specific way (to address this). |
||||||||||
| if isinstance(data, dict): | ||||||||||
| for key, value in data.items(): | ||||||||||
| if key not in payload: | ||||||||||
| payload[key] = value | ||||||||||
|
Comment on lines
+132
to
+136
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||||||||||
|
|
||||||||||
| return JSONResponse( | ||||||||||
| content=payload, | ||||||||||
| status_code=http_code, | ||||||||||
| media_type='application/problem+json', | ||||||||||
| ) | ||||||||||
|
|
||||||||||
|
|
||||||||||
| def rest_error_handler( | ||||||||||
| func: Callable[..., Awaitable[Response]], | ||||||||||
| ) -> Callable[..., Awaitable[Response]]: | ||||||||||
|
|
@@ -85,37 +152,18 @@ | |||||||||
| try: | ||||||||||
| return await func(*args, **kwargs) | ||||||||||
| except A2AError as error: | ||||||||||
| http_code = A2AErrorToHttpStatus.get( | ||||||||||
| cast('_A2AErrorType', type(error)), 500 | ||||||||||
| ) | ||||||||||
|
|
||||||||||
| log_level = ( | ||||||||||
| logging.ERROR | ||||||||||
| if isinstance(error, InternalError) | ||||||||||
| else logging.WARNING | ||||||||||
| ) | ||||||||||
| logger.log( | ||||||||||
| log_level, | ||||||||||
| "Request error: Code=%s, Message='%s'%s", | ||||||||||
| getattr(error, 'code', 'N/A'), | ||||||||||
| getattr(error, 'message', str(error)), | ||||||||||
| ', Data=' + str(getattr(error, 'data', '')) | ||||||||||
| if getattr(error, 'data', None) | ||||||||||
| else '', | ||||||||||
| ) | ||||||||||
| # TODO(#722): Standardize error response format. | ||||||||||
| return JSONResponse( | ||||||||||
| content={ | ||||||||||
| 'message': getattr(error, 'message', str(error)), | ||||||||||
| 'type': type(error).__name__, | ||||||||||
| }, | ||||||||||
| status_code=http_code, | ||||||||||
| ) | ||||||||||
| return _build_problem_details_response(error) | ||||||||||
| except Exception: | ||||||||||
| logger.exception('Unknown error occurred') | ||||||||||
| return JSONResponse( | ||||||||||
| content={'message': 'unknown exception', 'type': 'Exception'}, | ||||||||||
| content={ | ||||||||||
| 'type': 'about:blank', | ||||||||||
| 'title': 'Internal Error', | ||||||||||
| 'status': 500, | ||||||||||
| 'detail': 'Unknown exception', | ||||||||||
| }, | ||||||||||
| status_code=500, | ||||||||||
| media_type='application/problem+json', | ||||||||||
| ) | ||||||||||
|
|
||||||||||
| return wrapper | ||||||||||
|
|
||||||||||
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.
Not sure it should be wrapped here, that's true that it can throw, but
.well-known/agent-card.jsonis "outside" of transport in general, just one per server: 14.3. Well-Known URI Registration.It's another question why things are wired up like this in the current SDK, I'll take a look separately, let's for now keep "static" agent card as is.