Summary

Fix cache_memoize to work with non-sequence data.

Review Request #11856 — Created Oct. 17, 2021 and submitted Nov. 26, 2021, 3:08 p.m.

Information

Owner

david

Repository

Djblets

Branch

release-2.x

Bugs

Depends On

Blocks

11857

Reviewers

Groups

djblets

People

Description

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.

Commits

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

Issues

Description	From	Last Updated
E501 line too long (80 > 79 characters)	reviewbot	Oct. 17, 2021, 3:14 p.m.
We can't get rid of use_generator without a major version bump. Let's leave it in and put up a deprecation …	chipx86	Nov. 17, 2021, 9:10 a.m.
Can you add a "Version Changed" here to notify that non-string/list types are now supported?	chipx86	Nov. 17, 2021, 9:10 a.m.
This comment is old and wrong about some details, though the general problem remains. Maybe worth updating? Looking at the …	chipx86	Nov. 17, 2021, 10:01 a.m.
While here, can you change this to unicode?	chipx86	Nov. 17, 2021, 9:56 a.m.
Same here.	chipx86	Nov. 17, 2021, 9:56 a.m.
Minor thing, but want to change these as well to be cache_func and cache_called?	chipx86	Nov. 17, 2021, 9:56 a.m.

flake8 failed.

JSHint passed.

flake8

djblets/cache/backend.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (80 > 79 characters)
```

Commits:

	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.	77a67c2dcc611edaaa050d8fadb46897da43bb8d
	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.	b75ff82c039eb432e3112eb20cd71fdb2ded5e99

Diff:

Revision 2 (+84 -30)

Show changes

	djblets/cache/backend.py
	djblets/cache/tests/test_backend.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

djblets/cache/backend.py (Diff revision 2)

The issue has been resolved. 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.

djblets/cache/backend.py (Diff revision 2)

The issue has been resolved. Show all issues

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

djblets/cache/backend.py (Diff revision 2)

The issue has been resolved. 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.

david Nov. 17, 2021, 10:01 a.m.
```
Tried to make the comment clearer.
```

djblets/cache/backend.py (Diff revision 2)
The issue has been resolved. Show all issues
```
While here, can you change this to unicode?
```
djblets/cache/backend.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Same here.
```

djblets/cache/tests/test_backend.py (Diff revision 2)

The issue has been resolved. Show all issues

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

Commits:

	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.	b75ff82c039eb432e3112eb20cd71fdb2ded5e99
	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

Diff:

Revision 3 (+128 -48)

Show changes

	djblets/cache/backend.py
	djblets/cache/tests/test_backend.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-2.x (9a078ad)