django-cms / djangocms-versioning

General purpose versioning package for Django CMS 4 and above.
Other
33 stars 29 forks source link

Add caching to PageContent __bool__ #346

Closed stefanw closed 1 year ago

stefanw commented 1 year ago

Description

Add a cache to PageContent.__bool__. I've experienced that on a typical CMS edit page there are thousands of calls to this function which all result in a database query. Obviously this cache could become stale. If you have ideas where that could happen, please share. However, reducing a page load by 2000+ SQL queries sure seems like worth a try.

Checklist

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage has no change and project coverage change: -0.01% :warning:

Comparison is base (172a2b3) 90.93% compared to head (6f48a7e) 90.93%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #346 +/- ## ========================================== - Coverage 90.93% 90.93% -0.01% ========================================== Files 72 72 Lines 2537 2536 -1 Branches 358 357 -1 ========================================== - Hits 2307 2306 -1 Misses 170 170 Partials 60 60 ``` | [Files Changed](https://app.codecov.io/gh/django-cms/djangocms-versioning/pull/346?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=django-cms) | Coverage Δ | | |---|---|---| | [djangocms\_versioning/cms\_config.py](https://app.codecov.io/gh/django-cms/djangocms-versioning/pull/346?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=django-cms#diff-ZGphbmdvY21zX3ZlcnNpb25pbmcvY21zX2NvbmZpZy5weQ==) | `80.82% <ø> (-0.09%)` | :arrow_down: | | [djangocms\_versioning/test\_utils/factories.py](https://app.codecov.io/gh/django-cms/djangocms-versioning/pull/346?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=django-cms#diff-ZGphbmdvY21zX3ZlcnNpb25pbmcvdGVzdF91dGlscy9mYWN0b3JpZXMucHk=) | `94.50% <0.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

fsbraun commented 1 year ago

@stefanw Some tests are failing. Can you take a look?

stefanw commented 1 year ago

I replaced a __bool__ in the test factory with is not None and now the tests fail a bit nicer 😁 . Now it might actually be cache invalidation problems, I'll have a look.

stefanw commented 1 year ago

I fixed up a test that failed because of a change between 4.1.0rc3 and 4.1.0rc4 (0b0367e).

And I removed the __bool__ on PageContent and all tests pass locally and I wonder if __bool__ is really required, @fsbraun? I don't understand the consequences, but there's a lot of code that probably just wants to check for is None but is instead hitting the __bool__ on PageContent and then causing an additional database query.

fsbraun commented 1 year ago

It is thinkable that you have djangocms-versioning newly installed and there are already page content objects which do not have a version associated. To cover this situation, the database check is necessary.

I think checking for None actually is best. __bool__ now checks that the content object has a version object associated with it. I am not sure if this is used, however. @Aiky30 Do you know more?