Opened 8 years ago

Last modified 7 years ago

#2093 enhancement new

URLPath should parse netloc more thoroughly.

Reported by: dreid Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: oubiwann, radix, dreid Branch: branches/urlpath-2093
(diff, github, buildbot, log)
Author: oubiwann Launchpad Bug:

Description

A valid netloc could include up to four individual elements, username:password@host:port. URLPath should parse these elements properly.

Attachments (6)

urlpath.diff (1.7 KB) - added by oubiwann 8 years ago.
diff with the new authority class added
test_paths.diff (2.2 KB) - added by oubiwann 8 years ago.
diff for new test_paths with authority tests
test_paths.2.diff (2.5 KB) - added by oubiwann 8 years ago.
Added tests that actually test the new stuff
test_paths.3.diff (3.4 KB) - added by oubiwann 8 years ago.
a little more coverage on the netloc parsing tests
urlpath.2.diff (2.5 KB) - added by oubiwann 8 years ago.
updated bassed on comments/suggestions so far
test_paths.4.diff (5.3 KB) - added by oubiwann 8 years ago.
completely redone tests based on suggestions/comments

Download all attachments as: .zip

Change History (22)

comment:1 Changed 8 years ago by oubiwann

  • Cc oubiwann added
  • Owner changed from glyph to oubiwann

I will provide a patch (and one for unit tests) for t.p.urlpath that includes this functionality taken from the first class in this recipe:

http://aspn.activestate.com/ASPN/Cookbook/Python/Recipe/473864

comment:2 Changed 8 years ago by oubiwann

  • Status changed from new to assigned

Changed 8 years ago by oubiwann

diff with the new authority class added

Changed 8 years ago by oubiwann

diff for new test_paths with authority tests

comment:3 Changed 8 years ago by oubiwann

Doh. Jumped the gun on the test diff. Still need to add some tests.

Changed 8 years ago by oubiwann

Added tests that actually test the new stuff

comment:4 Changed 8 years ago by oubiwann

  • Cc radix added
  • Keywords review added
  • Owner changed from oubiwann to radix
  • Status changed from assigned to new

radix, we've got a game of hot potato here... or something. Anyway, you were the last person to touch this file -- would you mind reviewing this?

Thanks!

comment:5 Changed 8 years ago by dreid

Positive: It's good that you left self.netloc in place.
Negative: You'll want to add tests for partial constructions of the authority.
Is the authority class really necessary? I guess it's a good abstraction, the name is kind of unclear and overloaded though. Perhaps URIAuthority? I don't know that's just me nitpicking.

Changed 8 years ago by oubiwann

a little more coverage on the netloc parsing tests

comment:6 Changed 8 years ago by dreid

Also what about unparsing the authority?

comment:7 follow-up: Changed 8 years ago by exarkun

  • All the tests are missing docstrings.
  • I don't see any negative path tests. What is the behavior for malformed URLs?
  • The Authority class is very strange. Why not just a parse function?
  • The parse method of Authority doesn't actually parse anything - the init method does.
  • bareword except! Specify the exception expected to be caught there.

comment:8 Changed 8 years ago by oubiwann

  • Keywords review removed
  • Owner changed from radix to oubiwann

Notes from dried in #twisted.web:

[10:57pm] dreid: oubiwann: move Authority.__init__ to Authority.fromString
          (make it a classmethod) and then make Authority.__init__ take
          keyword arguments with None default values for username, password,
          host, and port
[10:58pm] dreid: def unparse(self) should just return a string built from
          the instance variables

comment:9 in reply to: ↑ 7 Changed 8 years ago by oubiwann

  • Status changed from new to assigned

Replying to exarkun:

  • The Authority class is very strange. Why not just a parse function?

There's a chance this could be useful elsewhere...

Changed 8 years ago by oubiwann

updated bassed on comments/suggestions so far

Changed 8 years ago by oubiwann

completely redone tests based on suggestions/comments

comment:10 Changed 8 years ago by oubiwann

  • Cc dreid added

I have some questions about catching exceptions in URIAuthority:

  • what level of checking is appropriate?
  • how smart does its exception handling need to be?
  • does it need to know anything about what users pass? (i.e., "oh, that string is really a URL, not simply the authority section of a URI")

Because right now, it's very, very dumb -- accepting any string, as long as the port portion can be converted into an integer.

comment:11 Changed 8 years ago by dreid

Couple of things:

  1. You shouldn't be catching ValueError for the port. If they pass us a non-integer port number then they should be told about it as soon as possible. (it would likely reveal a bug elsewhere)
  2. URIAuthority should only be concerned with parsing the authority section of the URI
  3. For exarkun and radix: Perhaps URIAuthority should be private?

(It looks like you forgot to update the urlpath diff to not catch ValueError)

comment:12 Changed 7 years ago by oubiwann

  • author set to oubiwann
  • Branch set to branches/urlpath-2093

(In [23216]) Branching to 'urlpath-2093'

comment:13 Changed 7 years ago by oubiwann

  • Keywords review added
  • Owner changed from oubiwann to dreid
  • Status changed from assigned to new

I've applied the patches in order, fixed a small bug, removed the ValueError check, and updated a docstring. This puppy's ready for another review.

comment:14 Changed 7 years ago by glyph

  • Owner dreid deleted

I'm un-assigning this so it'll get reviewed quicker if anyone else has time.

Also, it's worth noting that URLPath is almost entirely redundant with nevow.url and that's also getting some work done on it now:

and I believe some work was actually recently done in another branch to deal with this exact issue (although maybe not, a lot of issues are getting dealt with).

Not to discourage this work, fixes are great, but it would be even greater if we could reduce the amount of redundancy here. I'm glad at least we don't have a twisted.web.url and a twisted.web2.url to worry about too ;-).

comment:15 Changed 7 years ago by therve

  • Keywords review removed
  • Owner set to oubiwann

Here we go:

  • URIAuthority looks broken. fromString modify the class attributes, instead of creating an instance. First argument of classmethod should be called cls (that helps prevent this kind of bug). But I don't really see the point of URIAuthority: its features are only used in the tests, so we may just want a method in the tests. If you want to be able to test it easily, a staticmethod on urlpath would do the same thing IMHO
  • test methods should be named 'test_fooBar'
  • some blank lines missing
  • if I remove the or '' in fromString, nothing breaks. I don't think it's useful.
  • instead of if len(hostport) > 0, please do if hostport
  • URLPathAuthoritySectionTestCase and URIAuthorityClassTestCase are missing docstrings

Thanks!

comment:16 Changed 4 years ago by <automation>

  • Owner oubiwann deleted
Note: See TracTickets for help on using tickets.