[fix] Fixed problems found during testing

[nemesifier]

• Fixed loading of minified files in django
• Fixed double hash
• Fixed hasing and precompression

[coderabbitai]

Walkthrough

This PR modifies the static file storage system to change the minified filename format from name.min.{hash}.ext to name.{hash}.min.ext, updates the hashing algorithm from SHA-256 to MD5, introduces an allow_min parameter to compression methods, refactors manifest handling to map hashed original files to minified versions, and removes local filesystem fallback logic from file write operations. The changes affect how minified CSS/JS files are named, how they are compressed, and how Django's static file manifest is updated and consulted during post-processing.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• [fix] Fix collectstatic post-processing for Django 4.2+ #4 — Modifies storage post-processing, minification/compression mixins, manifest handling, and hashing utilities, directly overlapping with core refactored components in this PR
• [feature] Improved security, performance, test coverage, agents #5 — Updates the same modules (storage.py and utils.py) including generate_file_hash and hashed filename behavior, representing related prior work on hashing and minified file naming

🚥 Pre-merge checks | ✅ 1 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title is vague and generic, using non-specific language like 'problems found during testing' without describing what was actually fixed or the main change.	Provide a more specific title that describes the main fix, such as 'Fix minified filename hashing and compression handling' or similar.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description is somewhat vague but does reference real changes in the PR: minified file loading, double hashing, and hashing/precompression issues are all related to actual modifications in the changeset.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch staging

---

🧹 Recent nitpick comments

django_minify_compress_staticfiles/storage.py (3)

292-302: Consider logging the exception for debugging purposes.

While the fallback to original paths is appropriate, silently swallowing all exceptions makes troubleshooting difficult. Adding debug-level logging would help diagnose manifest-related issues without affecting normal operation.

♻️ Proposed fix to add debug logging

         try:
             if hasattr(self, "read_manifest"):
                 manifest_json = self.read_manifest()
                 if manifest_json:
                     manifest = json.loads(manifest_json)
                     processed_paths = list(manifest.get("paths", {}).values())
-        except Exception:
-            pass
+        except Exception as e:
+            logger.debug(f"Could not read manifest, using original paths: {e}")

---

238-243: The is_text parameter is no longer used.

The is_text parameter is still in the method signature but is not used in the implementation. Consider removing it for cleanliness, or document why it's retained for future use.

♻️ Proposed fix to remove unused parameter

-    def _write_file_content(self, path, content, is_text=True):
-        """Write file content using storage methods."""
+    def _write_file_content(self, path, content):
+        """Write file content using storage methods."""
         if not is_safe_path(path):
             logger.warning(f"Skipping unsafe path for writing: {path}")
             return
         self.save(path, ContentFile(content))

Then update call sites at lines 135, 190-191, and 200 to remove the is_text argument.

---

344-369: Consider extracting the duplicated path normalization logic.

The absolute-to-relative path conversion logic is duplicated for both hashed_path and minified_path. Extracting this into a helper method would improve maintainability.

♻️ Proposed refactor to extract path normalization

+    def _normalize_to_relative(self, path):
+        """Convert absolute path to relative path."""
+        if os.path.isabs(path):
+            path_obj = Path(path)
+            parts = path_obj.parts
+            if len(parts) > 1:
+                return os.path.join(*parts[1:])
+            else:
+                return os.path.basename(path)
+        return path
+
     def _update_manifest(self, minified_files):
         # ... existing code ...
         for hashed_path, minified_path in minified_files.items():
-            # Normalize paths
-            hashed_relative = hashed_path
-            if os.path.isabs(hashed_path):
-                path_obj = Path(hashed_path)
-                parts = path_obj.parts
-                if len(parts) > 1:
-                    hashed_relative = os.path.join(*parts[1:])
-                else:
-                    hashed_relative = os.path.basename(hashed_path)
-
-            minified_relative = minified_path
-            if os.path.isabs(minified_path):
-                path_obj = Path(minified_path)
-                parts = path_obj.parts
-                if len(parts) > 1:
-                    minified_relative = os.path.join(*parts[1:])
-                else:
-                    minified_relative = os.path.basename(minified_path)
+            hashed_relative = self._normalize_to_relative(hashed_path)
+            minified_relative = self._normalize_to_relative(minified_path)

tests/test_storage.py (1)

516-524: Move inline import to file-level imports.

The re module import at line 523 should be moved to the top of the file with other imports for consistency and to avoid repeated imports if the test runs multiple times.

♻️ Proposed fix

Add at top of file with other imports:

import re

Then remove line 523:

-        import re
-
         with self.settings(STATIC_ROOT=self.static_root):

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d5f3df5 and 76bb6b6.

📒 Files selected for processing (6)

