Issue 29321013: Issue 2682 - Fix GAE dispatcher default port handling (Closed)
|
Created:
June 23, 2015, 8:04 a.m. by mathias Modified:
July 9, 2015, 8:42 a.m. CC:
Fred, Felix Dahlke Visibility:
Public. |
DescriptionIssue 2682 - Fix GAE dispatcher default port handling
Patch Set 1 #Patch Set 2 : Issue 2682 - Fix GAE dispatcher default port handling #
Total comments: 20
Patch Set 3 : Issue 2682 - Fix GAE dispatcher default port handling #MessagesTotal messages: 19
Removed fragments left from testing but not required
https://codereview.adblockplus.org/29321013/diff/29321016/modules/rietveld/fi... File modules/rietveld/files/wrapper.py (right): https://codereview.adblockplus.org/29321013/diff/29321016/modules/rietveld/fi... modules/rietveld/files/wrapper.py:206: def dispatch_tasks_to_user_port(): This function name is inaccurate. This function doesn't dispatch anything, but patches the dispatcher. How about patch_dispatcher_to_consider_user_port or enable_user_port_for_dispatcher? https://codereview.adblockplus.org/29321013/diff/29321016/modules/rietveld/fi... modules/rietveld/files/wrapper.py:219: def callback(dispatcher, authority, path): It's arguable whether a method override should be called "callback". How about: orig_resolve_target = Dispatcher._resolve_target def resolve_target(dispatcher, authority, path): ... Dispatcher._resolve_target = resolve_target This is the naming scheme we are usually going for when monkey patching. https://codereview.adblockplus.org/29321013/diff/29321016/modules/rietveld/fi... modules/rietveld/files/wrapper.py:220: hostname, port = authority.split(":") if ":" in authority else (authority, dispatcher._port) It seems that the logic here can be simplified: if ":" not in authority: authority = "%s:%d" % (authority, dispatcher._port)
https://codereview.adblockplus.org/29321013/diff/29321016/modules/rietveld/fi... File modules/rietveld/files/wrapper.py (right): https://codereview.adblockplus.org/29321013/diff/29321016/modules/rietveld/fi... modules/rietveld/files/wrapper.py:206: def dispatch_tasks_to_user_port(): I'd rather go with fix_target_resolution(). https://codereview.adblockplus.org/29321013/diff/29321016/modules/rietveld/fi... modules/rietveld/files/wrapper.py:219: def callback(dispatcher, authority, path): Also, please keep the parameter names of the original function - it's hostname, not authority. https://codereview.adblockplus.org/29321013/diff/29321016/modules/rietveld/fi... modules/rietveld/files/wrapper.py:220: hostname, port = authority.split(":") if ":" in authority else (authority, dispatcher._port) On 2015/06/23 09:40:27, Sebastian Noack wrote: > It seems that the logic here can be simplified: > > if ":" not in authority: > authority = "%s:%d" % (authority, dispatcher._port) Actually, that's not quite it. The dispatcher assumes that port is 80 if no port is present. However, in our case it's 443 because we are using SSL. So we should make sure to specify the port explicitly in that case: if ":" not in authority: authority = "%s:443" https://codereview.adblockplus.org/29321013/diff/29321016/modules/rietveld/fi... modules/rietveld/files/wrapper.py:223: return result Nit: drop the intermediate result variable? return resolve_target(dispatcher, new_authority, path)
https://codereview.adblockplus.org/29321013/diff/29321016/modules/rietveld/fi... File modules/rietveld/files/wrapper.py (right): https://codereview.adblockplus.org/29321013/diff/29321016/modules/rietveld/fi... modules/rietveld/files/wrapper.py:206: def dispatch_tasks_to_user_port(): On 2015/06/23 09:40:27, Sebastian Noack wrote: > This function name is inaccurate. This function doesn't dispatch anything, but > patches the dispatcher. How about patch_dispatcher_to_consider_user_port or > enable_user_port_for_dispatcher? Done. https://codereview.adblockplus.org/29321013/diff/29321016/modules/rietveld/fi... modules/rietveld/files/wrapper.py:206: def dispatch_tasks_to_user_port(): On 2015/06/23 09:53:09, Wladimir Palant wrote: > I'd rather go with fix_target_resolution(). Done. https://codereview.adblockplus.org/29321013/diff/29321016/modules/rietveld/fi... modules/rietveld/files/wrapper.py:219: def callback(dispatcher, authority, path): On 2015/06/23 09:40:27, Sebastian Noack wrote: > It's arguable whether a method override should be called "callback". How about: > > orig_resolve_target = Dispatcher._resolve_target > def resolve_target(dispatcher, authority, path): > ... > Dispatcher._resolve_target = resolve_target > > This is the naming scheme we are usually going for when monkey patching. Done. https://codereview.adblockplus.org/29321013/diff/29321016/modules/rietveld/fi... modules/rietveld/files/wrapper.py:219: def callback(dispatcher, authority, path): On 2015/06/23 09:53:09, Wladimir Palant wrote: > Also, please keep the parameter names of the original function - it's hostname, > not authority. Done. https://codereview.adblockplus.org/29321013/diff/29321016/modules/rietveld/fi... modules/rietveld/files/wrapper.py:220: hostname, port = authority.split(":") if ":" in authority else (authority, dispatcher._port) On 2015/06/23 09:40:27, Sebastian Noack wrote: > It seems that the logic here can be simplified: > > if ":" not in authority: > authority = "%s:%d" % (authority, dispatcher._port) Overwriting function arguments is something I consider bad practice for various reasons. But I see what you mean, the patch-set has been updated accordingly. https://codereview.adblockplus.org/29321013/diff/29321016/modules/rietveld/fi... modules/rietveld/files/wrapper.py:220: hostname, port = authority.split(":") if ":" in authority else (authority, dispatcher._port) On 2015/06/23 09:53:10, Wladimir Palant wrote: > On 2015/06/23 09:40:27, Sebastian Noack wrote: > > It seems that the logic here can be simplified: > > > > if ":" not in authority: > > authority = "%s:%d" % (authority, dispatcher._port) > > Actually, that's not quite it. The dispatcher assumes that port is 80 if no port > is present. However, in our case it's 443 because we are using SSL. So we should > make sure to specify the port explicitly in that case: > > if ":" not in authority: > authority = "%s:443" It's not 443 either, we do not allow unauthorized invocations of Rietveld tasks via either port 80 or 443. It's the --port given to dev_appserver.py, as documented (and a bit improved now) above. And instead of parsing sys.argv manually here, why not take it from the dispatcher instance itself?
Sorry, haven't seen the last comment before. https://codereview.adblockplus.org/29321013/diff/29321016/modules/rietveld/fi... File modules/rietveld/files/wrapper.py (right): https://codereview.adblockplus.org/29321013/diff/29321016/modules/rietveld/fi... modules/rietveld/files/wrapper.py:223: return result On 2015/06/23 09:53:10, Wladimir Palant wrote: > Nit: drop the intermediate result variable? > > return resolve_target(dispatcher, new_authority, path) Again something I consider bad practice. Without the intermediate debugging this code and determining the result value requires modification before one can set a proper breakpoint, and even two lines of modification before one can just dump the value. Or searching for each invocation of the original method on Dispatcher instances.
LGTM
https://codereview.adblockplus.org/29321013/diff/29321016/modules/rietveld/fi... File modules/rietveld/files/wrapper.py (right): https://codereview.adblockplus.org/29321013/diff/29321016/modules/rietveld/fi... modules/rietveld/files/wrapper.py:220: hostname, port = authority.split(":") if ":" in authority else (authority, dispatcher._port) On 2015/06/23 10:13:18, mathias wrote: > On 2015/06/23 09:40:27, Sebastian Noack wrote: > > It seems that the logic here can be simplified: > > > > if ":" not in authority: > > authority = "%s:%d" % (authority, dispatcher._port) > > Overwriting function arguments is something I consider bad practice for various > reasons. But I see what you mean, the patch-set has been updated accordingly. I don't think that a bogus dislike of overwriting function arguments justifies violating the 80 chars per line or adding complexity. https://codereview.adblockplus.org/29321013/diff/29321016/modules/rietveld/fi... modules/rietveld/files/wrapper.py:223: return result On 2015/06/23 10:32:42, mathias wrote: > On 2015/06/23 09:53:10, Wladimir Palant wrote: > > Nit: drop the intermediate result variable? > > > > return resolve_target(dispatcher, new_authority, path) > > Again something I consider bad practice. Without the intermediate debugging this > code and determining the result value requires modification before one can set a > proper breakpoint, and even two lines of modification before one can just dump > the value. Or searching for each invocation of the original method on Dispatcher > instances. Frankly, I agree with Wladimir here. Bloating your code just to for the sake of slightly easier debugging seems like a weak argument.
https://codereview.adblockplus.org/29321013/diff/29321016/modules/rietveld/fi... File modules/rietveld/files/wrapper.py (right): https://codereview.adblockplus.org/29321013/diff/29321016/modules/rietveld/fi... modules/rietveld/files/wrapper.py:220: hostname, port = authority.split(":") if ":" in authority else (authority, dispatcher._port) On 2015/06/23 11:34:05, Sebastian Noack wrote: > On 2015/06/23 10:13:18, mathias wrote: > > On 2015/06/23 09:40:27, Sebastian Noack wrote: > > > It seems that the logic here can be simplified: > > > > > > if ":" not in authority: > > > authority = "%s:%d" % (authority, dispatcher._port) > > > > Overwriting function arguments is something I consider bad practice for > various > > reasons. But I see what you mean, the patch-set has been updated accordingly. > > I don't think that a bogus dislike of overwriting function arguments justifies > violating the 80 chars per line or adding complexity. > Again labeling something bogus without any rationale is not an argument in itself. And not hornoring any 80 character limitation is quite consistent with the rest of the script, which you've reviewed yourself before. However, since this obviously boils down to personal opinion but does not change any behavior, I consider this a way too minor issue to argue about. https://codereview.adblockplus.org/29321013/diff/29321016/modules/rietveld/fi... modules/rietveld/files/wrapper.py:223: return result On 2015/06/23 11:34:05, Sebastian Noack wrote: > On 2015/06/23 10:32:42, mathias wrote: > > On 2015/06/23 09:53:10, Wladimir Palant wrote: > > > Nit: drop the intermediate result variable? > > > > > > return resolve_target(dispatcher, new_authority, path) > > > > Again something I consider bad practice. Without the intermediate debugging > this > > code and determining the result value requires modification before one can set > a > > proper breakpoint, and even two lines of modification before one can just dump > > the value. Or searching for each invocation of the original method on > Dispatcher > > instances. > > Frankly, I agree with Wladimir here. Bloating your code just to for the sake of > slightly easier debugging seems like a weak argument. Let me point out at "bloating" is way over the top in this case, and allowing to properly debug code without modification is always a good practice. Anyway, this is yet another minor diversion of opinion that does not seem like it justifies further changes. There's no negative or otherwise significant impact, so either way should be fine.
https://codereview.adblockplus.org/29321013/diff/29321016/modules/rietveld/fi... File modules/rietveld/files/wrapper.py (right): https://codereview.adblockplus.org/29321013/diff/29321016/modules/rietveld/fi... modules/rietveld/files/wrapper.py:223: return result > Anyway, this is yet another minor diversion of opinion that does not seem like > it justifies further changes. There's no negative or otherwise significant > impact, so either way should be fine. Yes, this is primarily a matter of opinion which is why I approved this change nevertheless. Don't be surprised however if I change things my way when I touch that code later.
https://codereview.adblockplus.org/29321013/diff/29321016/modules/rietveld/fi... File modules/rietveld/files/wrapper.py (right): https://codereview.adblockplus.org/29321013/diff/29321016/modules/rietveld/fi... modules/rietveld/files/wrapper.py:220: hostname, port = authority.split(":") if ":" in authority else (authority, dispatcher._port) On 2015/06/23 11:53:45, mathias wrote: > Again labeling something bogus without any rationale is not an argument in > itself. Interesting, since labeling something "bad practice" without any rationale apparently is totally fine. ;P
On 2015/06/23 12:00:24, Sebastian Noack wrote: > https://codereview.adblockplus.org/29321013/diff/29321016/modules/rietveld/fi... > File modules/rietveld/files/wrapper.py (right): > > https://codereview.adblockplus.org/29321013/diff/29321016/modules/rietveld/fi... > modules/rietveld/files/wrapper.py:220: hostname, port = authority.split(":") if > ":" in authority else (authority, dispatcher._port) > On 2015/06/23 11:53:45, mathias wrote: > > Again labeling something bogus without any rationale is not an argument in > > itself. > > Interesting, since labeling something "bad practice" without any rationale > apparently is totally fine. ;P I did not say that this was an argument. I just explained why I don't believe that changing the part in question is necessary. And I've also explained why I don't believe that this is important enough to actually bring up arguments and start a discussion.
On 2015/06/23 12:03:17, mathias wrote: > On 2015/06/23 12:00:24, Sebastian Noack wrote: > > Interesting, since labeling something "bad practice" without any rationale > > apparently is totally fine. ;P > > I did not say that this was an argument. I just explained why I don't believe > that changing the part in question is necessary. And I've also explained why I > don't believe that this is important enough to actually bring up arguments and > start a discussion. And I told you there that I don't think that working around the limitations of debugging tools justifies to bloat your code. Not that this would be a valid argument here anyway, as you can simply set a break point before changing the variable. But anyway, there is no point in continuing this discussion.
Can we agree on this patch being fine as is? The discussion only continues about minor details. This piece of code doesn't have to be polished, it's a hack.
On 2015/07/08 22:06:01, Wladimir Palant wrote: > Can we agree on this patch being fine as is? The discussion only continues about > minor details. This piece of code doesn't have to be polished, it's a hack. I was actually waiting for Felix' LGTM anyway.
On 2015/07/08 22:18:39, mathias wrote: > I was actually waiting for Felix' LGTM anyway. Well, Felix obviously didn't know that he needs to review this - and a change like this usually shouldn't need more than one reviewer anyway.
On 2015/07/08 22:06:01, Wladimir Palant wrote: > Can we agree on this patch being fine as is? The discussion only continues about > minor details. This piece of code doesn't have to be polished, it's a hack. For some reason I assumed that Matze already merged this patch. Yeah, my criticism isn't really important here. So lets just LGTM this.
Very well, I'll push this now. 
|
