Code

Opened 6 years ago

Last modified 4 months ago

#8809 new New feature

Better error message when can't import url callback

Reported by: TP Owned by: nobody
Component: Core (URLs) Version: master
Severity: Normal Keywords: error message
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

in django.core.urlresolvers _get_callback it would be helpful to display not just the module name that couldn't be imported, but the entire urls.py line that was the problem. Without more information it's incredibly hard to track down WHICH urls.py file has the problem and which line in that file is problematic.

Attachments (3)

fix_ticket_8809_urlsresolvers_error_message.diff (2.1 KB) - added by v1v3kn 3 years ago.
Changed the import line to match PEP8 standards.
django8809-urlerror.diff (2.4 KB) - added by bpeschier 3 years ago.
django8809-urlerror.2.diff (8.2 KB) - added by bpeschier 3 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 5 years ago by jacob

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 3 years ago by v1v3kn

Hi,

I would like to implement this by searching through the urls.py or add a field where the RegexUrlPattern is created.
By the way, where in the source are the RegexUrlPattern objects created?

comment:3 Changed 3 years ago by v1v3kn

  • Owner changed from nobody to v1v3kn
  • Status changed from new to assigned

comment:4 follow-up: Changed 3 years ago by v1v3kn

  • Has patch set
  • Needs tests set

comment:5 in reply to: ↑ 4 Changed 3 years ago by v1v3kn

I have added a patch for this ticket. I tested it and it works fine, mentions the file and line number. Could someone please test it?

Changed 3 years ago by v1v3kn

Changed the import line to match PEP8 standards.

comment:6 Changed 3 years ago by carljm

  • Patch needs improvement set

This patch makes a number of assumptions that are not reliable:

  1. That url patterns are always found in a file named "urls.py" (in fact they can be in any module, or even just an object with a get_urls() method).
  2. That the exact string in the RegexURLPattern._callback attribute can be found literally in the urls.py file (this will fail when the prefix argument to "patterns" is used; also, the view string could be constructed dynamically via string interpolation).

Also, the patch needs an automated test, integrated in Django's test suite.

I don't see any way that this approach (trying to reconstruct the information after the fact by literal searching in code files) will be workable. The information instead needs to be preserved from the initial definition in the urls.py if it's to be provided later in the error. The easiest way to do this would be to resolve the import immediately rather than lazily, but that's not acceptable as it could cause circular import issues in cases that currently work. The only alternatives I can see at the moment (having not looked into it deeply yet) would involve some rather nasty frame-object magic. This is not going to be a simple or easy fix.

comment:7 Changed 3 years ago by bpeschier

That frame-object magic must happen at the moment of loading the patterns, since that information is lost when the url-files are read. The patterns function will have to introspect which module is actually calling it and attach that information to the RexegURLPattern before handing it off.

It sounds like a lot of ugly meta-work for a problem where just adding the original regex (which is available in the context) will help considerably without having to pinpoint it exactly.

Changed 3 years ago by bpeschier

comment:8 Changed 3 years ago by bpeschier

Made a patch which adds the needed info in debug-mode because I am not a big supporter of adding this kind of meta-data in production. Need to think about how to create a test for it :-)

comment:9 Changed 3 years ago by v1v3kn

  • Owner changed from v1v3kn to nobody
  • Status changed from assigned to new

Changed 3 years ago by bpeschier

comment:10 Changed 3 years ago by bpeschier

Well, this one makes the information slightly more reliable. When a pattern is created without an url()-call, line-information is not reliable so it only shows the module. Included a set of tests.

comment:11 Changed 3 years ago by lukeplant

  • Severity set to Normal
  • Type set to New feature

comment:12 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:13 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:14 Changed 16 months ago by aaugustin

  • Component changed from Core (Other) to Core (URLs)

comment:15 Changed 16 months ago by lasko

  • Needs tests unset

Changed 'Needs Tests' to unset. Current patch has test cases.

comment:16 Changed 4 months ago by anubhav9042

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

comment:17 Changed 4 months ago by anubhav9042

In django.core.urlresolvers.py:

    if not callable(lookup_view):
        mod_name, func_name = get_mod_func(lookup_view)
        if func_name == '':
            return lookup_view

        try:
            mod = import_module(mod_name)
        except ImportError:
            parentmod, submod = get_mod_func(mod_name)
            if (not can_fail and submod != '' and
                    not module_has_submodule(import_module(parentmod), submod)):
                raise ViewDoesNotExist(
                    "Could not import %s. Parent module %s does not exist." %
                    (lookup_view, mod_name))
            if not can_fail:
                raise
        else:
            try:
                lookup_view = getattr(mod, func_name)
                if not callable(lookup_view):
                    raise ViewDoesNotExist(
                        "Could not import %s.%s. View is not callable." %
                        (mod_name, func_name))
            except AttributeError:
                if not can_fail:
                    raise ViewDoesNotExist(
                        "Could not import %s. View does not exist in module %s." %
                        (lookup_view, mod_name))
    return lookup_view

This explains why I have closed this as fixed...

comment:18 follow-up: Changed 4 months ago by aaugustin

  • Resolution fixed deleted
  • Status changed from closed to new

I'm not convinced that code addresses the request in this ticket. Can you at least check that the code is more recent than the latest discussions here?

comment:19 in reply to: ↑ 18 Changed 4 months ago by anubhav9042

Replying to aaugustin:

I'm not convinced that code addresses the request in this ticket. Can you at least check that the code is more recent than the latest discussions here?

The issue was to change the message such that we could know which line has problem(if there are several url files, it will be difficult.)

I think when we are specifying which particular view does not exists in which module, the user now knows what is missing and where, which wasn't there before. I think that very much solves this issue.
So I closed the ticket.
Am I wrong??

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.