django / django/new-features

Don't let cache backends swallow explicit errors in cache arguments

Open
#171 1 comment 6 reactions 0 assignees View on GitHub
Dominant language
No language data
Stars
188
Forks
7
PR merge metrics
No merged PRs in 30d

Description

### Code of Conduct

- [x] I agree to follow Django's Code of Conduct

### Feature Description

Several [cache arguments](https://docs.djangoproject.com/en/6.1/topics/cache/#cache-arguments) to the caching backend automatically and silently revert to defaults if there's an explicit error in the configuration. I propose that Django should instead complain about the misconfiguration.

### Problem

The following ​[cache arguments](https://docs.djangoproject.com/en/6.1/topics/cache/#cache-arguments)

- `TIMEOUT`
- `OPTIONS['MAX_ENTRIES']`
- `OPTIONS['CULL_FREQUENCY']`

are integer values that are handled by ​[`BaseCache`](https://github.com/django/django/blob/4bbc27c8686f10f9556cef02dbfa9f5157fbcf56/django/core/cache/backends/base.py) such that non-integer values are ignored and fallback to the default. For example:

- If a Django app didn't set a `TIMEOUT` for the caching backend, the cache timeout would be set to `300`, the default;
- If a Django app set a `TIMEOUT` for the caching backend to `0`, the cache timeout would be set to `0`, as expected;
- If a Django app somehow set a `TIMEOUT` of `'abc'`, the cache timeout would be set to `300`.

My feeling is that the third behavior is wrong. If any of these settings can't be parsed by Django as an integer, Django should throw an error to make it clear to the user that the app is improperly configured. At present, it's possible for a Django application to, for example, silently be using a lower timeout on cache entries than the developer intended.

### Request or proposal

proposal

### Additional Details

- (I originally ticketed this issue on Trac: https://code.djangoproject.com/ticket/37113. It was suggested that I propose the issue here instead.)
- This behavior does not seem to be documented. The cache arguments section of the documentation mentions the default values, and makes it fairly clear that the settings are for integers, but it doesn't mention that inappropriate values are forced to the defaults.
- This behavior does not seem to be tested in the test suite. If the error swallowing logic is removed, the test suite still passes.
- Per the previous two points: even if this proposal _isn't_ accepted, the current behavior of the caching backend should be documented and tested.

### Implementation Suggestions

The implementation should be straightforward: modify the logic in [`BaseCache.__init__`](https://github.com/django/django/blob/4bbc27c8686f10f9556cef02dbfa9f5157fbcf56/django/core/cache/backends/base.py#L61) to not ignore errors like it currently does. At the most basic level, drop the exception handling, e.g., pivot the handling of the timeout from
```python
timeout = params.get("timeout", params.get("TIMEOUT", 300))
if timeout is not None:
try:
timeout = int(timeout)
except (ValueError, TypeError):
timeout = 300
self.default_timeout = timeout
```
to something like
```python
timeout = params.get("timeout", params.get("TIMEOUT", 300))
if timeout is not None:
timeout = int(timeout)
self.default_timeout = timeout
```
But it would almost certainly be better to continue to catch bad values, but instead of ignoring, raise a Django exception instead, like [`ImproperlyConfigured`](https://github.com/django/django/blob/4bbc27c8686f10f9556cef02dbfa9f5157fbcf56/django/core/exceptions.py#L123).

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.