Add exemption for lang param in locale middleware. #55

Merged
merged 1 commit into from Sep 16, 2013

Conversation

Projects
None yet
4 participants
Member

pmclanahan commented Sep 12, 2013

There may be some URLs which have other uses for a
'lang' query parameter. This allows you to set such URLs as exempt.

This is mostly for bedrock, as we need the /firefox/download/ page
to accept a ?lang= parameter that refers to the language of the
Fx build you wish to download. That param for that page has existed
for a long time and we're trying to port it from the PHP side
to bedrock.

funfactory/middleware.py
@@ -18,6 +18,9 @@
from .helpers import urlparams
+EXEMPT_URLS = getattr(settings, 'FF_EXEMPT_LANG_PARAM_URLS', None)
@jgmize

jgmize Sep 12, 2013

Owner

I would default this to an empty list instead of None here so you can always iterate through it.

@jgmize

jgmize Sep 12, 2013

Owner

I also wonder if s/URL/PATH/g would make more sense here.

@pmclanahan

pmclanahan Sep 12, 2013

Member

There are just other settings that use URL. Figured it was more consistent.

funfactory/middleware.py
+ if 'lang' not in request.GET:
+ return False
+
+ if EXEMPT_URLS:
@jgmize

jgmize Sep 12, 2013

Owner

This becomes unecessary if you make the change I recommended on line 21.

funfactory/middleware.py
+
+ if EXEMPT_URLS:
+ for url in EXEMPT_URLS:
+ if request.path.endswith(url):
@jgmize

jgmize Sep 12, 2013

Owner

Why are you using endswith here?

@pmclanahan

pmclanahan Sep 12, 2013

Member

So you don't have to list every locale to exempt.

def process_request(self, request):
prefixer = urlresolvers.Prefixer(request)
urlresolvers.set_url_prefix(prefixer)
full_path = prefixer.fix(prefixer.shortened_path)
- if 'lang' in request.GET:
+ if self._is_lang_change(request):
@jgmize

jgmize Sep 12, 2013

Owner

Why not just change this to

if 'lang' in request.GET and request.path not in EXEMPT_PATHS:

instead of using the separate function? wouldn't that accomplish the same purpose?

(This only works with the changes recommended on line 21)

@pmclanahan

pmclanahan Sep 12, 2013

Member

Because of what I said earlier. This also leaves the possibility open of someone improving this later by adding optional regex support doing some kind of isinstance check.

@jgmize

jgmize Sep 12, 2013

Owner

Good ideas, I like it. Bonus points for pointing out that it currently matches the end of the path in a docstring, double bonus points if you go ahead and use regex matching-- I don't think an isinstance check is necessary for that, you can just iterate through and feed each string to re.search, which would accomplish the same thing as endswith, effectively.

@jgmize

jgmize Sep 13, 2013

Owner

On second thought, scratch the double bonus points for using regex-- I don't think the performance and maintainability tradeoff would be worth it for something there isn't a clear use case for. This is good just the way it is.

@pmclanahan

pmclanahan Sep 13, 2013

Member

That's what I thought as well. I doubt it'll need to be that flexible. Someone can always change it if so.

funfactory/middleware.py
@@ -31,12 +34,23 @@ def __init__(self):
"loaded. Consider removing funfactory.middleware."
"LocaleURLMiddleware from your MIDDLEWARE_CLASSES setting.")
+ def _is_lang_change(self, request):
+ """Return True if the lang param is present and URL isn't exempt."""
+ if 'lang' not in request.GET:
@jgmize

jgmize Sep 13, 2013

Owner

suggested replacement:

if not request.GET.get('lang'):

That way a url like /?lang=&dude=abides won't cause the middleware to ignore the Accept-Language header. Also, the hash lookup is theoretically more performant than iterating (yes, in practice the O(1) vs O(n) doesn't really matter b/c it's a small n, but still...)

@jgmize

jgmize Sep 13, 2013

Owner

On further relection, how about this instead?

return request.GET.get('lang') and not any(request.path.endswith(url) for url in EXEMPT_URLS)
@pmclanahan

pmclanahan Sep 13, 2013

Member

I don't think the hit to readability is worth it.

@pmclanahan

pmclanahan Sep 13, 2013

Member

Also the original middleware would attempt to use the ?lang parameter even if it was just /path/?lang. I'm pretty sure no one is counting on that, but my intent is not to change the default operation.

@Osmose

Osmose Sep 13, 2013

Owner

Why not Zoidberg?

return 'lang' in request.GET and not any(request.path.endswith(url) for url in EXEMPT_URLS)
@pmclanahan

pmclanahan Sep 13, 2013

Member

I'd be fine using:

return not any(request.path.endswith(url) for url in EXEMPT_URLS)

As a replacement for line 42. But I'm not sure it won't be slightly slower due to it finishing all the urls before returning. A reasonable list likely won't matter though, and I can't imaging the EXEMPT_URLS growing past 2 or 3.

@Osmose

Osmose Sep 13, 2013

Owner

any short circuits: http://stackoverflow.com/questions/14730046/is-the-shortcircuit-behaviour-of-pythons-any-all-explicit

Also why are we arguing about performance when there's databases people.

funfactory/middleware.py
@@ -18,6 +18,9 @@
from .helpers import urlparams
+EXEMPT_URLS = getattr(settings, 'FF_EXEMPT_LANG_PARAM_URLS', ())
@Osmose

Osmose Sep 13, 2013

Owner

In general it's a bad idea to read a setting on module import, as it makes testing difficult and anything else that may modify the settings at runtime (not that that should happen, but it could).

I'd change this to be a property on the class.

@pmclanahan

pmclanahan Sep 13, 2013

Member

I had it like that. Should do that again.

funfactory/middleware.py
+ if 'lang' not in request.GET:
+ return False
+
+ return not any(request.path.endswith(url) for url in EXEMPT_URLS)
@pmclanahan

pmclanahan Sep 13, 2013

Member

What do y'all think of this?

@Osmose

Osmose Sep 13, 2013

Owner

I think you're worrying way too much about this.

@pmclanahan

pmclanahan Sep 13, 2013

Member

I'm not. I liked what I had. I'm just following the review.

Member

pmclanahan commented Sep 13, 2013

If no one else has any objections I'm going to rebase and merge.

Add exemption for lang param in locale middleware.
There may be some URLs which have other uses for a
'lang' query parameter. This allows you to set such
URLs as exempt.
Owner

jgmize commented Sep 13, 2013

r+

pmclanahan pushed a commit that referenced this pull request Sep 16, 2013

Merge pull request #55 from pmclanahan/add-locale-middleware-exemption
Add exemption for lang param in locale middleware.

@pmclanahan pmclanahan merged commit 31bd1b4 into mozilla:master Sep 16, 2013

1 check failed

default The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment