[GitHub] [airflow] houqp commented on a change in pull request #10333: Unify error messages and complete type field in response

[GitBox <gi...@apache.org>]

houqp commented on a change in pull request #10333:
URL: https://github.com/apache/airflow/pull/10333#discussion_r479612607



##########
File path: airflow/api_connexion/exceptions.py
##########
@@ -16,48 +16,134 @@
 # under the License.
 from typing import Dict, Optional
 
-from connexion import ProblemException
+import werkzeug
+from connexion import FlaskApi, ProblemException, problem
+
+from airflow import version
+
+doc_link = f'https://airflow.apache.org/docs/{version.version}/stable-rest-api-ref.html'
+if 'dev' in version.version:
+    doc_link = "https://airflow.readthedocs.io/en/latest/stable-rest-api-ref.html"
+
+EXCEPTIONS_LINK_MAP = {
+    400: doc_link + "#section/Errors/BadRequest",
+    404: doc_link + "#section/Errors/NotFound",
+    401: doc_link + "#section/Errors/Unauthenticated",
+    409: doc_link + "#section/Errors/AlreadyExists",
+    403: doc_link + "#section/Errors/PermissionDenied",
+    500: doc_link + "#section/Errors/Unknown",

Review comment:
       nitpick, it would be better to use f string here as well to be consistent with the rest of the code base. f string is also slightly faster than manual string cocnat.

##########
File path: airflow/api_connexion/exceptions.py
##########
@@ -16,48 +16,134 @@
 # under the License.
 from typing import Dict, Optional
 
-from connexion import ProblemException
+import werkzeug
+from connexion import FlaskApi, ProblemException, problem
+
+from airflow import version
+
+doc_link = f'https://airflow.apache.org/docs/{version.version}/stable-rest-api-ref.html'
+if 'dev' in version.version:
+    doc_link = "https://airflow.readthedocs.io/en/latest/stable-rest-api-ref.html"
+
+EXCEPTIONS_LINK_MAP = {
+    400: doc_link + "#section/Errors/BadRequest",
+    404: doc_link + "#section/Errors/NotFound",
+    401: doc_link + "#section/Errors/Unauthenticated",
+    409: doc_link + "#section/Errors/AlreadyExists",
+    403: doc_link + "#section/Errors/PermissionDenied",
+    500: doc_link + "#section/Errors/Unknown",
+}
+
+
+def common_error_handler(exception):
+    """
+    Used to capture connexion exceptions and add link to the type field
+    :type exception: Exception
+    """
+    if isinstance(exception, ProblemException):
+
+        if EXCEPTIONS_LINK_MAP.get(exception.status, None):
+            response = problem(
+                status=exception.status,
+                title=exception.title,
+                detail=exception.detail,
+                type=EXCEPTIONS_LINK_MAP[exception.status],
+                instance=exception.instance,
+                headers=exception.headers,
+                ext=exception.ext,
+            )

Review comment:
       nitpick, the subsequent map lookup can be avoided if we reuse the result from the first get call.
   
   ```suggestion
           link = EXCEPTIONS_LINK_MAP.get(exception.status)
           if link:
               response = problem(
                   status=exception.status,
                   title=exception.title,
                   detail=exception.detail,
                   type=link,
                   instance=exception.instance,
                   headers=exception.headers,
                   ext=exception.ext,
               )
   ```




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org