[Enhancement] Routing layer refactor

matthewmcgarvey commented 2 years ago

For a few months now (actually over a year 😱 ), an effort has been underway to refactor Invidious' routing layer and generally addressing the huge file that was src/invidious.cr (you can see some of the connected PRs through this issue https://github.com/iv-org/invidious/issues/1121). I thought I'd make this issue to have one open to make the effort more "official" and I might come and go with the work as life gets more busy or I start feeling burnt out again so I want to have my thoughts written down so others can pick up where I left off if they so choose or make fun of how bad of an idea it was. Whichever makes more sense.

Need

Before this work began, src/invidious.cr held all the endpoint code and was close to 6000 lines of code.

Here's some of the main pain points

Large files
Global routes
Global methods
No testing around endpoint code

Considerations

My very first PR setting up the initial patterns of routing and the jobs https://github.com/iv-org/invidious/pull/1399

The goal is to extract routes into classes/modules that can be more easily read in isolation. By doing this, we also pave the way to potential opportunities to switch to a different framework that would be a better fit for this codebase.

Goals

[x] Extract jobs from main file and into new pattern
[x] Extract routes from main file and into new pattern
[ ] Organize global helper methods into better places
[ ] Migrate to official new routing (not my made up way)
[ ] Testing???

Routing Options

There's a bunch of options as for how we might land this effort:

What we have is good enough

This option would be that somehow, everyone is satisfied with my temporary solution https://github.com/iv-org/invidious/blob/82f3eda82b9e97a94f98e6f325b428d985d9852d/src/invidious/routing.cr#L1-L11

It wouldn't be the worst thing in the world. The only thing I would want, probably is to get the routing moved to a different file because they're starting to take up a lot of space as well. It does bother me that this is not an "official" way of writing kemal apps though.

We switch to a different framework

This might be the "best" solution as far as the outcome might be nicer than any of the other options, but I think it would take the most work and bring the most risk.

There are definitely plenty of other options:

Some tradeoffs to consider when evaluating these frameworks would be:

How "different" of a setup do they impose that would require us to change more or less to fit into
Does the framework impose a strictness that would require a lot more work to fit our code into
Are there dependency requirements we wouldn't be able to accept
Is the framework active and open to help us as we migrate and maybe even contribute
What does it look like to migrate gradually

In order to do this, we'd also have to evaluate our usage of Kemal to identify what we require in the framework to migrate (auth, session, cookies, etc)

To put this out there, I am a core maintainer of Lucky so that colors the recommendations I will make. Practically, I'm leaning towards either Lucky or Athena, and that is without any investigation into the migration process. Of the two Athena might be an easier target to migrate to as Lucky has some drawbacks when migrating existing code that doesn't already follow some of its assumptions. I've never used Athena, though.

We submit updates to Kemal to allow for modularity

The drawbacks that Invidious has experience with Kemal is that everything is global and there is no official way to extract out pieces and compose them back together. But this is open source and nothing is set in stone. There could be an opportunity for us to reach out to Kemal and make those changes that are necessary to allow us to continue down this path of extraction without going against the grain.

I have already taken the first steps here: https://github.com/kemalcr/kemal/pull/621

SamantazFox commented 2 years ago

Regarding the "We switch to a different framework", there is #2498 :)

SamantazFox commented 2 years ago

@matthewmcgarvey your latest PR got me thinking about that again.

One thing I'm considering the use of a route declaration system similar to Athena's:

class ExampleController < ATH::Controller
  @[ARTA::Get("/")]
  def root : String
    "At the index"
  end
end

That would allow to clean the existing routing system and ease an hypothetical framework migration (both Athena and Lucky declare the routes in classes that inherit from a base class).

matthewmcgarvey commented 2 years ago

Another option that I thought about last night is forking kemal and including it in the codebase directly and then go our own way from there. That would allow us to gradually change kemal into something that would either be what we want or allow jumping to a different framework.

The reason why that is appealing is because if we want to switch to a new framework, I'd like to be able to do it gradually. That'd be difficult/impossible with the way kemal works right now, but if we could change how it makes the HTTP::Server, we could potentially plug in a different library/framework

Blacksmoke16 commented 2 years ago

The reason why that is appealing is because if we want to switch to a new framework, I'd like to be able to do it gradually. That'd be difficult/impossible with the way kemal works right now

I came up with this approach a little while ago that could be useful if you decide to the go the Athena route:

require "athena"
require "kemal"

get "/index" do
  "INDEX"
end

@[ARTA::Route(path: "/v2")]
class ExampleController < ATH::Controller
  get "/athena" do
    "ATHNEA"
  end
end

class AthenaHandler < Kemal::Handler
  def call(context)
    # This could also be a specific header/content-type.
    return call_next(context) unless context.request.path.starts_with? "/v2"

    channel = Channel(Bool).new

    # Do this in a dedicated fiber so that the DI container is unique per request.
    spawn do
      handler = ADI.container.athena_route_handler
      request = ATH::Request.new context.request
      athena_response = handler.handle request
      athena_response.send request, context.response
      handler.terminate request, athena_response
    rescue
      channel.send false
    else
      channel.send true
    end

    channel.receive
  end
end

add_handler AthenaHandler.new

Kemal.run

Tl;dr because Athena itself isn't tightly coupled to an HTTP::Server, you totally can just inject it as an HTTP::Handler into Kemal, using some sort of path/header constraint to denote that request should be handled by Athena and not Kemal. Not sure if this would also work with Lucky, but would allow you to gradually move things over, then at some point replace Kemal.run with like ATH.run.

iv-org / invidious