Repository navigation
fix(http): pass mypy --strict in the retry handler and pipeline and fix the bugs it shows - #810
Open
Max Azatian (HardMax71) wants to merge 2 commits into
Conversation
…ix the bugs it shows Type MiddlewarePipeline, BaseMiddleware and RetryHandler. This also clears the two errors in AsyncKiotaTransport that came from the untyped pipeline. Part of microsoft#722. Bugs found on the way, each with a test: - MiddlewarePipeline with no middleware deleted a `request_options` header that is never set, so every request raised KeyError. It now drops the request options from `request.extensions`, like BaseMiddleware does. - A middleware span with no parent span got a context with no span in it, so it started a new trace. It now uses the current context, as the docstring says. - RetryHandler never ended the span of an attempt it retried. It now ends it before the retry. - RetryHandler() shared one default RetryHandlerOption between all handlers. Each handler now gets its own. - get_delay_time(retry_count) with the default response=None raised AttributeError. It now uses the backoff. - A Retry-After value that is neither seconds nor an HTTP date raised ValueError and failed the request. It now falls back to the backoff. - RedirectHandler put the redirect requests in response.history, but httpx keeps the earlier responses there. It now stores the responses.
7 of 11 tasks
Contributor
Author
|
hehe, first PR with 7 bugs detected (and fixed). If you need repros for each bug, I'm ready to provide them |
| request, response, current_options, request_options | ||
| ) | ||
| history.append(request) | ||
| history.append(response) |
There was a problem hiding this comment.
I'm not sure why we're attaching an history at all to the response here. If this is not part of a public API, we should probably remove it all together and use a counter instead.
Contributor
Author
There was a problem hiding this comment.
Vincent Biret (@baywet) agreed, it isn't public in practice: httpx's AsyncClient sets response.history itself after the transport returns, so through the adapter it's always []. Replaced the list with a redirect counter, like the C# handler, in 4127807.
…andler httpx's AsyncClient sets response.history itself after the transport returns, so the list RedirectHandler built never reached callers. A redirect counter, like the one in the C# handler, now drives the span name, the redirect count attribute and the error message.
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Eighth item in #722. The retry handler and the middleware pipeline now pass
mypy --strict, and errors inkiota_httpgo from 88 to 33 (mypy 2.4.0). TypingBaseMiddleware.sendalso clears untyped calls in the other handlers.Related Issue
Part of #722.
Demo
Not needed for this change.
Notes
Adding the types showed seven bugs, each fixed with a test.
MiddlewarePipelinewith no middleware raisedKeyErroron every request (L42), and a middleware span with no parent span started a new trace instead of using the current one (L81).RetryHandlershared one default option between handlers (L45), never ended the span of a retried attempt (L91), crashed inget_delay_timewithout a response (L185) and failed the request on aRetry-Afterthat is neither seconds nor a date (L201).RedirectHandlerbuilt a redirect history forresponse.history(L96), but httpx'sAsyncClientsets that field itself after the transport returns, so the list never reached callers. A redirect counter replaces it, like in the C# handler.Testing Instructions
mypy kiota_httppasses inpackages/http/httpx, andmypy --strict kiota_httpshows nothing inmiddleware.py,retry_handler.pyorasync_kiota_transport.py, with 33 errors overall instead of 88.yapf -dr kiota_http,pylintandpytestpass.