Merge issue-162-podcast-canonicals: Restore .html podcast canonical routes (#162)

Author: alexeygrigorev

content/public_data.py

@@ -124,15 +124,18 @@ class PodcastSeason:
 
 
 def podcast_public_path(record: dict[str, Any]) -> str:
-    """Return the canonical season/episode podcast URL without a file extension."""
+    """Return the checked canonical podcast detail path."""
 
-    season = _podcast_number(record, "season")
-    episode = _podcast_number(record, "episode")
-    title = record.get("title")
-    if not isinstance(title, str) or not title.strip():
-        raise ImproperlyConfigured("Public podcast title is required for its URL.")
-    key = f"s{season:02d}e{episode:02d}"
-    return f"/podcast/{key}/{event_title_slug(title)}"
+    slug = record.get("slug")
+    public_path = record.get("public_path")
+    if (
+        not isinstance(slug, str)
+        or not slug
+        or not isinstance(public_path, str)
+        or public_path != f"/podcast/{slug}.html"
+    ):
+        raise ImproperlyConfigured("Public podcast canonical path is invalid.")
+    return public_path
 
 
 def _podcast_number(record: dict[str, Any], field: str) -> int:
@@ -320,61 +323,6 @@ def _apply_runtime_event_public_paths(
         )
 
 
-def _replace_exact_public_paths(value: Any, replacements: dict[str, str]) -> Any:
-    if isinstance(value, str):
-        return replacements.get(value, value)
-    if isinstance(value, list):
-        return [_replace_exact_public_paths(item, replacements) for item in value]
-    if isinstance(value, dict):
-        return {key: _replace_exact_public_paths(item, replacements) for key, item in value.items()}
-    return value
-
-
-def _apply_runtime_podcast_public_paths(projection: dict[str, Any]) -> None:
-    """Expose season/episode podcast paths while retaining `.html` source routes as aliases."""
-
-    replacements: dict[str, str] = {}
-    podcasts: list[dict[str, Any]] = []
-    for raw_podcast in projection["podcasts"]:
-        podcast = dict(raw_podcast)
-        canonical = podcast_public_path(podcast)
-        replacements[podcast["public_path"]] = canonical
-        podcast["public_path"] = canonical
-        podcasts.append(podcast)
-    if len({podcast["public_path"] for podcast in podcasts}) != len(podcasts):
-        raise ImproperlyConfigured("Public podcast canonical paths are not unique.")
-    projection["podcasts"] = tuple(podcasts)
-    projection["podcasts_by_path"] = {podcast["public_path"]: podcast for podcast in podcasts}
-    projection["podcasts_by_slug"] = {
-        slug: projection["podcasts_by_path"].get(podcast_public_path(podcast), podcast)
-        for slug, podcast in projection["podcasts_by_slug"].items()
-    }
-
-    for person in projection["people"]:
-        relationships = person.get("relationships", ())
-        person["relationships"] = tuple(
-            {
-                **relationship,
-                "public_path": replacements.get(
-                    relationship.get("public_path"), relationship.get("public_path", "")
-                ),
-            }
-            for relationship in relationships
-        )
-
-    projection["wiki_graph"] = _replace_exact_public_paths(projection["wiki_graph"], replacements)
-    projection["wiki_search"] = _replace_exact_public_paths(projection["wiki_search"], replacements)
-
-    route_manifest = projection["editorial_route_migration"]
-    projection["editorial_route_aliases_by_path"] = {
-        item["source_path"]: {
-            **item,
-            "final_path": replacements.get(item["final_path"], item["final_path"]),
-        }
-        for item in route_manifest["aliases"]
-    }
-
-
 def _expected_editorial_routes(
     projection: dict[str, Any],
 ) -> tuple[list[dict[str, Any]], list[dict[str, Any]]]:
@@ -700,7 +648,6 @@ def _adapted_public_projection(
         }
         for person in source["people"]
     )
-    _apply_runtime_podcast_public_paths(projection)
     _apply_runtime_event_public_paths(projection, runtime_identities)
     # The adapters mutate copied people records; refresh their lookup indexes as well so detail
     # pages and relationship tests do not retain references to the checked source records.

content/public_urls.py

@@ -36,23 +36,18 @@
     path("podcast", public_views.podcast_hub, name="podcast"),
     path("podcast.html", public_views.permanent_public_redirect, {"target": "/podcast"}),
     path("podcast/", public_views.permanent_public_redirect, {"target": "/podcast"}),
-    path(
-        "podcast/<slug:episode_key>/<slug:title_slug>",
-        public_views.podcast_detail_canonical,
-        name="public-podcast-canonical",
-    ),
     path(
         "podcast/<path:slug>/",
         public_views.permanent_detail_redirect,
         {"collection": "podcast"},
     ),
     path(
         "podcast/s24e06-how-to-build-ai-that-actually-ships-in-production.html",
-        public_views.podcast_detail_legacy,
+        public_views.podcast_detail,
         {"slug": "s24e06-how-to-build-ai-that-actually-ships-in-production"},
         name="podcast-ai-production",
     ),
