airbytehq / airbyte

The leading data integration platform for ETL / ELT data pipelines from APIs, databases & files to data warehouses, data lakes & data lakehouses. Both self-hosted and Cloud-hosted.
https://airbyte.com
Other
16.04k stars 4.11k forks source link

Incorrect Python CDK raise_on_http_errors logging behaviour #11691

Closed itaseskii closed 2 years ago

itaseskii commented 2 years ago
## Environment - **Airbyte version**: Latest - **OS Version / Instance**: Ubuntu 20 - **Deployment**: Docker - **Source Connector and version**: N/A - **Destination Connector and version**: N/A - **Severity**: Medium - **Step where error happened**: Python CDK ## Current Behavior The following commit https://github.com/airbytehq/airbyte/commit/78cafc58ab89253883c8ab47729322822046ebc7 has introduced an incorrect logging statement which causes all response types (including **_200 OK_**) to be logged as errors. If **_raise_on_http_errors_** is set to true the code will always enter the conditional block of code and further evaluate the response with **raise_for_status** to check if an error/exception needs to be thrown which might not be the case and it will only lead to a misleading error message. This can also have some security implications since sensitive data might be persisted in the log files. ## Expected Behavior The method should only log error messages. A simple wrap of the **_raise_for_status_** call in a **_try except_** should fix the issue. ``` try: response.raise_for_status() except: self.logger.error(f"Request raised an error with response: {response.text}") raise ``` ## Logs
LOG ``` ```
## Steps to Reproduce 1. Run any source connector unit test suite which calls the **_read_records()_** method. ## Are you willing to submit a PR?

I am willing to submit a PR.

marcosmarxm commented 2 years ago

Solved by https://github.com/airbytehq/airbyte/pull/11650