Skip to content

fix: raise when presigned URL response is missing or malformed#5073

Merged
aicam merged 10 commits into
apache:mainfrom
Ma77Ball:fix/get_presigned_url_none
May 20, 2026
Merged

fix: raise when presigned URL response is missing or malformed#5073
aicam merged 10 commits into
apache:mainfrom
Ma77Ball:fix/get_presigned_url_none

Conversation

@Ma77Ball
Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

DatasetFileDocument.get_presigned_url previously returned response.json().get("presignedUrl") on a 200 response, so a body that omitted the key (or wasn't valid JSON) silently returned None. This PR parses the JSON inside a try/except block and validates that presignedUrl is a non-empty string; otherwise, it raises RuntimeError with the response body.

Any related issues, documentation, or discussions?

Closes: #4725

How was this PR tested?

Manually reproduced the missing-key case (mocked 200 + empty JSON) and confirmed the new RuntimeError is raised at the presign step instead of a downstream requests.get(None) failure.

Was this PR authored or co-authored using generative AI tooling?

Co-authored with Claude Opus 4.7 in compliance with ASF

@Ma77Ball
Copy link
Copy Markdown
Contributor Author

/request-review @aicam

@github-actions github-actions Bot requested a review from aicam May 15, 2026 08:25
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 15, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 43.02%. Comparing base (13c4836) to head (0c34245).

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5073      +/-   ##
============================================
- Coverage     43.15%   43.02%   -0.13%     
+ Complexity     2213     2201      -12     
============================================
  Files          1045     1045              
  Lines         40353    40147     -206     
  Branches       4267     4229      -38     
============================================
- Hits          17413    17272     -141     
+ Misses        21865    21815      -50     
+ Partials       1075     1060      -15     
Flag Coverage Δ *Carryforward flag
access-control-service 39.53% <ø> (ø) Carriedforward from 6538e20
agent-service 33.72% <ø> (ø) Carriedforward from 6538e20
amber 43.67% <ø> (-0.13%) ⬇️ Carriedforward from 6538e20
computing-unit-managing-service 0.00% <ø> (ø) Carriedforward from 6538e20
config-service 0.00% <ø> (ø) Carriedforward from 6538e20
file-service 32.18% <ø> (ø) Carriedforward from 6538e20
frontend 33.87% <ø> (-0.20%) ⬇️ Carriedforward from 6538e20
python 90.50% <100.00%> (+0.01%) ⬆️
workflow-compiling-service 47.72% <ø> (-9.10%) ⬇️ Carriedforward from 6538e20

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Contributor

@aicam aicam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Replaces a silent None return (which previously caused requests.get(None) downstream in read_file() with a confusing TypeError) with explicit RuntimeErrors for the three failure modes: invalid JSON, missing key, and non-string/empty value. Error messages quote response.text for diagnosability.

Verified locally (Python 3.12)

$ pytest src/test/python/pytexera/storage/test_dataset_file_document.py -v
============================== 21 passed in 0.50s ==============================

All 21 tests pass, including the 3 new negative-path tests (test_raises_when_response_body_lacks_presigned_url_key, test_raises_when_response_body_is_not_valid_json, test_raises_when_presigned_url_is_empty_string, test_raises_when_presigned_url_is_not_a_string).

Tight, focused fix.

@aicam aicam enabled auto-merge (squash) May 15, 2026 23:36
@aicam aicam merged commit 976cdae into apache:main May 20, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DatasetFileDocument.get_presigned_url silently returns None when response lacks presignedUrl

3 participants