fix: preserve text/plain error body in raise_if_error() after stream is consumed#736
fix: preserve text/plain error body in raise_if_error() after stream is consumed#736stvoutsin wants to merge 2 commits intoastropy:mainfrom
Conversation
1feb70f to
90f350b
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #736 +/- ##
=======================================
Coverage 82.62% 82.63%
=======================================
Files 91 91
Lines 10287 10286 -1
=======================================
Hits 8500 8500
+ Misses 1787 1786 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pyvo/dal/query.py
Outdated
| # Cache the response body before returning the raw stream. If the | ||
| # caller consumes the stream before calling raise_if_error() | ||
| # this would lead to an empty error message | ||
| _ = response.content |
There was a problem hiding this comment.
Hu. That's too magic for me. How would this local reference "cache" the response body? I've not researched the implementation of the content attribute in requests; it probably reads the stream as a side effect?
If that's what it is, there should at least be a comment to that effect, but frankly I'd like that to be a lot more explicit. Do we have a strong reason to not digest the respone content to a user-accessible message right here?
There was a problem hiding this comment.
Ok I don't disagree with that take.
The initial approach of accessing response.content worked because requests caches the body as a side effect, but that's implicit and probably fragile.
I've updated the fix to build the DALServiceError eagerly in execute_stream while the response body is still available, rather than deferring to raise_if_error.
Now raise_if_error will just re-raise the constructed exception so there should be no timing dependency on stream consumption now.
Does this seem like a better approach?
Description
DALServiceError raised by
raise_if_errorwas showing an empty messageExample:
DALServiceError: for https://.../syncwhen the caller used the pattern:
The root cause seemed to be that
execute_streamreturns the raw response stream without reading the body.If a caller consumed that stream before calling
raise_if_errortheresponse_textthatDALServiceError.from_exceptreads was empty because presumably the underlying buffer was already exhausted.Fix
The fix reads and caches the response body in the exception handler, before the raw stream is returned to the caller. This ensures response.text remains available when
raise_if_erroris called later regardless of whether the caller has already read the stream.