You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@airflow.apache.org by "dabla (via GitHub)" <gi...@apache.org> on 2024/12/27 09:56:50 UTC

[I] What's the purpose of the response_check parameter in HttpOperator? [airflow]

dabla opened a new issue, #45237:
URL: https://github.com/apache/airflow/issues/45237

   ### Apache Airflow version
   
   2.10.4
   
   ### If "Other Airflow 2 version" selected, which one?
   
   _No response_
   
   ### What happened?
   
   I wanted to raise an AirflowSkipException if a HTTP 404 is returned by a REST endpoint if the requested resource isn't found
   
   ### What you think should happen instead?
   
   I would have expected the custom response_check code being invoked, but it isn't, as the response status_code is already checked within the check_response method of the HttpHook.  There, when the status_code isn't within the 2x or 3x range, an AirflowException is being raised, thus making the possibility to check the response yourself through a custom response_check obsolete, as the code will never be invoked.  A possible solution would be to pass the custom response_check defined int the operator to the HttpHook, that way the same logic would be applied.
   
   ### How to reproduce
   
   Just define a custom response_check which ignores a 404 and run the HttpOperator for a non existing resource which will return a 404, you'll see that the custom response_check will never be invoked and the task will fail.
   
   ### Operating System
   
   RedHat
   
   ### Versions of Apache Airflow Providers
   
   apache-airflow-providers-http 5.0.0
   
   ### Deployment
   
   Docker-Compose
   
   ### Deployment details
   
   _No response_
   
   ### Anything else?
   
   _No response_
   
   ### Are you willing to submit PR?
   
   - [X] Yes I am willing to submit a PR!
   
   ### Code of Conduct
   
   - [X] I agree to follow this project's [Code of Conduct](https://github.com/apache/airflow/blob/main/CODE_OF_CONDUCT.md)
   


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

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

Re: [I] What's the purpose of the response_check parameter in HttpOperator? [airflow]

Posted by "topherinternational (via GitHub)" <gi...@apache.org>.

topherinternational commented on issue #45237:
URL: https://github.com/apache/airflow/issues/45237#issuecomment-2573021373

   > > > > {'check_response': False}
   > 
   > > > 
   > 
   > > > 
   > 
   > > > Just tested it and it fails with following error as it considers the check_response passed in the extra_options as a HTTP header:
   > 
   > > > `requests.exceptions.InvalidHeader: Header part (False) from ('check_response', False) must be of type str or bytes, not <class 'bool'>`
   > 
   > > 
   > 
   > > What exactly was your code here? Did you put the `check_response` pair in the Operator constructor's `extra_options`? Or in the Connection object?
   > 
   > > 
   > 
   > > Edit: I ask bc I played with the tests and the only way I could get that InvalidHeader error was to pass the `check_response` param in the operator's headers arg instead of the extra_options, or to put check_response=false in the Connection's extra table.
   > 
   > 
   > 
   > I've defined those in the extra_options of the connection, tested on the latest Airflow version.
   
   Ah, I think what the other commentor meant was to pass `extra_options` to the `HttpOperator`, not the Connection. When I pass it to the operator the setting is passed to the hook as expected and the `response_check` function is called.
   
   This should solve your immediate problem, checking status and response content with your callback instead of the hook raising a preemptive exception.


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

Re: [I] What's the purpose of the response_check parameter in HttpOperator? [airflow]

Posted by "topherinternational (via GitHub)" <gi...@apache.org>.

topherinternational commented on issue #45237:
URL: https://github.com/apache/airflow/issues/45237#issuecomment-2573054118

   > The issue is in this method, it pops all non related extra_options parameters and keeps the rest to pass it as headers, an extra pop should be needed on the check_response key to avoid the issue, I'll open a PR to fix that:
   > 
   > 
   > 
   > ```
   > 
   >     def _configure_session_from_extra(
   > 
   >             self, session: requests.Session, connection: Connection
   > 
   >         ) -> requests.Session:
   > 
   >             extra = connection.extra_dejson
   > 
   >             extra.pop("timeout", None)
   > 
   >             extra.pop("allow_redirects", None)
   > 
   >             extra.pop("check_response", None)  # this should be added
   > 
   >             session.proxies = extra.pop("proxies", extra.pop("proxy", {}))
   > 
   >             session.stream = extra.pop("stream", False)
   > 
   >             session.verify = extra.pop("verify", extra.pop("verify_ssl", True))
   > 
   >             session.cert = extra.pop("cert", None)
   > 
   >             session.max_redirects = extra.pop("max_redirects", DEFAULT_REDIRECT_LIMIT)
   > 
   >             session.trust_env = extra.pop("trust_env", True)
   > 
   >             try:
   > 
   >                 session.headers.update(extra)
   > 
   >             except TypeError:
   > 
   >                 self.log.warning("Connection to %s has invalid extra field.", connection.host)
   > 
   >             return session
   > 
   > ```
   
   This is the call of someone in the core contributor club, so I'm just giving my opinion - 
   
   This does look like a bug in the HTTP hook, but it seems like a separate issue to this issue. I see the two breaking down like this:
   - When I use the `HttpOperator`, I want to tell it to skip any status checking and let me check status through the existing `response_check` callback argument
   - When I declare a connection to use with the HttpHook, I want to specify that the hook should not do a status check on any call made with this connection
   
   Using the hook-connection contract to configure the control flow of the operator perpetuates the API leaking problem that spawned this issue in the first place. 
   
   It's a separate issue/question whether the hook should check it status based on an option in the connection object, or if that behavior should always be configured by the caller (args to the constructor or run()). I lean towards yes but that can be discussed in a new issue filing.


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