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

Allow preparing of Requests from Session settings without sending. #1507

Merged
merged 6 commits into from Aug 1, 2013

Conversation

Projects
None yet
4 participants
@erydo
Contributor

erydo commented Jul 31, 2013

Attempt to address #1445. All tests pass.

Note that I think it could likely make sense to change Session.update_request to be an internal method, since Session.prepare_request is really the only public use I can think of having use in practice.

erydo added some commits Jul 31, 2013

Shallow copy of Request fields in Request.copy()
This prevents e.g. modifying the headers of a copied request from
affecting the headers of its source and vice versa. Copying is used with
the intent to mutuate, so allowing this kind of mutation of fields makes
sense.

Is a deep copy better?
@Lukasa

This comment has been minimized.

Show comment
Hide comment
@Lukasa

Lukasa Jul 31, 2013

Member

In general I think this PR is good. =) I've got a few notes though.

  1. Why does prepare_request copy the request? I don't think I'd expect an allocation to happen here, myself.
  2. I don't think we need to expose both update_request and prepare_request on the Session. If it were me, my vote would be for exposing update_request (and then inlining prepare_request).
  3. I'm not sure update_request is the best name, though I can't think of a better one off the top if my head. =)

This is excellent work, thankyou! 🍪

Member

Lukasa commented Jul 31, 2013

In general I think this PR is good. =) I've got a few notes though.

  1. Why does prepare_request copy the request? I don't think I'd expect an allocation to happen here, myself.
  2. I don't think we need to expose both update_request and prepare_request on the Session. If it were me, my vote would be for exposing update_request (and then inlining prepare_request).
  3. I'm not sure update_request is the best name, though I can't think of a better one off the top if my head. =)

This is excellent work, thankyou! 🍪

@erydo

This comment has been minimized.

Show comment
Hide comment
@erydo

erydo Jul 31, 2013

Contributor
  • In the use-case described in #1445, I would not expect Session.prepare_request to alter any of the members of the originating request. The alternative to making a copy has strange and undesirable behaviour:
session1 = Session()
session1.auth = AuthA()

session2 = Session()
session2.auth = AuthB()

# r.auth starts off as None
r = Request('GET', url)

# r.auth will be unexpectedly modified!
prep1 = session1.prepare_request(r)

# Sent with AuthA, as expected.
session1.send(prep1)

prep2 = session2.prepare_request(r)
# Also sent with AuthA, not session2's AuthB! Unexpected.
session2.send(prep2)

Note that prepare_request() is a new user method, not called internally; Session.request() only uses update_request() and doesn't make any new allocations.

Maybe create_prepared_request would be a more clear name?

  • I agree that it's not great having both update_request and prepare_request exposed. If I were to choose one, though, it would be prepare_request (possibly renamed to create_prepared_request). Since update_request destructively modifies its argument, which makes bugs like the above easy to introduce, I consider it the less friendly/safe of the two. I split it out that way only to avoid creating a temporary Request instance in Session.request() that would otherwise be immediately copied. I can't really think of another use-case where a user would need update_request where prepare_request wouldn't do.
  • I agree…though if it can be made private it matters a little less?
Contributor

erydo commented Jul 31, 2013

  • In the use-case described in #1445, I would not expect Session.prepare_request to alter any of the members of the originating request. The alternative to making a copy has strange and undesirable behaviour:
session1 = Session()
session1.auth = AuthA()

session2 = Session()
session2.auth = AuthB()

# r.auth starts off as None
r = Request('GET', url)

# r.auth will be unexpectedly modified!
prep1 = session1.prepare_request(r)

# Sent with AuthA, as expected.
session1.send(prep1)

prep2 = session2.prepare_request(r)
# Also sent with AuthA, not session2's AuthB! Unexpected.
session2.send(prep2)

Note that prepare_request() is a new user method, not called internally; Session.request() only uses update_request() and doesn't make any new allocations.

Maybe create_prepared_request would be a more clear name?

  • I agree that it's not great having both update_request and prepare_request exposed. If I were to choose one, though, it would be prepare_request (possibly renamed to create_prepared_request). Since update_request destructively modifies its argument, which makes bugs like the above easy to introduce, I consider it the less friendly/safe of the two. I split it out that way only to avoid creating a temporary Request instance in Session.request() that would otherwise be immediately copied. I can't really think of another use-case where a user would need update_request where prepare_request wouldn't do.
  • I agree…though if it can be made private it matters a little less?
