VNG-Realisatie / vng-api-common

Gedeelde code voor RESTful APIs
5 stars 12 forks source link

Cache header implementation rework #198

Closed sergei-maertens closed 2 years ago

sergei-maertens commented 2 years ago

While implementing tests in Open Zaak, I realised that the current signal/model based approach is completely broken, so this PR reworks the caching funtionality. The public API should remain unmodified.

The approach is:

codecov-commenter commented 2 years ago

Codecov Report

Merging #198 (de5133a) into master (43067d1) will increase coverage by 0.44%. The diff coverage is 89.55%.

@@            Coverage Diff             @@
##           master     #198      +/-   ##
==========================================
+ Coverage   51.09%   51.53%   +0.44%     
==========================================
  Files          87       88       +1     
  Lines        3687     3735      +48     
  Branches      691      687       -4     
==========================================
+ Hits         1884     1925      +41     
- Misses       1711     1714       +3     
- Partials       92       96       +4     
Impacted Files Coverage Δ
vng_api_common/caching/models.py 85.71% <ø> (ø)
vng_api_common/caching/signals.py 85.24% <83.33%> (-1.33%) :arrow_down:
vng_api_common/caching/registry.py 87.93% <87.93%> (ø)
vng_api_common/caching/decorators.py 94.11% <100.00%> (+1.26%) :arrow_up:
vng_api_common/caching/etags.py 95.16% <100.00%> (+1.04%) :arrow_up:
vng_api_common/utils.py 60.15% <0.00%> (-2.35%) :arrow_down:
vng_api_common/geo.py 32.35% <0.00%> (-1.94%) :arrow_down:
vng_api_common/checks.py 60.00% <0.00%> (-1.91%) :arrow_down:
vng_api_common/authorizations/serializers.py 56.66% <0.00%> (-1.40%) :arrow_down:
vng_api_common/validators.py 56.47% <0.00%> (-1.01%) :arrow_down:
... and 10 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 43067d1...de5133a. Read the comment docs.

sergei-maertens commented 2 years ago

Looks good, just want to double check something to see if I understand it correctly: if I have a Zaak with a ZaakEigenschap, Resultaat etc. and I update the Zaak's status (causing the Zaak's etag to be updated), should all of the related resources' etags (ZaakEigenschap, Resultaat etc.) be updated as well?

I believe now it sort-of triggers a re-calculate indeed because the dependency tree is:

In practice this isn't really needed or will not lead to many changes, because only the UUID (which doesn't change) is used in the URL references to those second-level resources (zaak-url is the only zaak-property referenced in the serializer).

Additionally, with my latest fixes from this morning, the Zaak etag update will not trigger a re-calculate, since this happens with update_fields=["_etag"] and the post_save signal handler detects this case and stops the cascading updates in this case, as the _etag is not supposed to be directly exposed in any serializer (although this is implicit).