Fix cache_memoize to work with non-sequence data.

Review Request #11856 — Created Oct. 17, 2021 and submitted

Information

Djblets
release-2.x

Reviewers

The size checks in cache_memoize were causing it to fail when the
cached function was returning something other than a sequence. This
change fixes it so we only do those length checks for string types.

While I was in here I fixed up a few small style issues.

  • Used with a change that required this.
  • Ran unit tests.
Summary ID
Fix cache_memoize to work with non-sequence data.
The size checks in cache_memoize were causing it to fail when the cached function was returning something other than a sequence. This change fixes it so we only do those length checks for string types. While I was in here I fixed up a few small style issues. Testing Done: - Used with a change that required this. - Ran unit tests.
dcfd5df252230cd99ee1995d573918812fb2f714
Description From Last Updated

E501 line too long (80 > 79 characters)

reviewbotreviewbot

We can't get rid of use_generator without a major version bump. Let's leave it in and put up a deprecation …

chipx86chipx86

Can you add a "Version Changed" here to notify that non-string/list types are now supported?

chipx86chipx86

This comment is old and wrong about some details, though the general problem remains. Maybe worth updating? Looking at the …

chipx86chipx86

While here, can you change this to unicode?

chipx86chipx86

Same here.

chipx86chipx86

Minor thing, but want to change these as well to be cache_func and cache_called?

chipx86chipx86
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

david
chipx86
  1. 
      
  2. djblets/cache/backend.py (Diff revision 2)
     
     
    Show all issues

    We can't get rid of use_generator without a major version bump. Let's leave it in and put up a deprecation notice in the docs and call, schedule it for removal in 3.0.

  3. djblets/cache/backend.py (Diff revision 2)
     
     
     
     
    Show all issues

    Can you add a "Version Changed" here to notify that non-string/list types are now supported?

  4. djblets/cache/backend.py (Diff revision 2)
     
     
     
     
     
    Show all issues

    This comment is old and wrong about some details, though the general problem remains. Maybe worth updating?

    Looking at the code, it's not so much that python-memcached/pylibmc fails silently anymore, or even has an exception for this. It actually returns a result. We just don't know about it. Django, however, does, and clears out the key.

    (Interestingly, pylibmc attempts to compress the data to make it fit, before deciding whether to give up.)

    Original problem remains: We don't know, we can't find out through setting the data, and we still have to check. As the comment says, it's still not good enough. Pylibmc checks against encoded UTF-8 data, for strings, and Pickled data for anything not otherwise handled (non-ints/longs/strings).

    Crappy situation. I really wish Django returned a result or something for us. It'd be easier to more effectively support other backends, too.

    1. Tried to make the comment clearer.

  5. djblets/cache/backend.py (Diff revision 2)
     
     
    Show all issues

    While here, can you change this to unicode?

  6. djblets/cache/backend.py (Diff revision 2)
     
     
    Show all issues

    Same here.

  7. djblets/cache/tests/test_backend.py (Diff revision 2)
     
     
     
     
    Show all issues

    Minor thing, but want to change these as well to be cache_func and cache_called?

  8. 
      
david
chipx86
  1. Ship It!
  2. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to release-2.x (9a078ad)