Repository: almarklein/asgineer

Issue:
Dont compress files that already are
`make_asset_handler()` compresses all files above a certain size, including e.g. `.png`. Instead, it should only use the compressed version if it's 90% or less of the original.

Discussion:

Patch:
diff --git a/asgineer/utils.py b/asgineer/utils.py
index 8a3c28d..eee51c6 100644
--- a/asgineer/utils.py
+++ b/asgineer/utils.py
@@ -11,14 +11,16 @@ from ._app import normalize_response, guess_content_type_from_body
 
 __all__ = ["normalize_response", "make_asset_handler", "guess_content_type_from_body"]
 
+VIDEO_EXTENSIONS = ".mp4", ".3gp", ".webm"
+
 
 def make_asset_handler(assets, max_age=0, min_compress_size=256):
     """
     Get a coroutine function for efficiently serving in-memory assets.
     The resulting handler functon takes care of setting the appropriate
-    content-type header, sending compressed responses when possible, and
-    applying appropriate HTTP caching (using etag and cache-control headers).
-    Usage:
+    content-type header, sending compressed responses when
+    possible/sensible, and applying appropriate HTTP caching (using
+    etag and cache-control headers). Usage:
     
     .. code-block:: python
     
@@ -53,7 +55,7 @@ def make_asset_handler(assets, max_age=0, min_compress_size=256):
     * If the given path is not present in the asset dict (case insensitive),
       a 404-not-found response is returned.
     * The ``etag`` header is set to a (sha256) hash of the body of the asset.
-    * The ``cache-control`` header is set to "public must-revalidate max-age=xx".
+    * The ``cache-control`` header is set to "public, must-revalidate, max-age=xx".
     * If the request has a ``if-none-match`` header that matches the etag,
       the handler responds with 304 (indicating to the client that the resource
       is still up-to-date).
@@ -61,9 +63,10 @@ def make_asset_handler(assets, max_age=0, min_compress_size=256):
       based on the filename extensions of the keys in the asset dicts. If the
       key does not contain a dot, the ``content-type`` will be based on the
       body of the asset.
-    * If the asset is over ``min_compress_size`` bytes, and the request
-      has a ``accept-encoding`` header that contains "gzip", the data
-      is send in compressed form.
+    * If the asset is over ``min_compress_size`` bytes, is not a video, the
+      request has a ``accept-encoding`` header that contains "gzip",
+      and the compressed data is less that 90% of the raw data, the
+      data is send in compressed form.
     """
 
     if not isinstance(assets, dict):
@@ -85,9 +88,12 @@ def make_asset_handler(assets, max_age=0, min_compress_size=256):
             raise ValueError("Asset bodies must be str or bytes.")
         # Store hash
         hashes[key] = hashlib.sha256(bbody).hexdigest()
-        # Store zipped version if above limit
+        # Store zipped version if it makes sense
         if len(bbody) >= min_compress_size:
-            zipped[key] = gzip.compress(bbody)
+            if not key.endswith(VIDEO_EXTENSIONS):
+                bbody_zipped = gzip.compress(bbody)
+                if len(bbody_zipped) < 0.90 * len(bbody):
+                    zipped[key] = bbody_zipped
         # Store mimetype
         ctype, enc = mimetypes.guess_type(key)
         if ctype:
@@ -109,7 +115,7 @@ def make_asset_handler(assets, max_age=0, min_compress_size=256):
         request_etag = request.headers.get("if-none-match", None)
         headers = {
             "etag": content_etag,
-            "cache-control": f"public must-revalidate max-age={max_age:d}",
+            "cache-control": f"public, must-revalidate, max-age={max_age:d}",
         }
 
         # If client already has the exact asset, send confirmation now
@@ -127,6 +133,9 @@ def make_asset_handler(assets, max_age=0, min_compress_size=256):
         else:
             body = assets[path]
 
+        # Note that we always return bytes, not a stream-response. The
+        # assets used with this utility are assumed to be small-ish,
+        # since they are in-memory.
         return 200, headers, body
 
     return asset_handler