@Lukasa

This comment has been minimized.

Show comment
Hide comment
@Lukasa

Lukasa Jul 31, 2013

Member

Mm. I agree that you can see many situations in which modifying the Request itself creates unexpected behaviour. Conversely, though, I can definitely see situations in which prepare_request's allocation hurts you: specifically, situations when you have only one Session. Then you have to absorb two allocations per request instead of one, which seems like a nasty waste of time for the GC.

My engineering inclination is to leave the Request.copy method in, and say that by default we don't do any extra allocations, and if you want to you have to do it yourself. This turns your code into:

session1 = Session()
session1.auth = AuthA()

session2 = Session()
session2.auth = AuthB()

# r.auth starts off as None
r = Request('GET', url)

# r.auth will now not be modified
prep1 = session1.prepare_request(r.copy())

# Sent with AuthA, as expected.
session1.send(prep1)

prep2 = session2.prepare_request(r.copy())

# Now sent with AuthB
session2.send(prep2)

The problem is, I think that the API isn't as good here. I'm pretty torn. My intuition is that the general use-case does not have the same unprepared Request being sent to different Sessions, so we should optimise for that case. I'm prepared to be wrong here, though. @sigmavirus24, thoughts?

EDIT: I should note that while the cost of extra allocations is usually not that high, it's inescapable, while the bug introduced by failing to copy a Request can be easily worked around.

Member

Lukasa commented Jul 31, 2013

Mm. I agree that you can see many situations in which modifying the Request itself creates unexpected behaviour. Conversely, though, I can definitely see situations in which prepare_request's allocation hurts you: specifically, situations when you have only one Session. Then you have to absorb two allocations per request instead of one, which seems like a nasty waste of time for the GC.

My engineering inclination is to leave the Request.copy method in, and say that by default we don't do any extra allocations, and if you want to you have to do it yourself. This turns your code into:

session1 = Session()
session1.auth = AuthA()

session2 = Session()
session2.auth = AuthB()

# r.auth starts off as None
r = Request('GET', url)

# r.auth will now not be modified
prep1 = session1.prepare_request(r.copy())

# Sent with AuthA, as expected.
session1.send(prep1)

prep2 = session2.prepare_request(r.copy())

# Now sent with AuthB
session2.send(prep2)

The problem is, I think that the API isn't as good here. I'm pretty torn. My intuition is that the general use-case does not have the same unprepared Request being sent to different Sessions, so we should optimise for that case. I'm prepared to be wrong here, though. @sigmavirus24, thoughts?

EDIT: I should note that while the cost of extra allocations is usually not that high, it's inescapable, while the bug introduced by failing to copy a Request can be easily worked around.

@erydo

This comment has been minimized.

Show comment
Hide comment
@erydo

erydo Jul 31, 2013

Contributor

Two alternative ideas:

Instead of calling Request.prepare(), create the PreparedRequest directly in Session.prepare_request() and pass in the merged arguments, e.g.

# in sessions.py, class Session
def prepare_request(self, request)
    p = PreparedRequest()

    p.prepare_method(request.method)
    p.prepare_url(request.url, request.params)
    p.prepare_headers(merge_setting(request.headers, self.headers))
    p.prepare_cookies(merge_setting(request.cookies, self.cookies))
    p.prepare_body(request.data, request.files)
    p.prepare_auth(request.auth or self.auth, request.url)
    p.prepare_hooks(request.hooks)
    return p

Another would be to add those as **kwargs into Request.prepare() to keep that code centralized:

# in sessions.py, class Session
def prepare_request(self, request):
    return request.prepare(
        auth=self.auth,
        cookies=self.cookies,
        # etc.
    )
Contributor

erydo commented Jul 31, 2013

Two alternative ideas:

Instead of calling Request.prepare(), create the PreparedRequest directly in Session.prepare_request() and pass in the merged arguments, e.g.

