Edgewall Software
Modify

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#13042 closed enhancement (fixed)

Request.send_error shouldn't load and render the error template

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.3.3
Component: web frontend Version:
Severity: normal Keywords:
Cc: Branch:
Release Notes:
API Changes:

Moved error template rendering from Request.send_error to trac.web.main. Request.send_error has a new signature that sends a rendered template.

Internal Changes:

Description (last modified by Ryan J Ollos)

The Request.send_error method has a method-scoped import for Chrome and does the work of loading and rendering the error template.

For a normal request the work of rendering a template is done in trac.web.main. I propose having a _send_error method in trac.web.main that does the work of loading and rendering the error template, and have Request.send_error behave more like Request.send.

Request.send_error has rendered the template since it was added in r2957. At that time, the Request.display method (appears to be similar to Request.send) would also render the HDF template. Request.send was added in r3832 and Request.display was removed in r10405 (when ClearSilver support was removed).

Similar to the changes in r14604, there is some nesting of exception handlers in trac.web.main that makes the code difficult to follow. There will be some proposed changes to attempt to clean up the logic.

Attachments (0)

Change History (25)

comment:1 by Ryan J Ollos, 6 years ago

Description: modified (diff)

comment:2 by Ryan J Ollos, 6 years ago

Description: modified (diff)

comment:3 by Ryan J Ollos, 6 years ago

Milestone: next-major-releases1.3.3
Owner: set to Ryan J Ollos
Status: newassigned

comment:4 by Ryan J Ollos, 6 years ago

Description: modified (diff)

comment:5 by Ryan J Ollos, 6 years ago

