Re: [PR] Ensure `HttpHook.run()` does not alter `extra_options` passed to it [airflow]

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

jroachgolf84 commented on code in PR #51893:
URL: https://github.com/apache/airflow/pull/51893#discussion_r2155795973


##########
providers/http/src/airflow/providers/http/hooks/http.py:
##########
@@ -194,21 +215,33 @@ def _extract_auth(self, connection: Connection) -> Any | None:
         return None
 
     def _configure_session_from_extra(
-        self, session: Session, connection: Connection, extra_options: dict[str, Any] | None = None
+        self, session: Session, connection: Connection, extra_options: dict[str, Any]
     ) -> Session:
-        if extra_options is None:
-            extra_options = {}
-        headers = _process_extra_options_from_connection(connection, extra_options)
-        session.proxies = extra_options.pop("proxies", extra_options.pop("proxy", {}))
-        session.stream = extra_options.pop("stream", False)
-        session.verify = extra_options.pop("verify", extra_options.pop("verify_ssl", True))
-        session.cert = extra_options.pop("cert", None)
-        session.max_redirects = extra_options.pop("max_redirects", DEFAULT_REDIRECT_LIMIT)
-        session.trust_env = extra_options.pop("trust_env", True)
+        """
+        Configure the session using both the extra field from the Connection and passed in extra_options.
+
+        :param session: (Session)
+        :param connection: HTTP Connection passed into Hook
+        :param extra_options: (dict)
+        :return: (Session)
+        """
+        # This is going to update self.merged_extra, which will be used below
+        conn_extra_options, self.merged_extra = _process_extra_options_from_connection(
+            connection, extra_options
+        )
+
+        session.proxies = self.merged_extra.pop("proxies", self.merged_extra.pop("proxy", {}))

Review Comment:
   This is the reason that a change needed to be made. Previously, the original `extra_options` that was being passed in was being manipulated such that when the `run` method of the hook was called again, the dictionary did not maintain its original key-value pairs. However, you do raise a good point.
   
   `self.merged_extra` needs to be defined so that the merged dictionary can be passed to the `run_and_check(...)` method when called in `run`? Why's that? Changing the return value of `get_conn` would be a breaking change, hence the need for an instance-level variable to be leveraged.
   
   You do make a good point, and that is that I do not need to `pop` values from `self.merged_extra`. I'll go ahead and plan on making that change.



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