Repository: VeryGoodOpenSource/dart_frog
Pull request #605 (2023-04-13 00:04:27)

fix(dart_frog): `Pipeline` does not maintain `RequestContext`
<!--
  Thanks for contributing!

  Provide a description of your changes below and a general summary in the title

  Please look at the following checklist to ensure that your PR can be accepted quickly:
-->

## Status

**READY**

## Description

<!--- Describe your changes in detail -->
Currently, when using Dart Frog in a situation where middleware is applied, the `RequestContext` which the `Handler` is called with is not the same `RequestContext` which the `Middleware` is called with. This is problematic especially in tests because it is not possible to mock dependencies which are accessed via `context.read` from within middleware.

See the following example:

```dart
Future<Response> onRequest(RequestContext context, String id) async {
  Future<Response> handler(RequestContext context) async {
    final user = context.read<User>();
    return Response();
  }
  return handler.use(middleware)(context);
}

Middleware middleware(String id) {
  return (handler) {
    return (context) async {
      final database = context.read<Database>();
      final user = await database.getUserById(id);
      return handler(context.provide(() => user));
    };
  };
}
```

In the above example, the `RequestContext` in the middleware is not the same as the `RequestContext` in the handlers because dart frog creates a new `RequestContext` internally. This makes it impossible to mock `context.read<Database>()` in the above example.

## Type of Change

<!--- Put an `x` in all the boxes that apply: -->

- [ ] ✨ New feature (non-breaking change which adds functionality)
- [X] 🛠️ Bug fix (non-breaking change which fixes an issue)
- [ ] ❌ Breaking change (fix or feature that would cause existing functionality to change)
- [ ] 🧹 Code refactor
- [ ] ✅ Build configuration change
- [ ] 📝 Documentation
- [ ] 🗑️ Chore



Patch:
diff --git a/packages/dart_frog/lib/src/pipeline.dart b/packages/dart_frog/lib/src/pipeline.dart
index 6ba82e5..41343fc 100644
--- a/packages/dart_frog/lib/src/pipeline.dart
+++ b/packages/dart_frog/lib/src/pipeline.dart
@@ -6,44 +6,28 @@ part of '_internal.dart';
 /// {@endtemplate}
 class Pipeline {
   /// {@macro pipeline}
-  const Pipeline() : this._(const shelf.Pipeline());
-
-  const Pipeline._(this._pipeline);
-
-  final shelf.Pipeline _pipeline;
+  const Pipeline();
 
   /// Returns a new [Pipeline] with [middleware] added to the existing set of
   /// [Middleware].
   ///
   /// [middleware] will be the last [Middleware] to process a request and
   /// the first to process a response.
-  Pipeline addMiddleware(Middleware middleware) {
-    return Pipeline._(
-      _pipeline.addMiddleware((innerHandler) {
-        return (request) async {
-          final response = await middleware(
-            (context) async {
-              final response = await innerHandler(context.request._request);
-              return Response._(response);
-            },
-          )(RequestContext._(request));
-          return response._response;
-        };
-      }),
-    );
-  }
+  Pipeline addMiddleware(Middleware middleware) =>
+      _Pipeline(middleware, addHandler);
 
   /// Returns a new [Handler] with [handler] as the final processor of a
   /// [Request] if all of the middleware in the pipeline have passed the request
   /// through.
-  Handler addHandler(Handler handler) {
-    return (context) async {
-      final response = await _pipeline.addHandler((request) async {
-        final context = RequestContext._(request);
-        final response = await handler(context);
-        return response._response;
-      })(context.request._request);
-      return Response._(response);
-    };
-  }
+  Handler addHandler(Handler handler) => handler;
+}
+
+class _Pipeline extends Pipeline {
+  _Pipeline(this._middleware, this._parent);
+
+  final Middleware _middleware;
+  final Middleware _parent;
+
+  @override
+  Handler addHandler(Handler handler) => _parent(_middleware(handler));
 }


Test patch:
diff --git a/examples/counter/test/routes/_middleware_test.dart b/examples/counter/test/routes/_middleware_test.dart
index dc41462..8f511b1 100644
--- a/examples/counter/test/routes/_middleware_test.dart
+++ b/examples/counter/test/routes/_middleware_test.dart
@@ -9,25 +9,22 @@ class _MockRequestContext extends Mock implements RequestContext {}
 void main() {
   group('middleware', () {
     test('provides incremented count', () async {
-      int? count;
-      final handler = middleware(
-        (context) {
-          count = context.read<int>();
-          return Response(body: '');
-        },
-      );
+      final handler = middleware((context) => Response());
       final request = Request.get(Uri.parse('http://localhost/'));
       final context = _MockRequestContext();
+
       when(() => context.request).thenReturn(request);
+      when(() => context.provide<int>(any())).thenReturn(context);
 
       await handler(context);
-      expect(count, equals(1));
 
-      await handler(context);
-      expect(count, equals(2));
+      final create = verify(() => context.provide<int>(captureAny()))
+          .captured
+          .single as int Function();
 
-      await handler(context);
-      expect(count, equals(3));
+      expect(create(), equals(1));
+      expect(create(), equals(2));
+      expect(create(), equals(3));
     });
   });
 }
diff --git a/examples/kitchen_sink/test/routes/_middleware_test.dart b/examples/kitchen_sink/test/routes/_middleware_test.dart
index 5a1b981..ccb766f 100644
--- a/examples/kitchen_sink/test/routes/_middleware_test.dart
+++ b/examples/kitchen_sink/test/routes/_middleware_test.dart
@@ -9,19 +9,20 @@ class _MockRequestContext extends Mock implements RequestContext {}
 void main() {
   group('middleware', () {
     test('provides greeting', () async {
-      String? greeting;
-      final handler = middleware(
-        (context) {
-          greeting = context.read<String>();
-          return Response(body: '');
-        },
-      );
+      final handler = middleware((_) => Response());
       final request = Request.get(Uri.parse('http://localhost/'));
       final context = _MockRequestContext();
+
       when(() => context.request).thenReturn(request);
+      when(() => context.provide<String>(any())).thenReturn(context);
 
       await handler(context);
-      expect(greeting, equals('Hello'));
+
+      final create = verify(() => context.provide<String>(captureAny()))
+          .captured
+          .single as String Function();
+
+      expect(create(), equals('Hello'));
     });
   });
 }
diff --git a/examples/web_socket_counter/test/routes/_middleware_test.dart b/examples/web_socket_counter/test/routes/_middleware_test.dart
index 113f61b..263a95f 100644
--- a/examples/web_socket_counter/test/routes/_middleware_test.dart
+++ b/examples/web_socket_counter/test/routes/_middleware_test.dart
@@ -10,19 +10,19 @@ class _MockRequestContext extends Mock implements RequestContext {}
 void main() {
   group('middleware', () {
     test('provides a CounterCubit instance.', () async {
-      CounterCubit? cubit;
-      final handler = middleware(
-        (context) {
-          cubit = context.read<CounterCubit>();
-          return Response();
-        },
-      );
+      final handler = middleware((_) => Response());
       final request = Request.get(Uri.parse('http://localhost/'));
       final context = _MockRequestContext();
+
       when(() => context.request).thenReturn(request);
+      when(() => context.provide<CounterCubit>(any())).thenReturn(context);
 
       await handler(context);
-      expect(cubit, isNotNull);
+
+      final create = verify(() => context.provide<CounterCubit>(captureAny()))
+          .captured
+          .single as CounterCubit Function();
+      expect(create(), isA<CounterCubit>());
     });
   });
 }
diff --git a/packages/dart_frog/test/src/middleware_test.dart b/packages/dart_frog/test/src/middleware_test.dart
index 9b9dc54..6b632df 100644
--- a/packages/dart_frog/test/src/middleware_test.dart
+++ b/packages/dart_frog/test/src/middleware_test.dart
@@ -1,6 +1,7 @@
 import 'dart:io';
 
 import 'package:dart_frog/dart_frog.dart';
+import 'package:http/http.dart' as http;
 import 'package:mocktail/mocktail.dart';
 import 'package:test/test.dart';
 
