Skip to content

Commit 59f4587

Browse files
committed
Fix mixed implicit/explicit returns and HTTP retry logic
- Add explicit 'return None' to methods with -> None annotation for clarity - Fix _update, _delete, _delete_table methods in odata.py - Fix update method in pandas_adapter.py (bare return -> return None) - Fix delete_table method in client.py - Fix critical HTTP retry bug: add missing 'continue' statement for transient status retries - Ensures consistent return patterns and proper retry behavior for 429/502/503/504 errors - All 48 tests pass
1 parent e255450 commit 59f4587

4 files changed

Lines changed: 78 additions & 11 deletions

File tree

src/PowerPlatform/Dataverse/client.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -481,6 +481,7 @@ def delete_table(self, tablename: str) -> None:
481481
client.delete_table("SampleItem")
482482
"""
483483
self._get_odata()._delete_table(tablename)
484+
return None
484485

485486
def list_tables(self) -> list[str]:
486487
"""

src/PowerPlatform/Dataverse/core/http.py

Lines changed: 61 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,13 +99,72 @@ def request(self, method: str, url: str, **kwargs: Any) -> requests.Response:
9999
raise
100100
delay = self._calculate_retry_delay(attempt)
101101
time.sleep(delay)
102-
continue
103102

104103
# This should never be reached due to the logic above
105104
raise RuntimeError("Unexpected end of retry loop")
106105

107106
def _calculate_retry_delay(self, attempt: int, response: Optional[requests.Response] = None) -> float:
108-
"""Calculate retry delay with exponential backoff, optional jitter, and Retry-After header support."""
107+
"""
108+
Calculate the delay before the next retry attempt using exponential backoff with jitter.
109+
110+
This method implements an intelligent retry delay strategy that prioritizes server-provided
111+
guidance (Retry-After header) over client-calculated delays, uses exponential backoff to
112+
reduce load on failing services, and applies jitter to prevent thundering herd problems
113+
when multiple clients retry simultaneously.
114+
115+
The delay calculation follows this priority order:
116+
1. **Retry-After header**: If present and valid, use the server-specified delay (capped at max_backoff)
117+
2. **Exponential backoff**: Calculate base_delay * (2^attempt), capped at max_backoff
118+
3. **Jitter**: If enabled, add ±25% random variation to prevent synchronized retries
119+
120+
:param attempt: Zero-based retry attempt number (0 = first retry, 1 = second retry, etc.).
121+
:type attempt: int
122+
:param response: Optional HTTP response object containing headers. Used to extract Retry-After
123+
header when available. If None, only exponential backoff calculation is performed.
124+
:type response: requests.Response or None
125+
126+
:return: Delay in seconds before the next retry attempt. Always >= 0, capped at max_backoff.
127+
:rtype: float
128+
129+
:raises: Does not raise exceptions. Invalid Retry-After values fall back to exponential backoff.
130+
131+
.. note::
132+
**Retry-After Header Handling (RFC 7231):**
133+
134+
- Supports integer seconds format: ``Retry-After: 120``
135+
- Invalid formats (non-integer, HTTP-date) fall back to exponential backoff
136+
- Server-provided delays are capped at ``max_backoff`` to prevent excessive waits
137+
138+
.. note::
139+
**Exponential Backoff Formula:**
140+
141+
- Base calculation: ``delay = base_delay * (2^attempt)``
142+
- Example with ``base_delay=0.5``: 0.5s, 1.0s, 2.0s, 4.0s, 8.0s, 16.0s...
143+
- Always capped at ``max_backoff`` (default 60s) to prevent unbounded delays
144+
145+
.. note::
146+
**Jitter Application:**
147+
148+
- When ``jitter=True``, adds random variation of ±25% of calculated delay
149+
- Prevents thundering herd when multiple clients retry simultaneously
150+
- Example: 4.0s delay becomes random value between 3.0s and 5.0s
151+
- Final result is always >= 0 even with negative jitter
152+
153+
Example:
154+
Calculate delays for successive retry attempts::
155+
156+
client = HttpClient(base_delay=1.0, max_backoff=30.0, jitter=True)
157+
158+
# Attempt 0: ~1.0s ± 25% jitter = 0.75s - 1.25s
159+
delay0 = client._calculate_retry_delay(0)
160+
161+
# Attempt 2: ~4.0s ± 25% jitter = 3.0s - 5.0s
162+
delay2 = client._calculate_retry_delay(2)
163+
164+
# With Retry-After header (takes precedence)
165+
response_with_retry_after = Mock(headers={"Retry-After": "15"})
166+
delay = client._calculate_retry_delay(1, response_with_retry_after) # Returns 15.0
167+
"""
109168

