Issue 2682 - Fix GAE dispatcher default port handling

modules/rietveld/files/wrapper.py

Index: modules/rietveld/files/wrapper.py
diff --git a/modules/rietveld/files/wrapper.py b/modules/rietveld/files/wrapper.py
index 34ed099a19360cef1cb0bebdc184d0fd77ce1468..ea191713422ec3347b5150e99c9011f6c5ea1784 100644
--- a/modules/rietveld/files/wrapper.py
+++ b/modules/rietveld/files/wrapper.py
@@ -203,6 +203,26 @@ def enable_oauth2(client_id, client_secret, admins):
 
   user_service_stub.UserServiceStub._Dynamic_GetOAuthUser = _Dynamic_GetOAuthUser
 
+def dispatch_tasks_to_user_port():

[Sebastian Noack, 2015/06/23 09:40:27]

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?

[Wladimir Palant, 2015/06/23 09:53:09]

I'd rather go with fix_target_resolution().

[mathias, 2015/06/23 10:13:18]

Done.

[mathias, 2015/06/23 10:13:18]

Done.

+  """
+  By default, the dispatcher assumes port 80 for target authorities that
+  only contain a hostname but no port part. This hard-coded behavior is
+  altered in function dispatch_tasks_to_user_port() so that the port given
+  as --port option to the appserver-script is used instead. Without this
+  monkey-patch, dispatching tasks from an application run behind a HTTP
+  proxy server on port 80 will fail, because applications will omit the
+  default port when addressing resources.
+  """
+  from google.appengine.tools.devappserver2.dispatcher import Dispatcher
+  resolve_target = Dispatcher._resolve_target
+
+  def callback(dispatcher, authority, path):

[Sebastian Noack, 2015/06/23 09:40:27]

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.

[Wladimir Palant, 2015/06/23 09:53:09]

Also, please keep the parameter names of the original function - it's hostname,
not authority.

[mathias, 2015/06/23 10:13:17]

Done.

[mathias, 2015/06/23 10:13:17]

Done.

+    hostname, port = authority.split(":") if ":" in authority else (authority, dispatcher._port)

[Sebastian Noack, 2015/06/23 09:40:27]

It seems that the logic here can be simplified:

  if ":" not in authority:
    authority = "%s:%d" % (authority, dispatcher._port)

[Wladimir Palant, 2015/06/23 09:53:10]

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"

[mathias, 2015/06/23 10:13:18]

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.

[mathias, 2015/06/23 10:13:18]

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?

[Sebastian Noack, 2015/06/23 11:34:05]

I don't think that a bogus dislike of overwriting function arguments justifies
violating the 80 chars per line or adding complexity.

[mathias, 2015/06/23 11:53:45]

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.

[Sebastian Noack, 2015/06/23 12:00:23]

Interesting, since labeling something "bad practice" without any rationale
apparently is totally fine. ;P

+    new_authority = "%s:%d" % (hostname, port)
+    result = resolve_target(dispatcher, new_authority, path)
+    return result

[Wladimir Palant, 2015/06/23 09:53:10]

Nit: drop the intermediate result variable?

  return resolve_target(dispatcher, new_authority, path)

[mathias, 2015/06/23 10:32:42]

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.

[Sebastian Noack, 2015/06/23 11:34:05]

Frankly, I agree with Wladimir here. Bloating your code just to for the sake of
slightly easier debugging seems like a weak argument.

[mathias, 2015/06/23 11:53:45]

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.

[Wladimir Palant, 2015/06/23 11:58:07]

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.

+
+  Dispatcher._resolve_target = callback
 
 if __name__ == '__main__':
   engine_dir = '/opt/google_appengine'
@@ -223,5 +243,6 @@ if __name__ == '__main__':
       config.get('oauth2', 'client_secret'),
       config.get('main', 'admins').split()
     )
+    dispatch_tasks_to_user_port()
 
   execfile(script_file)