Fixed issue #314 - include information about view/serializer in warnings #322

spookylukey · 2021-03-02T09:16:15Z

I've used two techniques for doing this:

If we already have a view/viewset/serailizer object in our local scope, we can add its name into the warning/error message.
For other deeply nested places from which we call error() or warn(), we instead use a stack of traces that get added as prefixes to the message. Although this uses a global variable, it uses a fairly safe context manager pattern for doing so (as long as drf_spectacular doesn't go multi-threaded for the schema generation, which seems unlikely), and overall this seems much cleaner and more usable than having to pass this context information through many layers of code.

This PR also fixes a set of consistency issues with warning/error messages:

Always start message with {serializer/view name}: if possible. This means that method 1 and 2 above produce the same format of message.
First sentence shouldn't have a capital letter (the message is printed after colon from the prefix)
Subsequent sentences should have a capital letter.

I've added tests for quite a few cases that are improved, but not every possible combination because there are too many. On the current project I'm using drf_spectacular for, which has 500+ errors/warnings generated, this is a big boost in the consistency and usefulness of the error messages. I now have a list that looks like:

Warning #0: MyViewSet: could not derive type of path parameter "thing_id" for view  because <...>. Defaulting to "string".
Warning #1: MyViewSet: failed to obtain model through view's queryset due to raised exception. Prevent this either by <...>
Warning #2: MyThingSerializerr: unable to resolve type hint for function <...>
<several hundred more...>

etc. This makes it massively easier to track down the errors.

In some cases it is possible to get MyViewSet: MySerializer: ... and even MySerializer: MyNestedSerializer: ..., but usually there is just a single prefixed name.

I think in most cases this is about ideal. We probably don't want to include module names as well - in most cases this would add unnecessary noise.

Due to the addition of prefixes into the message, this change can result in more messages being printed, because there are more "unique" messages:

It's possible that messages that were dupes before actually have distinct causes (e.g. distinct serializers that have the same issue).
It's also possible that messages that had the same root cause and were deduplicated before are now being treated as distinct.

It's very difficult to distinguish between these two situations, overall I think this part of the change is neutral/positive.

Thanks for your time and consideration of this PR!

…in warnings. Two techniques for doing this: 1. If we already have a view/viewset object in our local scope, we can add it into the message 2. For other deeply nested places from which we call `error()` or `warn()`, we instead use a stack of traces that get added as prefices to the message (rather than having to pass this context information through many layers of code). This also fixes a set of consistency issues with warning/error messages: - Always start message with `{serializer/view name}: ` if possible. This means that method 1 and 2 above produce the same format of message. - First sentence shouldn't have a capital letter (the message is printed after colon from the prefix) - Subsequent sentences should have a capital letter.

codecov · 2021-03-02T09:18:52Z

Codecov Report

Merging #322 (f1d48d0) into master (02fcef2) will increase coverage by 0.02%.
The diff coverage is 98.27%.

@@            Coverage Diff             @@
##           master     #322      +/-   ##
==========================================
+ Coverage   98.22%   98.24%   +0.02%     
==========================================
  Files          53       54       +1     
  Lines        4729     4788      +59     
==========================================
+ Hits         4645     4704      +59     
  Misses         84       84

Impacted Files	Coverage Δ
drf_spectacular/hooks.py	`100.00% <ø> (ø)`
drf_spectacular/openapi.py	`95.85% <95.23%> (+<0.01%)`	⬆️
drf_spectacular/drainage.py	`98.21% <100.00%> (+0.34%)`	⬆️
drf_spectacular/plumbing.py	`96.74% <100.00%> (+0.01%)`	⬆️
tests/__init__.py	`98.14% <100.00%> (+0.23%)`	⬆️
tests/test_command.py	`100.00% <100.00%> (ø)`
tests/test_warnings.py	`100.00% <100.00%> (ø)`
tests/contrib/test_django_filters.py	`100.00% <0.00%> (ø)`
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02fcef2...f1d48d0. Read the comment docs.

tfranzel

Hey @spookylukey, i love this this change and some people already asked for it. thank you very much!

the majority of comments is in regard to a single point. by adding the context manager to the generator too, we can remove the test hack and manual string prefix hacks. the view trace would be consistently added to everything. what do you think?

Due to the addition of prefixes into the message, this change can result in more messages being printed, because there are more "unique" messages.

i think the benefits of traceability significantly outweighs the added noise. it's worth it imho.

tfranzel · 2021-03-06T10:07:15Z

drf_spectacular/openapi.py

-                    f'is untyped and obtaining queryset from {self.view.__class__} failed. '
-                    f'consider adding a type to the path (e.g. <int:{variable}>) or annotating '
-                    f'the parameter type with @extend_schema. defaulting to "string".'
+                    f'{self.view.__class__.__name__}: could not derive type of path parameter '


prefix can be dropped again (see other comment)

tfranzel · 2021-03-06T10:07:22Z

drf_spectacular/openapi.py

-                        f'could not derive type of path parameter "{variable}" because '
-                        f'model "{model}" did contain no such field. consider annotating '
-                        f'parameter with @extend_schema. defaulting to "string".'
+                        f'{self.view.__class__.__name__}: could not derive type of path '


prefix can be dropped again (see other comment)

tfranzel · 2021-03-06T10:07:44Z

drf_spectacular/openapi.py

@@ -835,15 +836,15 @@ def _get_serializer(self):
                    return view.serializer_class
                else:
                    error(
-                        f'Unable to guess serializer for {view.__class__.__name__}. This is graceful '
+                        f'{view.__class__.__name__}: unable to guess serializer. This is graceful '


prefix can be dropped again (see other comment)

tfranzel · 2021-03-06T10:08:05Z

drf_spectacular/plumbing.py

-            f'failed to obtain model through view\'s queryset due to raised exception. '
-            f'prevent this either by setting "queryset = Model.objects.none()" on the view, '
-            f'having an empty fallback in get_queryset() or by using @extend_schema. '
+            f'{view.__class__.__name__}: failed to obtain model through view\'s queryset due to '


prefix can be dropped again (see other comment)

tfranzel · 2021-03-06T10:08:57Z

tests/__init__.py

-    schema = generator.get_schema(request=None, public=True)
-    validate_schema(schema)  # make sure generated schemas are always valid
-    return schema
+    with add_trace_message(trace_message):


usage of the context manager and manual traces can be dropped in favor of the other solution. this also keeps the tests closer to reality

tfranzel · 2021-03-06T10:17:08Z

drf_spectacular/openapi.py

-            del self.registry[component]
-            return ResolvedComponent(None, None)  # sentinel
-        return component
+        with add_trace_message(serializer.__class__.__name__):


my OCD complains here due to increasing indent and obfuscating the git blame, but what can you do if that is the proper thing to do.

Yep, me too, but I couldn't see a way around it. This consideration was in fact one of the reasons why I didn't want to put add_trace_message round the big block in the generators.py file. However I found in the end I only needed to put in around one line there. See #327

tfranzel · 2021-03-09T08:00:48Z

closed in favor of #327

tfranzel reviewed Mar 6, 2021

View reviewed changes

spookylukey mentioned this pull request Mar 8, 2021

Fixed issue #314 - include information about view/serializer in warnings #327

Merged

tfranzel closed this Mar 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed issue #314 - include information about view/serializer in warnings #322

Fixed issue #314 - include information about view/serializer in warnings #322

spookylukey commented Mar 2, 2021 •

edited

Loading

codecov bot commented Mar 2, 2021 •

edited

Loading

tfranzel left a comment

tfranzel Mar 6, 2021

tfranzel Mar 6, 2021

tfranzel Mar 6, 2021

tfranzel Mar 6, 2021

tfranzel Mar 6, 2021

tfranzel Mar 6, 2021

spookylukey Mar 8, 2021

tfranzel commented Mar 9, 2021

Fixed issue #314 - include information about view/serializer in warnings #322

Fixed issue #314 - include information about view/serializer in warnings #322

Conversation

spookylukey commented Mar 2, 2021 • edited Loading

codecov bot commented Mar 2, 2021 • edited Loading

Codecov Report

tfranzel left a comment

Choose a reason for hiding this comment

tfranzel Mar 6, 2021

Choose a reason for hiding this comment

tfranzel Mar 6, 2021

Choose a reason for hiding this comment

tfranzel Mar 6, 2021

Choose a reason for hiding this comment

tfranzel Mar 6, 2021

Choose a reason for hiding this comment

tfranzel Mar 6, 2021

Choose a reason for hiding this comment

tfranzel Mar 6, 2021

Choose a reason for hiding this comment

spookylukey Mar 8, 2021

Choose a reason for hiding this comment

tfranzel commented Mar 9, 2021

spookylukey commented Mar 2, 2021 •

edited

Loading

codecov bot commented Mar 2, 2021 •

edited

Loading