Proposed changes in log:rjollos.git:t13042_move_error_rendering.1. [391643004/rjollos.git] is targeted to Trac 1.5.1 (#12787).

comment:6 by Ryan J Ollos, 6 years ago

Fixed test failure on AppVeyor in [4176ef214/rjollos.git].

comment:7 by Jun Omae, 6 years ago

In [50be720ee/rjollos.git], match_request(req) is not invoked when req.path_info is empty or '/'. I think behavior of IRequestHandler would be changed (e.g. a component has match_request(req) which returns True if req.path_info is empty).

We should call match_request(req) even if req.path_info is empty or '/', otherwise we should add documentation to IRequestHandler.match_request about the changes.

I'm reviewing Request.send_error and _send_error yet….

comment:8 by Ryan J Ollos, 6 years ago

I was thinking that the default handler should always handle a request to the path /. However, I'm open to possibilities that there might be a use case in which a plugin would handle the request rather than the default handler.

comment:10 by Ryan J Ollos, 6 years ago

API Changes: modified (diff)
Resolution: fixed
Status: assignedclosed

Committed to trunk in r16690 and r16691. Related: comment:13:ticket:12787.

Last edited 6 years ago by Ryan J Ollos (previous) (diff)

comment:11 by Ryan J Ollos, 6 years ago

Found an issue while comment:5:ticket:13038:

Traceback (most recent call last):
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/web/api.py", line 814, in send_error
    if template.endswith('.html'):
AttributeError: 'generator' object has no attribute 'endswith'

Proposed fix, but I'll also add a test:

  • trac/web/api.py

    diff --git a/trac/web/api.py b/trac/web/api.py
    index 8d3bd5da2..87f5813fa 100644
    a b class Request(object):  
    811811            that renders an error page will be removed in 1.5.1.
    812812        """
    813813        try:
    814             if template.endswith('.html'):
     814            if isinstance(template, basestring) and template.endswith('.html'):
    815815                if env:
    816816                    from trac.web.chrome import Chrome, add_stylesheet
    817817                    add_stylesheet(self, 'common/css/code.css')

comment:12 by Ryan J Ollos, 6 years ago

I had an error in the TestCase that resulted in no test coverage with chunked encoding enabled. Fixed in r16692.

comment:13 by Ryan J Ollos, 6 years ago

I noticed an issue while testing #12611. Previously an invalid value for tracd's projenv argument would result in a browser message like the following when navigating to the URL:

Error

TracError: No Trac environment found at /Users/rjollos/Documents/Workspace/trac-dev/not-an-env
[Errno 2] No such file or directory: '/Users/rjollos/Documents/Workspace/trac-dev/not-an-env/VERSION'

Now, an exception occurs:

Exception happened during processing of request from ('127.0.0.1', 64823)
Traceback (most recent call last):
  File "/Users/rjollos/.pyenv/versions/2.7.14/lib/python2.7/SocketServer.py", line 596, in process_request_thread
    self.finish_request(request, client_address)
  File "/Users/rjollos/.pyenv/versions/2.7.14/lib/python2.7/SocketServer.py", line 331, in finish_request
    self.RequestHandlerClass(request, client_address, self)
  File "/Users/rjollos/.pyenv/versions/2.7.14/lib/python2.7/SocketServer.py", line 652, in __init__
    self.handle()
  File "/Users/rjollos/.pyenv/versions/2.7.14/lib/python2.7/BaseHTTPServer.py", line 340, in handle
    self.handle_one_request()
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/web/wsgi.py", line 212, in handle_one_request
    gateway.run(self.server.application)
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/web/wsgi.py", line 112, in run
    response = application(self.environ, self._start_response)
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/web/standalone.py", line 95, in __call__
    return self.application(environ, start_response)
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/web/main.py", line 633, in dispatch_request
    dispatcher = RequestDispatcher(env)
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/core.py", line 144, in __call__
    "First argument must be a ComponentManager instance"
AssertionError: First argument must be a ComponentManager instance

It looks like the issue is fixed by the following:

  • trac/web/main.py

    diff --git a/trac/web/main.py b/trac/web/main.py
    index 0bdcd6cad..43310314f 100644
    a b def dispatch_request(environ, start_response):  
    630630        env.abs_href = req.abs_href
    631631    translation.make_activable(lambda: req.locale, env.path if env else None)
    632632    resp = []
    633     dispatcher = RequestDispatcher(env)
    634     dispatcher.set_default_callbacks(req)
    635633    try:
    636634        if not env and env_error:
    637635            raise HTTPInternalServerError(env_error)
     636        dispatcher = RequestDispatcher(env)
     637        dispatcher.set_default_callbacks(req)
    638638        try:
    639639            dispatcher.dispatch(req)
    640640        except RequestDone as req_done:

I will look at adding test coverage.

Last edited 6 years ago by Ryan J Ollos (previous) (diff)

comment:14 by Ryan J Ollos, 6 years ago

I've written a test, but I get slightly different error messages depending on whether Babel is installed. Without Babel installed, the first line is "Trac Error". With Babel installed, the first line is "Error". With Babel installed, an AttributeError is raised in HTTPException.title, which results in "Error" being returned as the title property rather than self.reason. Printing the exception with traceback:

Traceback (most recent call last):
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/web/api.py", line 268, in title
    title = _("Error")
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/util/translation.py", line 200, in gettext
    if not self.isactive:
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/util/translation.py", line 195, in __getattr__
    return getattr(self.active, name)
AttributeError: NullTranslationsBabel instance has no attribute 'isactive'

I'm unsure whether this the correct behavior.

in reply to:  13 ; comment:15 by Jun Omae, 6 years ago

Replying to Ryan J Ollos:

It looks like the issue is fixed by the following:

-    dispatcher = RequestDispatcher(env)
-    dispatcher.set_default_callbacks(req)
     try:
         if not env and env_error:
             raise HTTPInternalServerError(env_error)
+        dispatcher = RequestDispatcher(env)
+        dispatcher.set_default_callbacks(req)

Yeah. Component class cannot accept None to construct. The same issue exists in send_internal_error.

  • trac/web/main.py

    diff --git a/trac/web/main.py b/trac/web/main.py
    index 0bdcd6cad..d88da9238 100644
    a b def send_internal_error(env, req, exc_info):  
    754754                    plugin_name = info.get('home_page', '').rstrip('/') \
    755755                                                           .split('/')[-1]
    756756                    tracker_args = {'component': plugin_name}
    757         interface_custom = Chrome(env).get_interface_customization_files()
     757            interface_custom = Chrome(env).get_interface_customization_files()
    758758
    759759    def get_description(_):
    760760        if env and has_admin:
    User agent: `#USER_AGENT#`  
    822822            'tracker': tracker, 'tracker_args': tracker_args,
    823823            'description': description, 'description_en': description_en}
    824824
    825     Chrome(env).add_jquery_ui(req)
     825    if env:
     826        Chrome(env).add_jquery_ui(req)
    826827    _send_error(req, sys.exc_info(), status=500, env=env, data=data)
    827828
    828829

in reply to:  14 ; comment:16 by Jun Omae, 6 years ago

Replying to Ryan J Ollos:

AttributeError: NullTranslationsBabel instance has no attribute 'isactive'

I'm unsure whether this the correct behavior.

I modified the isactive method of TranslationsProxy to catch the orignal AttributeError:

  • trac/util/translation.py

    diff --git a/trac/util/translation.py b/trac/util/translation.py
    index 5095b4b4c..8e2ddf990 100644
    a b try:  
    174174
    175175        @property
    176176        def isactive(self):
    177             if self._current.args is not None:
    178                 get_locale, env_path = self._current.args
    179                 self._current.args = None
    180                 self.activate(get_locale(), env_path)
    181             # FIXME: The following always returns True: either a translation is
    182             # active, or activation has failed.
    183             return self._current.translations is not None \
    184                    or self._activate_failed
     177            try:
     178                if self._current.args is not None:
     179                    get_locale, env_path = self._current.args
     180                    self._current.args = None
     181                    self.activate(get_locale(), env_path)
     182                # FIXME: The following always returns True: either a translation is
     183                # active, or activation has failed.
     184                return self._current.translations is not None \
     185                       or self._activate_failed
     186            except Exception as e:
     187                from trac.util.text import exception_to_unicode
     188                print(exception_to_unicode(e, traceback=True))
     189                raise
    185190
    186191        # Internal methods
    187192

I get the original AttributeError after the modification:

Traceback (most recent call last):
  File "/src/tracdev/git/trac/util/translation.py", line 181, in isactive
    self.activate(get_locale(), env_path)
  File "/src/tracdev/git/trac/web/main.py", line 631, in <lambda>
    translation.make_activable(lambda: req.locale, env.path if env else None)
  File "/src/tracdev/git/trac/web/api.py", line 602, in __getattr__
    raise AttributeError(name)
AttributeError: locale

We must prevent the AttributeError to retrieve req.locale. Work around:

  • trac/web/main.py

    diff --git a/trac/web/main.py b/trac/web/main.py
    index 0bdcd6cad..df9237d06 100644
    a b def dispatch_request(environ, start_response):  
    628628    # fixup env.abs_href if `[trac] base_url` was not specified
    629629    if env and not env.abs_href.base:
    630630        env.abs_href = req.abs_href
    631     translation.make_activable(lambda: req.locale, env.path if env else None)
     631    translation.make_activable(lambda: req.locale if hasattr(req, 'locale')
     632                                       else None,
     633                               env.path if env else None)
    632634    resp = []
    633     dispatcher = RequestDispatcher(env)
    634     dispatcher.set_default_callbacks(req)
    635635    try:
    636636        if not env and env_error:
    637637            raise HTTPInternalServerError(env_error)
     638        dispatcher = RequestDispatcher(env)
     639        dispatcher.set_default_callbacks(req)
    638640        try:
    639641            dispatcher.dispatch(req)
    640642        except RequestDone as req_done:

Another work around:

  • trac/web/api.py

    diff --git a/trac/web/api.py b/trac/web/api.py
    index 87f5813fa..ebe7adf76 100644
    a b class Request(object):  
    582582            'args': lambda req: arg_list_to_args(req.arg_list),
    583583            'languages': Request._parse_languages,
    584584            'incookie': Request._parse_cookies,
    585             '_inheaders': Request._parse_headers
     585            '_inheaders': Request._parse_headers,
     586            'locale': lambda req: None,  # prevent AttributeError
    586587        }
    587588        self.redirect_listeners = []
    588589

in reply to:  16 comment:17 by Ryan J Ollos, 6 years ago

Replying to Jun Omae:

Another work around:

  • trac/web/api.py

    diff --git a/trac/web/api.py b/trac/web/api.py
    index 87f5813fa..ebe7adf76 100644
    a b class Request(object):  
    582582            'args': lambda req: arg_list_to_args(req.arg_list),
    583583            'languages': Request._parse_languages,
    584584            'incookie': Request._parse_cookies,
    585             '_inheaders': Request._parse_headers
     585            '_inheaders': Request._parse_headers,
     586            'locale': lambda req: None,  # prevent AttributeError
    586587        }
    587588        self.redirect_listeners = []
    588589

Thanks, I missed the exception in isactive. Your fix looks nice.

in reply to:  15 comment:18 by Ryan J Ollos, 6 years ago

Replying to Jun Omae:

Yeah. Component class cannot accept None to construct. The same issue exists in send_internal_error.

I'll add those changes. I can reproduce the issue by adding a raise Exception in the code.

  • trac/web/main.py

    diff --git a/trac/web/main.py b/trac/web/main.py
    index bba0a3eaf..455347f0a 100644
    a b def dispatch_request(environ, start_response):  
    634634    resp = []
    635635    try:
    636636        if env_error:
     637            raise Exception("Raw exception with no environment")
    637638            raise HTTPInternalServerError(env_error)
    638639        dispatcher = RequestDispatcher(env)
    639640        dispatcher.set_default_callbacks(req)

I'm having trouble adding a test though. I don't see a way that we could have an Exception that results in send_internal_error without an environment since env_error is raised immediately, leading to handled HTTPException. We could add coverage for just send_internal_error, but I don't see as much value in that, and that method should probably be internal anyway (_send_internal_error, like _send_user_error).

comment:19 by Ryan J Ollos, 6 years ago

The browser error message on environment not found has duplicate backslashes on Windows:

Trac Error

TracError: No Trac environment found at c:\users\rjollos\documents\workspace\trac-dev\trac-1.0-stable\trac
[Errno 2] No such file or directory: 'c:\\users\\rjollos\\documents\\workspace\\trac-dev\\trac-1.0-stable\\trac\\VERSION'

The IOError must use repr for filename:

>>> try:
...   open('C:\Path\Does\Not\Exist')
... except Exception as e:
...   print(e)
...   print(e.filename)
...   repr(e)
...   repr(e.filename)
...   print('[Errno %s] %s %r' % (e.errno, e.strerror, e.filename))
...
[Errno 2] No such file or directory: 'C:\\Path\\Does\\Not\\Exist'
C:\Path\Does\Not\Exist
"IOError(2, 'No such file or directory')"
"'C:\\\\Path\\\\Does\\\\Not\\\\Exist'"
[Errno 2] No such file or directory 'C:\\Path\\Does\\Not\\Exist'

I don't know if it's worth trying to fix that up, or if there would be a better way than:

>>> s = ('%s' %e).replace('\\\\', '\\')
>>> print(s)
[Errno 2] No such file or directory: 'C:\Path\Does\Not\Exist'

in reply to:  19 comment:21 by Jun Omae, 6 years ago

Replying to Ryan J Ollos:

I don't know if it's worth trying to fix that up, or if there would be a better way than:

>>> s = ('%s' %e).replace('\\\\', '\\')
>>> print(s)
[Errno 2] No such file or directory: 'C:\Path\Does\Not\Exist'
  • Windows:
    • strerror can be localized error string (ANSI string).
    • filename can be unicode string or ANSI string.
  • Linux:
    • strerror can be localized error string (LC_MESSAGES).
    • filename can be unicode string or 8-bit string.

For Linux, I think it is safe to use %r for filename. But we could improve to_unicode() with EnvironmentError for Windows.

  • trac/util/text.py

    diff --git a/trac/util/text.py b/trac/util/text.py
    index 839f7a8d1..0d383225e 100644
    a b def to_unicode(text, charset=None):  
    106106        except UnicodeDecodeError:
    107107            return unicode(text, 'latin1')
    108108    elif isinstance(text, Exception):
    109         if os.name == 'nt' and isinstance(text, (OSError, IOError)):
     109        if os.name == 'nt' and isinstance(text, EnvironmentError):
     110            strerror = text.strerror
     111            filename = text.filename
     112            if strerror and filename:
     113                try:
     114                    strerror = unicode(strerror, 'mbcs')
     115                    filename = unicode(filename, 'mbcs')
     116                except UnicodeError:
     117                    pass
     118                else:
     119                    if isinstance(text, WindowsError):
     120                        return u"[Error %s] %s: '%s'" % (text.winerror,
     121                                                         strerror, filename)
     122                    else:
     123                        return u"[Errno %s] %s: '%s'" % (text.errno, strerror,
     124                                                         filename)
    110125            # the exception might have a localized error string encoded with
    111126            # ANSI codepage if OSError and IOError on Windows
    112127            try:
Python 2.7.10 (default, May 23 2015, 09:44:00) [MSC v.1500 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> from trac.util.text import to_unicode
>>> try:
...   open(r'C:\notfound.txt')
... except Exception as e:
...   print(to_unicode(e))
...
[Errno 2] No such file or directory: 'C:\notfound.txt'
>>> try:
...   open(u'C:\あ.txt')
... except Exception as e:
...   print(to_unicode(e))
...
[Errno 2] No such file or directory: 'C:\あ.txt'
>>> try:
...   os.mkdir('C:\\')
... except Exception as e:
...   print(to_unicode(e))
...
[Error 5] アクセスが拒否されました。: 'C:\'
>>> e.strerror  # "Access denied" in Japanese (cp932)
'\x83A\x83N\x83Z\x83X\x82\xaa\x8b\x91\x94\xdb\x82\xb3\x82\xea\x82\xdc\x82\xb5\x82\xbd\x81B'
Last edited 6 years ago by Jun Omae (previous) (diff)

comment:22 by Ryan J Ollos, 6 years ago

I added the comment:21 changes in [6e0696697/rjollos.git]. I made one minor change ([7c94d0ca3/rjollos.git]) to address test failures. The change is patterned after tags/trac-1.3.3/trac/util/text.py@:113#L91.

======================================================================
ERROR: test_get_authz_file_notfound_raises (trac.versioncontrol.tests.svn_authz.AuthzSourcePolicyTestCase)
ConfigurationError exception is raised if file not found.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\projects\trac\trac\versioncontrol\tests\svn_authz.py", line 288, in test_get_authz_file_notfound_raises
    'BROWSER_VIEW', 'user', None, None)
  File "C:\Python27\lib\unittest\case.py", line 473, in assertRaises
    callableObj(*args, **kwargs)
  File "C:\projects\trac\trac\versioncontrol\svn_authz.py", line 140, in check_permission
    authz, users = self._get_authz_info()
  File "C:\projects\trac\trac\versioncontrol\svn_authz.py", line 217, in _get_authz_info
    "file: %s", exception_to_unicode(e))
  File "C:\projects\trac\trac\util\text.py", line 147, in exception_to_unicode
    message = '%s: %s' % (e.__class__.__name__, to_unicode(e))
  File "C:\projects\trac\trac\util\text.py", line 115, in to_unicode
    filename = unicode(filename, 'mbcs')
TypeError: decoding Unicode is not supported
======================================================================
ERROR: test_get_authz_file_removed_raises (trac.versioncontrol.tests.svn_authz.AuthzSourcePolicyTestCase)
ConfigurationError exception is raised if file is removed.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\projects\trac\trac\versioncontrol\tests\svn_authz.py", line 311, in test_get_authz_file_removed_raises
    'BROWSER_VIEW', 'user', None, None)
  File "C:\Python27\lib\unittest\case.py", line 473, in assertRaises
    callableObj(*args, **kwargs)
  File "C:\projects\trac\trac\versioncontrol\svn_authz.py", line 140, in check_permission
    authz, users = self._get_authz_info()
  File "C:\projects\trac\trac\versioncontrol\svn_authz.py", line 217, in _get_authz_info
    "file: %s", exception_to_unicode(e))
  File "C:\projects\trac\trac\util\text.py", line 147, in exception_to_unicode
    message = '%s: %s' % (e.__class__.__name__, to_unicode(e))
  File "C:\projects\trac\trac\util\text.py", line 115, in to_unicode
    filename = unicode(filename, 'mbcs')
TypeError: decoding Unicode is not supported

comment:23 by Jun Omae, 6 years ago

Sorry, fixed it and added unit tests in [cc5c36619/jomae.git] (jomae.git@t13042_fix_exception_handling.1). Unit tests pass on AppVeyor and Travis CI.

comment:24 by Ryan J Ollos, 6 years ago

Committed minor changes in r16804, r16805.

comment:25 by Ryan J Ollos, 6 years ago

comment:23 and other changes committed to trunk in r16806.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.