# in sessions.py, class Session
def prepare_request(self, request)
    p = PreparedRequest()

    p.prepare_method(request.method)
    p.prepare_url(request.url, request.params)
    p.prepare_headers(merge_setting(request.headers, self.headers))
    p.prepare_cookies(merge_setting(request.cookies, self.cookies))
    p.prepare_body(request.data, request.files)
    p.prepare_auth(request.auth or self.auth, request.url)
    p.prepare_hooks(request.hooks)
    return p

Another would be to add those as **kwargs into Request.prepare() to keep that code centralized:

# in sessions.py, class Session
def prepare_request(self, request):
    return request.prepare(
        auth=self.auth,
        cookies=self.cookies,
        # etc.
    )
@Lukasa

This comment has been minimized.

Show comment
Hide comment
@Lukasa

Lukasa Jul 31, 2013

Member

Both of these solutions seem like unnecessary hoops to leap through. Preparing a request is conceptually non-destructive, so it's not the problem here. The problem is only whether we think it's OK to mutate a Request passed to a Session. If it isn't, then doing a copy is mandatory, even if we don't like the allocation. If it is, then we can avoid the allocation by requiring users to copy their Request.

Member

Lukasa commented Jul 31, 2013

Both of these solutions seem like unnecessary hoops to leap through. Preparing a request is conceptually non-destructive, so it's not the problem here. The problem is only whether we think it's OK to mutate a Request passed to a Session. If it isn't, then doing a copy is mandatory, even if we don't like the allocation. If it is, then we can avoid the allocation by requiring users to copy their Request.

@erydo

This comment has been minimized.

Show comment
Hide comment
@erydo

erydo Jul 31, 2013

Contributor

I'm not sure I clarified my thought process there properly.

The point of the above is that it supports the same behaviour as copying the Request but without the extra allocation. The only reason to copy the Request is to modify it safely, but the only reason it needs to be modified is to change what happens when you call prepare on it. Essentially the Request object is acting as implicit arguments a generic "create a PreparedRequest bundle" function.

What do you think of just moving the preparation into the PreparedRequest constructor?

# class PreparedRequest, models.py
def __init__(self, method, url, headers, files,
             data, params, auth, cookies, hooks):
    self.prepare_method(method)
    self.prepare_url(url, params)
    self.prepare_headers(headers)
    self.prepare_cookies(cookies)
    self.prepare_body(data, files)
    self.prepare_auth(auth, url)
    self.prepare_hooks(hooks)
# class Request, models.py
def prepare(self):
    """Constructs a :class:`PreparedRequest <PreparedRequest>` for transmission and returns it."""
    return PreparedRequest(
        method=self.method,
        url=self.url,
        headers=self.headers,
        files=self.files,
        data=self.data,
        params=self.params,
        auth=self.auth,
        cookies=self.cookies,
        hooks=self.hooks,
    )
# class Session, sessions.py
def prepare_request(self, request):
    return PreparedRequest(
        method=request.method,
        url=request.url,
        headers=merge_setting(request.headers, self.headers),
        files=request.files,
        data=request.data,
        params=request.params,
        auth=merge_setting(request.auth, self.auth),
        cookies=merge_cookies(request.cookies, self.cookies),
        hooks=merge_setting(request.hooks, self.hooks),
    )
Contributor

erydo commented Jul 31, 2013

I'm not sure I clarified my thought process there properly.

The point of the above is that it supports the same behaviour as copying the Request but without the extra allocation. The only reason to copy the Request is to modify it safely, but the only reason it needs to be modified is to change what happens when you call prepare on it. Essentially the Request object is acting as implicit arguments a generic "create a PreparedRequest bundle" function.

What do you think of just moving the preparation into the PreparedRequest constructor?

# class PreparedRequest, models.py
def __init__(self, method, url, headers, files,
             data, params, auth, cookies, hooks):
    self.prepare_method(method)
    self.prepare_url(url, params)
    self.prepare_headers(headers)
    self.prepare_cookies(cookies)
    self.prepare_body(data, files)
    self.prepare_auth(auth, url)
    self.prepare_hooks(hooks)
# class Request, models.py
def prepare(self):
    """Constructs a :class:`PreparedRequest <PreparedRequest>` for transmission and returns it."""
    return PreparedRequest(
        method=self.method,
        url=self.url,
        headers=self.headers,
        files=self.files,
        data=self.data,
        params=self.params,
        auth=self.auth,
        cookies=self.cookies,
        hooks=self.hooks,
    )
