Re: [PR] Add `to_dict`, `from_json`, `as_json`, `get_extra_dejson` to SDK Connection [airflow]

["ashb (via GitHub)" <gi...@apache.org>]

ashb commented on code in PR #53870:
URL: https://github.com/apache/airflow/pull/53870#discussion_r2253765726


##########
airflow-core/src/airflow/models/connection.py:
##########
@@ -529,6 +529,18 @@ def to_dict(self, *, prune_empty: bool = False, validate: bool = True) -> dict[s
 
         :meta private:
         """
+        if hasattr(sys.modules.get("airflow.sdk.execution_time.task_runner"), "SUPERVISOR_COMMS"):

Review Comment:
   Do we need this on to_dict ? It's an instance method, and I'm not sure you end up with an object of this class in SDK context?



##########
task-sdk/src/airflow/sdk/definitions/connection.py:
##########
@@ -30,6 +30,49 @@
 log = logging.getLogger(__name__)
 
 
+def _prune_dict(val: Any, mode="strict"):
+    """
+    Given dict ``val``, returns new dict based on ``val`` with all empty elements removed.
+
+    What constitutes "empty" is controlled by the ``mode`` parameter.  If mode is 'strict'
+    then only ``None`` elements will be removed.  If mode is ``truthy``, then element ``x``
+    will be removed if ``bool(x) is False``.
+    """
+
+    def is_empty(x):
+        if mode == "strict":
+            return x is None
+        if mode == "truthy":
+            return bool(x) is False
+        raise ValueError("allowable values for `mode` include 'truthy' and 'strict'")

Review Comment:
   However I only ever see this called with "strict"?



##########
task-sdk/src/airflow/sdk/definitions/connection.py:
##########
@@ -30,6 +30,49 @@
 log = logging.getLogger(__name__)
 
 
+def _prune_dict(val: Any, mode="strict"):
+    """
+    Given dict ``val``, returns new dict based on ``val`` with all empty elements removed.
+
+    What constitutes "empty" is controlled by the ``mode`` parameter.  If mode is 'strict'
+    then only ``None`` elements will be removed.  If mode is ``truthy``, then element ``x``
+    will be removed if ``bool(x) is False``.
+    """
+
+    def is_empty(x):
+        if mode == "strict":
+            return x is None
+        if mode == "truthy":
+            return bool(x) is False
+        raise ValueError("allowable values for `mode` include 'truthy' and 'strict'")

Review Comment:
   ```suggestion
       if mode == "strict":
           is_empty = lambda x: x is None
       if mode == "truthy":
           is_empty = lambda x: bool(x) is False
       else:
           raise ValueError("allowable values for `mode` include 'truthy' and 'strict'")
   ```



##########
airflow-core/src/airflow/models/connection.py:
##########
@@ -567,6 +591,18 @@ def from_json(cls, value, conn_id=None) -> Connection:
 
     def as_json(self) -> str:
         """Convert Connection to JSON-string object."""
+        if hasattr(sys.modules.get("airflow.sdk.execution_time.task_runner"), "SUPERVISOR_COMMS"):

Review Comment:
   Ditto -- I think the only compat we need is the classmethod



##########
task-sdk/src/airflow/sdk/definitions/connection.py:
##########
@@ -160,6 +203,14 @@ def get(cls, conn_id: str) -> Any:
     @property
     def extra_dejson(self) -> dict:
         """Deserialize `extra` property to JSON."""
+        return self.get_extra_dejson()
+
+    def get_extra_dejson(self, nested: bool = False) -> dict:

Review Comment:
   Nit: Do it the other way around please so we can deprecate the method someday



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