@@ -31,16 +32,14 @@ void main() {
     final handler =
         const Pipeline().addMiddleware(middleware).addHandler(onRequest);
 
-    final request = Request.get(Uri.parse('http://localhost/'));
-    final context = _MockRequestContext();
-    when(() => context.request).thenReturn(request);
-    final response = await handler(context);
+    final server = await serve(handler, 'localhost', 3020);
+    final client = http.Client();
+    final response = await client.get(Uri.parse('http://localhost:3020/'));
 
     await expectLater(response.statusCode, equals(HttpStatus.ok));
-    await expectLater(
-      await response.body(),
-      equals('$stringValue $intValue'),
-    );
+    await expectLater(response.body, equals('$stringValue $intValue'));
+
+    await server.close();
   });
 
   test('middleware can be used to read the request body', () async {
@@ -119,4 +118,27 @@ void main() {
     expect(response.statusCode, equals(HttpStatus.ok));
     expect(response.body(), completion(equals(body)));
   });
+
+  test('chaining middleware retains request context', () async {
+    const value = 'test-value';
+    Middleware noop() => (handler) => (context) => handler(context);
+
+    Future<Response> onRequest(RequestContext context) async {
+      final value = context.read<String>();
+      return Response(body: value);
+    }
+
+    final handler =
+        const Pipeline().addMiddleware(noop()).addHandler(onRequest);
+    final request = Request.get(Uri.parse('http://localhost/'));
+    final context = _MockRequestContext();
+
+    when(() => context.read<String>()).thenReturn(value);
+    when(() => context.request).thenReturn(request);
+
+    final response = await handler(context);
+
+    expect(response.statusCode, equals(HttpStatus.ok));
+    expect(response.body(), completion(equals(value)));
+  });
 }
diff --git a/packages/dart_frog/test/src/provider_test.dart b/packages/dart_frog/test/src/provider_test.dart
index e90694c..e75b4e3 100644
--- a/packages/dart_frog/test/src/provider_test.dart
+++ b/packages/dart_frog/test/src/provider_test.dart
@@ -1,11 +1,9 @@
 import 'dart:io';
 
 import 'package:dart_frog/dart_frog.dart';
-import 'package:mocktail/mocktail.dart';
+import 'package:http/http.dart' as http;
 import 'package:test/test.dart';
 
-class _MockRequestContext extends Mock implements RequestContext {}
-
 void main() {
   test('values can be provided and read via middleware', () async {
     const value = '__test_value__';
@@ -21,13 +19,14 @@ void main() {
     final handler =
         const Pipeline().addMiddleware(middleware).addHandler(onRequest);
 
-    final request = Request.get(Uri.parse('http://localhost/'));
-    final context = _MockRequestContext();
-    when(() => context.request).thenReturn(request);
-    final response = await handler(context);
+    final server = await serve(handler, 'localhost', 3010);
+    final client = http.Client();
+    final response = await client.get(Uri.parse('http://localhost:3010/'));
 
     await expectLater(response.statusCode, equals(HttpStatus.ok));
-    await expectLater(await response.body(), equals(value));
+    await expectLater(response.body, equals(value));
+
+    await server.close();
   });
 
   test('descendant providers can access provided values', () async {
@@ -46,18 +45,24 @@ void main() {
     final handler =
         const Pipeline().addMiddleware(middleware).addHandler(onRequest);
 
-    final request = Request.get(Uri.parse('http://localhost/'));
-    final context = _MockRequestContext();
-    when(() => context.request).thenReturn(request);
-    final response = await handler(context);
+    final server = await serve(handler, 'localhost', 3011);
+    final client = http.Client();
+    final response = await client.get(Uri.parse('http://localhost:3011/'));
 
     await expectLater(response.statusCode, equals(HttpStatus.ok));
-    await expectLater(await response.body(), equals(url));
+    await expectLater(response.body, equals(url));
+
+    await server.close();
   });
 
   test('A StateError is thrown when reading an un-provided value', () async {
+    Object? exception;
     Response onRequest(RequestContext context) {
-      context.read<Uri>();
+      try {
+        context.read<Uri>();
+      } catch (e) {
+        exception = e;
+      }
       return Response();
     }
 
@@ -65,10 +70,13 @@ void main() {
         .addMiddleware((handler) => handler)
         .addHandler(onRequest);
 
-    final request = Request.get(Uri.parse('http://localhost/'));
-    final context = _MockRequestContext();
-    when(() => context.request).thenReturn(request);
+    final server = await serve(handler, 'localhost', 3012);
+    final client = http.Client();
+    final response = await client.get(Uri.parse('http://localhost:3012/'));
+
+    await expectLater(response.statusCode, equals(HttpStatus.ok));
+    expect(exception, isA<StateError>());
 
-    await expectLater(() => handler(context), throwsStateError);
+    await server.close();
   });
 }