# class Session, sessions.py
def prepare_request(self, request):
    return PreparedRequest(
        method=request.method,
        url=request.url,
        headers=merge_setting(request.headers, self.headers),
        files=request.files,
        data=request.data,
        params=request.params,
        auth=merge_setting(request.auth, self.auth),
        cookies=merge_cookies(request.cookies, self.cookies),
        hooks=merge_setting(request.hooks, self.hooks),
    )
@Lukasa

This comment has been minimized.

Show comment
Hide comment
@Lukasa

Lukasa Jul 31, 2013

Member

That's understandable, but I did get what you meant. =) My key argument here is that preparing a Request is orthogonal to this problem. We should be restricting ourselves to thinking about whether or not it's OK to mutate the original Request when passed to the Session. =D

Member

Lukasa commented Jul 31, 2013

That's understandable, but I did get what you meant. =) My key argument here is that preparing a Request is orthogonal to this problem. We should be restricting ourselves to thinking about whether or not it's OK to mutate the original Request when passed to the Session. =D

@erydo

This comment has been minimized.

Show comment
Hide comment
@erydo

erydo Jul 31, 2013

Contributor

Roger :) The way I think of a Request is as an abstract descriptor of "how to make a request from an HTTP endpoint", and not tied to the particular instance of sending it nor even the session. From that perspective, mutating a user-supplied Request object under the covers when you're preparing for a specific dispatch seems wrong.

The problem trying to be solved here is that there's currently no way to take a user-created Request object and send it with a session whilst having those session parameters (specifically the auth setting) applied to the request.

If it's not okay to modify the Request object—and I believe it is not ok—then the only solutions are:

  1. Copy the Request before mutating that internal copy, as in the original PR.
  2. Merge the Session's settings with that of the Request during the creation of the PreparedRequest, as in my previous comment.
  3. Modify the PreparedRequest itself, which I believe is infeasible to do correctly.

I believe number 2 is the correct solution, based on my understanding of the abstractions involved. It introduces no extra allocations and the request-preparation can be isolated to the PreparedRequest constructor where it probably ought to have been anyway.

I'd actually like to update the PR with that new thinking—it doesn't need to add a potentially-error-prone Request.copy() method, doesn't need to introduce a Session.update_request() method, and overall seems better to me.

Contributor

erydo commented Jul 31, 2013

Roger :) The way I think of a Request is as an abstract descriptor of "how to make a request from an HTTP endpoint", and not tied to the particular instance of sending it nor even the session. From that perspective, mutating a user-supplied Request object under the covers when you're preparing for a specific dispatch seems wrong.

The problem trying to be solved here is that there's currently no way to take a user-created Request object and send it with a session whilst having those session parameters (specifically the auth setting) applied to the request.

If it's not okay to modify the Request object—and I believe it is not ok—then the only solutions are:

  1. Copy the Request before mutating that internal copy, as in the original PR.
  2. Merge the Session's settings with that of the Request during the creation of the PreparedRequest, as in my previous comment.
  3. Modify the PreparedRequest itself, which I believe is infeasible to do correctly.

I believe number 2 is the correct solution, based on my understanding of the abstractions involved. It introduces no extra allocations and the request-preparation can be isolated to the PreparedRequest constructor where it probably ought to have been anyway.

I'd actually like to update the PR with that new thinking—it doesn't need to add a potentially-error-prone Request.copy() method, doesn't need to introduce a Session.update_request() method, and overall seems better to me.

@@ -422,7 +441,7 @@ def send(self, request, **kwargs):
# It's possible that users might accidentally send a Request object.
# Guard against that specific failure case.
if getattr(request, 'prepare', None):
if not isinstance(request, PreparedRequest):
raise ValueError('You can only send PreparedRequests.')

This comment has been minimized.

@erydo

erydo Jul 31, 2013

Contributor

Out of curiousity, why doesn't this just call request.prepare() (or self.prepare_request(request)) instead of failing?

@erydo

erydo Jul 31, 2013

Contributor

Out of curiousity, why doesn't this just call request.prepare() (or self.prepare_request(request)) instead of failing?

This comment has been minimized.

@sigmavirus24

