-
Notifications
You must be signed in to change notification settings - Fork 76
Unobtanium migration #1369
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Unobtanium migration #1369
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR migrates the Dev Proxy from the Titanium.Web.Proxy library to the new Unobtanium.Web.Proxy library, introducing a functional plugin API to replace the previous event-based system. The migration includes updated proxy backend infrastructure, new control flow patterns where plugins can return responses directly, and comprehensive telemetry integration with OpenTelemetry.
Key changes:
- Complete replacement of Titanium.Web.Proxy with Unobtanium.Web.Proxy (0.9.1-beta-2)
- Introduction of functional plugin API methods (OnRequestAsync, OnRequestLogAsync, OnResponseAsync, OnResponseLogAsync)
- New proxy storage system with IProxyStorage interface for global and request-specific data
- Enhanced OpenTelemetry integration for distributed tracing and metrics
Reviewed Changes
Copilot reviewed 82 out of 82 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
DevProxy/packages.lock.json | Updated Unobtanium.Web.Proxy to 0.9.1-beta-2 and added OpenTelemetry packages |
DevProxy/Proxy/ProxyEngine.cs | Complete rewrite using new Unobtanium proxy events and functional plugin architecture |
DevProxy/Proxy/EfficientProxyHttpClientFactory.cs | New HTTP client factory for efficient port reuse |
DevProxy/Proxy/CertificateDiskCache.cs | Removed (replaced by Unobtanium certificate management) |
DevProxy/Program.cs | Added Unobtanium proxy services and OpenTelemetry configuration |
DevProxy/Plugins/ProxyStorage.cs | New storage implementation for global and request-specific plugin data |
DevProxy/Extensions/IServiceCollectionExtensions.cs | Added proxy services, HTTP client factory, and OpenTelemetry configuration |
DevProxy.Plugins/migration.md | Comprehensive migration guide for plugin developers |
DevProxy.Plugins/inventory.md | Complete inventory of existing plugins and their migration status |
DevProxy.Plugins/Mocking/*.cs | Multiple plugins migrated to new functional API |
DevProxy.Plugins/Reporting/*.cs | Reporting plugins updated to use new IProxyStorage interface |
if (string.IsNullOrEmpty(requestId) || textWriter is null) | ||
{ | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The textWriter null check is redundant since the method parameter is not nullable. Consider removing the '|| textWriter is null' condition or making the parameter nullable if null values are expected.
} | |
if (string.IsNullOrEmpty(requestId)) | |
{ | |
return; | |
} |
Copilot uses AI. Check for mistakes.
var token = jwtToken.Split(' ')[1]; | ||
var token = jwtToken!.StartsWith("Bearer ", StringComparison.OrdinalIgnoreCase) | ||
? jwtToken["Bearer ".Length..].Trim() | ||
: jwtToken.Trim(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code uses a magic string 'Bearer ' twice. Consider extracting this to a constant for better maintainability: const string BearerPrefix = \"Bearer \";
: jwtToken.Trim(); | |
var token = jwtToken!.StartsWith(BearerPrefix, StringComparison.OrdinalIgnoreCase) | |
? jwtToken[BearerPrefix.Length..].Trim() | |
: jwtToken.Trim(); |
Copilot uses AI. Check for mistakes.
@@ -221,7 +205,7 @@ private void LoadData() | |||
} | |||
} | |||
|
|||
private (Action<SessionEventArgs, CrudApiAction, IDictionary<string, string>> handler, CrudApiAction action, IDictionary<string, string> parameters)? GetMatchingActionHandler(Request request) | |||
private (Func<HttpRequestMessage, CrudApiAction, IDictionary<string, string>, CancellationToken, Task<HttpResponseMessage>> handler, CrudApiAction action, IDictionary<string, string> parameters)? GetMatchingActionHandler(HttpRequestMessage request) | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method signature is extremely long and complex. Consider extracting the Func delegate to a separate type alias or interface to improve readability: private delegate Task<HttpResponseMessage> ActionHandler(HttpRequestMessage request, CrudApiAction action, IDictionary<string, string> parameters, CancellationToken cancellationToken);
{ | |
// Delegate type for action handlers to improve readability | |
private delegate Task<HttpResponseMessage> ActionHandler(HttpRequestMessage request, CrudApiAction action, IDictionary<string, string> parameters, CancellationToken cancellationToken); | |
private (ActionHandler handler, CrudApiAction action, IDictionary<string, string> parameters)? GetMatchingActionHandler(HttpRequestMessage request) | |
{ |
Copilot uses AI. Check for mistakes.
new("content-type", "application/json; charset=utf-8") | ||
var response = new HttpResponseMessage(statusCode) | ||
{ | ||
Content = new StringContent(body, Encoding.UTF8, "application/json") |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
exception information
This information exposed to the user depends on
exception information
This information exposed to the user depends on
exception information
This information exposed to the user depends on
exception information
This information exposed to the user depends on
exception information
This information exposed to the user depends on
exception information
Awesome! As it's WIP, let's mark it as draft so that we won't merge it just yet |
Thanks for the PR @svrooij awesome work 👏 A few comments from me:
The order in which the plugins are defined in the config file is important today, for example, if you don't put a reporter plugin after a plugin that generates a report then you won't get any output.
All plugins can have |
public virtual Option[] GetOptions() => []; | ||
public virtual Command[] GetCommands() => []; | ||
/// <inheritdoc/> | ||
public virtual Func<RequestArguments, CancellationToken, Task>? ProvideRequestGuidanceAsync { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the benefit of using delegates vs. virtual methods that we can override? Since we need to implement the logic anyway, isn't using a delegate an unnecessary complexity? Ie. I need to implement the method and add a reference to it in the delegate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The benefit is performance, before calling the plugin we already know if it implemented this. So in the ProxyEngine it skips plugins that did not implement this. We can do the null check on the function, instead of the base plugin returning completed task
And on the other hand, this way we force the plugin to think about what it is doing next, early response/modify request/continue as normal.
@@ -158,7 +157,7 @@ public static IList<MockResponseHeader> BuildGraphResponseHeaders(Request reques | |||
new ("Date", requestDate), | |||
new ("Content-Type", "application/json") | |||
}; | |||
if (request.Headers.FirstOrDefault((h) => h.Name.Equals("Origin", StringComparison.OrdinalIgnoreCase)) is not null) | |||
if (request.Headers.Contains("Origin")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this comparison case-insensitive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the http specs the headers should be case insensitive, but they come through as they come through. Kestrel parses them to an actual object no matter the casing. So I thought let's make it case insensitive here as well.
No you're right that might be a mistake, let me check
@@ -28,19 +29,18 @@ public override Task AfterRecordingStopAsync(RecordingArgs e, CancellationToken | |||
} | |||
|
|||
var requestLogs = e.RequestLogs | |||
.Where(l => ProxyUtils.MatchesUrlToWatch(UrlsToWatch, l.Context?.Session.HttpClient.Request.RequestUri.AbsoluteUri ?? "")); | |||
.Where(l => ProxyUtils.MatchesUrlToWatch(UrlsToWatch, l.Request!.RequestUri!.AbsoluteUri ?? "")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The !.
looks a bit messy. If it can be null, we need to do more robust checks to avoid runtime failures.
A new proxy backend and a new control flow, each plugin can decide to return a response.
This is a work-in-progress, looking for feedback. Everything changed and not everything is fixed but I think it is the way to go.
Still has several
// TODO: …
notes in the code, but it builds and runs 🤯Discuss
Control flow
The next proxy has two entry points
OnRequest
andOnResponse
. TheOnRequest
event has to be answered withModifyRequest
,EarlyResponse
orContinue
. This means that the first plugin that wants to send a response can do do just that.This also means there is not session or state to modify. The plugin can decide to answer or do nothing. I've split guidance plugins (which will always receive the request since they were not modifying it anyway), from modifying plugins. With the latter if the requests reached that plugin it is not modified before, no need to check if it is modified, just decide what to do.
Is this way workable? This will make the order in which the plugins are added important.
DevTools plugin
I seem to be doing something wrong, there is no longer any log in the console of the DevTools plugin
Plugins in general
Plugins can implement the any of the plugin functions, which are all optional, which means the ProxyEngine can select just the plugins that implement that function.
Plugin URLS
Maybe plugins should optionally have a list of urls for that plugin. Then we could skip plugins that don't do stuff with the current url.