Source Freshdesk: do not call response.json() inside should_retry

airbyte-integrations/connectors/source-freshdesk/Dockerfile

@@ -34,5 +34,5 @@ COPY source_freshdesk ./source_freshdesk
 ENV AIRBYTE_ENTRYPOINT "python /airbyte/integration_code/main.py"
 ENTRYPOINT ["python", "/airbyte/integration_code/main.py"]
 
-LABEL io.airbyte.version=0.3.7
+LABEL io.airbyte.version=0.3.8
 LABEL io.airbyte.name=airbyte/source-freshdesk

airbyte-integrations/connectors/source-freshdesk/source_freshdesk/streams.py

@@ -51,13 +51,10 @@ def backoff_time(self, response: requests.Response) -> Optional[float]:
             return float(response.headers.get("Retry-After", 0))
 
     def should_retry(self, response: requests.Response) -> bool:
-        if isinstance(response.json(), dict):
-            if response.status_code == requests.codes.FORBIDDEN and response.json().get("code") == "require_feature":
-                self.forbidden_stream = True
-                setattr(self, "raise_on_http_errors", False)
-                self.logger.warn(f"Stream `{self.name}` is not available. {response.json().get('message')}")
-            else:
-                return super().should_retry(response)
+        if response.status_code == requests.codes.FORBIDDEN:
+            self.forbidden_stream = True
+            setattr(self, "raise_on_http_errors", False)
+            self.logger.warn(f"Stream `{self.name}` is not available. {response.text}")
         return super().should_retry(response)
 
     def next_page_token(self, response: requests.Response) -> Optional[Mapping[str, Any]]:
@@ -98,8 +95,9 @@ def read_records(
         )
 
     def parse_response(self, response: requests.Response, **kwargs) -> Iterable[Mapping]:
-        data = response.json()
-        return {} if self.forbidden_stream else data if data else []
+        if self.forbidden_stream:
+            return []
+        return response.json() or []
 
 
 class IncrementalFreshdeskStream(FreshdeskStream, IncrementalMixin):

airbyte-integrations/connectors/source-freshdesk/unit_tests/test_streams.py

@@ -4,7 +4,7 @@
 
 import random
 from typing import Any, MutableMapping
-from unittest.mock import PropertyMock, patch
+from unittest.mock import PropertyMock
 
 import pytest
 from airbyte_cdk.models import SyncMode
@@ -41,6 +41,11 @@
 )
 
 
+@pytest.fixture(autouse=True)
+def mock_tickets_use_cache(mocker):
+    mocker.patch("source_freshdesk.streams.Tickets.use_cache", new_callable=PropertyMock, return_value=False)
+
+
 def _read_full_refresh(stream_instance: Stream):
     records = []
     slices = stream_instance.stream_slices(sync_mode=SyncMode.full_refresh)
@@ -126,19 +131,19 @@ def test_incremental(stream, resource, authenticator, config, requests_mock):
     highest_updated_at = "2022-04-25T22:00:00Z"
     other_updated_at = "2022-04-01T00:00:00Z"
     highest_index = random.randint(0, 24)
-    with patch(f"source_freshdesk.streams.{stream.__name__}.use_cache", new_callable=PropertyMock, return_value=False):
-        requests_mock.register_uri(
-            "GET",
-            f"/api/v2/{resource}",
-            json=[{"id": x, "updated_at": highest_updated_at if x == highest_index else other_updated_at} for x in range(25)],
-        )
 
-        stream = stream(authenticator=authenticator, config=config)
-        records, state = _read_incremental(stream, {})
+    requests_mock.register_uri(
+        "GET",
+        f"/api/v2/{resource}",
+        json=[{"id": x, "updated_at": highest_updated_at if x == highest_index else other_updated_at} for x in range(25)],
+    )
 
-        assert len(records) == 25
-        assert "updated_at" in state
-        assert state["updated_at"] == highest_updated_at
+    stream = stream(authenticator=authenticator, config=config)
+    records, state = _read_incremental(stream, {})
+
+    assert len(records) == 25
+    assert "updated_at" in state
+    assert state["updated_at"] == highest_updated_at
 
 
 @pytest.mark.parametrize(
@@ -204,3 +209,13 @@ def test_full_refresh_discussion_comments(requests_mock, authenticator, config):
     records = _read_full_refresh(stream)
 
     assert len(records) == 120
+
+
+def test_403_skipped(requests_mock, authenticator, config):
+    # this case should neither raise an error nor retry
+    requests_mock.register_uri("GET", "/api/v2/tickets", json=[{"id": 1705, "updated_at": "2022-05-05T00:00:00Z"}])
+    requests_mock.register_uri("GET", "/api/v2/tickets/1705/conversations", status_code=403)
+    stream = Conversations(authenticator=authenticator, config=config)
+    records = _read_full_refresh(stream)
+    assert records == []
+    assert len(requests_mock.request_history) == 2

docs/integrations/sources/freshdesk.md

@@ -49,7 +49,8 @@ The Freshdesk connector should not run into Freshdesk API limitations under norm
 ## Changelog
 
 | Version | Date       | Pull Request                                             | Subject                                                                               |
-| :------ | :--------- | :------------------------------------------------------- | :------------------------------------------------------------------------------------ |
+|:--------|:-----------|:---------------------------------------------------------|:--------------------------------------------------------------------------------------|
+| 0.3.8   | 2022-11-11 | [19349](https://github.com/airbytehq/airbyte/pull/19349) | Do not rely on response.json() when deciding to retry a request                       |
 | 0.3.7   | 2022-11-03 | [18397](https://github.com/airbytehq/airbyte/pull/18397) | Fix base url for v2 API.                                                              |
 | 0.3.6   | 2022-09-29 | [17410](https://github.com/airbytehq/airbyte/pull/17410) | Migrate to per-stream states.                                                         |
 | 0.3.5   | 2022-09-27 | [17249](https://github.com/airbytehq/airbyte/pull/17249) | Added nullable to all stream schemas, added transformation into declared schema types |