-    path("podcast/<path:slug>.html", public_views.podcast_detail_legacy, name="public-podcast"),
+    path("podcast/<path:slug>.html", public_views.podcast_detail, name="public-podcast"),
     path(
         "podcast/<path:slug>",
         public_views.permanent_detail_redirect,

content/public_views.py

@@ -622,24 +622,6 @@ def podcast_detail(request: HttpRequest, slug: str) -> HttpResponse:
     return _render_podcast_detail(request, episode)
 
 
-@require_safe
-def podcast_detail_canonical(
-    request: HttpRequest, episode_key: str, title_slug: str
-) -> HttpResponse:
-    episode = public_projection()["podcasts_by_path"].get(f"/podcast/{episode_key}/{title_slug}")
-    if episode is None:
-        raise Http404
-    return _render_podcast_detail(request, episode)
-
-
-@require_safe
-def podcast_detail_legacy(request: HttpRequest, slug: str) -> HttpResponse:
-    episode = public_projection()["podcasts_by_slug"].get(slug)
-    if episode is None:
-        raise Http404
-    return permanent_public_redirect(request, target=episode["public_path"])
-
-
 @require_safe
 def book_detail(request: HttpRequest, slug: str) -> HttpResponse:
     book = public_projection()["books_by_slug"].get(slug)

content/tests/test_podcast_catalog.py

@@ -185,6 +185,53 @@ def test_each_actual_season_contains_one_complete_season_and_all_details_once(se
             types = {item.get("@type") for item in json.loads(payload_match.group(1))["@graph"]}
             self.assertIn("PodcastEpisode", types)
 
+    def test_detail_routes_keep_html_finals_and_reject_competing_season_paths(self) -> None:
+        projection = public_projection()
+        podcasts = projection["podcasts"]
+        migration = projection["editorial_route_migration"]
+        podcast_finals = {
+            item["final_path"] for item in migration["finals"] if item["collection"] == "podcasts"
+        }
+        podcast_aliases = [
+            item for item in migration["aliases"] if item["collection"] == "podcasts"
+        ]
+
+        self.assertEqual(len(podcasts), 205)
+        self.assertEqual({episode["public_path"] for episode in podcasts}, podcast_finals)
+        self.assertEqual(len(podcast_finals), 205)
+        self.assertTrue(
+            all(path.startswith("/podcast/") and path.endswith(".html") for path in podcast_finals)
+        )
+        self.assertEqual(len(podcast_aliases), 410)
+        self.assertEqual({item["final_path"] for item in podcast_aliases}, podcast_finals)
+
+        episode = podcasts[0]
+        final_path = episode["public_path"]
+        query = "utm_source=oncall%2Btest&x=a%2Fb&blank="
+        for method in ("GET", "HEAD"):
+            response = self.client.generic(method, f"{final_path}?{query}", follow=False)
+            self.assertEqual(response.status_code, 200)
+            self.assertNotIn("Location", response.headers)
+        self.assertEqual(self.client.post(final_path).status_code, 405)
+
+        aliases = (final_path.removesuffix(".html"), f"{final_path.removesuffix('.html')}/")
+        for alias_path in aliases:
+            for method in ("GET", "HEAD"):
+                response = self.client.generic(method, f"{alias_path}?{query}", follow=False)
+                self.assertEqual(response.status_code, 301)
+                self.assertEqual(response.headers["Location"], f"{final_path}?{query}")
+            self.assertEqual(self.client.post(alias_path).status_code, 405)
+
+        competing_path = (
+            f"/podcast/s{episode['season']:02d}e{episode['episode']:02d}/competing-title"
+        )
+        for method in ("GET", "HEAD"):
+            response = self.client.generic(method, competing_path, follow=False)
+            self.assertEqual(response.status_code, 404)
+            self.assertNotIn("Location", response.headers)
+            self.assertNotContains(response, 'rel="canonical"', status_code=404)
+        self.assertEqual(self.client.post(competing_path).status_code, 405)
+
     def test_latest_middle_and_oldest_emit_exact_seo_and_navigation(self) -> None:
         scenarios = (
             ("/podcast", 24, "/podcast", "DataTalks.Club Podcast — DataTalks.Club", None, 23),

content/tests/test_public_routes_and_seo.py

@@ -89,17 +89,6 @@ def test_editorial_detail_aliases_redirect_directly_to_html_canonicals(self) ->
         self.assertEqual(len(migration["aliases"]), 1_592)
         canonical_paths = {item["final_path"] for item in migration["finals"]}
         alias_map = {item["source_path"]: item["final_path"] for item in migration["aliases"]}
-        runtime_canonical_paths = set()
-        for item in migration["finals"]:
-            runtime_path = item["final_path"]
-            if item["collection"] == "podcasts":
-                podcast = next(
-                    record
-                    for record in projection["podcasts"]
-                    if record["provenance"]["source_key"] == item["record_key"]
-                )
-                runtime_path = podcast["public_path"]
-            runtime_canonical_paths.add(runtime_path)
         self.assertEqual(len(alias_map), 1_592)
         self.assertEqual(set(alias_map.values()), canonical_paths)
         self.assertTrue(set(alias_map).isdisjoint(canonical_paths))
@@ -118,31 +107,24 @@ def test_editorial_detail_aliases_redirect_directly_to_html_canonicals(self) ->
             with self.subTest(source=source):
                 response = self.client.get(f"{source}?{query}", follow=False)
                 self.assertEqual(response.status_code, 301)
-                migration_entry = next(
-                    item for item in migration["aliases"] if item["source_path"] == source
-                )
-                expected_target = target
-                if migration_entry["collection"] == "podcasts":
-                    podcast = next(
-                        record
-                        for record in projection["podcasts"]
-                        if record["provenance"]["source_key"] == migration_entry["record_key"]
-                    )
-                    expected_target = podcast["public_path"]
-                self.assertEqual(response.headers["Location"], f"{expected_target}?{query}")
+                self.assertEqual(response.headers["Location"], f"{target}?{query}")
                 self.assertEqual(response.headers["X-Robots-Tag"], "noindex, nofollow")
                 head = self.client.head(f"{source}?{query}", follow=False)
                 self.assertEqual(head.status_code, 301)
-                self.assertEqual(head.headers["Location"], f"{expected_target}?{query}")
+                self.assertEqual(head.headers["Location"], f"{target}?{query}")
                 self.assertEqual(self.client.post(source).status_code, 405)
 
-        for target in runtime_canonical_paths:
+        for target in canonical_paths:
             with self.subTest(target=target):
                 final = self.client.get(target, follow=False)
                 self.assertEqual(final.status_code, 200)
                 self.assertNotIn("Location", final.headers)
                 self.assertEqual(final.headers["X-Robots-Tag"], "noindex, nofollow")
                 self.assertEqual(self.client.post(target).status_code, 405)
+                head = self.client.head(target, follow=False)
+                self.assertEqual(head.status_code, 200)
+                self.assertEqual(head.content, b"")
+                self.assertNotIn("Location", head.headers)
                 canonical_url = f"https://datatalks.club{target}"
                 self.assertContains(
                     final,

playwright_tests/test_podcast_seasons.py

@@ -331,6 +331,39 @@ def test_alias_query_and_safe_denial_browser_matrix(page: Page, live_server) ->
         assert invalid_redirect.status == 301
         assert invalid_redirect.headers["location"] == "/podcast?page=2"
 
+    episode = ordered_podcasts()[0]
+    final_path = episode["public_path"]
+    detail_query = "utm_source=oncall%2Btest&x=a%2Fb&blank="
+    for alias in (final_path.removesuffix(".html"), f"{final_path.removesuffix('.html')}/"):
+        redirected = page.request.get(
+            f"{origin}{alias}?{detail_query}",
+            max_redirects=0,
+        )
+        assert redirected.status == 301
+        assert redirected.headers["location"] == f"{final_path}?{detail_query}"
+        head = page.request.head(f"{origin}{alias}?{detail_query}", max_redirects=0)
+        assert head.status == 301
+        assert head.headers["location"] == f"{final_path}?{detail_query}"
+
+    final = page.goto(f"{origin}{final_path}?{detail_query}", wait_until="networkidle")
+    assert final is not None and final.status == 200
+    expect(page).to_have_url(f"{origin}{final_path}?{detail_query}")
+    expect(page.get_by_role("heading", name=episode["title"], exact=True)).to_be_visible()
+    expect(page.locator('link[rel="canonical"]')).to_have_attribute(
+        "href",
+        f"https://datatalks.club{final_path}",
+    )
+    expect(page.locator('meta[property="og:url"]')).to_have_attribute(
+        "content",
+        f"https://datatalks.club{final_path}",
+    )
+
+    competing_path = f"/podcast/s{episode['season']:02d}e{episode['episode']:02d}/competing-title"
+    competing = page.request.get(f"{origin}{competing_path}", max_redirects=0)
+    assert competing.status == 404
+    assert "location" not in competing.headers
+    assert "canonical" not in competing.text().casefold()
+
     denials = (
         ("GET", "/podcast?page=2", 400),
         ("GET", "/podcast?season=01", 400),