• README.rst
• django_minify_compress_staticfiles/storage.py
• django_minify_compress_staticfiles/utils.py
• tests/test_integration.py
• tests/test_security.py
• tests/test_storage.py

💤 Files with no reviewable changes (1)

• tests/test_security.py

🧰 Additional context used

📓 Path-based instructions (3)

**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Code must be formatted using openwisp-qa-format command
Avoid unnecessary blank lines - use them sparingly, only where PEP 8 requires them (e.g., between class methods, between imports and code)
Ensure is_safe_path() is used for file path validation to prevent path traversal vulnerabilities
Python code must pass isort, black, and flake8 validation checks
All Python files must end with a blank line

Files:

• django_minify_compress_staticfiles/utils.py
• tests/test_integration.py
• tests/test_storage.py
• django_minify_compress_staticfiles/storage.py

**/test*.py

📄 CodeRabbit inference engine (AGENTS.md)

Keep test coverage above 90% - every new feature must include tests

Files:

• tests/test_integration.py
• tests/test_storage.py

**/*.rst

📄 CodeRabbit inference engine (AGENTS.md)

ReStructuredText files must have valid syntax as validated by QA checks

Files:

• README.rst

🧬 Code graph analysis (1)

tests/test_storage.py (1)

django_minify_compress_staticfiles/storage.py (4)

• process_minification (90-141)
• process_compression (147-206)
• MinicompressStorage (278-376)
• post_process (283-319)

🪛 Ruff (0.14.14)

django_minify_compress_staticfiles/utils.py

[error] 47-47: Probable use of insecure hash functions in hashlib: md5

(S324)

---

[error] 60-60: Probable use of insecure hash functions in hashlib: md5

(S324)

django_minify_compress_staticfiles/storage.py

[error] 298-299: try-except-pass detected, consider logging the exception

(S110)

---

[warning] 298-298: Do not catch blind exception: Exception

(BLE001)

🔇 Additional comments (11)

django_minify_compress_staticfiles/utils.py (1)

39-60: MD5 usage is appropriate here for Django compatibility.

The static analysis tool flags MD5 as insecure (S324), but this is a false positive in this context. MD5 is used for content-addressable file hashing to match Django's ManifestFilesMixin algorithm, not for cryptographic security purposes. The docstring appropriately documents this rationale.

django_minify_compress_staticfiles/storage.py (3)

36-48: LGTM on the allow_min parameter addition.

The logic correctly bypasses the standard eligibility check when allow_min=True, while still validating extensions. This enables compression of minified files that would otherwise be excluded by the *.min.* pattern.

---

121-136: LGTM on minified filename convention.

The new naming pattern (name.{hash}.min.ext) correctly preserves Django's hash while adding .min before the extension. This addresses the "double hash" issue mentioned in the PR objectives.

---

208-236: LGTM on the read file content logic.

The method correctly prioritizes storage methods while maintaining a useful fallback to the local filesystem for edge cases.

README.rst (2)

32-35: LGTM on documentation update.

The minified filename format documentation (name.{hash}.min.ext) accurately reflects the code changes and explains the benefits for serving precompressed files.

---

198-201: LGTM on MD5 explanation.

The documentation clearly explains the rationale for using MD5 (matching Django's ManifestFilesMixin algorithm) and its purpose for ensuring correct manifest mapping.

tests/test_storage.py (4)

182-218: LGTM on the minified filename test.

This test effectively validates the core fix for the "double hash" issue by verifying that Django's existing hash is preserved and .min is inserted before the extension.

---

220-259: LGTM on compression test for minified files.

The test correctly verifies that compressed files are created for minified versions (not originals) and validates the expected filename pattern including the allow_min=True flow.

---

658-711: LGTM on manifest structure validation test.

This test ensures the manifest has the proper structure (paths, version, and settings/hash keys) required by Django's ManifestFilesMixin, which is critical for the {% static %} template tag to work correctly.

---

745-769: LGTM on collectstatic integration test.

This test provides valuable end-to-end validation that collectstatic produces a valid manifest that Django can use.

tests/test_integration.py (1)

117-139: LGTM on the relaxed assertion.

The change from assertEqual to assertGreaterEqual correctly accommodates the updated behavior where post_process now yields both original and minified files. The comment at line 137-138 clearly explains the expected behavior.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coveralls]

Pull Request Test Coverage Report for Build 21552626626

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 21 unchanged lines in 2 files lost coverage.
• Overall coverage decreased (-2.6%) to 92.458%

---

Files with Coverage Reduction	New Missed Lines	%
utils.py	2	95.58%
storage.py	19	90.87%

Totals
Change from base Build 21551053582:	-2.6%
Covered Lines:	331
Relevant Lines:	358

---

💛 - Coveralls