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.

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

PreparedRequest check: use duck typing? #3102

Closed
kevinburke opened this issue Apr 18, 2016 · 2 comments
Closed

PreparedRequest check: use duck typing? #3102

kevinburke opened this issue Apr 18, 2016 · 2 comments

Comments

@kevinburke
Copy link
Contributor

kevinburke commented Apr 18, 2016

Okay, this is admittedly an edge case.

I was attempting to deploy to AWS from a CI server, and boto3 was returning this stack trace:

Traceback (most recent call last):
  File "deploy.py", line 43, in <module>
    main(args.realm)
  File "deploy.py", line 29, in main
    results = [client.update_function_code(FunctionName=arn, ZipFile=data) for arn in arns]
  File "/home/ubuntu/webhooks/src/botocore/botocore/client.py", line 236, in _api_call
    return self._make_api_call(operation_name, kwargs)
  File "/home/ubuntu/webhooks/src/botocore/botocore/client.py", line 489, in _make_api_call
    operation_model, request_dict)
  File "/home/ubuntu/webhooks/src/botocore/endpoint.py", line 117, in make_request
    return self._send_request(request_dict, operation_model)
  File "/home/ubuntu/webhooks/src/botocore/endpoint.py", line 146, in _send_request
    success_response, exception):
  File "/home/ubuntu/webhooks/src/botocore/endpoint.py", line 219, in _needs_retry
    caught_exception=caught_exception)
  File "/home/ubuntu/webhooks/src/botocore/botocore/hooks.py", line 226, in emit
    return self._emit(event_name, kwargs)
  File "/home/ubuntu/webhooks/src/botocore/botocore/hooks.py", line 209, in _emit
    response = handler(**kwargs)
  File "/home/ubuntu/webhooks/src/botocore/botocore/retryhandler.py", line 183, in __call__
    if self._checker(attempts, response, caught_exception):
  File "/home/ubuntu/webhooks/src/botocore/botocore/retryhandler.py", line 250, in __call__
    caught_exception)
  File "/home/ubuntu/webhooks/src/botocore/botocore/retryhandler.py", line 265, in _should_retry
    return self._checker(attempt_number, response, caught_exception)
  File "/home/ubuntu/webhooks/src/botocore/botocore/retryhandler.py", line 313, in __call__
    caught_exception)
  File "/home/ubuntu/webhooks/src/botocore/botocore/retryhandler.py", line 222, in __call__
    return self._check_caught_exception(attempt_number, caught_exception)
  File "/home/ubuntu/webhooks/src/botocore/botocore/retryhandler.py", line 355, in _check_caught_exception
    raise caught_exception
ValueError: You can only send PreparedRequests.

The related code in Requests is here:

        # It's possible that users might accidentally send a Request object.
        # Guard against that specific failure case.
        if not isinstance(request, PreparedRequest):
            raise ValueError('You can only send PreparedRequests.')

This was failing because unbeknownst to me, the server was running an outdated version of pip, vulnerable to this issue, which creates multiple subdirectories (src/requests, src/requests/requests) when you rerun pip install --target: pypa/pip#1489. So the PreparedRequest was being created in one version of the library and compared against the other version of the library, and throwing this exception, even though they were both PreparedRequest instances!

I'm not sure of the best reference here, but as I understand it, it's preferable to check an object's behavior rather than its type, see for example Alex Martelli's message here:

In other words, don't check whether it IS-a duck: check
whether it QUACKS-like-a duck, WALKS-like-a duck,
etc, etc, depending on exactly what subset of duck-like
behaviour you need to play your language-games with.

I may be the only human in the history of existence to hit this exact edge case, but it seems like Requests could still perform the request. Is there a better way to check for the necessary behaviour?

@Lukasa
Copy link
Member

Lukasa commented Apr 18, 2016

That's a good question.

Generally speaking it seems like a bad idea to pass objects from one form of a library to another form (it's not even really possible to do in Python unless the namespacing has gone all to hell. One way we could resolve this problem is to flip the check to catch the specific error case we're explicitly worried about (isinstance(request, Request)). That might be the least destructive way to avoid this problem.

@kevinburke
Copy link
Contributor Author

Sounds good!

kevinburke added a commit to kevinburke/requests that referenced this issue Apr 21, 2016
Previously we checked that the `request` being sent was an instance of a
PreparedRequest. If a user somehow created a PreparedRequest using a different
Requests library instance, this check makes the request un-sendable.

(This happened recently - unbeknownst to me, my server was running an outdated
version of pip, vulnerable to this issue - pypa/pip#1489, which creates
multiple subdirectories (src/requests, src/requests/requests) when you rerun
pip install --target. So the PreparedRequest was being created in one version
of the library and compared against the other version of the library, and
throwing this exception, even though they were both PreparedRequest instances!)

It would probably be preferable to check the object's behavior (instead of
its type), but a PreparedRequest has a lot of behavior, and it wouldn't be
really feasible or allow us to provide a helpful error message to check all
of it here. Instead flip the conditional to guard against the user sending an
unprepared Request, which should still give us most of the benefits of the
better error message.

Fixes psf#3102
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants