Implementing redirect on dartdoc-generated redirect file.

isoos · isoos · commit 2cc31a1fc7c5 · 2024-11-29T18:24:25.000+01:00
diff --git a/app/lib/dartdoc/models.dart b/app/lib/dartdoc/models.dart
@@ -42,3 +42,52 @@ class ResolvedDocUrlVersion {
   bool get isEmpty => version.isEmpty || urlSegment.isEmpty;
   bool get isLatestStable => urlSegment == 'latest';
 }
+
+/// Describes the status of a dartdoc page.
+@JsonSerializable(includeIfNull: false)
+class DocPageStatus {
+  final DocPageStatusCode code;
+  final String? redirectPath;
+  final String? errorMessage;
+
+  DocPageStatus({
+    required this.code,
+    this.redirectPath,
+    this.errorMessage,
+  });
+
+  factory DocPageStatus.ok() {
+    return DocPageStatus(code: DocPageStatusCode.ok);
+  }
+
+  factory DocPageStatus.redirect(String redirectPath) {
+    return DocPageStatus(
+      code: DocPageStatusCode.redirect,
+      redirectPath: redirectPath,
+    );
+  }
+
+  factory DocPageStatus.missing(String errorMessage) {
+    return DocPageStatus(
+      code: DocPageStatusCode.missing,
+      errorMessage: errorMessage,
+    );
+  }
+
+  factory DocPageStatus.fromJson(Map<String, dynamic> json) =>
+      _$DocPageStatusFromJson(json);
+
+  Map<String, dynamic> toJson() => _$DocPageStatusToJson(this);
+}
+
+/// Explicit status of [DocPageStatus].
+enum DocPageStatusCode {
+  /// page generated and ready to be served
+  ok,
+
+  /// page generated and redirects to a new URL
+  redirect,
+
+  /// page does not exists
+  missing,
+}
diff --git a/app/lib/dartdoc/models.g.dart b/app/lib/dartdoc/models.g.dart
diff --git a/app/lib/shared/redis_cache.dart b/app/lib/shared/redis_cache.dart
@@ -378,6 +378,22 @@ class CachePatterns {
           .withPrefix('dartdoc-html/')
           .withTTL(Duration(minutes: 10))['$package/$urlSegment/$path'];
 
+  /// Cache for sanitized and re-rendered dartdoc HTML files.
+  Entry<DocPageStatus> dartdocPageStatus(
+    String package,
+    String urlSegment,
+    String path,
+  ) =>
+      _cache
+          .withPrefix('dartdoc-status/')
+          .withTTL(Duration(minutes: 10))
+          .withCodec(utf8)
+          .withCodec(json)
+          .withCodec(wrapAsCodec(
+              encode: (DocPageStatus s) => s.toJson(),
+              decode: (data) => DocPageStatus.fromJson(
+                  data as Map<String, dynamic>)))['$package/$urlSegment/$path'];
+
   /// Stores the OpenID Data (including the JSON Web Key list).
   ///
   /// GitHub does not provide `Cache-Control` header for their
diff --git a/app/lib/task/backend.dart b/app/lib/task/backend.dart
@@ -869,6 +869,18 @@ class TaskBackend {
     return index;
   }
 
+  /// Whether the task result for the given [package]/[version] has any result
+  /// dartdoc-generated file in [path].
+  Future<bool> hasDartdocTaskResult(
+      String package, String version, String path) async {
+    version = canonicalizeVersion(version)!;
+    final index = await _taskResultIndex(package, version);
+    if (index == null) {
+      return false;
+    }
+    return index.lookup('doc/$path') != null;
+  }
+
   /// Return gzipped result from task for the given [package]/[version] or
   /// `null`.
   Future<List<int>?> gzippedTaskResult(
diff --git a/app/lib/task/handlers.dart b/app/lib/task/handlers.dart
@@ -1,6 +1,7 @@
 import 'dart:convert';
 import 'dart:io' show gzip;
 
+import 'package:logging/logging.dart';
 import 'package:path/path.dart' as p;
 
 import 'package:pub_dev/dartdoc/dartdoc_page.dart';
@@ -42,6 +43,8 @@ const _safeMimeTypes = {
   // should not add it here.
 };
 
+final _logger = Logger('task.handlers');
+
 Future<shelf.Response> handleDartDoc(
   shelf.Request request,
   String package,
@@ -80,26 +83,75 @@ Future<shelf.Response> handleDartDoc(
   // Handle HTML requests
   final isHtml = ext == 'html';
   if (isHtml) {
-    final htmlBytes = await cache
-        .dartdocHtmlBytes(package, resolvedDocUrlVersion.urlSegment, path)
-        .get(() async {
+    // try cached bytes first
+    final htmlBytesCacheEntry =
+        cache.dartdocHtmlBytes(package, resolvedDocUrlVersion.urlSegment, path);
+    final htmlBytes = await htmlBytesCacheEntry.get();
+    if (htmlBytes != null) {
+      return htmlBytesResponse(htmlBytes);
+    }
+
+    // check cached status for redirect or missing pages
+    final statusCacheEntry = cache.dartdocPageStatus(
+        package, resolvedDocUrlVersion.urlSegment, path);
+    final cachedStatus = await statusCacheEntry.get();
+    final cachedStatusCode = cachedStatus?.code;
+
+    shelf.Response redirectPathResponse(String redirectPath) {
+      final newPath = p.normalize(p.joinAll([
+        p.dirname(path),
+        redirectPath,
+        if (!redirectPath.endsWith('.html')) 'index.html',
+      ]));
+      return redirectResponse(pkgDocUrl(
+        package,
+        version: resolvedDocUrlVersion.urlSegment,
+        relativePath: newPath,
+      ));
+    }
+
+    if (cachedStatusCode == DocPageStatusCode.redirect) {
+      final redirectPath = cachedStatus!.redirectPath;
+      if (redirectPath == null) {
+        _logger.shout('DocPageStatus redirect without path.');
+        return notFoundHandler(request);
+      }
+      return redirectPathResponse(redirectPath);
+    }
+
+    if (cachedStatusCode == DocPageStatusCode.missing) {
+      return notFoundHandler(request,
+          body: cachedStatus!.errorMessage ?? 'Documentation page not found.');
+    }
+
+    // try loading bytes;
+    final (status, bytes) = await () async {
+      final dataGz = await taskBackend.dartdocFile(package, version, path);
+      if (dataGz == null) {
+        return (
+          DocPageStatus(
+            code: DocPageStatusCode.missing,
+            errorMessage: await _missingPageErrorMessage(
+              package,
+              version,
+              isLatestStable: resolvedDocUrlVersion.isLatestStable,
+            ),
+          ),
+          null, // bytes
+        );
+      }
+
       try {
-        final dataGz = await taskBackend.dartdocFile(package, version, path);
-        if (dataGz == null) {
-          return const <int>[]; // store empty string for missing data
-        }
         final dataJson = gzippedUtf8JsonCodec.decode(dataGz);
         if (path.endsWith('-sidebar.html')) {
           final sidebar =
               DartDocSidebar.fromJson(dataJson as Map<String, dynamic>);
-          return utf8.encode(sidebar.content);
+          return (DocPageStatus.ok(), utf8.encode(sidebar.content));
         }
-        var page = DartDocPage.fromJson(dataJson as Map<String, dynamic>);
 
-        // NOTE: If the loaded page is redirecting, we try to load it and render
-        //       the page it is pointing to. Instead, we should redirect the page
-        //       to the new URL.
-        // TODO: restructure cache logic and implement proper redirect
+        final page = DartDocPage.fromJson(dataJson as Map<String, dynamic>);
+
+        // check for redirect page
         final redirectPath = page.redirectPath;
         if (page.isEmpty() &&
             redirectPath != null &&
@@ -109,11 +161,14 @@ Future<shelf.Response> handleDartDoc(
             redirectPath,
             if (!redirectPath.endsWith('.html')) 'index.html',
           ]));
-          final newDataGz =
-              await taskBackend.dartdocFile(package, version, newPath);
-          if (newDataGz != null) {
-            final newDataJson = gzippedUtf8JsonCodec.decode(newDataGz);
-            page = DartDocPage.fromJson(newDataJson as Map<String, dynamic>);
+          if (await taskBackend.hasDartdocTaskResult(
+              package, version, newPath)) {
+            return (DocPageStatus.redirect(redirectPath), null);
+          } else {
+            return (
+              DocPageStatus.missing('Dartdoc redirect is missing.'),
+              null, // bytes
+            );
           }
         }
 
@@ -125,42 +180,25 @@ Future<shelf.Response> handleDartDoc(
           path: path,
           searchQueryParameter: searchQueryParameter,
         ));
-        return utf8.encode(html.toString());
-      } on FormatException {
-        // store empty string for invalid data, we treat it as a bug in
-        // the documentation generation.
-        return const <int>[];
-      }
-    });
-    // We use empty string to indicate missing file or bug in the file
-    if (htmlBytes == null || htmlBytes.isEmpty) {
-      final status = await taskBackend.packageStatus(package);
-      final vs = status.versions[version];
-      if (vs == null) {
-        return notFoundHandler(
-          request,
-          body: resolvedDocUrlVersion.isLatestStable
-              ? 'Analysis has not started yet.'
-              : 'Version not selected for analysis.',
-        );
+        return (DocPageStatus.ok(), utf8.encode(html.toString()));
+      } on FormatException catch (e, st) {
+        _logger.shout(
+            'Unable to parse dartdoc page for $package $version', e, st);
+        return (DocPageStatus.missing('Unable to render page.'), null);
       }
-      String? message;
-      switch (vs.status) {
-        case PackageVersionStatus.pending:
-        case PackageVersionStatus.running:
-          message = 'Analysis has not finished yet.';
-          break;
-        case PackageVersionStatus.failed:
-          message =
-              'Analysis has failed, no `dartdoc` output has been generated.';
-          break;
-        case PackageVersionStatus.completed:
-          message = '`dartdoc` did not generate this page.';
-          break;
-      }
-      return notFoundHandler(request, body: message);
+    }();
+    await statusCacheEntry.set(status);
+
+    switch (status.code) {
+      case DocPageStatusCode.ok:
+        await htmlBytesCacheEntry.set(bytes!);
+        return htmlBytesResponse(bytes);
+      case DocPageStatusCode.redirect:
+        return redirectPathResponse(status.redirectPath!);
+      case DocPageStatusCode.missing:
+        return notFoundHandler(request,
+            body: status.errorMessage ?? 'Documentation page not found.');
     }
-    return htmlBytesResponse(htmlBytes);
   }
 
   // Handle any non-HTML request
@@ -191,6 +229,34 @@ Future<shelf.Response> handleDartDoc(
   );
 }
 
+Future<String> _missingPageErrorMessage(
+  String package,
+  String version, {
+  required bool isLatestStable,
+}) async {
+  final status = await taskBackend.packageStatus(package);
+  final vs = status.versions[version];
+  if (vs == null) {
+    return isLatestStable
+        ? 'Analysis has not started yet.'
+        : 'Version not selected for analysis.';
+  }
+  String? message;
+  switch (vs.status) {
+    case PackageVersionStatus.pending:
+    case PackageVersionStatus.running:
+      message = 'Analysis has not finished yet.';
+      break;
+    case PackageVersionStatus.failed:
+      message = 'Analysis has failed, no `dartdoc` output has been generated.';
+      break;
+    case PackageVersionStatus.completed:
+      message = '`dartdoc` did not generate this page.';
+      break;
+  }
+  return message;
+}
+
 /// Handles GET `/packages/<package>/versions/<version>/gen-res/<path|[^]*>`
 Future<shelf.Response> handleTaskResource(
   shelf.Request request,
diff --git a/pkg/pub_integration/test/dartdoc_search_test.dart b/pkg/pub_integration/test/dartdoc_search_test.dart
@@ -60,6 +60,18 @@ void main() {
             startsWith('$origin/documentation/oxygen/latest/oxygen/'));
         expect(page.url, endsWith('.html'));
       });
+
+      // test library page (future redirect)
+      await user.withBrowserPage((page) async {
+        await page.gotoOrigin(
+            '/documentation/oxygen/latest/oxygen/oxygen-library.html');
+        await Future.delayed(Duration(milliseconds: 200));
+        // NOTE: This test does nothing substantial right now, but once we migrate
+        //       to dartdoc 8.3, this will test the redirect handler.
+        expect(page.url,
+            '$origin/documentation/oxygen/latest/oxygen/oxygen-library.html');
+        expect(await page.content, contains('TypeEnum'));
+      });
     });
   }, timeout: Timeout.factor(testTimeoutFactor));
 }
