Make randomid deterministic to improve caching
This commit is contained in:
@@ -9,30 +9,26 @@
|
|||||||
- Only caches `template` path calls; `tag_name` calls are already nanosecond string ops
|
- Only caches `template` path calls; `tag_name` calls are already nanosecond string ops
|
||||||
- Verified working: identical calls return identical output, different inputs produce separate cache entries
|
- Verified working: identical calls return identical output, different inputs produce separate cache entries
|
||||||
|
|
||||||
## Pending
|
### Non-deterministic IDs
|
||||||
|
`randomid()` was replaced with `hashlib.sha1(content_hash.encode()).hexdigest()[:10]` for deterministic ID generation.
|
||||||
|
- `Popover()` passes content hash (`wrapped_content:popover_content:wrapped_classes`) so IDs are deterministic per unique content
|
||||||
|
- `games/templatetags/randomid.py` uses the same hash-based approach
|
||||||
|
- Fixes: caching (Popover output now cacheable), page consistency, thread safety
|
||||||
|
|
||||||
### 1. Non-deterministic IDs
|
### 1. Inconsistent return types
|
||||||
`randomid()` uses `random.choices()` producing unique IDs every call. Breaks:
|
|
||||||
- Caching (can't cache Popover output because IDs change between requests)
|
|
||||||
- Page consistency (same content produces different HTML across requests)
|
|
||||||
- Thread safety of the `random` module for ID generation
|
|
||||||
|
|
||||||
**Fix**: Replace with `hashlib.sha1(content_hash.encode()).hexdigest()[:10]` based on deterministic content hash.
|
|
||||||
|
|
||||||
### 2. Inconsistent return types
|
|
||||||
`Div()`/`A()`/`Button()` return `str`, but `LinkedPurchase()`/`NameWithIcon()` return `SafeText`.
|
`Div()`/`A()`/`Button()` return `str`, but `LinkedPurchase()`/`NameWithIcon()` return `SafeText`.
|
||||||
Forces callers to remember `mark_safe()` wrapping.
|
Forces callers to remember `mark_safe()` wrapping.
|
||||||
|
|
||||||
**Fix**: Standardize — all component functions should return the same type.
|
**Fix**: Standardize — all component functions should return the same type.
|
||||||
|
|
||||||
### 3. Fragile A() URL resolution
|
### 2. Fragile A() URL resolution
|
||||||
Tries `reverse(url)` first, then falls back to literal string. Uses `type(url) is str`
|
Tries `reverse(url)` first, then falls back to literal string. Uses `type(url) is str`
|
||||||
instead of `isinstance()`. Intentional but error-prone — a string matching a URL name
|
instead of `isinstance()`. Intentional but error-prone — a string matching a URL name
|
||||||
will be reversed, while one that doesn't pass through as-is.
|
will be reversed, while one that doesn't pass through as-is.
|
||||||
|
|
||||||
**Fix**: Add explicit parameter like `url_name="view_game"` vs `href="/literal/path"`.
|
**Fix**: Add explicit parameter like `url_name="view_game"` vs `href="/literal/path"`.
|
||||||
|
|
||||||
### 4. Toast XSS vulnerability
|
### 3. Toast XSS vulnerability
|
||||||
Custom string escaping for Alpine.js interpolation:
|
Custom string escaping for Alpine.js interpolation:
|
||||||
```python
|
```python
|
||||||
safe_message = message.replace("\\", "\\\\").replace("`", "\\`")
|
safe_message = message.replace("\\", "\\\\").replace("`", "\\`")
|
||||||
@@ -42,13 +38,13 @@ Alpine expression early).
|
|||||||
|
|
||||||
**Fix**: Use proper HTML escaping + JSON serialization for safe template interpolation.
|
**Fix**: Use proper HTML escaping + JSON serialization for safe template interpolation.
|
||||||
|
|
||||||
### 5. No tests
|
### 4. No tests
|
||||||
Zero test coverage for the entire component system.
|
Zero test coverage for the entire component system.
|
||||||
|
|
||||||
**Fix**: Add unit tests for each component function — basic rendering, edge cases,
|
**Fix**: Add unit tests for each component function — basic rendering, edge cases,
|
||||||
and cache hit/miss verification.
|
and cache hit/miss verification.
|
||||||
|
|
||||||
### 6. Default mutable arguments
|
### 5. Default mutable arguments
|
||||||
`attributes: list[HTMLAttribute] = []` is a classic Python gotcha (though harmless
|
`attributes: list[HTMLAttribute] = []` is a classic Python gotcha (though harmless
|
||||||
here since the list is only read, never mutated in place).
|
here since the list is only read, never mutated in place).
|
||||||
|
|
||||||
|
|||||||
@@ -1,7 +1,6 @@
|
|||||||
|
import hashlib
|
||||||
import json
|
import json
|
||||||
from functools import lru_cache
|
from functools import lru_cache
|
||||||
from random import choices as random_choices
|
|
||||||
from string import ascii_lowercase
|
|
||||||
from typing import Any, Callable
|
from typing import Any, Callable
|
||||||
|
|
||||||
from django.conf import settings
|
from django.conf import settings
|
||||||
@@ -63,8 +62,13 @@ def Component(
|
|||||||
return mark_safe(tag)
|
return mark_safe(tag)
|
||||||
|
|
||||||
|
|
||||||
def randomid(seed: str = "", length: int = 10) -> str:
|
def randomid(seed: str = "", content: str = "", length: int = 10) -> str:
|
||||||
return seed + "".join(random_choices(ascii_lowercase, k=length))
|
if not seed and not content:
|
||||||
|
return seed
|
||||||
|
hash_input = f"{seed}:{content}" if seed else content
|
||||||
|
content_hash = hashlib.sha1(hash_input.encode()).hexdigest()
|
||||||
|
base = content_hash[:length] if not seed else content_hash[:max(0, length - len(seed))]
|
||||||
|
return seed + base
|
||||||
|
|
||||||
|
|
||||||
def Popover(
|
def Popover(
|
||||||
@@ -76,7 +80,7 @@ def Popover(
|
|||||||
) -> str:
|
) -> str:
|
||||||
if not wrapped_content and not children:
|
if not wrapped_content and not children:
|
||||||
raise ValueError("One of wrapped_content or children is required.")
|
raise ValueError("One of wrapped_content or children is required.")
|
||||||
id = randomid()
|
id = randomid(content=f"{wrapped_content}:{popover_content}:{wrapped_classes}")
|
||||||
return Component(
|
return Component(
|
||||||
attributes=attributes
|
attributes=attributes
|
||||||
+ [
|
+ [
|
||||||
|
|||||||
@@ -1,5 +1,4 @@
|
|||||||
import random
|
import hashlib
|
||||||
import string
|
|
||||||
|
|
||||||
from django import template
|
from django import template
|
||||||
|
|
||||||
@@ -8,4 +7,7 @@ register = template.Library()
|
|||||||
|
|
||||||
@register.simple_tag
|
@register.simple_tag
|
||||||
def randomid(seed: str = "") -> str:
|
def randomid(seed: str = "") -> str:
|
||||||
return str(hash(seed + "".join(random.choices(string.ascii_lowercase, k=10))))
|
content_hash = hashlib.sha1(seed.encode()).hexdigest()
|
||||||
|
if seed:
|
||||||
|
return content_hash[:max(0, 10 - len(seed))] + seed
|
||||||
|
return content_hash[:10]
|
||||||
|
|||||||
Reference in New Issue
Block a user