Skip to content
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

[BREAKING]: Overhaul Handlers framework + rewrite Router to be trie-based and support dynamic use-cases #818

Merged
merged 2 commits into from
Mar 30, 2022

Conversation

quinnj
Copy link
Member

@quinnj quinnj commented Mar 27, 2022

Ok, this is the start of the server-side overhaul that matches #789 which was client-side. The main changes here include:

  • Removing the abstract Handler type in the Handlers module
  • The middleware/handler interfaces now consists of the following, which match what we have on the client side:
    • Handler: a function of the form req::Request -> resp::Response (technically, there's also a "stream" handler interface of the form stream::Stream -> Nothing, but that's the more advanced form)
    • Middleware: a function of the form handler::Handler -> handler::Handler

The key (and only current) example of a middleware we have right now is:

function streamhandler(handler)
  return function(stream::Stream)
      request::Request = stream.message
      request.body = read(stream)
      closeread(stream)
      request.response::Response = handler(request)
      request.response.request = request
      startwrite(stream)
      write(stream, request.response.body)
      return
  end
end

which is a middleware that takes a request handler and and returns a stream handler that we wrap request handlers with by default in HTTP.serve. If the user passes stream=true, then we won't wrap, assuming they're passing their own stream handler.

  • For better or worse, this PR also overhauls the HTTP.Router type to a more traditional/standard approach. Routes can now be registered dynamically. They are parsed into a single trie-based structure. We allow ant-style path patterns in paths, which means you can do things like /*/ to wildcard match any segment (we already supported this previously), /** double wildcard as the final segment to match any length of path after the **, and named variable matching like /{id}/, which will be stored in the req.context[:params] in a Dict with the path variable name as key. You can also add a regex to conditionally match a variable, like /{id:[0-9]+}/ to only match numbers. Note this doesn't parse the argument, only does the regex match. The Router also supports 405 responses in addition to 404 (405 for when the path matches, but not the method).

Planning to add some inline docs here.

@codecov-commenter
Copy link

codecov-commenter commented Mar 27, 2022

Codecov Report

Merging #818 (bdfb31a) into master (4b3c633) will increase coverage by 0.66%.
The diff coverage is 96.92%.

@@            Coverage Diff             @@
##           master     #818      +/-   ##
==========================================
+ Coverage   78.10%   78.77%   +0.66%     
==========================================
  Files          36       36              
  Lines        2430     2502      +72     
==========================================
+ Hits         1898     1971      +73     
+ Misses        532      531       -1     
Impacted Files Coverage Δ
src/Handlers.jl 96.92% <96.89%> (+5.54%) ⬆️
src/HTTP.jl 83.05% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@quinnj
Copy link
Member Author

quinnj commented Mar 27, 2022

In terms of broader vision, I still like the idea of including Router in HTTP.jl, since it's such a fundamental middleware. But I'm thinking maybe we make a Middlewares.jl package to collect other various middlewares (auto json marshalling, variable marshalling, auth, whatever else).

@quinnj
Copy link
Member Author

quinnj commented Mar 27, 2022

Ok, here's something a little more speculative (and perhaps heretical); what we have have the following construct that would allow a very traditional way of defining an "app" similar to other languages/frameworks:

mutable struct App
    router::Router
    middleware::Any
end

App() = App(Router(), nothing)

ismethod(x) = x in (:get, :post, :put, :delete, :head, :options, :patch)

function Base.getproperty(app::App, x::Symbol)
    if x === :use
        return function(handler)
            app.middleware = handler(app.middleware)
        end
    elseif ismethod(x)
        return function(path, handler)
            register!(app.router, uppercase(String(x)), path, handler)
        end
    else
        return getfield(app, x)
    end
end

This would allow you do so something like:

const app = HTTP.App()

app.use(logging_middleware)

app.get("/api/widgets", req -> HTTP.Response(200, getWidgets()))

HTTP.serve(app, host, port)

So the idea is that we have an App object where we can collect middlewares (they just wrap each other) and routes and then we can pass it to HTTP.serve (or even be cute there and allow doing app.serve or something?).

I don't know, it's pretty un-Julian in style, but it would certainly feel very similar for those coming from other languages/frameworks. I guess a more Julia style would be something like:

const app = HTTP.App()

HTTP.use!(app, logging_middleware)

HTTP.get(app, "/api/widgets", getWidgetsHandler)

HTTP.serve(app, host, port)

or something like that. The HTTP.use! part isn't too bad, but man the app.get(...) syntax just feels so simple and easy. Maybe I've been doing too much backend node stuff lately.

@quinnj quinnj changed the title Overhaul Handlers framework + rewrite Router to be trie-based and support dynamic use-cases [BREAKING]: Overhaul Handlers framework + rewrite Router to be trie-based and support dynamic use-cases Mar 28, 2022
@fonsp
Copy link
Member

fonsp commented Mar 28, 2022

Awesome work on the new router Jacob! Maybe we could take this breaking release as a chance to split it into a new package?

To me, this PR (improved router API) and your interesting questions above (about API choices for the middleware framework) show exactly why a split makes sense: there are different philosophies about the best way to write a web framework, and the core HTTP stdlib/package shouldn't fix that decision. By having frameworks as separate packages, HTTP.jl can converge to a stable, unopinionated HTTP implementation, while other framework packages can continue evolving!

My motivations behind making the split would be:

  • maintainability (core HTTP and web frameworks are different types of software, but having them together requires contributors to be comfortable doing both, will mix issues of both, and requires versioning both together)
  • learnability (You can use next.js without learning about the http stdlib. Search results, docs and autocomplete for next.js will never show confusing results for client-side HTTP)

@clarkevans
Copy link
Contributor

Maybe we could take this breaking release as a chance to split it into a new package?

I'd also prefer anything that isn't necessary to go outside of HTTP. The mime type sniffing is another example, sometimes you want to use mime detection without using HTTP at all.

@fredrikekre
Copy link
Member

I think it is nice to include something in HTTP.jl still. That doesn't stop anyone from building something else or something better (xref #658 and #658 (comment)).

@clarkevans
Copy link
Contributor

clarkevans commented Mar 30, 2022

@fredrikekre Ok. If your thought is that this might be replaced later, the router types and functions shouldn't be exported by default? Perhaps they could be in sub-module, HTTP.Router or something like this? Otherwise it'll be less convenient to use an alternative router due to name collisions, etc. Even so, I still think there's no reason to include it, it could just be another package, the HTTP documentation can point to the other package if it's helpful, etc.

@quinnj
Copy link
Member Author

quinnj commented Mar 30, 2022

I agree with @fredrikekre that keeping the now-very-simple streamhandler definition and Router object in HTTP.jl is a much smaller footprint compared to the heavier Handler interface that was defined previously, and provides a fundamental piece of functionality that a lot of users will want to use out of the box. Note that HTTP.jl has typically been a batteries-included kind of package, and given the current state of maintainership, I'd like to keep this in HTTP.jl for now.

As I stated above, however, I would like to start a Middlewares.jl package that would be an extra repository for common, useful middlewares using the new interface/framework proposed here. That provides an additional layer of separate for those wishing to build other frameworks/middlewares: they can either write their own HTTP.serve + entirely different middleware framework (full customizability), or they can use the existing HTTP.serve and write their own middleware/handlers adhering to the interfaces (functions of the type handler = request -> response or middleware = handler -> handler).

Perhaps somewhat arrogantly, I'm always drawn to the approach of finding the most natural/best/flexible interface and trying to make that the ecosystem "standard". The Julia ecosystem has had great success with consistency/cohesiveness in this way and I'd like to think we could achieve the same here with the interfaces; both on client-side and server-side.

@quinnj quinnj merged commit 78ad43d into master Mar 30, 2022
@quinnj quinnj deleted the jq/handlers_overhaul branch March 30, 2022 22:36
@c42f
Copy link
Contributor

c42f commented May 26, 2022

I know I've been very absent here, but I just wanted to say: awesome work @quinnj! The new router seems like a great improvement. The middleware stuff also seems like a nice simplification, it will be interesting to try it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants