From c1131f8ec3c078a8ddefb3dd310f3c1dd67cd157 Mon Sep 17 00:00:00 2001 From: Johannes Raggam Date: Mon, 26 May 2025 20:11:21 +0200 Subject: [PATCH 1/3] Improve error messages by including more detailed information. --- CHANGES.rst | 5 +- webresource/_api.py | 80 +++++++++++++++++---------- webresource/tests.py | 129 +++++++++++++++++++++++++++++++++++++++---- 3 files changed, 175 insertions(+), 39 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 919aac4..e0dc537 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -4,6 +4,9 @@ Changelog 1.3 (unreleased) ---------------- +- Improve error messages by including more detailed information. + [thet] + - Do not wrap resource ``__repr__`` output in ``<>`` to render tracebacks properly in browser. [lenadax] @@ -68,7 +71,7 @@ Changelog Modernize setup.[py|cfg]. [jensens] -- Added ``GracefulResourceRenderer``. +- Added ``GracefulResourceRenderer``. Fixes #1. [jensens] diff --git a/webresource/_api.py b/webresource/_api.py index a4be088..fce9cc5 100644 --- a/webresource/_api.py +++ b/webresource/_api.py @@ -85,7 +85,10 @@ def include(self, include): def remove(self): """Remove resource or resource group from parent group.""" if not self.parent: - raise ResourceError('Object is no member of a resource group') + raise ResourceError( + 'Cannot remove resource or resource group {}.' + ' It is no member of a resource group.'.format(self) + ) self.parent.members.remove(self) self.parent = None @@ -93,6 +96,25 @@ def copy(self): """Return a deep copy of this object.""" return copy.deepcopy(self) + def __repr__(self): + """Return a string representation of the resource or resource group.""" + + reprs = [] + if getattr(self, "name", None): + reprs.append('name="{}"'.format(self.name)) + elif getattr(self, "path", None): + reprs.append('path="{}"'.format(self.path)) + elif getattr(self, "url", None): + reprs.append('url="{}"'.format(self.url)) + + if getattr(self, "depends", None): + reprs.append('depends="{}"'.format(self.depends)) + + return '<{} {}>'.format( + self.__class__.__name__, + ", ".join(reprs) if reprs else "unnamed" + ) + class Resource(ResourceMixin): """A web resource.""" @@ -135,12 +157,16 @@ def __init__( additional attributes on resource tag. :raise ResourceError: No resource and no url given. """ - if resource is None and url is None: - raise ResourceError('Either resource or url must be given') super(Resource, self).__init__( name=name, directory=directory, path=path, include=include, group=group ) + if resource is None and url is None: + raise ResourceError( + 'Either resource or url must be given for resource {}'.format( + self + ) + ) self.depends = ( (depends if isinstance(depends, (list, tuple)) else [depends]) if depends else None @@ -169,7 +195,9 @@ def file_path(self): """Absolute resource file path depending on operation mode.""" directory = self.directory if not directory: - raise ResourceError('No directory set on resource.') + raise ResourceError( + 'No directory set on resource {}'.format(self) + ) return os.path.join(directory, self.file_name) @property @@ -235,15 +263,6 @@ def _render_tag(self, tag, closing_tag, **attrs): return u'<{tag}{attrs} />'.format(tag=tag, attrs=attrs_) return u'<{tag}{attrs}>'.format(tag=tag, attrs=attrs_) - def __repr__(self): - return ( - '{} name="{}", depends="{}"' - ).format( - self.__class__.__name__, - self.name, - self.depends - ) - class ScriptResource(Resource): """A Javascript resource.""" @@ -323,8 +342,10 @@ def integrity(self): def integrity(self, integrity): if integrity is True: if self.url is not None: - msg = 'Cannot calculate integrity hash from external resource' - raise ResourceError(msg) + raise ResourceError( + 'Cannot calculate integrity hash from external resource ' + '{}'.format(self) + ) self._integrity_hash = None else: self._integrity_hash = integrity @@ -564,8 +585,8 @@ def add(self, member): """ if not isinstance(member, (ResourceGroup, Resource)): raise ResourceError( - 'Resource group can only contain instances ' - 'of ``ResourceGroup`` or ``Resource``' + 'Resource group {} can only contain instances ' + 'of ``ResourceGroup`` or ``Resource``'.format(self) ) member.parent = self self._members.append(member) @@ -584,12 +605,6 @@ def _filtered_resources(self, type_, members=None): resources.append(member) return resources - def __repr__(self): - return '{} name="{}"'.format( - self.__class__.__name__, - self.name - ) - class ResourceConflictError(ResourceError): """Multiple resources declared with the same name.""" @@ -634,7 +649,7 @@ def __init__(self, members): for member in members: if not isinstance(member, (Resource, ResourceGroup)): raise ResourceError( - 'members can only contain instances ' + 'ResourceResolver members can only contain instances ' 'of ``ResourceGroup`` or ``Resource``' ) self.members = members @@ -726,10 +741,19 @@ class GracefulResourceRenderer(ResourceRenderer): def render(self): lines = [] for resource in self.resolver.resolve(): + error_message = None try: lines.append(resource.render(self.base_url)) - except (ResourceError, FileNotFoundError): - msg = u'Failure to render resource "{}"'.format(resource.name) - lines.append(u''.format(msg)) - logger.exception(msg) + except FileNotFoundError: + error_message = u'File not found for resource {}'.format( + resource + ) + except ResourceError as e: + error_message = str(e) + finally: + if error_message: + lines.append(u''.format( + error_message + )) + logger.exception(error_message) return u'\n'.join(lines) diff --git a/webresource/tests.py b/webresource/tests.py index 71f18c3..56778c6 100644 --- a/webresource/tests.py +++ b/webresource/tests.py @@ -111,7 +111,7 @@ def test_Resource(self, tempdir): self.assertEqual(resource.type_, None) self.assertEqual( repr(resource), - 'Resource name="res", depends="None"' + '' ) resource = Resource(name='res', resource='res.ext') @@ -239,7 +239,7 @@ def test_ScriptResource(self, tempdir): self.assertEqual(script.nomodule, None) self.assertEqual( repr(script), - 'ScriptResource name="js_res", depends="None"' + """""" ) self.assertEqual( script.render('https://tld.org'), @@ -301,7 +301,7 @@ def test_LinkMixin(self): self.assertEqual(link.title, None) self.assertEqual( repr(link), - 'LinkMixin name="link_res", depends="None"' + '' ) link.hreflang = 'en' link.media = 'screen' @@ -327,7 +327,7 @@ def test_LinkResource(self): self.assertIsInstance(link, LinkMixin) self.assertEqual( repr(link), - 'LinkResource name="icon_res", depends="None"' + '' ) link.rel = 'icon' link.type_ = 'image/png' @@ -355,7 +355,7 @@ def test_StyleResource(self): self.assertEqual(style.rel, 'stylesheet') self.assertEqual( repr(style), - 'StyleResource name="css_res", depends="None"' + """""" ) self.assertEqual(style.render('https://tld.org'), ( '') res = wr.ScriptResource(name='name', resource='name.js') group.add(res) @@ -442,6 +442,115 @@ def test_ResourceGroup(self): self.assertEqual(group.members, []) self.assertEqual(resource.parent, None) + def test_resource_repr(self): + res = ResourceMixin(name='res') + self.assertEqual( + repr(res), + '' + ) + + # Fallback, if nothing is set. + res = ResourceMixin() + self.assertEqual( + repr(res), + '' + ) + + res = ResourceMixin(path="/a/b/c") + self.assertEqual( + repr(res), + '' + ) + + res = Resource(url="https://example.com") + self.assertEqual( + repr(res), + '' + ) + + # Name takes precendence over all other attributes. + res = Resource(name='res', path="/a/b/c", url="https://example.com") + self.assertEqual( + repr(res), + '' + ) + + # Path takes precendence over url. + res = Resource(path="/a/b/c", url="https://example.com") + self.assertEqual( + repr(res), + '' + ) + + # Example with depends + res = Resource( + name='res', + url="https://example.com", + depends="other.ext", + ) + self.assertEqual( + repr(res), + """""" + ) + + def test_ResourceError(self): + with self.assertRaises(wr.ResourceError) as cm: + Resource() + + self.assertEqual( + str(cm.exception), + 'Either resource or url must be given for resource ' + '' + ) + + with self.assertRaises(wr.ResourceError) as cm: + res = ResourceMixin(name="res") + res.remove() + + self.assertEqual( + str(cm.exception), + 'Cannot remove resource or resource group ' + '. It is no member of a resource group.' + ) + + with self.assertRaises(wr.ResourceError) as cm: + res = Resource(url='https://example.com') + res.file_path + + self.assertEqual( + str(cm.exception), + 'No directory set on resource ' + ) + + with self.assertRaises(wr.ResourceError) as cm: + res = wr.ScriptResource(url='https://example.com') + res.integrity = True + + self.assertEqual( + str(cm.exception), + 'Cannot calculate integrity hash from external resource ' + '' + ) + + with self.assertRaises(wr.ResourceError) as cm: + res = wr.ResourceGroup(name='okay') + res.add(object()) + + self.assertEqual( + str(cm.exception), + 'Resource group can only contain ' + 'instances of ``ResourceGroup`` or ``Resource``' + ) + + with self.assertRaises(wr.ResourceError) as cm: + res = wr.ResourceResolver(None) + + self.assertEqual( + str(cm.exception), + 'ResourceResolver members can only contain instances of ' + '``ResourceGroup`` or ``Resource``' + ) + def test_ResourceConflictError(self): counter = Counter(['a', 'b', 'b', 'c', 'c']) err = wr.ResourceConflictError(counter) @@ -452,7 +561,7 @@ def test_ResourceCircularDependencyError(self): err = wr.ResourceCircularDependencyError([resource]) self.assertEqual(str(err), ( 'Resources define circular dependencies: ' - '[Resource name="res1", depends="[\'res2\']"]' + """[]""" )) def test_ResourceMissingDependencyError(self): @@ -460,7 +569,7 @@ def test_ResourceMissingDependencyError(self): err = wr.ResourceMissingDependencyError(resource) self.assertEqual(str(err), ( 'Resource defines missing dependency: ' - 'Resource name="res", depends="[\'missing\']"' + """""" )) def test_ResourceResolver__flat_resources(self): @@ -687,7 +796,7 @@ def test_GracefulResourceRenderer(self): # check if its ours self.assertEqual( captured.records[0].getMessage().split('\n')[0], - 'Failure to render resource "js2"', + """File not found for resource """ ) else: # pragma: nocover rendered = renderer.render() @@ -699,7 +808,7 @@ def test_GracefulResourceRenderer(self): '\n' '\n' - '' + """""" )) From 4ff656ad8251999a06026f2cee304fa768f4da79 Mon Sep 17 00:00:00 2001 From: Johannes Raggam Date: Mon, 26 May 2025 20:32:55 +0200 Subject: [PATCH 2/3] Also handle resource resolver errors gracefully in the GracefulResourceRenderer. --- CHANGES.rst | 3 +++ webresource/_api.py | 14 +++++++++++++- webresource/tests.py | 29 +++++++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 1 deletion(-) diff --git a/CHANGES.rst b/CHANGES.rst index e0dc537..69f6fb9 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -4,6 +4,9 @@ Changelog 1.3 (unreleased) ---------------- +- Also handle resource resolver errors gracefully in the GracefulResourceRenderer. + [thet] + - Improve error messages by including more detailed information. [thet] diff --git a/webresource/_api.py b/webresource/_api.py index fce9cc5..8fd224c 100644 --- a/webresource/_api.py +++ b/webresource/_api.py @@ -740,7 +740,19 @@ class GracefulResourceRenderer(ResourceRenderer): def render(self): lines = [] - for resource in self.resolver.resolve(): + resources = [] + + try: + resources = self.resolver.resolve() + except ( + ResourceConflictError, + ResourceCircularDependencyError, + ResourceMissingDependencyError, + ) as e: + error_message = str(e) + logger.exception(error_message) + + for resource in resources: error_message = None try: lines.append(resource.render(self.base_url)) diff --git a/webresource/tests.py b/webresource/tests.py index 56778c6..9116eed 100644 --- a/webresource/tests.py +++ b/webresource/tests.py @@ -811,6 +811,35 @@ def test_GracefulResourceRenderer(self): """""" )) + def test_GreacefulResourceRenderer_resolver_errors(self): + # Get log level to restore it later + import logging + original_log_level = logging.getLogger().getEffectiveLevel() + + # Create a resource with a circular dependency + resource = Resource(name='res1', resource='res1.ext', depends='res1') + + resolver = wr.ResourceResolver([resource]) + renderer = wr.GracefulResourceRenderer(resolver) + + rendered = None + try: + # Supress error traceback in logs + logging.disable(logging.CRITICAL) + rendered = renderer.render() + # Restore logging level + logging.disable(original_log_level) + except wr.ResourceCircularDependencyError: # pragma: nocover + self.fail( + 'GracefulResourceRenderer should not raise ' + 'ResourceCircularDependencyError' + ) + + # No error is raised, but rendered is also empty. + # However, a server rendered HTML based UI form should still be + # interactible. + self.assertEqual(rendered, "") + if __name__ == '__main__': unittest.main() From 9cb6ecd6998a533e2491a0f40a765742a3048cb6 Mon Sep 17 00:00:00 2001 From: Johannes Raggam Date: Mon, 26 May 2025 20:57:13 +0200 Subject: [PATCH 3/3] Allow to pass an error_callback for the GracefulResourceRenderer. This allows to run some code in an error case, e.g. to add a user-visible status message. --- CHANGES.rst | 5 ++++ webresource/_api.py | 6 +++- webresource/tests.py | 65 ++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 70 insertions(+), 6 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 69f6fb9..a49d53f 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -4,6 +4,11 @@ Changelog 1.3 (unreleased) ---------------- +- Allow to pass an error_callback for the GracefulResourceRenderer. + This allows to run some code in an error case, e.g. to add a user-visible + status message. + [thet] + - Also handle resource resolver errors gracefully in the GracefulResourceRenderer. [thet] diff --git a/webresource/_api.py b/webresource/_api.py index 8fd224c..ca72958 100644 --- a/webresource/_api.py +++ b/webresource/_api.py @@ -738,7 +738,7 @@ def render(self): class GracefulResourceRenderer(ResourceRenderer): """Resource renderer, which does not fail but logs an exception.""" - def render(self): + def render(self, error_callback=None): lines = [] resources = [] @@ -751,6 +751,8 @@ def render(self): ) as e: error_message = str(e) logger.exception(error_message) + if error_callback: + error_callback(error_message) for resource in resources: error_message = None @@ -768,4 +770,6 @@ def render(self): error_message )) logger.exception(error_message) + if error_callback: + error_callback(error_message) return u'\n'.join(lines) diff --git a/webresource/tests.py b/webresource/tests.py index 9116eed..9d0537b 100644 --- a/webresource/tests.py +++ b/webresource/tests.py @@ -7,19 +7,23 @@ ResourceConfig, ResourceMixin ) +import logging import os import shutil import tempfile import unittest import webresource as wr - try: FileNotFoundError except NameError: # pragma: nocover FileNotFoundError = EnvironmentError +# Get log level to restore it later +original_log_level = logging.getLogger().getEffectiveLevel() + + def temp_directory(fn): def wrapper(*a, **kw): tempdir = tempfile.mkdtemp() @@ -812,10 +816,6 @@ def test_GracefulResourceRenderer(self): )) def test_GreacefulResourceRenderer_resolver_errors(self): - # Get log level to restore it later - import logging - original_log_level = logging.getLogger().getEffectiveLevel() - # Create a resource with a circular dependency resource = Resource(name='res1', resource='res1.ext', depends='res1') @@ -840,6 +840,61 @@ def test_GreacefulResourceRenderer_resolver_errors(self): # interactible. self.assertEqual(rendered, "") + def test_GreacefulResourceRenderer_callback_resolver(self): + # Callback which should be called on error + error_message = [] + + def callback(message): + error_message.append(message) + + # Create a resource with a circular dependency + resource = Resource(name='res1', resource='res1.ext', depends='res1') + + resolver = wr.ResourceResolver([resource]) + renderer = wr.GracefulResourceRenderer(resolver) + + # Supress error traceback in logs + logging.disable(logging.CRITICAL) + renderer.render(error_callback=callback) + # Restore logging level + logging.disable(original_log_level) + + self.assertEqual(len(error_message), 1) + self.assertEqual( + error_message[-1], + "Resources define circular dependencies: " + """[]""" + ) + + def test_GreacefulResourceRenderer_callback_render(self): + # Callback which should be called on error + error_message = [] + + def callback(message): + error_message.append(message) + + # Create a resource with a circular dependency + resource = wr.ScriptResource( + name='res1', + resource='res1.ext', + unique=True + ) + + resolver = wr.ResourceResolver([resource]) + renderer = wr.GracefulResourceRenderer(resolver) + + # Supress error traceback in logs + logging.disable(logging.CRITICAL) + renderer.render(error_callback=callback) + # Restore logging level + logging.disable(original_log_level) + + self.assertEqual(len(error_message), 1) + self.assertEqual( + error_message[-1], + """No directory set on resource """ + ) + if __name__ == '__main__': unittest.main()