Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix UnboundLocalError #19744

Merged
merged 21 commits into from
Nov 2, 2021
Merged

fix UnboundLocalError #19744

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
127f94a
fix UnboundLocalError
xiangyan99 Jul 9, 2021
44023ca
update
xiangyan99 Jul 9, 2021
1bf46b6
update
xiangyan99 Jul 9, 2021
e4db6df
update
xiangyan99 Jul 9, 2021
386976b
update
xiangyan99 Jul 9, 2021
decabff
update
xiangyan99 Jul 9, 2021
b1689c9
Merge branch 'main' into core_UnboundLocalError
xiangyan99 Jul 13, 2021
0007de8
Merge branch 'main' into core_UnboundLocalError
xiangyan99 Aug 11, 2021
a501a58
update
xiangyan99 Aug 11, 2021
6ab9c13
update
xiangyan99 Sep 16, 2021
7c34edd
Merge branch 'main' into core_UnboundLocalError
xiangyan99 Sep 16, 2021
4825fbf
update
xiangyan99 Sep 16, 2021
f6de1e3
FI
xiangyan99 Sep 16, 2021
6ca9760
update
xiangyan99 Sep 16, 2021
75631f4
update
xiangyan99 Sep 16, 2021
d768c41
update
xiangyan99 Sep 16, 2021
70c5c97
Merge branch 'main' into core_UnboundLocalError
xiangyan99 Oct 6, 2021
7c67ea1
update
xiangyan99 Oct 25, 2021
ca5dd99
update
xiangyan99 Oct 25, 2021
87249bd
Merge branch 'main' into core_UnboundLocalError
xiangyan99 Nov 2, 2021
0423a11
update
xiangyan99 Nov 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions sdk/core/azure-core/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

### Fixed

- UnboundLocalError when SansIOHTTPPolicy handles an exception #15222

## 1.16.0 (2021-07-01)

Expand Down
3 changes: 2 additions & 1 deletion sdk/core/azure-core/azure/core/pipeline/_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ def send(self, request):
:return: The PipelineResponse object.
:rtype: ~azure.core.pipeline.PipelineResponse
"""
response = None
_await_result(self._policy.on_request, request)
try:
response = self.next.send(request)
Expand All @@ -74,7 +75,7 @@ def send(self, request):
raise
else:
_await_result(self._policy.on_response, request, response)
return response
return response # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

mypy is right to warn about this though. The annotation is wrong: this method now returns Optional[PipelineResponse]

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. But unfortunately, changing return type from PipelineResponse to Optional[PipelineResponse] is a breaking change.

It will break mypy for its consumers.

Copy link
Member

Choose a reason for hiding this comment

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

The code change itself is breaking. Preventing mypy from warning people about it doesn't help.

Copy link
Member Author

Choose a reason for hiding this comment

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

The behavior change is from raise -> return None. This only breaks non-working code. (in 99% of the case)

mypy breaking will be more impactful and breaks working code.

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't make that non-working code work though. It just replaces an UnboundLocalError in this method with an AttributeError somewhere else because none of our code expects Pipeline.run() to return None. Broken code stays broken while working code gains a new failure mode hidden from mypy.



class _TransportRunner(HTTPPolicy):
Expand Down
3 changes: 2 additions & 1 deletion sdk/core/azure-core/azure/core/pipeline/policies/_authentication.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ def send(self, request):
:param request: The pipeline request object
:type request: ~azure.core.pipeline.PipelineRequest
"""
response = None
self.on_request(request)
try:
response = self.next.send(request)
Expand All @@ -136,7 +137,7 @@ def send(self, request):
if not handled:
raise

return response
return response # type: ignore

def on_challenge(self, request, response):
# type: (PipelineRequest, PipelineResponse) -> bool
Expand Down
1 change: 1 addition & 0 deletions sdk/core/azure-core/azure/core/pipeline/policies/_redirect.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ def send(self, request):
:rtype: ~azure.core.pipeline.PipelineResponse
:raises: ~azure.core.exceptions.TooManyRedirectsError if maximum redirects exceeded.
"""
response = None
retryable = True
redirect_settings = self.configure_redirects(request.context.options)
while retryable:
Expand Down
1 change: 1 addition & 0 deletions sdk/core/azure-core/azure/core/pipeline/policies/_redirect_async.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ async def send(self, request): # pylint:disable=invalid-overridden-method
:rtype: ~azure.core.pipeline.PipelineResponse
:raises: ~azure.core.exceptions.TooManyRedirectsError if maximum redirects exceeded.
"""
response = None
redirects_remaining = True
redirect_settings = self.configure_redirects(request.context.options)
while redirects_remaining:
Expand Down
12 changes: 12 additions & 0 deletions sdk/core/azure-core/tests/test_pipeline.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,5 +434,17 @@ def send(*args):
with pytest.raises(ValueError):
client = PipelineClient(base_url="test", policies=policies, per_retry_policies=[foo_policy])

def test_sansiohttppolicy_exception(self):
class ReproPolicy(SansIOHTTPPolicy):
def on_exception(self, request):
return True

class ErrorPolicy(HTTPPolicy):
def send(self, request):
raise Exception("oops")

pipeline = Pipeline(transport=object(), policies=[ReproPolicy(), ErrorPolicy()])
pipeline.run(HttpRequest("GET", "..."))

if __name__ == "__main__":
unittest.main()