Introduce DynamicURLResolver, and use it for extensions.

Review Request #3577 — Created Nov. 28, 2012 and submitted

chipx86
Djblets
master
djblets
smacleod
Introduce DynamicURLResolver, and use it for extensions.

Getting dynamic URL patterns working in Django is tricky, since Django
assumes URL patterns will be set once and will never change.

DynamicURLResolver is the answer to that. It takes the place of a url()
in a urlpatterns list, and provides add_patterns() and
remove_patterns() functions that callers can use to modify the URL
patterns list.

To do this, it manipulates its own list of URL patterns, and the caching
of every URL resolver between the root resolver and itself. Whenever the
list is manipulated, all resolvers in that chain have their caches
emptied, causing the next lookup to traverse up to the
DynamicURLResolver. This means there's a little bit of overhead every
time the list is manipulated, but it's not terrible, and won't affect
resolvers outside that chain.

This is being used for URLHooks, WebAPIResources on extensions, and
admin URLs for extensions.
Tested a repro case we found where API resources for models in extensions
weren't able to reverse a URL from inside that extension's API tree properly.
This now works fine.

Added unit tests, which pass (and failed before).

More work will need to be done to ensure URLHooks work properly in all cases,
and unit tests should be written there as well.
Description From Last Updated

'clear_url_caches' isn't being used here. Did you use it here initially and then factor out to DynamicURLResolver?

SM smacleod

I'm not sure if this is a typo, or purposeful, but shouldn't we have: super(DynamicURLResolver, self).__init__(regex=regex, If it is purposeful, ...

SM smacleod

I'm a little confused about this whole urlconf_name and urlconf_module business. Have you repurposed urlconf_module to be what RegexURLResolver's url_patterns ...

SM smacleod
SM
  1. Did a thorough sweep, and I like it, looks good! The only real gotcha is the urlconf stuff.
    It's functional, and works if you trace the code, I'm interested to know why you went
    about it that way.
  2. djblets/extensions/hooks.py (Diff revision 1)
     
     
     
    'clear_url_caches' isn't being used here. Did you use it here initially and then factor out
    to DynamicURLResolver?
    1. Yeah, meant to remove that again.
  3. djblets/extensions/hooks.py (Diff revision 1)
     
     
    Awesome, I love it :D
  4. djblets/util/urlresolvers.py (Diff revision 1)
     
     
     
    I'm not sure if this is a typo, or purposeful, but shouldn't we have:
    
    super(DynamicURLResolver, self).__init__(regex=regex,
    
    If it is purposeful, and we're just discarding the passed in regex, maybe we should document
    that, or not specify it as a kwarg.
  5. djblets/util/urlresolvers.py (Diff revision 1)
     
     
    I'm a little confused about this whole urlconf_name and urlconf_module business. Have you
    repurposed urlconf_module to be what RegexURLResolver's url_patterns usually is?
    
    If I'm correct, what was the purpose of this? I feel like we're jumping through a lot of
    hoops just to have url_patterns return a list. Wouldn't it be more readable if we just
    did away with the concept of urlconf_* for this class, and had url_patterns be the list?
    1. I'll answer this question and the one below.
      
      The way RegexURLResolver works is that it can take in one of three things to define what it will resolve against:
      
      1) A string (which represents a Python module that will later be imported)
      2) A module (or something, but intended as a module) that contains a 'urlpatterns' property (this is what #1 turns into when imported). Basically, urls.py file.
      3) A list (generally built from patterns(...))
      
      The parameter that RegexURLResolver expects is urlconf_name. This can be any of the above, and it will figure out what to do with it (either in the constructor, or later, depending on what it is).
      
      url_patterns later will try to return the resulting list. It does this by seeing if whatever is passed has a 'urlpatterns' parameter, and returning that, or just returning the thing itself otherwise. This covers the module and list cases.
      
      Since urlconf_module and urlconf_name are part of RegexURLResolver's API, the last thing I want to do is ignore it outright and assume it won't be used. The name is a bit odd, yes, but it fits in with how that class views the world.
      
      As for url_patterns being a list instead of a function, I'm not comfortable overriding a @property for this case. It also makes it messier if for some reason this class were to be subclassed and the subclass wanted to override but call the parent.
    2. By "url_patterns be the list" I more meant have the property just return the list.
      The way it currently stands, to understand you're getting a list back, you have to
      follow through all the logic inside RegexURLResolver, and see that urlconf_module
      eventually becomes urlconf_name.
      
      I do understand about keeping the API though. Do you think it would be worthwile
      to add a little bit of documentation to the url_patterns property to quickly explain
      that we're returning the list passed in as urlconf_name?
    3. Yeah, I can certainly document that.
  6. djblets/util/urlresolvers.py (Diff revision 1)
     
     
    Looking at RegexURLResolver's url_patterns property, it appears to return:
    
        getattr(self.urlconf_module, "urlpatterns", self.urlconf_module)
    
    Is there a reason why we're no longer trying to return self.urlconf_module.urlpatterns,
    and just skipping straight to self.urlconf_module? Is it because we don't care about
    supporting urls.py files like regular django apps, with this?
    
    This is related to my other comment about urlconf_name
    1. We specify the urlconf_name, so we know it'll always be a list (unless someone does something weird). There's no reason to keep the extra logic in this case, since we have to override the function anyway to not fail if the list is empty.
  7. 
      
chipx86
chipx86
SM
  1. Ship It!
  2. 
      
chipx86
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (fbb5abe)
Loading...