diff --git a/examples/example_assets.py b/examples/example_assets.py
index 79c8ae2..42467ed 100644
--- a/examples/example_assets.py
+++ b/examples/example_assets.py
@@ -20,7 +20,7 @@ assets = {
 
 
 # Create a handler to server the dicts of assets
-asset_handler = asgineer.utils.make_asset_handler(assets)
+asset_handler = asgineer.utils.make_asset_handler(assets, max_age=100)
 
 
 @asgineer.to_asgi


Test patch:
diff --git a/tests/test_utils.py b/tests/test_utils.py
index 1dc7040..1654195 100644
--- a/tests/test_utils.py
+++ b/tests/test_utils.py
@@ -1,3 +1,5 @@
+import random
+
 import asgineer.utils
 
 from common import make_server, get_backend
@@ -5,10 +7,13 @@ from common import make_server, get_backend
 from pytest import raises, skip
 
 
+compressable_data = b"x" * 1000
+uncompressable_data = bytes([int(random.uniform(0, 255)) for i in range(1000)])
+
 # def test_normalize_response()  -> tested as part of test_app
 
 
-def test_make_asset_handler():
+def test_make_asset_handler_fails():
 
     if get_backend() != "mock":
         skip("Can only test this with mock server")
@@ -18,8 +23,18 @@ def test_make_asset_handler():
     with raises(ValueError):
         asgineer.utils.make_asset_handler({"notstrorbytes": 4})
 
+
+def test_make_asset_handler():
+
+    if get_backend() != "mock":
+        skip("Can only test this with mock server")
+
     # Make a server
-    assets = {"foo.html": "bla", "foo.png": b"x" * 10000}
+    assets = {
+        "foo.html": "bla",
+        "foo.bmp": compressable_data,
+        "foo.png": uncompressable_data,
+    }
     assets.update({"b.xx": b"x", "t.xx": "x", "h.xx": "<html>x</html>"})
     assets.update({"big.html": "x" * 10000, "bightml": "<html>" + "x" * 100000})
     handler = asgineer.utils.make_asset_handler(assets)
@@ -28,35 +43,41 @@ def test_make_asset_handler():
     # Do simple requests and check validity
     r0 = server.get("foo")
     r1 = server.get("foo.html")
-    r2 = server.get("foo.png")
+    r2a = server.get("foo.bmp")
+    r2b = server.get("foo.png")
 
     assert r0.status == 404
-    assert r1.status == 200 and r2.status == 200
-    assert len(r1.body) == 3 and len(r2.body) == 10000
+    assert r1.status == 200 and r2a.status == 200 and r2b.status == 200
+    assert len(r1.body) == 3 and len(r2a.body) == 1000, len(r2b.body) == 1000
 
     assert r1.headers["content-type"] == "text/html"
-    assert r2.headers["content-type"] == "image/png"
+    assert r2b.headers["content-type"] == "image/png"
 
     assert server.get("h.xx").headers["content-type"] == "text/html"
     assert server.get("t.xx").headers["content-type"] == "text/plain"
     assert server.get("b.xx").headers["content-type"] == "application/octet-stream"
 
     assert r1.headers["etag"]
-    assert r2.headers["etag"]
-    assert r1.headers["etag"] != r2.headers["etag"]
+    assert "max-age=0" in [x.strip() for x in r1.headers["cache-control"].split(",")]
+    assert r2a.headers["etag"]
+    assert r2b.headers["etag"]
+    assert r2a.headers["etag"] != r2b.headers["etag"]
 
     assert r1.headers.get("content-encoding", "identity") == "identity"
-    assert r2.headers.get("content-encoding", "identity") == "identity"
+    assert r2a.headers.get("content-encoding", "identity") == "identity"
+    assert r2b.headers.get("content-encoding", "identity") == "identity"
 
     # Now do request with gzip on
     r3 = server.get("foo.html", headers={"accept-encoding": "gzip"})
-    r4 = server.get("foo.png", headers={"accept-encoding": "gzip"})
+    r4a = server.get("foo.bmp", headers={"accept-encoding": "gzip"})
+    r4b = server.get("foo.png", headers={"accept-encoding": "gzip"})
 
-    assert r3.status == 200 and r4.status == 200
-    assert len(r3.body) == 3 and len(r4.body) < 100
+    assert r3.status == 200 and r4a.status == 200 and r4b.status == 200
+    assert len(r3.body) == 3 and len(r4a.body) < 50 and len(r4b.body) == 1000
 
     assert r3.headers.get("content-encoding", "identity") == "identity"  # small
-    assert r4.headers.get("content-encoding", "identity") == "gzip"  # big enough
+    assert r4a.headers.get("content-encoding", "identity") == "gzip"  # big enough
+    assert r4b.headers.get("content-encoding", "identity") == "identity"  # entropy
 
     # Now do a request with etag
     r5 = server.get(
@@ -65,7 +86,7 @@ def test_make_asset_handler():
     )
     r6 = server.get(
         "foo.png",
-        headers={"accept-encoding": "gzip", "if-none-match": r2.headers["etag"]},
+        headers={"accept-encoding": "gzip", "if-none-match": r2b.headers["etag"]},
     )
 
     assert r5.status == 304 and r6.status == 304
@@ -77,14 +98,14 @@ def test_make_asset_handler():
     # Dito, but with wrong etag
     r7 = server.get(
         "foo.html",
-        headers={"accept-encoding": "gzip", "if-none-match": r2.headers["etag"]},
+        headers={"accept-encoding": "gzip", "if-none-match": r2b.headers["etag"]},
     )
     r8 = server.get(
         "foo.png", headers={"accept-encoding": "gzip", "if-none-match": "xxxx"}
     )
 
     assert r7.status == 200 and r8.status == 200
-    assert len(r1.body) == 3 and len(r2.body) == 10000
+    assert len(r7.body) == 3 and len(r8.body) == 1000
 
     # Big html files will be zipped, but must retain content type
     for fname in ("big.html", "bightml"):
@@ -101,5 +122,28 @@ def test_make_asset_handler():
         assert len(r.body) < len(plainbody)
 
 
+def test_make_asset_handler_max_age():
+    if get_backend() != "mock":
+        skip("Can only test this with mock server")
+
+    # Make a server
+    assets = {
+        "foo.html": "bla",
+        "foo.bmp": compressable_data,
+        "foo.png": uncompressable_data,
+    }
+    handler = asgineer.utils.make_asset_handler(assets, max_age=9999)
+    server = make_server(asgineer.to_asgi(handler))
+
+    # Do simple requests and check validity
+    r1 = server.get("foo.html")
+    assert r1.status == 200
+
+    assert r1.headers["etag"]
+    assert "max-age=9999" in [x.strip() for x in r1.headers["cache-control"].split(",")]
+
+
 if __name__ == "__main__":
+    test_make_asset_handler_fails()
     test_make_asset_handler()
+    test_make_asset_handler_max_age()