110169
# Check for Retry-After header (RFC 7231)
111170
if response and "Retry-After" in response.headers:

src/PowerPlatform/Dataverse/data/odata.py

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,8 @@ def __init__(
4949
backoff=self.config.http_backoff,
5050
max_backoff=getattr(self.config, 'http_max_backoff', None),
5151
timeout=self.config.http_timeout,
52-
jitter=getattr(self.config, 'http_jitter', None) if getattr(self.config, 'http_jitter', None) is not None else True,
53-
retry_transient_errors=getattr(self.config, 'http_retry_transient_errors', None) if getattr(self.config, 'http_retry_transient_errors', None) is not None else True,
52+
jitter=getattr(self.config, 'http_jitter', True),
53+
retry_transient_errors=getattr(self.config, 'http_retry_transient_errors', True),
5454
)
5555
# Cache: logical name -> entity set name (plural) resolved from metadata
5656
self._logical_to_entityset_cache: dict[str, str] = {}
@@ -388,6 +388,7 @@ def _update(self, logical_name: str, key: str, data: Dict[str, Any]) -> None:
388388
entity_set = self._entity_set_from_logical(logical_name)
389389
url = f"{self.api}/{entity_set}{self._format_key(key)}"
390390
r = self._request("patch", url, headers={"If-Match": "*"}, json=data)
391+
return None
391392

392393
def _update_multiple(self, entity_set: str, logical_name: str, records: List[Dict[str, Any]]) -> None:
393394
"""Bulk update existing records via the collection-bound UpdateMultiple action.
@@ -445,6 +446,7 @@ def _delete(self, logical_name: str, key: str) -> None:
445446
entity_set = self._entity_set_from_logical(logical_name)
446447
url = f"{self.api}/{entity_set}{self._format_key(key)}"
447448
self._request("delete", url, headers={"If-Match": "*"})
449+
return None
448450

449451
def _get(self, logical_name: str, key: str, select: Optional[str] = None) -> Dict[str, Any]:
450452
"""Retrieve a single record.
@@ -912,9 +914,16 @@ def _request_metadata_with_retry(self, method: str, url: str, **kwargs) -> Any:
912914
913915
Retries 404 errors for metadata operations since attribute metadata
914916
may not be immediately available after creation or schema changes.
917+
918+
:param method: HTTP method (e.g., 'get', 'post').
919+
:type method: str
920+
:param url: Target URL for the metadata request.
921+
:type url: str
922+
:param kwargs: Additional arguments passed to the underlying request method.
923+
:return: HTTP response object.
924+
:rtype: Any
925+
:raises HttpError: If the request fails after all retry attempts.
915926
"""
916-
import time
917-
918927
last_error = None
919928
for attempt in range(3): # 3 attempts for metadata operations
920929
try:
@@ -927,10 +936,6 @@ def _request_metadata_with_retry(self, method: str, url: str, **kwargs) -> Any:
927936
time.sleep(delay)
928937
continue
929938
raise
930-
931-
# This should never be reached due to logic above, but just in case
932-
if last_error:
933-
raise last_error
934939

935940
def _optionset_map(self, logical_name: str, attr_logical: str) -> Optional[Dict[str, int]]:
936941
"""Build or return cached mapping of normalized label -> value for a picklist attribute.
@@ -1166,6 +1171,7 @@ def _delete_table(self, tablename: str) -> None:
11661171
metadata_id = ent["MetadataId"]
11671172
url = f"{self.api}/EntityDefinitions({metadata_id})"
11681173
r = self._request("delete", url)
1174+
return None
11691175

11701176
def _create_table(
11711177
self,

src/PowerPlatform/Dataverse/utils/pandas_adapter.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,8 +84,9 @@ def update(self, logical_name: str, record_id: str, entity_data: pd.Series) -> N
8484
raise TypeError("entity_data must be a pandas Series")
8585
payload = {k: v for k, v in entity_data.items()}
8686
if not payload:
87-
return # nothing to send
87+
return None # nothing to send
8888
self._c.update(logical_name, record_id, payload)
89+
return None
8990

9091
# ---------------------------- Delete ---------------------------------
9192
def delete_ids(self, logical_name: str, ids: Sequence[str] | pd.Series | pd.Index) -> pd.DataFrame:

0 commit comments

Comments
 (0)