sigmavirus24 Aug 1, 2013

Member

Why would we raise such a horrible stacktrace when we can raise a much cleaner one and provide the user with an exact message as to why their request cannot be sent?

@sigmavirus24

sigmavirus24 Aug 1, 2013

Member

Why would we raise such a horrible stacktrace when we can raise a much cleaner one and provide the user with an exact message as to why their request cannot be sent?

This comment has been minimized.

@erydo

erydo Aug 1, 2013

Contributor

I meant this:

if isinstance(request, Request):
    request = request.prepare()
elif not isinstance(request, PreparedRequest):
    raise ValueError('You can only send Requests or PreparedRequests')

This is outside this PR though; not important.

@erydo

erydo Aug 1, 2013

Contributor

I meant this:

if isinstance(request, Request):
    request = request.prepare()
elif not isinstance(request, PreparedRequest):
    raise ValueError('You can only send Requests or PreparedRequests')

This is outside this PR though; not important.

This comment has been minimized.

@sigmavirus24

sigmavirus24 Aug 1, 2013

Member

Why would we contradict ourselves in the documentation then? We documented the API before we made this change. This was to ensure the API (and its documentation) was correct.

@sigmavirus24

sigmavirus24 Aug 1, 2013

Member

Why would we contradict ourselves in the documentation then? We documented the API before we made this change. This was to ensure the API (and its documentation) was correct.

This comment has been minimized.

@erydo

erydo Aug 1, 2013

Contributor

I can't seem to find where that restriction is documented. That said, my question was more along the lines of why that restriction was designed in without supporting Request objects in the first place. I suspect that most users using Request objects only create PreparedRequest with directly before calling Session.send(), without modifying it, and so that extra hoop just gets in the way of the common case. It seemed like an artificial obstacle.

Avoiding the burden lf supporting multiple argument types was the kind of rationale I expected; because it was documented before it was designed isn't.

However: this doesn't affect my use case and I was just trying to understand the design decision better. Thanks for your answer.

@erydo

erydo Aug 1, 2013

Contributor

I can't seem to find where that restriction is documented. That said, my question was more along the lines of why that restriction was designed in without supporting Request objects in the first place. I suspect that most users using Request objects only create PreparedRequest with directly before calling Session.send(), without modifying it, and so that extra hoop just gets in the way of the common case. It seemed like an artificial obstacle.

Avoiding the burden lf supporting multiple argument types was the kind of rationale I expected; because it was documented before it was designed isn't.

However: this doesn't affect my use case and I was just trying to understand the design decision better. Thanks for your answer.

This comment has been minimized.

@Lukasa

Lukasa Aug 1, 2013

Member

The reasoning, essentially, is that Requests assumes that if you're building Request and PreparedRequest objects yourself, you are doing everything about preparing them yourself. The Request -> PreparedRequest flow is basically an advanced use case, when you need to work around or change something Requests does internally.

When considered in that light, passing a Request to Session.send() is likely to be a mistake: you have a flow through your code where the Request doesn't get prepared prior to sending. That was the purpose of this exception: rather than quietly succeed but potentially do something wrong, we assume that advanced users will just fix the exception.

@Lukasa

Lukasa Aug 1, 2013

Member

The reasoning, essentially, is that Requests assumes that if you're building Request and PreparedRequest objects yourself, you are doing everything about preparing them yourself. The Request -> PreparedRequest flow is basically an advanced use case, when you need to work around or change something Requests does internally.

When considered in that light, passing a Request to Session.send() is likely to be a mistake: you have a flow through your code where the Request doesn't get prepared prior to sending. That was the purpose of this exception: rather than quietly succeed but potentially do something wrong, we assume that advanced users will just fix the exception.

@sigmavirus24

This comment has been minimized.

Show comment
Hide comment
@sigmavirus24

sigmavirus24 Aug 1, 2013

Member

