Issue 2682 - Fix GAE dispatcher default port handling

modules/rietveld/files/wrapper.py

 #!/usr/bin/env python
 
 from ConfigParser import SafeConfigParser
 import hashlib
 import hmac
 import json
 import os
 import re
 import sys
 import urllib
@@ skipping 185 matching lines @@
 
     response.set_email(email)
     response.set_user_id(user_id)
     response.set_auth_domain(user_service_stub._DEFAULT_AUTH_DOMAIN)
     response.set_is_admin(is_admin)
     response.set_client_id(client_id)
     response.add_scopes(OAUTH2_SCOPE)
 
   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.

+def fix_target_resolution():
   """
   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
+  altered in function fix_target_resolution() 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.
+  proxy server on port 80 (or HTTPS on 443) 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)
+  orig_resolve_target = Dispatcher._resolve_target
+
+  def resolve_target(dispatcher, hostname, path):
+    new_hostname = hostname if ":" in hostname else "%s:%d" % (hostname, dispatcher._port)
+    result = orig_resolve_target(dispatcher, new_hostname, 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
+  Dispatcher._resolve_target = resolve_target
 
 if __name__ == '__main__':
   engine_dir = '/opt/google_appengine'
   storage_path = '/var/lib/rietveld'
 
   script_name, script_file = setup_paths(engine_dir)
   adjust_server_id()
   fix_request_scheme()
 
   if script_name == 'dev_appserver.py':
     config = read_config(os.path.join(storage_path, 'config.ini'))
 
     set_storage_path(storage_path)
     replace_runtime()
     protect_cookies(config.get('main', 'cookie_secret'))
     enable_oauth2(
       config.get('oauth2', 'client_id'),
       config.get('oauth2', 'client_secret'),
       config.get('main', 'admins').split()
     )
-    dispatch_tasks_to_user_port()
+    fix_target_resolution()
 
   execfile(script_file)