Add correlation ID fallback for async polling (Movable Ink)#8034
Conversation
Some APIs (e.g. Movable Ink) return an empty body after the initial request, so extracting the correlation ID from the response JSON fails. This adds a fallback that resolves the correlation ID from param_value_map when the response body is empty or missing the expected field.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
| correlation_id = pydash.get(response_data, correlation_id_path) | ||
| if correlation_id: | ||
| return str(correlation_id) | ||
| except ValueError: |
There was a problem hiding this comment.
I agree that this is something to be reviewed/checked. I dont think we should have a blanket pass on the ValueError, since a malformed JSON wouldn't get caught.
If what we are trying to pass on is an Empty ValueError we should aim for that specifcially with another check, and keep the ValueError exception to catch errors on the JSON responses, I.E:
response_data = None
if response.content:
try:
response_data = response.json() ## Becomes null on empty response
except ValueError as exc:
raise FidesopsException(f"Invalid JSON response: {exc}")
if response_data and correlation_id_path:
correlation_id = pydash.get(response_data, correlation_id_path)
if correlation_id:
return str(correlation_id)
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (85.71%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #8034 +/- ##
==========================================
+ Coverage 85.14% 85.17% +0.02%
==========================================
Files 637 637
Lines 41936 41942 +6
Branches 4927 4930 +3
==========================================
+ Hits 35707 35722 +15
+ Misses 5121 5111 -10
- Partials 1108 1109 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Vagoasdf
left a comment
There was a problem hiding this comment.
There Are a few changes required around the managment of errors, but besides that. the logic looks solid and the tests are complete. 👍🏽
| correlation_id = pydash.get(response_data, correlation_id_path) | ||
| if correlation_id: | ||
| return str(correlation_id) | ||
| except ValueError: |
There was a problem hiding this comment.
I agree that this is something to be reviewed/checked. I dont think we should have a blanket pass on the ValueError, since a malformed JSON wouldn't get caught.
If what we are trying to pass on is an Empty ValueError we should aim for that specifcially with another check, and keep the ValueError exception to catch errors on the JSON responses, I.E:
response_data = None
if response.content:
try:
response_data = response.json() ## Becomes null on empty response
except ValueError as exc:
raise FidesopsException(f"Invalid JSON response: {exc}")
if response_data and correlation_id_path:
correlation_id = pydash.get(response_data, correlation_id_path)
if correlation_id:
return str(correlation_id)
- Replace blanket except ValueError with explicit response.content check and FidesopsException on malformed JSON - Update tests: empty body fallback test, add malformed JSON error test
Ticket [ENG-XXXX]
Description Of Changes
Adds a fallback mechanism for correlation ID extraction in async polling. Some APIs (e.g. Movable Ink) return an empty response body after the initial DSR request, so extracting the correlation ID from the JSON response fails. This change allows the framework to resolve the correlation ID from
param_value_mapwhen the response body is empty — for example, usingprivacy_request_idwhich is always auto-populated.This is fully backward-compatible: connectors that return correlation IDs in their response bodies continue to work via the primary extraction path.
Companion PR: ethyca/fidesplus — Movable Ink connector config, dataset, and tests.
Code Changes
_extract_correlation_id()static method toAsyncPollingStrategythat tries the response body first, then falls back toparam_value_map_handle_polling_initial_requestand_handle_polling_initial_erasure_requestto use the new shared methodSteps to Confirm
pytest tests/ops/service/async_dsr/polling/test_async_polling_strategy.py -vpytest tests/ops/service/async_dsr/polling/test_async_polling_strategy.py -k test_extract_correlation_id -vPre-Merge Checklist
CHANGELOG.mdupdated