Opened 7 years ago

Closed 6 years ago

#9921 closed (duplicate)

request.urlconf incorrect behavoir

Reported by: strelnikovdmitrij Owned by: nobody
Component: Core (Other) Version: 1.0
Severity: Keywords: request.urlconf
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: UI/UX:

Description

if dynamically load urls file through request.urlconf django will not append trailing slash
so if I will have rule

(r'^(?P<user>[a-zA-Z0-9-]{4,20})/$', 'user')

, it have to call it

http://site.com/user/

only,

http://site.com/user

(wihtout slash) will not work and django will try load ROOT_URLCONF from settings file.

If I will have the same rules in ROOT_URLCONF file it will work correctly.
May be its happen due bad parsing hostname. Im using subdomains..so I have x.site.com - load x.urls to request.urlconf, y.site.com - load y.urls to request.urlconf. Prepend project name -project.x.urls doesnt solve problem. File is load correctly, but didnt append slash. Global append settings - On. Maybe bug in cache systems.
Don't know :(
Using latest stable 1.0.2 django, python 2.5, windows xp sp3.
Dmitrij

Change History (11)

comment:1 Changed 7 years ago by strelnikovdmitrij

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 7 years ago by Alex

  • Resolution set to duplicate
  • Status changed from new to closed

Marking as a duplicate, this is because reverse() doesn't take request.urlconf into account.

comment:3 Changed 7 years ago by strelnikovdmitrij

  • Has patch set
  • Needs tests set
  • Resolution duplicate deleted
  • Status changed from closed to reopened

SOLVED :
in django\middleware\common.py file near by line 55 (not sure, I've change some sources)

if settings.APPEND_SLASH and (not old_url[1].endswith('/')):
            if (not _is_valid_path(request.path_info) '''and'''
                    _is_valid_path("%s/" % request.path_info)):
                new_url[1] = new_url[1] + '/'

have to be

if settings.APPEND_SLASH and (not old_url[1].endswith('/')):
            print "not end with /"
            if (not _is_valid_path(request.path_info) '''&''' _is_valid_path("%s/" % request.path_info)):
                new_url[1] = new_url[1] + '/'
                print "new_url[1] = %s " % new_url[1]
                if settings.DEBUG and request.method == 'POST':

for more info see the Python doc describing different between logical and arithmetical operators or look through folowing code executed in standart python shell ;)

>>> t = True
>>> f = False
>>> not t and f
False
>>> not t & f
True

comment:4 Changed 7 years ago by strelnikovdmitrij

  • Resolution set to duplicate
  • Status changed from reopened to closed

comment:5 Changed 7 years ago by strelnikovdmitrij

  • Resolution duplicate deleted
  • Status changed from closed to reopened

comment:6 Changed 7 years ago by kmtracey

I do not see how changing from using boolean to arithmetic/bitwise "AND" (but not "NOT") in code where we are dealing with boolean values could possibly be correct.

Given the precedence (http://docs.python.org/reference/expressions.html#summary) of boolean "AND" and "NOT", the code as written today is saying:

if (the path we have been asked to locate is not resolvable) and (adding a slash to the end it makes it resolvable) then (proceed with settings things up so we'll do a redirect to the added-slash version of the url)

When you change the boolean "AND" to a bitwise one, but leave the boolean "NOT", the precedence changes (bitwise "AND" has higher precedence than boolean "NOT") and the code then says:

if not ("the path as given is resolvable" and "adding a slash to the end makes it resolvable") then (proceed with settings things up so we'll do a redirect to the added-slash version of the url)

which means that any not-resolvable-as-originally-specified path will get redirected to the added-slash version, which is incorrect. We only want to do the redirect when adding the slash actually results in a resolvable url.

I'm not re-closing as a dupe, because I'm not sure what it's supposed to be a dupe of. reverse() is not involved here. I do see there are two other tickets (#3530 and #5034) with other cases where request.urlconf is not being taken into account when it should be. James tried to get those consolidated into one but that doesn't seem to have stuck. It does seem like we have a general problem with request.urlconf not being considered in all the places it should be, and it would be good if they were all found and fixed together rather than piecemeal (higher likelihood of doing it in a consistent fashion), but I don't really know enough about the code involved here to say whether one consistent fix is a reasonable goal or if these really all need to be fixed individually as they are tripped over.

comment:7 Changed 7 years ago by strelnikovdmitrij

thanks for reply and detail describing ;)
I've gone through code again :)

The result:
my changes goes back ;)

now look..

common.py

process_request(self, request):
...
if (not _is_valid_path(request.path_info) and 
                    _is_valid_path("%s/" % request.path_info)):
                new_url[1] = new_url[1] + '/'
...

going to _is_valid_path(...

def _is_valid_path(path):
    try:
        urlresolvers.resolve(path)
        return True
    except urlresolvers.Resolver404:
        return False

going to urlresolvers.resolve(path)..
(file urlresolvers.py)

def resolve(path, urlconf=None): #urlconf is always None (I hope)
    return get_resolver(urlconf).resolve(path)

going to get_resolver(urlconf) # urlconf = None

def get_resolver(urlconf):
    if urlconf is None:
        from django.conf import settings
        urlconf = settings.ROOT_URLCONF #get default settings
    return RegexURLResolver(r'^/', urlconf)
get_resolver = memoize(get_resolver, _resolver_cache, 1)

I think this should be better..and no operators will changed ;)

common.py !!! + means added

def process_request(self, request):
...
host = request.get_host()
old_url = [host, request.path]
new_url = old_url[:]

+ urlconf = None +
...
if (not _is_valid_path(request.path_info, +urlconf+) and 
                    _is_valid_path("%s/" % request.path_info, +urlconf+)):
                new_url[1] = new_url[1] + '/'

going to _is_valid_path..

def _is_valid_path(path, +urlconf=None+):
    try:
        urlresolvers.resolve(path, +urlconf+)
        return True
    except urlresolvers.Resolver404:
        return False

and urlresolvers.resolve will get correct urlconf from request, if there is no urlconf it will be getted from settings ;)

Any better ideas?
Thanks.
Dmitrij

comment:8 Changed 7 years ago by strelnikovdmitrij

sorry, forgot one piece of code

if settings.APPEND_SLASH and (not old_url[1].endswith('/')): 
           
             #default None
           + if hasattr(request, 'urlconf'):
           +     urlconf = request.urlconf

            #if not (_is_valid_path(request.path_info) and _is_valid_path("%s/" % request.path_info)):
            if (not _is_valid_path(request.path_info, + urlconf +) and 
                    _is_valid_path("%s/" % request.path_info, + urlconf +)):
                new_url[1] = new_url[1] + '/'

comment:9 Changed 6 years ago by SmileyChris

I think that #5034 would just fix this transparently.

comment:10 Changed 6 years ago by Alex

Smiley I agree with your assment, the issue is COMMON_MIDDLWEAR tries to see if it can reverse the url with a / appended. This is why I originally marked this as a dupe, it's a symptom, not a cause.

comment:11 Changed 6 years ago by jacob

  • Resolution set to duplicate
  • Status changed from reopened to closed

Duplicate of #5034

Note: See TracTickets for help on using tickets.
Back to Top