Replace BaseHTTPMiddleware with pure ASGI middlewares by erosselli · Pull Request #7230 · ethyca/fides

erosselli · 2026-01-15T18:10:16Z

Description Of Changes

While debugging performance issues for the consent v3 APIs, we also ran some benchmarks on a no-op /ping endpoint and found it couldn't handle a large number of RPS. Trying to find the cause, we found that the BaseHTTPMiddleware class (used by all our middlewares!) is extremely slow . Even adding a no-op BaseHTTPMiddleware that just calls the next on the stack majorly affects performance.

This PR replace our middleware implementation with a pure ASGI middleware that does not rely on BaseHTTPMiddleware class. This is meant to be just a refactor of the underlying implementation, but all middleware behavior should stay the same.

Steps to Confirm

All tests should still pass, since middlewares should stay functionally the same

Pre-Merge Checklist

vercel · 2026-01-15T18:10:22Z

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments

Project	Deployment	Review	Updated (UTC)
fides-plus-nightly	Ignored	Preview	Jan 28, 2026 7:31pm
fides-privacy-center	Ignored		Jan 28, 2026 7:31pm

greptile-apps · 2026-01-26T19:42:58Z

Greptile Overview

Greptile Summary

This PR refactors all middleware from BaseHTTPMiddleware to pure ASGI implementations to address significant performance issues discovered during benchmarking. The BaseHTTPMiddleware class has documented performance problems that severely impact request throughput.

Key Changes:

Converted 5 middleware classes to pure ASGI: LogRequestMiddleware, AnalyticsLoggingMiddleware, AuditLogMiddleware, ProfileRequestMiddleware, SecurityHeadersMiddleware, and RateLimitIPValidationMiddleware
Created new src/fides/api/asgi_middleware.py with 4 middleware implementations
Removed ~150 lines of old middleware code from main.py
Added comprehensive test coverage (1,400+ new test lines) for all new middleware implementations
All middleware behavior remains functionally identical - this is purely a performance optimization

Previous Issues Addressed:
All issues from previous review threads have been resolved:

Request body buffering in AuditLogMiddleware now properly captures and replays body for downstream handlers
ProfileRequestMiddleware is properly registered in app_setup.py for dev mode
API_PREFIX constant is imported from endpoint_utils instead of hardcoded
asyncio.create_task references are stored in class-level set to prevent premature garbage collection

Confidence Score: 4/5

This PR is safe to merge with moderate risk due to the performance-critical refactoring nature
Score reflects the high-quality implementation with comprehensive test coverage (1,400+ new test lines) and successful resolution of all previously identified issues. The refactoring maintains functional equivalence while improving performance. However, a score of 4 rather than 5 is appropriate because: (1) this is a high-risk change affecting core request processing middleware, (2) the PR exceeds recommended size limits with 13 files changed and 2,410 insertions, and (3) ASGI middleware operates at a lower level than BaseHTTPMiddleware, making subtle bugs harder to detect in testing
Pay close attention to src/fides/api/asgi_middleware.py during deployment and monitor request handling behavior

Important Files Changed

Filename	Overview
src/fides/api/asgi_middleware.py	New pure ASGI middleware implementations replacing BaseHTTPMiddleware for better performance
src/fides/api/app_setup.py	Updates middleware registration to use new ASGI middleware classes
src/fides/api/main.py	Removes old BaseHTTPMiddleware-based middleware implementations (~150 lines deleted)
src/fides/api/util/rate_limit.py	Converts RateLimitIPValidationMiddleware to pure ASGI middleware
src/fides/api/util/security_headers.py	Converts SecurityHeadersMiddleware to pure ASGI middleware

greptile-apps

_{4 files reviewed, 4 comments}

_{Edit Code Review Agent Settings | Greptile}

src/fides/api/util/asgi_middleware.py

src/fides/api/asgi_middleware.py

erosselli · 2026-01-27T13:31:13Z

@greptile please re-review

greptile-apps

_{4 files reviewed, 4 comments}

_{Edit Code Review Agent Settings | Greptile}

src/fides/api/asgi_middleware.py

erosselli · 2026-01-27T16:10:38Z

@greptile please re-review

greptile-apps

_{No files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

adamsachs

great job finding what seems to be a serious performance improvement. from what i can tell, your pure ASGI replacements achieve parity with the existing middleware functionality 👍

the major tradeoff here is readability/maintainability: the pure ASGI versions are more verbose and 'clunky', as expected, since we're deliberately not using the BaseHTTPMiddleware abstraction anymore. but could we regain some of the benefits of that abstraction by just making our own? that's my main feedback here - it seems like there is quite a bit of room to make our 'own' base http middleware abstraction that's lighter weight (and more performant) than starlette's, but at least gives us some of benefits of a base class: shared/consistent code, less boilerplate, etc. that's not necessarily a blocker, but i'd like to at least know we're planning a quick followup, or else we risk never doing that. unless there's a specific reason you've decided not to make that abstraction?

beyond that, just a couple of nits i spotted along the way.

generally, what do you think are the biggest risks? i know we have some pretty solid test coverage, but some middleware things (e.g. rate limiting) can be pretty hard to cover in an automated test suite, so i'd be curious to know if there's anything you're particularly worried about and whether we can get ahead of some 'manual' (or more integration) testing to de-risk that.

adamsachs · 2026-01-27T19:56:42Z

src/fides/api/asgi_middleware.py

+        if scope["type"] != "http":
+            await self.app(scope, receive, send)
+            return
+
+        start_time = perf_counter()
+        status_code = 500  # Default in case of exception
+
+        # Extract request info from scope
+        method = scope.get("method", "UNKNOWN")
+        path = scope.get("path", "/")
+
+        # Get Fides-Client header
+        headers = dict(scope.get("headers", []))
+        fides_client = headers.get(b"fides-client", b"unknown").decode("latin-1")


i think there's quite a bit of room here for a useful abstraction/standardization, specifically around the basic parsing/validation of the scope object: e.g. filtering out non-HTTP requests, retrieving the headers dict, etc.

should we not create a base class here that these can all inherit from, so that we get some of that boiler plate 'for free' on each middleware class, and so that its standardized? or at least some helper methods to reduce code duplication? i know we don't want to reinvent the BaseHTTPMiddleware wholesale, but i think it'd be prudent to build in a few more abstractions here, unless there's a compelling reason not to...

done in 1f7fcd8

adamsachs · 2026-01-27T20:07:49Z

src/fides/api/util/rate_limit.py

is this used anymore?

weird, it has a bunch of tests but does seem unused..

well you removed a use of it, so that would sorta make sense :)

Addressed as part of 1f7fcd8

adamsachs · 2026-01-27T20:12:29Z

src/fides/api/app_setup.py

        # Validate header before SlowAPI processes the request
        fastapi_app.add_middleware(RateLimitIPValidationMiddleware)
        # Required for default rate limiting to work
        fastapi_app.add_middleware(SlowAPIMiddleware)


i think this is also a basehttpmiddleware. looks like they have a pure ASGI version though, SlowAPIASGIMiddleware (see laurentS/slowapi#113 for some background, i haven't looked into whether there are any tradeoffs/reasons to not switch over...)

oh interesting! I kind of skipped over the external packages ones. Since we don't enable them by default, I might look into changing it as a follow-up PR

oh ok so now we use our self-rolled rate limiting middleware instead of this one, by default?

because we do have rate limiting by default, right?

So we might have it on in fides cloud but it requires setting rate_limit_client_ip_header , which is empty by default

I have sanity checked: if configured, we use both our rate limit one (which I think just does IP validation? perhaps confusingly named? ) and the slow API one.

I'll look into replacing the SlowAPI one with the ASGI version in a follow up PR

adamsachs · 2026-01-27T20:13:11Z

src/fides/api/app_setup.py

@@ -112,6 +118,19 @@
        GZipMiddleware, minimum_size=1000, compresslevel=5


this one looks good, seems to already be pure ASGI 👍

vcruces

LGTM, really nice work! I imagine this wasn’t easy. Thanks for adding so many tests, they give confidence for something that seems hard to test 😓

vcruces · 2026-01-27T20:58:58Z

tests/api/test_logging.py

+            sent_messages.append(message)
+
+        # Call the middleware - it should catch the exception and send a 500 response
+        await middleware(scope, receive, send)


I might be mistaken and this may just be similar logic, but is this logic being repeated in a few places? If so, would it be worth extracting it for reuse?

vcruces · 2026-01-27T21:35:02Z

tests/api/middleware/test_middleware.py

@@ -0,0 +1,115 @@
+# pylint: disable=missing-docstring, redefined-outer-name


Given that we already have tests/ops/api/v1/test_middleware.py, would it make sense to rename this one (or the other)? I find it a bit confusing to tell what each file contains

oh that's some git mishap , I meant to have moved test_middleware into the /middleware folder but I see that both files are kept , I'll fix. thanks for the callout!

adamsachs

looks good, thank you for taking the time to put that base class in place - i think it makes all this code a lot cleaner, and will continue to pay dividends as we add more middleware!

adamsachs · 2026-01-28T17:19:55Z

src/fides/api/asgi_middleware.py

+ASGIApp = Callable[[Scope, Receive, Send], Awaitable[None]]
+
+
+class BaseASGIMiddleware:


love it! even more extensive than i imagined, this makes the middleware implementations so much cleaner.

we should submit a PR to https://github.com/Kludex/starlette that adds this to the core library :)

adamsachs · 2026-01-28T18:14:49Z

src/fides/api/asgi_middleware.py

+            return
+        await self.handle_http(scope, receive, send)
+
+    async def handle_http(self, scope: Scope, receive: Receive, send: Send) -> None:


you prefer to leave this with a default impl rather than as an abstractmethod (and making this class an ABC)? not saying that's wrong, just wanted to check it's a deliberate (or at least defensible) choice :)

hmm yeah abstract method might be better, I did debate about it since this seemed like a reasonable default, but also the original BaseHTTPMiddleware does raise NotImplemented for the dispatch method.

sure that works, makes sense to model BaseHTTPMiddleware closely in that respect 👍

Addressed in 054b051

Replace BaseHTTPMiddleware with pure ASGI middlewares

6e3a7b9

erosselli added the do not merge Please don't merge yet, bad things will happen if you do label Jan 15, 2026

erosselli and others added 2 commits January 15, 2026 18:00

only add middlewares if they will be used

b5bc7b5

Merge branch 'main' into erosselli/asgi-middlewares

2816172

vercel bot deployed to Preview – fides-plus-nightly January 26, 2026 14:16 View deployment

erosselli and others added 3 commits January 26, 2026 16:35

Fix tests

670a97a

Add changelog

19f7012

Merge branch 'main' into erosselli/asgi-middlewares

ff53430

erosselli marked this pull request as ready for review January 26, 2026 19:39

erosselli requested a review from a team as a code owner January 26, 2026 19:39

erosselli requested review from vcruces and removed request for a team January 26, 2026 19:39

greptile-apps bot reviewed Jan 26, 2026

View reviewed changes

src/fides/api/util/asgi_middleware.py Outdated Show resolved Hide resolved

src/fides/api/util/asgi_middleware.py Outdated Show resolved Hide resolved

src/fides/api/asgi_middleware.py Outdated Show resolved Hide resolved

src/fides/api/asgi_middleware.py Outdated Show resolved Hide resolved

vercel bot deployed to Preview – fides-plus-nightly January 26, 2026 19:43 View deployment

Fixes and tests

8859bd4

greptile-apps bot reviewed Jan 27, 2026

View reviewed changes

src/fides/api/asgi_middleware.py Outdated Show resolved Hide resolved

src/fides/api/asgi_middleware.py Outdated Show resolved Hide resolved

src/fides/api/asgi_middleware.py Outdated Show resolved Hide resolved

src/fides/api/asgi_middleware.py Outdated Show resolved Hide resolved

erosselli and others added 3 commits January 27, 2026 13:09

fixes, more tests

50681c5

Merge branch 'main' into erosselli/asgi-middlewares

d80cb28

Update changelog

5df3012

greptile-apps bot reviewed Jan 27, 2026

View reviewed changes

erosselli requested a review from adamsachs January 27, 2026 16:14

erosselli removed the do not merge Please don't merge yet, bad things will happen if you do label Jan 27, 2026

Fix test

a15fc1c

adamsachs reviewed Jan 27, 2026

View reviewed changes

vcruces reviewed Jan 27, 2026

View reviewed changes

Create base ASGI middleware class for code reuse

1f7fcd8

erosselli and others added 3 commits January 28, 2026 10:46

Clean up tests, reuse fixtures

2bdfac4

Move tests out of ops folder

7707a7c

Merge branch 'main' into erosselli/asgi-middlewares

b19fac5

vercel bot deployed to Preview – fides-plus-nightly January 28, 2026 13:52 View deployment

erosselli added 2 commits January 28, 2026 12:10

Add tests for rate limit middleware

54e6082

static checks

0b13129

adamsachs approved these changes Jan 28, 2026

View reviewed changes

Make handle_http an abstract method

054b051

erosselli added this pull request to the merge queue Jan 28, 2026

Merged via the queue into main with commit a0d2d04 Jan 28, 2026
54 of 55 checks passed

erosselli deleted the erosselli/asgi-middlewares branch January 28, 2026 20:54

		@@ -112,6 +118,19 @@
		GZipMiddleware, minimum_size=1000, compresslevel=5

		@@ -0,0 +1,115 @@
		# pylint: disable=missing-docstring, redefined-outer-name

		ASGIApp = Callable[[Scope, Receive, Send], Awaitable[None]]


		class BaseASGIMiddleware:

Conversation

erosselli commented Jan 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description Of Changes

Steps to Confirm

Pre-Merge Checklist

Uh oh!

vercel bot commented Jan 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greptile-apps bot commented Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Overview

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erosselli commented Jan 27, 2026

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erosselli commented Jan 27, 2026

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

adamsachs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vcruces left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsachs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

erosselli commented Jan 15, 2026 •

edited

Loading

vercel bot commented Jan 15, 2026 •

edited

Loading

greptile-apps bot commented Jan 26, 2026 •

edited

Loading

erosselli Jan 28, 2026 •

edited

Loading