[GitHub] [airflow] uranusjr commented on a change in pull request #21809: Default args type check

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

uranusjr commented on a change in pull request #21809:
URL: https://github.com/apache/airflow/pull/21809#discussion_r814504669



##########
File path: airflow/models/dag.py
##########
@@ -341,7 +341,10 @@ def __init__(
 
         self.user_defined_macros = user_defined_macros
         self.user_defined_filters = user_defined_filters
-        self.default_args = copy.deepcopy(default_args or {})
+        if not isinstance(default_args, Dict) and default_args is not None:
+            raise TypeError("default_args must be a dictionary")
+        else:
+            self.default_args = copy.deepcopy(default_args or {})

Review comment:
       This is the only place `default_args` must be a dict, but please use the built-in class instead of the typing alias.
   
   ```suggestion
           if default_args and not isinstance(default_args, dict):
               raise TypeError("default_args must be a dict")
           self.default_args = copy.deepcopy(default_args or {})
   ```

##########
File path: airflow/models/baseoperator.py
##########
@@ -147,9 +150,12 @@ def _merge_defaults(
 ) -> Tuple[dict, ParamsDict]:
     if task_params:
         dag_params.update(task_params)
-    with contextlib.suppress(KeyError):
-        dag_params.update(task_default_args.pop("params"))
-    dag_args.update(task_default_args)
+    if not isinstance(task_default_args, Dict) and task_default_args is not None:
+        raise TypeError("default_args must be a dictionary")

Review comment:
       Same for this.

##########
File path: airflow/models/baseoperator.py
##########
@@ -135,7 +135,10 @@ def _get_dag_defaults(dag: Optional["DAG"], task_group: Optional["TaskGroup"]) -
     dag_args = copy.copy(dag.default_args)
     dag_params = copy.deepcopy(dag.params)
     if task_group:
-        dag_args.update(task_group.default_args)
+        if not isinstance(task_group.default_args, Dict) and task_group.default_args is not None:
+            raise TypeError("default_args must be a dictionary")
+        else:
+            dag_args.update(task_group.default_args)

Review comment:
       ```suggestion
           if task_group.default_args and not isinstance(task_group.default_args, collections.abc.Mapping):
               raise TypeError("default_args must be a mapping")
           dag_args.update(task_group.default_args)
   ```
   
   Technically `default_args` does not need to be a dict, but only needs a [_dict-like interface_](https://docs.python.org/3/glossary.html#term-mapping).




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

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