Here's my hang-up. It consists of two parts:

  1. 90% of our users won't ever need this as such, our existing strategy is to use Request objects as organizational throw away objects, as such we never present them to the user directly as part of a Response. What we do allow is for a user to replicate what we do in order to send a PreparedRequest. So as the API is concerned, the most import objects are the Session and Response object. Next most important are PreparedRequest objects because those are directly exposed via the Response object and finally the Request object. I fail to see how we can not just explicitly document for the user that the Request object will be mutated. We're not using a purely functional language so there's no reasonable expectation that the Request will not be mutated.

  2. The second part, contingent on that first, is that when you prepare a request, you're not going to get it back and so it may be mutated. If you're using this advanced API then you should have read the docs where we can explicitly document that those objects will be mutated. Does this hamper your use case? Yes. Does it meet the needs of what is likely > 90% of our users? More emphatically, yes.

I haven't skimmed your PR because it seems (from the conversation that I've read via email) that it's very much in flux. That said, if you can sell @Lukasa and me on why we need Ruby-ish methods here, we can probably sell @kennethreitz and frankly you haven't sold me.

Member

sigmavirus24 commented Aug 1, 2013

Here's my hang-up. It consists of two parts:

  1. 90% of our users won't ever need this as such, our existing strategy is to use Request objects as organizational throw away objects, as such we never present them to the user directly as part of a Response. What we do allow is for a user to replicate what we do in order to send a PreparedRequest. So as the API is concerned, the most import objects are the Session and Response object. Next most important are PreparedRequest objects because those are directly exposed via the Response object and finally the Request object. I fail to see how we can not just explicitly document for the user that the Request object will be mutated. We're not using a purely functional language so there's no reasonable expectation that the Request will not be mutated.

  2. The second part, contingent on that first, is that when you prepare a request, you're not going to get it back and so it may be mutated. If you're using this advanced API then you should have read the docs where we can explicitly document that those objects will be mutated. Does this hamper your use case? Yes. Does it meet the needs of what is likely > 90% of our users? More emphatically, yes.

I haven't skimmed your PR because it seems (from the conversation that I've read via email) that it's very much in flux. That said, if you can sell @Lukasa and me on why we need Ruby-ish methods here, we can probably sell @kennethreitz and frankly you haven't sold me.

@kennethreitz

This comment has been minimized.

Show comment
Hide comment
@kennethreitz

kennethreitz Aug 1, 2013

Member

There are way too many words on this page for me to read.

We need a Session.prepare in addition to a Request.prepare.

Member

kennethreitz commented Aug 1, 2013

There are way too many words on this page for me to read.

We need a Session.prepare in addition to a Request.prepare.

@erydo

This comment has been minimized.

Show comment
Hide comment
@erydo

erydo Aug 1, 2013

Contributor

@kennethreitz That's what this diff does.

Contributor

erydo commented Aug 1, 2013

@kennethreitz That's what this diff does.

@kennethreitz

This comment has been minimized.

Show comment
Hide comment
@kennethreitz
Member

kennethreitz commented Aug 1, 2013

🍰

kennethreitz added a commit that referenced this pull request Aug 1, 2013

Merge pull request #1507 from buzztabapp/prepared-session-requests
Allow preparing of Requests from Session settings without sending.

@kennethreitz kennethreitz merged commit 3e6e68b into requests:master Aug 1, 2013

1 check passed

default The Travis CI build passed
Details
@Lukasa

This comment has been minimized.

Show comment
Hide comment
@Lukasa

Lukasa Aug 1, 2013

Member

@erydo Despite what it looks like above, I really did want this change. =) Just wanted the best possible form of it. I think, looking at the diff that got merged, this is probably either that or extremely close to it. Thanks so much for your work! (And for putting up with my constant badgering. 😁)

Member

Lukasa commented Aug 1, 2013

@erydo Despite what it looks like above, I really did want this change. =) Just wanted the best possible form of it. I think, looking at the diff that got merged, this is probably either that or extremely close to it. Thanks so much for your work! (And for putting up with my constant badgering. 😁)

@erydo

This comment has been minimized.

Show comment
Hide comment
@erydo

erydo Aug 1, 2013

Contributor

@Lukasa I actually very much appreciate the level of feedback on this :) And I'm glad it ended up with a better solution than my original suggestion. 👍 Thanks!

Contributor

erydo commented Aug 1, 2013

@Lukasa I actually very much appreciate the level of feedback on this :) And I'm glad it ended up with a better solution than my original suggestion. 👍 Thanks!

@erydo erydo deleted the preo:prepared-session-requests branch Aug 1, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment