App/Middleware delegate should take Next delegate parameter
markrendle opened this issue · 65 comments
The App/Middleware delegate signature should be:
Func<IDictionary<string,object>, Func<Task>, Task>
Similar to Node.js Connect
This would provide a much clearer way for
- Application delegates to say "I don't know what this is", or,
- Middleware to integrate itself into the pipeline - see this blog post.
Some would say that changing the delegate at this stage would be deleterious to the fledgling ecosystem, but it seems that most of the work has currently been done around Katana and IAppBuilder, which adds so much to the base spec that I don't really think changing the core delegate would have a huge effect.
We never specified a middleware signature. We left it explicitly undefined. This is the first real proposal. Thanks!
👍
Also, this closely matches one of the existing IAppBuilder.Use overloads:
Func<IOwinContext, Func<Task>, Task>
How about taking this to the owin discussion list instead?
Why not use IEnumerator to handle it? this would require not external dependencies. BCL would be more then enough.
public static IEnumerable<Func<AppFunc, AppFunc>> OwinApps()
{
var app = new List<Func<AppFunc, AppFunc>>();
app.Add(Middleware1);
app.Add(Middleware2);
app.Add(Middleware3WithConfig(message: "response from ", moreMessage: " middleware 3 with config<br/>"));
app.Add(Middleware4);
app.Add(Middleware5);
return app;
}
public static AppFunc ToOwinApp()
{
var apps = OwinApps().ToList();
return
env =>
{
var enumerator = apps.GetEnumerator();
AppFunc next = null;
next = env2 => enumerator.MoveNext() ? enumerator.Current(env3 => next(env3))(env2) : CachedCompletedResultTupleTask;
return next(env);
};
}We can then create an extension method on IList<Func<AppFunc, AppFunc>>() called .Use which calls .Add.
and ToOwinApp or ToAppFunc which converts it to AppFunc.
var app = new new List<Func<AppFunc, AppFunc>>()
.Use(MiddleWare1)
.Use(MiddleWare2)
.ToOwinApp();
@prabirshrestha This was an early idea several of us threw out. It has long been ignored. I still like it. Thing is, both options are quite easy. If we could pick one, I think that would go a long way to resolving the problem of #19.
By the way, I find the IEnumerator bit a nice addition on top of the simpler: Func<AppFunc, AppFunc>.
Your implementation allows for a nicer, lazier construction than the pure function composition of the signature above. Expanded, that's: Func<Func<IDictionary<string, object>, Task>, Func<IDictionary<string, object>, Task>>, which is actually quite similar to Mark's, except that his uses closures and modifies the overall delegate.
I don't think changing the app delegate to add next is the solution. I think we need a defined middleware delegate that builders and hosts can consistently use. I've been using Func<AppFunc, AppFunc> pretty happily.
@prabirshrestha do you have a full app sample available somewhere? There are several components in your snyppit that need more explanation.
How about taking this to the owin discussion list instead?
A few reasons I can think of not to (and I admit I'm biased toward GitHub):
- the context (code) lives here
- I can pass a URL to a given comment to folks from here; can I do that on a Google list? Or can I only send a URL to the thread itself?
- Google lists are painfully slow on the web
- Email is terrible
@Tratcher You could do that with a fold, or Enumerable.Aggregate. Same thing, really, using Func<AppFunc, AppFunc> as the primary composition mechanism. (Thus why I keep saying this is "built into F#.")
@Tratcher here is the link to the repo https://github.com/prabirshrestha/simple-owin
Samples can be found at https://github.com/prabirshrestha/simple-owin/blob/master/src/SimpleOwinAspNetHost/Samples/MiddlewareApps.cs and https://github.com/prabirshrestha/simple-owin/blob/master/src/SimpleOwinAspNetHost/Samples/SimpleOwinApp.cs
- Plus - No dll reference
- Minus - Lots of extension methods on arbitrary framework types instead, which still need a home.
While this would deprecate Owin.dll, I think even more people would end up needing to carry around boiler plate code, something like the current Microsoft.Owin.dll, to make it consumable.
The issue at the moment is not the delegate itself, it is simply the lack of a standard. Without a uniform point of entry, we can't be sure that middleware will be truly universal.
From my point of view it doesn't matter which standard is picked, as long as it fits the basic need.
I'm not understanding the benefits of Func<AppFunc, AppFunc>. It seems more complicated than necessary.
And I wrote the Delegate of Doom™.
I'm not understanding the benefits of Func<AppFunc, AppFunc>. It seems more complicated than necessary.
Compared to what instead?
I do think we need to have some kind of linkage between this discussion and the mailing list. I originally chose Google Groups because everything else sucked worse for having a general discussion. But I do think that changes to THIS code should be in held in the context of the repo. I think changes to the OWIN spec should probably be located in the Google Group. That's my own
Action<Func<Action<Func<Task<T>>>>> Where T : IOpinion
Connect follows the "App .use(function)" pattern that IAppBuilder adopted.
I think changes to the OWIN spec should probably be located in the Google Group.
I'd personally like to see the spec move to its own repo and discussions, at least discussions about the spec itself, be in that repo (via issues). I agree that general OWIN stuff doesn't have a natural way to be discussed on GitHub. I think some amount of splitting is a fact of life for now.
@half-ogre Compared to Func<IDictionary<string, object>, Func<Task>, Task>
Which lets you write your middleware like
public Task XsrfCheck(IDictionary<string, object> env, Func<Task> next)
{
var xsrfHeader = GetHeaderValue(env, "x-xsrf-token");
if (IsValid(xsrfHeader))
{
return next();
}
else
{
SetResponseStatus(env, 400);
return TaskHelper.Completed;
}
}and also
public async Task ErrorHandler(IDictionary<string, object> env, Func<Task> next)
{
try
{
await next();
}
catch (AppException ex)
{
SetResponseStatus(env, 500);
WriteResponse(env, FormatException(ex);
}
}It also means that the signature for applications and middleware can be the same, so you can chain multiple app/service frameworks and mix them up in the pipeline - is this Web API? No, OK, now we need to add in this HTML-minifying middleware and call on to Nancy.
@markrendle That's certainly convenient, but I don't think it's the LCD. For example, that doesn't allow for shimming or replacing the environment dictionary, correct? (admittedly not common, but useful for linting and a few other scenarios).
I'm also curious how you address branching pipelines.
Hell, yes, you're absolutely right, and I even updated Fix to this signature:
Func<IDictionary<string, object>, Func<IDictionary<string, object>, Task>, Task>
So
public Task XsrfCheck(IDictionary<string, object> env, Func<IDictionary<string,object>, Task> next)
{
var xsrfHeader = GetHeaderValue(env, "x-xsrf-token");
if (IsValid(xsrfHeader))
{
return next(env);
}
else
{
SetResponseStatus(env, 400);
return TaskHelper.Completed;
}
}Which totally allows replacing the environment dictionary for whatever nefarious reasons you might do that.
I don't know what you mean by branching pipelines.
There's a small selection of middlewares that use the Func<Env, Funv<Env, Task>, Task> signature here: https://github.com/simple-owin/
Yeah, Func<IDictionary<string, object>, Func<IDictionary<string, object>, Task>, Task> is what I started with as well. I feel like that's a good LCD, but I probably haven't yet built nearly as much middleware as you fine folks.
I think you missed updating the first line of your sample.
By branching, I mean not all applications have a linear pipeline. You may branch the pipeline based on URL, Auth, or some other aspect of the request. You have one middleware that checks the condition, and then invokes next OR the side-branch. For reference, this is what the Map extensions in Katana do. Do you have any samples doing this in simple-owin?
I thought that was what you meant.
You'd just need a factory to build the AppFunc following some configuration. Simple.Owin.Static and Simple.Owin.Cors both have classes that take a bunch of config and then build the AppFunc delegate. There's even some very naughty implicit casting in there which makes them magically build if you pass them to Fix's Use function.
I'm envisioning something like
public class Dispatcher : IEnumerable
{
private readonly List<Tuple<Regex, AppFunc>> _list = new List<Tuple<Regex, AppFunc>>();
public void Add(Regex regex, AppFunc appFunc)
{
_list.Add(Tuple.Create(regex, appFunc);
}
public Task Dispatch(IDictionary<string, object> env, Func<IDictionary<string, object>, Task> next)
{
var url = GetUrl(env);
var match = _list.FirstOrDefault(t => t.Item1.IsMatch(url));
return match == null
? next(env)
: match(env, next);
}
}@markrendle Yeah, that's what I'm doing in my Dispatcher middleware as well.
That signature makes my widdle head hurt.
On Dec 2, 2013, at 4:20 PM, Drew Miller notifications@github.com wrote:
Yeah, Func<IDictionary<string, object>, Func<IDictionary<string, object>, Task>, Task> is what I started with as well. I feel like that's a good LCD, but I probably haven't yet built nearly as much middleware as you fine folks.
—
Reply to this email directly or view it on GitHub.
I'm not sure you guys realize this, but the Func<AppFunc, AppFunc> and Func<IDictionary<string, object>, AppFunc, Task> are the same thing with the parameters flipped.
This is very obvious using type aliases in the following F# example (run in TryFSharp):
open System
open System.Collections.Generic
open System.Threading.Tasks
type AppFunc = IDictionary<string, obj> -> Task
type MiddlewareFunc = AppFunc -> AppFunc
type FixFunc = IDictionary<string, obj> -> AppFunc -> Task
// MiddlewareFunc
let middleware (next: AppFunc) (env: IDictionary<string, obj>) =
// do stuff before
let task = next env
// do stuff after
task
// FixFunc
let fixMiddleware (env: IDictionary<string, obj>) (next: AppFunc) =
// do stuff before
let task = next env
// do stuff after
taskI'm for either, though I like the relative simplicity and aesthetics of Func<AppFunc, AppFunc>. Or perhaps I just like all the Func. I think the FixFunc is probably more familiar to C# devs, but they are ultimately the same thing.
I could definately get behind either
Func<IDictionary<string, object>, Func<IDictionary<string, object>, Task>, Task> or
Func<AppFunc, AppFunc>
Either of these would also be compatible with the object contructor/invoke syntax from IAppBuilder which I have currently used in Superscribe for doing middleware routing
http://superscribe.org/examples.html#tab_owinpipelining
I wonder if it is best to support one approach (as would be correct) or specify that any pipeline builder needs to support two or three signatures (as would be pragmatic)?
I think there is still some confusion about the middleware delegates. They are effectively the same thing. Here's the C# type definitions:
// OWIN Environment first:
Func<IDictionary<string, object>, Func<IDictionary<string, object>, Task>, Task> // long form
Func<IDictionary<string, object>, AppFunc, Task> // middle form
//no short form
// Next first:
Func<Func<IDictionary<string, object>, Task>, IDictionary<string, object>, Task> // long form
Func<AppFunc, IDictionary<string, object>, Task> // middle form
Func<AppFunc, AppFunc> // short form; see NOTE belowTo convert between the two, here are a few helpers:
static class MiddlewareConversion {
public static Func<AppFunc, AppFunc> Flip(Func<IDictionary<string, object>, AppFunc, Task> middleware) {
return next => env => middleware(env, next);
}
public static Func<IDictionary<string, object>, AppFunc, Task> Flip(Func<AppFunc, AppFunc> middleware) {
return (env, next) => middleware(next) (env);
}
}Now, is that really very bad? I don't think so. We could have both as "official", though picking one would probably reduce confusion. You can still pick the one you like best with minimal fuss.
NOTE: I know the short form above is not exactly the same as the other two forms, but it works effectively the same. To expand that short form, you would get: Func<Func<IDictionary<string, object>, Task>, Func<IDictionary<string, object>, Task>>. This signature would translate to the following base class structure, which is currently in use in Nancy and Superscribe:
public abstract class OwinMiddleware {
protected readonly Func<IDictionary<string, object>, Task> _next;
protected OwinMiddleware(Func<IDictionary<string, object>, Task> next) {
_next = next;
}
public abstract Task Invoke(IDictionary<string, object> environment);
}I needed to write the code to properly understand this.
So, here's my original proposal:
using AppFunc = Func<IDictionary<string, object>, Func<IDictionary<string,object>, Task>, Task>;
internal class Program
{
private static void Main(string[] args)
{
AppFunc app = OK;
var env = new Dictionary<string, object>();
app(env, End);
}
static Task OK(IDictionary<string, object> env, Func<IDictionary<string, object>, Task> next)
{
env["status"] = 200;
return next(env);
}
static Task End(IDictionary<string, object> env)
{
return TaskHelper.Completed;
}
}and here's the other proposed alternative:
using AppFunc = Func<IDictionary<string, object>, Task>;
internal class Program
{
private static void Main(string[] args)
{
AppFunc app = WrapOK(End);
var env = new Dictionary<string, object>();
app(env);
}
static Task OK(IDictionary<string, object> env, Func<IDictionary<string, object>, Task> next)
{
env["status"] = 200;
return next(env);
}
static AppFunc WrapOK(AppFunc next)
{
return env => OK(env, next);
}
static Task End(IDictionary<string, object> env)
{
return TaskHelper.Completed;
}
}Difference being, in the first case you're working directly with the method that will be called on each request, and in the second case you're calling a kind of wrapper method ahead of time to curry(?) the actual method with the Next AppFunc. Have I got that right?
If so, then I am on board with the wrapper method approach. Sorry for being slow.
If not, then, er, sorry for being even slower.
Yes, you are correct that the second approach curries the next AppFunc. It works the same as HttpMessageHandler. I wouldn't say it necessarily "wraps," but I can see from your example what you mean. I still see it as simply flipping the parameters. In my F# samples above, both are curried, so the conversion is simply a generic function:
// val flip : ('b -> 'a -> 'c) -> 'a -> 'b -> 'c
let flip f a b = f b a
where f takes the parameters in the order b, a.
Or put another way, the difference is between:
// Func<IDictionary<string, object>, Func<IDictionary<string, object>, Task>, Task>
static Task OK(IDictionary<string, object> env, Func<IDictionary<string, object>, Task> next)
{
env["status"] = 200;
return next(env);
}vs
// Func<Func<IDictionary<string, object>, Task>, Func<IDictionary<string, object>, Task>>
static Func<IDictionary<string, object> env, Task> OK(Func<IDictionary<string, object>, Task> next)
{
return env => {
env["status"] = 200;
return next(env);
};
}If you can implement one, you can implement the other, though the latter encourages an extra closure and is maybe less intuitive.
@panesofglass Your flip only applies to the long and middle forms of the func, though; the "short form" is actually different - AppFunc -> Env -> Task vs AppFunc -> AppFunc - that might be a simple case of precedence grouping in F# but is a big difference in C#.
@markrendle Correct.
I realize I haven't been involved with this project for quite some time, but I figured I'd throw in another wrench. If you choose to ignore it, my feelings will not be hurt. ;)
Since the early OWIN days, I've moved to spending a lot more time in Node.js. The same problem exists: building compose-able Web services with completely asynchronous behavior. One hassle with the Connect-style middleware is the lack of support for plugging into a response pipeline.
I address some of this with a project I work on named Argo: https://github.com/argo/argo.
This does complicate the signature a bit, but it allows simpler abstractions to be built on top. Taking this approach means the flexibility is always there at the low level even if the ease of developer consumption is not.
I remember us doing weird tricks in OWIN to try to break into a response pipeline, and it's shoddy.
Argo allows specifying a pipeline to which handlers are attached. By default, there are "request" and "response" pipelines. Taking a pipeline approach also means custom pipelines are possible. This has been very helpful for running pipelined execution of asynchronous code on "cache:miss" or "resource:authorization".
Just throwing that out there. Maybe this is a solved problem or a problem you consider not worth addressing. I'll leave it up to you. If you do think it's worth addressing, it does impact the signature. FWIW, I've been much happier programming with this flexibility, and it has enabled all sorts of use cases.
Cheers.
@kevinswiber Since OWIN targets .NET 4.0 and above, and is expected to be used primarily with .NET 4.5, the Task signature and async/await syntax deal with plugging into the response pipeline quite nicely.
To use your CORS example, and the delegate currently used by Fix:
public static async Task AllowAllOrigins(IDictionary<string,object> env, Func<IDictionary<string,object>, Task> next)
{
await next(env);
AddHeader(env, "Access-Control-Allow-Origin", "*");
}@kevinswiber That's a whole different can of worms involving the per request signature, where we've only been talking about the wire-up signature. @markrendle that example only works if the response is fully buffered. It won't work on servers without response buffering like HttpListener or Nowin. We've been working around it for the moment with an OnSendingHeaders event, which has been adequate for the moment. Only certain kinds of middleware have needed to plug into the response, so there hasn't been a big push for modifying the core signature/spec to support it.
Glad people at least agree on the concept of Func<AppFunc, AppFunc>, in one form or another. Let me pose another challenge to you:
So far all of the sample middlware have been very static, but that's less common in real middleware. With patterns like the ones we've discussed, how do you propose to get configuration data into instances of the middleware?
For reference, there are two kinds of data we've wanted to pass into middleware: 1. an options class with all of the config details, and/or 2. Any app/server initialization context available in the IAppBuilder.Properties IDictionary<string, object>. This is why the core IAppBuilder.Use signature is not Func<AppFunc, AppFunc>, but Func<AppFunc, [params]..., AppFunc>. With nested lamdas these params would be available via closure. With the constructor pattern these params are passed in as additional constructor arguments.
@kevinswiber That's an interesting point. If only we had specified the response Stream as Action<Stream>!
@Tratcher I'm happy with closures. I really prefer the simpler signature unless there exists a very compelling reason to change it. As you say, when you wrap that signature with a class, you can easily add additional parameters that would be treated as closures from a lambda.
@Tratcher here is how I would pass configurations for Func<AppFunc, AppFunc>.
- an options class with all of the config details,
- optional arguments
public static Func<AppFunc, AppFunc> MethodOverride1(string key = "_method", bool checkQuerystring = false, bool checkBody = true)
{
// do an expensive computation once here
return
next =>
env =>
{
// per request stuffs here
}
}- options class
public static Func<AppFunc, AppFunc> MethodOverride2(MethodOverrideConfig config)
{
}- class with properties
public class MethodOverride {
public string Key { get; set; }
public Func<AppFunc, AppFunc> Middleware() {
}
}- class with ctor
public class MethodOverride2 {
public MethodOverride(string key) {
}
public Func<AppFunc, AppFunc> Middleware() {
}
}it can then be chained using.
app
.Use(MethodOverride1())
.Use(MethodOverride2(new MethodOverrideConfig { Key = "_method" } ))
.Use(new MethodOverride { Key = "_method" }.Middleware())
.Use(new MethodOverride("_method").Middleware());
Middleware writer would then need to expose one method that returns Func<AppFunc, AppFunc> or is Func<AppFunc, AppFunc>
- Any app/server initialization context available in the IAppBuilder.Properties IDictionary<string, object>
In one of the owin call we did talk about passing startup environment to the request environment too.
Discussed capabilities detection in the following three scenarios:
1) In the startup properties, detecting general server support for a feature
2) Per request, include the same startup properties dictionary (as read only?) in the request environment ("host.Properties"?).
3) Per request, advertising that this specific request supports this specific feature (e.g. this request is a websocket request). For features that are exposed as a Func/Action, the presence of the associated key and delegate is sufficient to indicate support. For features that require the application to set environment keys, a matching key with ".Allowed" appended and a value indicating specific feature details could be used to advertise support. (e.g. websocket.Func.Allowed = "1.0" informs the application that it can set websocket.Func with the 1.0 version of the WebSocketFunc delegate.
Ok, cool, those were about the data patterns I was expecting.
Some of the properties (like capabilities) are included per request, but it's often cleaner & more efficient to check config just once at startup. You may even decide to construct your pipeline differently at startup depending on the capabilities advertised.
@Tratcher Wait, what? I thought we weren't doing unbuffered responses. Wasn't that the whole single-vs-double-tap debate?
@markrendle double-tap would have required response buffering in many scenarios, making it inappropriate for streamed responses. Single-tap allows for buffered or un-buffered responses. Asp.Net buffers responses unless you ask it not to. HttpListener has never supported buffering, but it could be added via middleware.
@panesofglass Argh! I pushed for Action<Stream> (or Func<Stream,Task>) as the response object in the first place. :-(
@markrendle Yes, I remember. I was for that, too. Ah well.
Tangent: You can build one on top of the other but not vice versa. The current abstraction is more powerful and it allows for building the Action, HttpMessageHandler, request/response pattern which is arguably better for certain kinds of http applications (like making APIs) but not for streaming (SignalR style) applications.
Sorry to be late to this party. Just comment to @markrendle Cors sample: why not add that header before calling next? That would solve problem with not buffering, doesn't it.
As I see it, one of the fundamental advantages of Func<Env, AppFunc, Task> over Func<AppFunc, AppFunc> is the ability to dynamically change the pipeline at execution time, not just at composition/startup.
Func<Env, AppFunc, Task> ability to dynamically change the pipeline is nice, I just don't see any interesting usecase for it (anybody care to share?) and resulting pipeline could be harder to reason about.
With Func<AppFunc, AppFunc> you don't need any builder you just build it backwards without any helper method (You still need 404 kind of AppFunc).
@Bobris Yeah, it wasn't a very good example, but there are cases where you'd want to await the next func and act on information in the response.
@EddieGarmon @Bobris I must be missing something. How are you going to dynamically change the pipeline at execution time? Also, how does Func<AppFunc, AppFunc> limit this? To the latter point, I could see that you would be stuck if you partially applied the first AppFunc, but that's not necessary. Perhaps I'm thinking too much in F#?
@panesofglass Changing pipeline would need to be build-in feature of the Builder somehow, I also don't know why it would be needed. It does limit it only in efficiency - closure has to be always created in Func<AppFunc, AppFunc>, but not in other case. Of course because it is not AppFunc, closure must be somewhere, but it does not need to be immutable closure, and that could be difference in C# and F# thinking :-). Note I would not do that and I am thinking C#.
For what it's worth I'm not smart enough to think of this myself (I needed @Bobris' help).
public static void UseCors(this IAppBuilder app)
{
app.Use((Func<AppFunc, AppFunc>)(app1 => env => Simple.Owin.Cors.Create(OriginMatcher.Wildcard).Build()(env, app1)));
}
What will OWIN peons like me to when smart guys aren't around?
Ask the Simple.Owin guys to provide the extension methods on IAppBuilder in their assembly
:D
seems like there are two simple owin stuffs which are totally different and has caused some confusion.
one is mine which can be found at https://github.com/prabirshrestha/simple-owin (includes the asp.net host and small middlewares as samples only for educational purposes)
and the other is @markrendle which can be found at https://github.com/simple-owin (middlewares only)
I believe @TheFastCat is talking about @markrendle's simple.owin
let's not deviate form the actual #20 discussion out here.
Was this issue discussed during the OWIN call yesterday?
@half-ogre No. We agreed we should wait until we have established a governance model before moving forward with additions or changes to the OWIN specification.
So Func<AppFunc, AppFunc> right? 😄
Sounds like it. I think we are waiting to convene a governing body. @skoon, are we waiting on you for that?
Yup. I'm in contact with Dale and I'm flying blind. So hold on tight!
On Dec 19, 2013, at 7:15 PM, Ryan Riley notifications@github.com wrote:
Sounds like it. I think we are waiting to convene a governing body. @skoon, are we waiting on you for that?
—
Reply to this email directly or view it on GitHub.
Is this still happening 😄
Yes, I got waylaid by the holidays. Working on draft governance documents for folks to review.
On Jan 3, 2014, at 12:51 PM, David Fowler notifications@github.com wrote:
Is this still happening
—
Reply to this email directly or view it on GitHub.
Post the initial draft for comments on Google Docs, I also posted the link in the HTTP Abstractions Google Group.
https://groups.google.com/forum/#!topic/net-http-abstractions/y78jpL6nEEY
The doc is located here: https://docs.google.com/document/d/1mn3dY6zNyKBU3P_TWoR-RdYpScJDbsXU2TRhwpSAha8/edit
I've called for a vote to modify the spec to specify the Func<AppFunc,AppFunc> as the middleware signature.
https://groups.google.com/forum/#!topic/net-http-abstractions/cq7DqSSFQ00
As of today, the proposal to add
Func<AppFunc,AppFunc>
to the official OWIN spec is passing 8-0 (+1: 7, 0: 1, -1:0)
We'll give it one more day I think just to make sure, then we'll close the voting.
I suggest we close this issue, and open a new pull request for the inclusion of MidFunc to a revised 1.1 version of the owin specification?