Fix inheritance bugs with @extend_schema_view().

[ngnpope]

When creating a copy of a method from a parent class we now:

• Ensure that __qualname__ is defined correctly
  • i.e. Child.method instead of Parent.method.
  • This isn't essential but helps diagnosing issues when debugging.
• Move application of the decorator to the last moment.
• Deep copy the existing schema extensions before applying decorator.

This fixes #218 where two child classes with @extend_schema_view affect each other - schema extensions are applied to the parent such that the second child overwrites the changes applied to the first child.

This also fixes my case where a child with @extend_schema_view clobbered the schema extensions of the parent which also used @extend_schema_view.

[codecov]

Codecov Report

Merging #554 (4aaaa81) into master (2a1f1ec) will decrease coverage by 0.00%.
The diff coverage is 97.22%.

@@            Coverage Diff             @@
##           master     #554      +/-   ##
==========================================
- Coverage   98.77%   98.76%   -0.01%     
==========================================
  Files          58       58              
  Lines        6508     6540      +32     
==========================================
+ Hits         6428     6459      +31     
- Misses         80       81       +1     

Impacted Files	Coverage Δ
tests/test_extend_schema_view.py	98.43% <88.88%> (-1.57%)	⬇️
drf_spectacular/utils.py	98.98% <100.00%> (+<0.01%)	⬆️
tests/test_regressions.py	100.00% <100.00%> (ø)

---

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 2a1f1ec...4aaaa81. Read the comment docs.

[ngnpope]

@tfranzel This was causing quite a mess in the code that I've been converting. I hope this all makes sense?

[ngnpope]

This tests the child with @extend_schema_view inheriting from a parent with @extend_schema_view scenario.

In this case:

• We want list to be removed from the schema only for the child.
• We want retrieve to have description overridden only for the child, but retain tags from the parent.
• We want extended_action to have responses overridden only for the child, but retain description from the parent.
• We want raw_action to override the tags via @extend_schema in the child and summary via @extend_schema_view on the child without affecting the parent.
  • The only thing I'm unsure about here is that we don't get the description from the parent's @extend_schema_view, but we would get the tags from the @extend_schema on the parent if we weren't overriding it. Perhaps this needs to be tweaked to ensure that we still get description?

[ngnpope]

Hi @tfranzel 👋🏻

Did you have any thoughts on this one?

[tfranzel]

hey @ngnpope! first of all thank you for tackling this issue! Somehow I never got around doing it. I had already set aside time today to review this in detail and I didn't want to give a partial review. As you likely noticed, the mechanics are tricky and I'd like to make sure that all corner cases are covered. On first sight it looks good to me.

ZViewSet in extend_schema_view.py is totally fine by me. However, i would like to remove the doctor/appointment stuff for conciseness and clarity. What I would like to see instead is a minimal regression tests in test_regressions that focuses on that issue with as little boilerplate as possible.

[ngnpope]

Somehow I never got around doing it.

I know the feeling! I tackled it out of necessity.

As you likely noticed, the mechanics are tricky and I'd like to make sure that all corner cases are covered.

💯

However, i would like to remove the doctor/appointment stuff for conciseness and clarity. What I would like to see instead is a minimal regression tests in test_regressions that focuses on that issue with as little boilerplate as possible.

I've had a go at producing a smaller regression test. I attempted to avoid defining a custom action by using list, but the test didn't fail - only when I defined a custom notes action.

[tfranzel]

Oh boy, that was a rookie mistake 🤦 I'm quite certain I finally understood the root cause. Thanks for pointing me in the right direction, otherwise I would have missed it again.

The whole point of wrapping_decorator and that setattr is to prevent "The Clobbering". It's supposed to act as a circuit breaker so that decoration does not creep into foreign classes. I could not grasp why this worked perfectly the first go round, but not afterwards. Turns out the root cause is id(wrapped_method.kwargs) == id(method.kwargs). the deepcopy is actually superfluous as there is no issue with the Schema inheritance class.

wraps creates a shallow copy of __dict__ and so the kwargs dictionary is still the same object after decoration, which of course breaks the whole purpose of the circuit breaker. So we only need to make sure that each method has it's own kwargs dict object and thus the essential fix is this:

        if hasattr(wrapped_method, 'kwargs'):
            wrapped_method.kwargs = dict(wrapped_method.kwargs)

• Please test if that works for all your cases.
• Please roll back the __qualname__ change. I get your sentiment, but it is supposed to highlight the origin of the method, i.e. where implementation and docstring actually came from.
• If you do the honors of putting in the fix, we can finally merge this. I'll just add 1 or 2 tests on top.

[ngnpope]

Oh boy, that was a rookie mistake facepalm I'm quite certain I finally understood the root cause. Thanks for pointing me in the right direction, otherwise I would have missed it again.

No problem.

Turns out the root cause is id(wrapped_method.kwargs) == id(method.kwargs). the deepcopy is actually superfluous as there is no issue with the Schema inheritance class.

Yup. I think I was just hedging my bets with the deepcopy. The implementation is currently replacing, not merging, so we aren't concerned about deeper levels, e.g. if responses was set to {200: ...} on the parent and responses was set to {400: ...} on the child it wouldn't merge and give {200: ..., 400: ...}. While in some way that might be nice and convenient, it's hard to get right and might not always be desired.

wraps creates a shallow copy of __dict__ and so the kwargs dictionary is still the same object after decoration, which of course breaks the whole purpose of the circuit breaker.

Bingo. Everything is stored under kwargs, not directly on __dict__.

Out of interest, kwargs seems like a rather generic name to use when assigning as an attribute on classes/methods, so is there a particular reason for that? Perhaps _drf_spectacular would make sense to make it unlikely to collide with anything else?

        if hasattr(wrapped_method, 'kwargs'):
            wrapped_method.kwargs = dict(wrapped_method.kwargs)

I've opted for wrapped_method.kwargs.copy() to make a shallow copy.

• Please test if that works for all your cases.

Seems to work fine for the test cases I added.

• Please roll back the __qualname__ change. I get your sentiment, but it is supposed to highlight the origin of the method, i.e. where implementation and docstring actually came from.

Done.

[tfranzel]

I've opted for wrapped_method.kwargs.copy() to make a shallow copy.

👍

The implementation is currently replacing, not merging, so we aren't concerned about deeper levels,

I would rather call it layering (inheritance). The Schema classes are layered on top of each other. That behavior is intended like this and it follow one simple known rule: inheritance. i wouldn't know how to come up with another mechanic that is easier to understand.

Out of interest, kwargs seems like a rather generic name to use

Yes, it actually needs to be called like this. I did not come up with this naming or mechanic. It is the native DRF behaviour and DRF uses the kwargs to initialize the view methods. This comment contains more context:

drf-spectacular/drf_spectacular/utils.py

Line 383 in 6bc1d2e

# custom actions have kwargs in their context, others don't. create it so our create_view

Seems to work fine for the test cases I added.

Awesome!

[tfranzel]

hey @ngnpope, so I did some more investigating and found 2 more isolation issues.

1. @extend_schema and extend_schema_view combinations breaking isolation
2. @api_view with @extend_schema_view

Had to refactor the isolation code as it is required at multiple places actually. Please test these changes if something is behaving in a weird way, though i'm quite sure this eliminated a lot of weird behavior.

the amount of magic that is generated by@api_view is astounding. we half half a dozen checks all over the codebase only for when @api_view is doing weird stuff.

[ngnpope]

Thanks @tfranzel, that's great. It looks good for me so far.