Merge pull request #2700 from SwiftPackageIndex/404-sitemaps

SwiftPackageIndex · Nov 8, 2023 · d0a5b2f · d0a5b2f
2 parents 56f7527 + 6adff4e
commit d0a5b2f
Show file tree

Hide file tree

Showing 7 changed files with 176 additions and 106 deletions.
diff --git a/Sources/App/Controllers/PackageController+routes.swift b/Sources/App/Controllers/PackageController+routes.swift
@@ -287,7 +287,7 @@ enum PackageController {
         return response
     }
 
-    static func siteMap(req: Request) async throws -> Response {
+    static func siteMap(req: Request) async throws -> SiteMap {
         guard
             let owner = req.parameters.get("owner"),
             let repository = req.parameters.get("repository")
@@ -316,33 +316,10 @@ enum PackageController {
             [String]()
         }
 
-        return try await siteMap(packageResult: packageResult, linkablePathUrls: urls)
-            .encodeResponse(for: req)
-    }
-
-    static func siteMap(packageResult: PackageResult, linkablePathUrls: [String]) async throws -> SiteMap {
-        guard let canonicalOwner = packageResult.repository.owner,
-              let canonicalRepository = packageResult.repository.name
-        else {
-            // This should never happen, but we should return an empty
-            // sitemap instead of an incorrect one.
-            return SiteMap()
-        }
-
-        // See SwiftPackageIndex-Server#2485 for context on the performance implications of this
-        let lastmod: Node<SiteMap.URLContext> = packageResult.repository.lastActivityAt
-            .map { .lastmod($0, timeZone: .utc) } ?? .empty
-        return SiteMap(
-            .url(
-                .loc(SiteURL.package(.value(canonicalOwner),
-                                     .value(canonicalRepository),
-                                     .none).absoluteURL()),
-                lastmod
-            ),
-            .forEach(linkablePathUrls, { url in
-                    .url(.loc(url), lastmod)
-            })
-        )
+        return try await SiteMapView.package(owner: packageResult.repository.owner,
+                                             repository: packageResult.repository.name,
+                                             lastActivityAt: packageResult.repository.lastActivityAt,
+                                             linkablePathUrls: urls)
     }
 
     static func linkablePathUrls(client: Client, packageResult: PackageResult) async -> [String] {

diff --git a/Sources/App/Controllers/SiteMapController.swift b/Sources/App/Controllers/SiteMapController.swift
@@ -31,16 +31,7 @@ enum SiteMapController {
         }
     }
 
-    static var staticRoutes: [SiteURL] = [
-        .home,
-        .addAPackage,
-        .faq,
-        .supporters,
-        .buildMonitor,
-        .privacy
-    ]
-
-    static func index(req: Request) async throws -> Response {
+    static func index(req: Request) async throws -> SiteMapIndex {
         guard let db = req.db as? SQLDatabase else {
             fatalError("Database must be an SQLDatabase ('as? SQLDatabase' must succeed)")
         }
@@ -55,31 +46,10 @@ enum SiteMapController {
             .orderBy(Search.repoName)
 
         let packages = try await query.all(decoding: Package.self)
-        return try await SiteMapIndex(
-            .sitemap(
-                .loc(SiteURL.siteMapStaticPages.absoluteURL()),
-                .lastmod(Current.date(), timeZone: .utc) // The home page updates every day.
-            ),
-            .group(
-                packages.map { package -> Node<SiteMapIndex.SiteMapIndexContext> in
-                        .sitemap(
-                            .loc(SiteURL.package(.value(package.owner),
-                                                 .value(package.repository),
-                                                 .siteMap).absoluteURL()),
-                            .unwrap(package.lastActivityAt, { .lastmod($0, timeZone: .utc) })
-                        )
-                }
-            )
-        ).encodeResponse(for: req)
+        return SiteMapView.index(packages: packages)
     }
 
-    static func staticPages(req: Request) async throws -> Response {
-        try await SiteMap(.group(
-            staticRoutes.map { page -> Node<SiteMap.URLSetContext> in
-                    .url(
-                        .loc(page.absoluteURL())
-                    )
-            }
-        )).encodeResponse(for: req)
+    static func staticPages(req: Request) async throws -> SiteMap {
+        return SiteMapView.staticPages()
     }
 }
diff --git a/Sources/App/Views/SiteMap/SiteMapView.swift b/Sources/App/Views/SiteMap/SiteMapView.swift
@@ -0,0 +1,87 @@
+// Copyright Dave Verwer, Sven A. Schmidt, and other contributors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+import Foundation
+import Plot
+
+enum SiteMapView {
+
+    static var staticRoutes: [SiteURL] = [
+        .home,
+        .addAPackage,
+        .faq,
+        .supporters,
+        .buildMonitor,
+        .privacy
+    ]
+
+    static func index(packages: [SiteMapController.Package]) -> SiteMapIndex {
+        SiteMapIndex(
+            .sitemap(
+                .loc(SiteURL.siteMapStaticPages.absoluteURL()),
+                .lastmod(Current.date(), timeZone: .utc) // The home page updates every day.
+            ),
+            .group(
+                packages.map { package -> Node<SiteMapIndex.SiteMapIndexContext> in
+                        .sitemap(
+                            .loc(SiteURL.package(.value(package.owner),
+                                                 .value(package.repository),
+                                                 .siteMap).absoluteURL()),
+                            .unwrap(package.lastActivityAt, { .lastmod($0, timeZone: .utc) })
+                        )
+                }
+            )
+        )
+    }
+
+    static func staticPages() -> SiteMap {
+        SiteMap(
+            .group(
+                staticRoutes.map { page -> Node<SiteMap.URLSetContext> in
+                        .url(
+                            .loc(page.absoluteURL())
+                        )
+                }
+            )
+        )
+    }
+
+    static func package(owner: String?,
+                        repository: String?,
+                        lastActivityAt: Date?,
+                        linkablePathUrls: [String]) async throws -> SiteMap {
+        guard let owner,
+              let repository
+        else {
+            // This should never happen, but we should return an empty
+            // sitemap instead of an incorrect one.
+            return SiteMap()
+        }
+
+        // See SwiftPackageIndex-Server#2485 for context on the performance implications of this
+        let lastmod: Node<SiteMap.URLContext> = lastActivityAt.map { .lastmod($0, timeZone: .utc) } ?? .empty
+
+        return SiteMap(
+            .url(
+                .loc(SiteURL.package(.value(owner),
+                                     .value(repository),
+                                     .none).absoluteURL()),
+                lastmod
+            ),
+            .forEach(linkablePathUrls, { url in
+                    .url(.loc(url), lastmod)
+            })
+        )
+    }
+}
diff --git a/Sources/App/routes.swift b/Sources/App/routes.swift
@@ -123,11 +123,14 @@ func routes(_ app: Application) throws {
         app.get(SiteURL.package(.key, .key, .maintainerInfo).pathComponents,
                 use: PackageController.maintainerInfo).excludeFromOpenAPI()
 
-        // Package specific site map, including all documentation URLs if available.
-        // Backend reporting currently disabled to avoid reporting costs for metrics we don't need.
-        app.group(BackendReportingMiddleware(path: .sitemapPackage, isActive: false)) {
-            $0.get(SiteURL.package(.key, .key, .siteMap).pathComponents,
-                    use: PackageController.siteMap).excludeFromOpenAPI()
+        // Only serve sitemaps in production.
+        if Current.environment() == .production {
+            // Package specific site map, including all documentation URLs if available.
+            // Backend reporting currently disabled to avoid reporting costs for metrics we don't need.
+            app.group(BackendReportingMiddleware(path: .sitemapPackage, isActive: false)) {
+                $0.get(SiteURL.package(.key, .key, .siteMap).pathComponents,
+                       use: PackageController.siteMap).excludeFromOpenAPI()
+            }
         }
     }
 
@@ -296,15 +299,18 @@ func routes(_ app: Application) throws {
         }
     }
 
-    do { // Site map index and site maps
-        app.group(BackendReportingMiddleware(path: .sitemapIndex)) {
-            $0.get(SiteURL.siteMapIndex.pathComponents, use: SiteMapController.index)
-                .excludeFromOpenAPI()
-        }
+    // Only serve sitemaps in production.
+    if Current.environment() == .production {
+        do { // Site map index and static page site map
+            app.group(BackendReportingMiddleware(path: .sitemapIndex)) {
+                $0.get(SiteURL.siteMapIndex.pathComponents, use: SiteMapController.index)
+                    .excludeFromOpenAPI()
+            }
 
-        app.group(BackendReportingMiddleware(path: .sitemapStaticPages)) {
-            $0.get(SiteURL.siteMapStaticPages.pathComponents, use: SiteMapController.staticPages)
-                .excludeFromOpenAPI()
+            app.group(BackendReportingMiddleware(path: .sitemapStaticPages)) {
+                $0.get(SiteURL.siteMapStaticPages.pathComponents, use: SiteMapController.staticPages)
+                    .excludeFromOpenAPI()
+            }
         }
     }
 

diff --git a/Tests/AppTests/SitemapTests.swift b/Tests/AppTests/SitemapTests.swift
@@ -33,39 +33,26 @@ class SitemapTests: SnapshotTestCase {
                                              reference: .branch("default")) }.save(on: app.db)
         try await Search.refresh(on: app.db).get()
 
+        let req = Request(application: app, on: app.eventLoopGroup.next())
+
         // MUT
-        try app.test(.GET, "/sitemap.xml") { res in
-            // Validation
-            XCTAssertEqual(res.status, .ok)
-            assertSnapshot(matching: res.body.asString(), as: .init(pathExtension: "xml", diffing: .lines))
-        }
+        let siteMapIndex = try await SiteMapController.index(req: req)
+
+        // Validation
+        assertSnapshot(matching: siteMapIndex.render(indentedBy: .spaces(2)),
+                       as: .init(pathExtension: "xml", diffing: .lines))
     }
 
     @MainActor
     func test_siteMapStaticPages() async throws {
-        // MUT
-        try app.test(.GET, "/sitemap-static-pages.xml") { res in
-            // Validation
-            XCTAssertEqual(res.status, .ok)
-            assertSnapshot(matching: res.body.asString(), as: .init(pathExtension: "xml", diffing: .lines))
-        }
-    }
-
-    func test_siteMap_basic_request() async throws {
-        // Test basic sitemap request
-        // setup
-        let package = Package(url: URL(stringLiteral: "https://example.com/owner/repo0"))
-        try await package.save(on: app.db)
-        try await Repository(package: package, defaultBranch: "default",
-                             lastCommitDate: Current.date(),
-                             name: "Repo0", owner: "Owner").save(on: app.db)
-        try await Version(package: package, latest: .defaultBranch, packageName: "SomePackage",
-                          reference: .branch("default")).save(on: app.db)
+        let req = Request(application: app, on: app.eventLoopGroup.next())
 
         // MUT
-        try app.test(.GET, "/owner/repo0/sitemap.xml") { res in
-            XCTAssertEqual(res.status, .ok)
-        }
+        let siteMap = try await SiteMapController.staticPages(req: req)
+
+        // Validation
+        assertSnapshot(matching: siteMap.render(indentedBy: .spaces(2)),
+                       as: .init(pathExtension: "xml", diffing: .lines))
     }
 
     func test_linkablePathUrls() async throws {
@@ -162,7 +149,10 @@ class SitemapTests: SnapshotTestCase {
             .query(on: app.db, owner: "owner", repository: "repo0")
 
         // MUT
-        let sitemap = try await PackageController.siteMap(packageResult: packageResult, linkablePathUrls: [])
+        let sitemap = try await SiteMapView.package(owner: packageResult.repository.owner,
+                                                    repository: packageResult.repository.name,
+                                                    lastActivityAt: packageResult.repository.lastActivityAt,
+                                                    linkablePathUrls: [])
         let xml = sitemap.render(indentedBy: .spaces(2))
 
         // Validation
@@ -188,8 +178,10 @@ class SitemapTests: SnapshotTestCase {
         ]
 
         // MUT
-        let sitemap = try await PackageController.siteMap(packageResult: packageResult,
-                                                          linkablePathUrls: linkablePathUrls)
+        let sitemap = try await SiteMapView.package(owner: packageResult.repository.owner,
+                                                    repository: packageResult.repository.name,
+                                                    lastActivityAt: packageResult.repository.lastActivityAt,
+                                                    linkablePathUrls: linkablePathUrls)
         let xml = sitemap.render(indentedBy: .spaces(2))
 
         // Validation

diff --git a/Tests/AppTests/__Snapshots__/SitemapTests/test_siteMapIndex.1.xml b/Tests/AppTests/__Snapshots__/SitemapTests/test_siteMapIndex.1.xml
@@ -1 +1,19 @@
-<?xml version="1.0" encoding="UTF-8"?><sitemapindex xmlns="http://www.sitemaps.org/schemas/sitemap/0.9"><sitemap><loc>http://localhost:8080/sitemap-static-pages.xml</loc><lastmod>1970-01-01</lastmod></sitemap><sitemap><loc>http://localhost:8080/foo/0/sitemap.xml</loc><lastmod>1970-01-01</lastmod></sitemap><sitemap><loc>http://localhost:8080/foo/1/sitemap.xml</loc><lastmod>1970-01-01</lastmod></sitemap><sitemap><loc>http://localhost:8080/foo/2/sitemap.xml</loc><lastmod>1970-01-01</lastmod></sitemap></sitemapindex>
+<?xml version="1.0" encoding="UTF-8"?>
+<sitemapindex xmlns="http://www.sitemaps.org/schemas/sitemap/0.9">
+  <sitemap>
+    <loc>http://localhost:8080/sitemap-static-pages.xml</loc>
+    <lastmod>1970-01-01</lastmod>
+  </sitemap>
+  <sitemap>
+    <loc>http://localhost:8080/foo/0/sitemap.xml</loc>
+    <lastmod>1970-01-01</lastmod>
+  </sitemap>
+  <sitemap>
+    <loc>http://localhost:8080/foo/1/sitemap.xml</loc>
+    <lastmod>1970-01-01</lastmod>
+  </sitemap>
+  <sitemap>
+    <loc>http://localhost:8080/foo/2/sitemap.xml</loc>
+    <lastmod>1970-01-01</lastmod>
+  </sitemap>
+</sitemapindex>
diff --git a/Tests/AppTests/__Snapshots__/SitemapTests/test_siteMapStaticPages.1.xml b/Tests/AppTests/__Snapshots__/SitemapTests/test_siteMapStaticPages.1.xml
@@ -1 +1,21 @@
-<?xml version="1.0" encoding="UTF-8"?><urlset xmlns="http://www.sitemaps.org/schemas/sitemap/0.9" xmlns:image="http://www.google.com/schemas/sitemap-image/1.1"><url><loc>http://localhost:8080/</loc></url><url><loc>http://localhost:8080/add-a-package</loc></url><url><loc>http://localhost:8080/faq</loc></url><url><loc>http://localhost:8080/supporters</loc></url><url><loc>http://localhost:8080/build-monitor</loc></url><url><loc>http://localhost:8080/privacy</loc></url></urlset>
+<?xml version="1.0" encoding="UTF-8"?>
+<urlset xmlns="http://www.sitemaps.org/schemas/sitemap/0.9" xmlns:image="http://www.google.com/schemas/sitemap-image/1.1">
+  <url>
+    <loc>http://localhost:8080/</loc>
+  </url>
+  <url>
+    <loc>http://localhost:8080/add-a-package</loc>
+  </url>
+  <url>
+    <loc>http://localhost:8080/faq</loc>
+  </url>
+  <url>
+    <loc>http://localhost:8080/supporters</loc>
+  </url>
+  <url>
+    <loc>http://localhost:8080/build-monitor</loc>
+  </url>
+  <url>
+    <loc>http://localhost:8080/privacy</loc>
+  </url>
+</urlset>