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

urllib2 requests history + HEAD support #44649

Closed
koderua mannequin opened this issue Mar 3, 2007 · 26 comments
Closed

urllib2 requests history + HEAD support #44649

koderua mannequin opened this issue Mar 3, 2007 · 26 comments
Assignees
Labels
easy stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@koderua
Copy link
Mannequin

koderua mannequin commented Mar 3, 2007

BPO 1673007
Nosy @orsenthil, @pitrou, @jackdied, @devdanzin, @ezio-melotti, @merwok, @briancurtin, @ipatrol
Files
  • http_history_and_rtype_svn.diff
  • 1673007_methods.patch
  • issue1673007_urllib_Request_method_v1.patch: patch in cpython tree at changeset c1787fa6a3d3 to add "method" parameter to Request constructor
  • issue1673007.diff: Updated patch
  • urllib.request-cleanup.diff
  • urllib.request-doc.diff
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/orsenthil'
    closed_at = <Date 2011-10-16.15:55:00.138>
    created_at = <Date 2007-03-03.14:01:08.000>
    labels = ['easy', 'type-feature', 'library']
    title = 'urllib2 requests history + HEAD support'
    updated_at = <Date 2011-10-18.16:42:25.820>
    user = 'https://bugs.python.org/koderua'

    bugs.python.org fields:

    activity = <Date 2011-10-18.16:42:25.820>
    actor = 'eric.araujo'
    assignee = 'orsenthil'
    closed = True
    closed_date = <Date 2011-10-16.15:55:00.138>
    closer = 'python-dev'
    components = ['Library (Lib)']
    creation = <Date 2007-03-03.14:01:08.000>
    creator = 'koder_ua'
    dependencies = []
    files = ['7818', '18369', '21432', '23366', '23424', '23448']
    hgrepos = []
    issue_num = 1673007
    keywords = ['patch', 'easy', 'needs review']
    message_count = 26.0
    messages = ['52030', '52031', '52032', '81793', '96705', '96706', '96708', '96791', '110528', '112748', '115366', '115370', '115372', '132359', '145294', '145298', '145302', '145329', '145331', '145335', '145337', '145622', '145623', '145686', '145734', '145840']
    nosy_count = 16.0
    nosy_names = ['jjlee', 'jorend', 'orsenthil', 'pitrou', 'dstanek', 'jackdied', 'ajaksu2', 'koder_ua', 'ezio.melotti', 'eric.araujo', 'brian.curtin', 'poke', 'ipatrol', 'santoso.wijaya', 'denversc', 'python-dev']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue1673007'
    versions = ['Python 3.3']

    @koderua
    Copy link
    Mannequin Author

    koderua mannequin commented Mar 3, 2007

    1)Add history off all sent and received headers/requests
    to addinfourl object. Save redirections history too.

    >>> fd = urllib2.urlopen("http://www.python.org/")
    >>> print fd.history[0].request_line
    GET / HTTP/1.1
    >>> print fd.history[0].sended
    [('Accept-Encoding', 'identity'), ('Host', 'www.python.org'), ('Connection', 'cl
    ose'), ('User-Agent', 'Python-urllib/2.6')]

    2)Add support for HEAD (and other) requests:

    >>> fd = urllib2.urlopen("http://www.python.org/",
                             request_cmd = "HEAD")
    >>> print len(fd.read())
    0

    Please send email here:
    koder_dot_mail_at_gmail_dot_com

    @koderua koderua mannequin added stdlib Python modules in the Lib dir labels Mar 3, 2007
    @koderua
    Copy link
    Mannequin Author

    koderua mannequin commented Mar 7, 2007

    Make diff file with svn and delete previos
    File Added: http_history_and_rtype_svn.diff

    @jorend
    Copy link
    Mannequin

    jorend mannequin commented Apr 20, 2007

    koder_ua, what's the history useful for? I'm not against it... exactly... it just seems like I would never use it. If I did want a history of HTTP activity, I would need to maintain it myself. I wouldn't want it tied to a particular object--these urlopen() objects, typically you open them, read them, and then throw them away.

    I'm iffy on the API for 2) as well, but I can see the appeal.

    The thing is... this patch is really, really rough. In one of the tests, an attribute is called "sended_hdrs" in one place and "sended_headers" in another. (Actually it should be called "request_headers". Likewise "recived" and "res" should each be spelled "response".) The whitespace isn't PEP-8. It's also missing a doc patch.

    @devdanzin
    Copy link
    Mannequin

    devdanzin mannequin commented Feb 12, 2009

    Patch has tests too, might need updating.

    :)

    @devdanzin devdanzin mannequin added type-feature A feature request or enhancement labels Feb 12, 2009
    @devdanzin devdanzin mannequin added easy labels Apr 22, 2009
    @orsenthil
    Copy link
    Member

    orsenthil commented Dec 20, 2009

    Having a HEAD request for urllib2 might be a good idea. I shall use this
    patch to add the functionality.

    But, having a history support in the urllib2 module is not a good idea
    IMO. It is best left to the clients which might use urllib2.

    @ezio-melotti
    Copy link
    Member

    ezio-melotti commented Dec 20, 2009

    +1 for HEAD

    @merwok
    Copy link
    Member

    merwok commented Dec 20, 2009

    If you know you want an HEAD request, it means you already know it
    will be an HTTP request, so why not directly use httplib or httplib2
    instead of urllib?

    Aside: s/sended/sent/

    Cheers

    @orsenthil
    Copy link
    Member

    orsenthil commented Dec 22, 2009

    Here is a discussion and explanation from the submitter on what is meant
    by history of requests.
    http://mail.python.org/pipermail/python-dev/2007-March/072069.html

    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Jul 16, 2010

    Seems to be +1 for HEAD, -1 for history, keyword says easy so should be feasable for 3.2. :)

    @dstanek
    Copy link
    Mannequin

    dstanek mannequin commented Aug 4, 2010

    I have attached a patch to add support for HEAD, PUT and DELETE methods. The code review is available here: http://codereview.appspot.com/1696061.

    I have started working on another patch that validates that the method is properly set. For instance, it doesn't make sense to have a HEAD or DELETE with post data. The problem is that the interface is so wide open that it is hard to catch all possible user errors. A user could call Request.__init__ correctly, but then set req.method to an invalid method. If there is some interest I'll finish up the patch.

    @ipatrol
    Copy link
    Mannequin

    ipatrol mannequin commented Sep 2, 2010

    Can this be somehow implemented as a bugfix patch as well on other versions?

    @merwok
    Copy link
    Member

    merwok commented Sep 2, 2010

    Only stable (2.7 and 3.1) and development versions (3.2) get bug fixes.

    @pitrou
    Copy link
    Member

    pitrou commented Sep 2, 2010

    New features can only go in 3.2. From a quick look, the patch looks ok.

    @denversc
    Copy link
    Mannequin

    denversc mannequin commented Mar 27, 2011

    I decided to take a look at this old, forgotten issue and propose an updated patch. I like the submitter's idea that urllib.Request.__init__() should take a "method" parameter to override the return value of get_method(). I've created and attached a patch (issue1673007_urllib_Request_method_v1.patch) which implements this functionality, adds unit tests, and updates the documentation.

    @ezio-melotti
    Copy link
    Member

    ezio-melotti commented Oct 10, 2011

    Attached an updated patch that addresses the comments of Éric in the review and adds an entry to the whatsnew.

    @orsenthil
    Copy link
    Member

    orsenthil commented Oct 10, 2011

    Hi Ezio, I had probably overlooked this one. But It's a very interesting one
    for me. Do you mind if I commit it ?
    On Oct 10, 2011 7:20 PM, "Ezio Melotti" <report@bugs.python.org> wrote:

    Ezio Melotti <ezio.melotti@gmail.com> added the comment:

    Attached an updated patch that addresses the comments of Éric in the review
    and adds an entry to the whatsnew.

    ----------
    assignee: orsenthil -> ezio.melotti
    keywords: +needs review
    nosy: -BreamoreBoy
    stage: patch review -> commit review
    Added file: http://bugs.python.org/file23366/issue1673007.diff


    Python tracker <report@bugs.python.org>
    <http://bugs.python.org/issue1673007\>



    Python-bugs-list mailing list
    Unsubscribe:
    http://mail.python.org/mailman/options/python-bugs-list/senthil%40uthcode.com

    @merwok
    Copy link
    Member

    merwok commented Oct 10, 2011

    I made some more comments.

    @ezio-melotti ezio-melotti removed their assignment Oct 11, 2011
    @ezio-melotti
    Copy link
    Member

    ezio-melotti commented Oct 11, 2011

    We have discussed the API a bit on IRC and these are the outcomes:

    1. should method always have priority or should 'POST' always be used whenever data is passed?
    2. if the method is e.g. 'GET' and data is passed, should an error be raised?
    3. should Request.method be private?
    4. should the value of Request.method be initialized in the __init__ or can it also be None?
    5. if the value of Request.method is always initialized, should we deprecate get_method?

    IMHO

      r = Request(url, data, method=foo)

    should behave like

      class FooRequest(Request):
          def get_method(self):
              return foo
      r = FooRequest(url, data)

    when foo is not None and

      class FooRequest(Request): pass
      r = FooRequest(url, data)

    when foo is None, so the answers to the 5 questions would be

    1. method always has the highest priority, data is used to decide between GET and POST when method is None;
    2. data is simply ignored if the method doesn't use it -- no errors are raised;
    3. maybe, currently is not (see below);
    4. method matches the value passed to the constructor, defaulting to None;
    5. since it's not initialized, get_method shouldn't be deprecated;

    This is also what the patch implements.

    Regarding 3), one might look at Request.method and see None instead of GET/POST, and this might be confusing. We could document that get_method() should be used instead to see what method will be actually used or make Request.method private and avoid the problem. If we do so there won't be a way to change the method after instance creation (but that wasn't even supported before[0], so it's probably not a problem). OTOH, the other args (e.g. data) are available and documented. I'm fine with either documenting that get_method() should be used instead or making Request.method private.

    Another approach is to move the get_method() logic in the __init__, set self.method to method (if specified) or either POST (if there are data) or GET. Then have get_method() simply return self.method (and possibly deprecate it).

    [0]: it's actually possible to replace the get_method() method, but if Request.method is public one could set it to a new value or to None to restore the normal behavior (GET or POST depending on data).

    @orsenthil
    Copy link
    Member

    orsenthil commented Oct 11, 2011

    Our discussion stemmed from this point. If you look at the change proposed, Request class is taking a new parameter by name 'method' and it is initialized to None:

     class Request:
         def __init__(self, url, data=None, headers={},
    -                 origin_req_host=None, unverifiable=False):
    +                 origin_req_host=None, unverifiable=False,
    +                 method=None):

    But that actually defaults to GET method in terms of HTTP request for often used scenarios where the only required parameter (url) is sent.

    This happens in the get_method call:

         def get_method(self):
    -        if self.data is not None:
    +        """Return a string indicating the HTTP request method."""
    +        if self.method is not None:
    +            return self.method
    +        elif self.data is not None:
                 return "POST"
             else:
                 return "GET"

    Since, it is understood that the default action of Request(url) is to do a GET, I proposed that we have Request's method parameter default to GET instead of None, so the change would look like:

     class Request:
         def __init__(self, url, data=None, headers={},
    -                 origin_req_host=None, unverifiable=False):
    +                 origin_req_host=None, unverifiable=False,
    +                 method="GET"):

    And it is more meaningful when someone is looking at the Request signature. Specifying method=None and implicitly meaning it as "GET" for normal situations was not intuitive to me. (This is not case when we do not pass an explicit method arg).

    At this point, Ezio's summary of API changes discussed becomes interesting. I read again and it seems to me that, the assumption is get_method is an important method which determines what method should be used and method should be given preference over data.

    My point is, get_method is an useful, helper function that is helpful is sending the correct method to the http.client code which does the actual task. In the current situation, get_method "determines" based on data parameter should it send a "GET" or a "POST", but if we start using method=arg then, get_method should just return what was initialized by the method arg (defaulting to "GET").

    1. The next problem comes when a user has specified both data and method="GET". This becomes an invalid scenario, but a decision has been to taken as what should be given preference?
    • As the user has sent "data", should the request be considered a POST?

    • But user specified it as "GET" (intentionally or by mistake), so should the data not be used and Request should do only a GET?

    • Or should we throw an error?

    My personal on this is -1 on throwing an error and when data is sent, just do the POST (data overrides method).

    BTW, this needs to discussed irrespective of point 1). But having method="GET" could give raise to his scenario more often. A person would just send data and forget about changing the method to "POST".

    Coming to specific questions which Ezio pointed:

    My take:

    1. should method always have priority or should 'POST' always be used whenever data is passed?

    If data is passed use POST.

    1. if the method is e.g. 'GET' and data is passed, should an error be raised?

    Nope, give data the priority and do POST. (As urllib is currently doing)

    1. should Request.method be private?

    Not necessarily, it should be public.

    1. should the value of Request.method be initialized in the __init__ or can it also be None?

    My take - It should be initialized to default (GET), instead of None.

    1. if the value of Request.method is always initialized, should we deprecate get_method?

    This is an interesting question. get_method essentially becomes less useful or it could serve as an arbiter when data and GET is sent and may be used as reliable way to get the Request's method. It should not be deprecated.

    @poke
    Copy link
    Mannequin

    poke mannequin commented Oct 11, 2011

    Senthil, I highly disagree with what you said:

    The next problem comes when a user has specified both data and method="GET".
    This becomes an invalid scenario, but a decision has been to taken as what
    should be given preference?
    That is incorrect, RFC2616 states that the server should ignore the message body when the request method does not define any semantics for it, but there is nothing that makes the inclusion of a message body along with the GET request method invalid.

    • As the user has sent "data", should the request be considered a POST?
      No, absolutely not. Sending data via other request methods, like DELETE or PUT, is semantically correct and should be supported completely if we are going to include a way to set the request method. If I set the method to PUT, and include data, I don’t want the library to override that to POST just because I set data.
    • But user specified it as "GET" (intentionally or by mistake), so should the
      data not be used and Request should do only a GET?
      If I data is included and the request method is explicitely set to GET, make a GET request and include the data in the request body. It might not be a semantically good decision to send data over GET, but it still is not disallowed and as such should be possible (for whatever reasons).
    • Or should we throw an error?
      We especially should’t throw an error, as this is not invalid at all.

    A person would just send data and forget about changing the method to "POST".
    I agree that the library should still default to POST if data is included and the request method was not explicitely set (see also below).

    1. should method always have priority or should 'POST' always be used whenever
      data is passed?
      If data is passed use POST.
      No, if data is passed and no special method is set, use POST, otherwise use the method the user specified, because that is what he expects.
    1. if the method is e.g. 'GET' and data is passed, should an error be raised?
      Nope, give data the priority and do POST. (As urllib is currently doing)
      Don't give data any priority if the method was set explicitely. Otherwise the ability to set a custom method is totally useless, especially with request methods where a message body is semantically useful (i.e. all other than HEAD and GET).
    1. should Request.method be private?
      Not necessarily, it should be public.
      Depends on the way the method will be set. Looking at the way, the other request parameters are set (especially with the accessors being “deprecated”), it makes sense to leave this public.
    1. should the value of Request.method be initialized in the __init__ or can it
      also be None?
      My take - It should be initialized to default (GET), instead of None.
      Initializing it to GET will make the implementation difficult, especially if we want to continue supporting the behaviour of setting the method to POST when data is changed (and the method was not explicitely set). Unless we override the built-in property accessors I don’t think this is possible.
    1. if the value of Request.method is always initialized, should we deprecate
      get_method?
      This is an interesting question. get_method essentially becomes less useful or
      it could serve as an arbiter when data and GET is sent and may be used as
      reliable way to get the Request's method. It should not be deprecated.
      To my understanding, and this is also why I provided the same patch on the duplicate bug, get_method is used by the other libraries to get the used request method. Unless we change the other libraries to determine the method in a different way, deprecating get_method won’t get us anywhere.

    What I tried to respect with the patch, and I think this was also Denver’s intention, is to add this functionality without breaking the current behaviour. That behaviour is that GET is default, and POST is set as default if there is any data. The functionality requires that the request method can be set (and the default behaviour can be overriden) without looking at the data (as explained above).

    Ideally I would probably like to see the functionality of get_method done in another library, which performs the request. I.e. check request.method and use that if it’s set, otherwise check request.data and choose either POST or GET. But again this would require far too many changes in other libraries for this simple change.

    And again on the data property: I think the name “data” is a bit confusing. Request does not provide any encoding details on that “data”, and it is actually just the request body in its original form. What I usually do in my subclass of Request is to provide a way to encode the data I pass to the constructor (often even with multipart encoding for file streams), while the request.data attribute to me still means “request body”.

    Regarding those questions on the implementation, I agree with Ezio, and as I said this is probably the only way to add this functionality without breaking previous usages. And if we break previous usages (or restrict its functionality with weird priority rules based on the data), we better not add this functionality at all.

    @orsenthil
    Copy link
    Member

    orsenthil commented Oct 11, 2011

    Patrick,

    Lots of valid points. I had not looked at the RFC spec when I mentioned about data over request (GET) method, but was trying to derive the current functionality of module (so that users can have a seamless experience) with additional method="GET" as default. Note, my intention was to be explicit when we give method arg.

    But yeah, when user has specified the methods (PUT/DELETE etc) and given the data, correct rules should apply on how that method should deal with data.

    As you pointed out to RFC, I realize RFC clearly points out the data (message-body) should be ignored and method should be given preference, whenever specification of method has nothing to with data. I should take back my argument on giving data as preference even over GET.

    Now, question arises- Can we in anyway default the method="GET" and maintain compatibility as well consistency with user expectations? At the moment, just by sending data over Request, the method is assumed to be POST. If that is not possible, then the way, current patch does seems to be a good way to acheive the purpose.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 16, 2011

    New changeset 0a0aafaa9bf5 by Senthil Kumaran in branch 'default':
    Fix closes bpo-1673007 urllib.request to support HEAD requests with a new method arg.
    http://hg.python.org/cpython/rev/0a0aafaa9bf5

    @python-dev python-dev mannequin closed this as completed Oct 16, 2011
    @python-dev python-dev mannequin closed this as completed Oct 16, 2011
    @orsenthil
    Copy link
    Member

    orsenthil commented Oct 16, 2011

    I have committed the patch accomodating the doc review comments which Ezio had mentioned. At the moment, the current way seems to be most backwards compatible one and did not want to delay it. Let's hope we get some feedback on this method arg. Thanks!

    @merwok
    Copy link
    Member

    merwok commented Oct 17, 2011

    Attached patch changes one occurrence of ugly whitespace, changes “not x == y” to “x != y” and “not x in y” to “x not in y”. Senthil, feel free to apply none, some or all of these minor cleanups.

    @orsenthil
    Copy link
    Member

    orsenthil commented Oct 17, 2011

    Hi Eric,

    The changes suggested in the patch are good for readability, I
    shall include them all. Thanks!

    @merwok
    Copy link
    Member

    merwok commented Oct 18, 2011

    Doc patch to fix a reST error and tweak a few things.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    easy stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants