Thoughts about this modified stack function?

[JockLawrie]

Hi there,

I am putting together some simple web services that need some middleware.
I like your stack function and have modified it as follows:

• Each piece of middleware modifies the request. E.g., content_type!(req) instead of content_type(req).
• If there is a problem, such as invalid auth credentials, req.response is modified and returned, short-circuiting the loop.

Thoughts?

function stack(fns::Vector{Function}, endpoint::Function; error_fn=rethrow)
    s = function(req::HTTP.Request)
        try
            res = req.response
            for f in fns
                f(req)  # f modifies req in place
                res.status != 0 && return res  # Short-circuit the loop
            end
            return endpoint(req)
        catch err
            error_fn(err)
        end
    end
    HTTP.HandlerFunction(s)
end

[amellnik]

Hi @JockLawrie -- I like both these points, although I think it's best to leave the res within req. I also realized recently that the try block should be at the per-middleware level so that if an uncaught error occurs, the partially-modified request (and response) can be passed along to the error handling function. Otherwise the uncaught error responses won't have things like CORS, etc. Let me play around with it tomorrow and I'll post an update here.

[JockLawrie]

Sounds good.
Why is it best to leave req.response untouched?

[amellnik]

Master now has

function stack(fns::Array{Function, 1}, endpoint::Function;
               error_fn=(req, err)->rethrow(err))
    # First we form the function that represents our middleware + endpoint
    # wrapped in a try-catch block so that we can deal with errors that are not
    # explicitly handled.
    s = function(req::HTTP.Request)
        try
            for f! in fns
                f!(req)
            end
            return endpoint(req)
        catch err
            error_fn(req, err)
        end
    end
    # Once we have this function we wrap it in a HandlerFunction which is used
    # to register routes
    return HTTP.HandlerFunction(s)
end

I think it's slightly clearer that the response is being modified by the middleware stack if we don't make res a reference to it. The default response has a 200 status so I need to think about a good way to allow middleware to exit the stack early and return a response, and I'm holding off for now.

[JockLawrie]

Agree it's clearer to not use res.
Isn't the default status 0? (HTTP.Request().response.status)
Even if it's 200, doesn't an early exit from the stack imply a non-200 response?
Including req in the signature for error_fn is a good idea too.

[amellnik]

It's 0 when you construct it like that, but when the HTTP server receives a request the corresponding response is set to 200. I think there might be cases where you would want to exit the stack for a non-200 response?

I think that I want to make the default error handler for the stack separate from the ones that are provided for use in endpoints -- I want to encourage users to handle likely errors inside the endpoints rather than relying on the one that's passed to stack.

Also, are you on the Julia slack by any chance? This might be easier to discuss there.

[JockLawrie]

Sure. Continuing on Slack...

[amellnik]

Closing -- stack function was modified a while back.