Use middleware instead Rails controller for model query #39

st0012 · 2023-04-19T21:51:31Z

By using middleware instead of a Rails controller, it skips many middlewares that aren't needed for our model query request. In big Rails apps, this could be a big performance improvement.
Although Rails engine also supports being used as a middleware, it still requires registering routes to the application. That doesn't always work depends on the app's own rules on how to register routes.
Compare to Rails engine, middleware is a more flexible way to integrate with Rails apps and also a lot lighter.

Closes https://github.com/Shopify/ruby-dev-exp-issues/issues/898

test/lib/middleware_test.rb

andyw8 · 2023-04-20T13:01:17Z

Gemfile.lock

@@ -206,6 +206,7 @@ GEM
    sorbet (0.5.10782)
      sorbet-static (= 0.5.10782)
    sorbet-runtime (0.5.10782)
+    sorbet-static (0.5.10782-universal-darwin-21)


I'm not sure why the Gemfile.lock was 'out of sync' but I was seeing these same changes locally so 👍 to this.

andyw8 · 2023-04-20T13:04:45Z

lib/ruby_lsp_rails/middleware.rb

+        match = request.path.match(PATH_REGEXP)
+
+        if match
+          resolve_database_info_from_model(match[:model_name])


nit: We could return early here, then change rescue to ensure, that way we only need @app.call(env) once.

I don't think ensure rescues exceptions:

def foo 1 / 0 ensure "bar" end puts foo

$ ruby test.rb test.rb:5:in `/': divided by 0 (ZeroDivisionError) from test.rb:5:in `foo' from test.rb:10:in `<main>'

Even if it works, I think it only reduces one @app.call(env) but sacrifices explicitness of rescue. So I prefer not making such change yet.

But I will see if I can reduce the number of conditions here.

andyw8 · 2023-04-20T13:06:02Z

lib/ruby_lsp_rails/middleware.rb

+
+module RubyLsp
+  module Rails
+    class Middleware


We might later want to consider extracting some of this into ruby-lsp, so there's less 'boilerplate' necessary for extension authors.

IMO, we don't want to encourage extension authors doing this kind of work themselves. It's the same as we don't want them to implement their own visitor, but a listener to a specific request.

Yes, that's what I mean. Things like the /ruby_lsp_rails/ path shouldn't be something for the extension developer to be concerned with.

But if we won't want them to do this, what's the point of extracting them to ruby-lsp?

This is Rack/Rails specific pattern and not what ruby-lsp should have provided?

I agree with Stan here, ruby-lsp-rails will be quite a unique extension since it will be communicating with a live server. I suspect most other extensions wouldn't need it - and we shouldn't create incentive for people to do so as getting this wrong can be detrimental to the overall experience.

I think there could be other neat extensions powered by a similar mechanism, but I agree with not trying to optimize for that until we actually have more real-world examples.

andyw8 · 2023-04-20T13:06:55Z

lib/ruby_lsp_rails/middleware.rb

+      sig { params(env: T::Hash[T.untyped, T.untyped]).returns(T::Array[T.untyped]) }
+      def call(env)
+        request = ActionDispatch::Request.new(env)
+        # TODO: improve the model name regex


I think this is fine for now. Once we add support for namespaces it may need adjusted.

I agree that we can keep it for the time being. In the future, we may need a simple router just to make the codebase maintainable. Something like

return @app.call(env) unless request.path.start_with?("/ruby_lsp_rails/") endpoint, argument = request.path.delete_prefix("/ruby_lsp_rails/").split("/") case endpoint when "models" # ... when "jobs" # ... end

andyw8 · 2023-04-20T13:10:21Z

I'll try it out shortly, but I assume that since this is done at the rack level, we'll no longer see any /ruby_rails_lsp/... requests in the Rails server log?

st0012 · 2023-04-20T14:47:17Z

I'll try it out shortly, but I assume that since this is done at the rack level, we'll no longer see any /ruby_rails_lsp/... requests in the Rails server log?

The middleware is not injected that early in the stack, so you should still see it in logs, like:

Started GET "/ruby_lsp_rails/models/User" for 127.0.0.1 at 2023-04-20 11:56:51 +0100

- By using middleware instead of a Rails controller, it skips many middlewares that aren't needed for our model query request. In big Rails apps, this could be a big performance improvement. - Although Rails engine also supports being used as a middleware, it still requires registering routes to the application. That doesn't always work depends on the app's own rules on how to register routes. - Compare to Rails engine, middleware is a more flexible way to integrate with Rails apps and also a lot lighter.

vinistock

This works really well and greatly simplifies the gem. Amazing work

vinistock · 2023-04-20T16:32:06Z

lib/ruby_lsp_rails/railtie.rb

+  module Rails
+    class Railtie < ::Rails::Railtie
+      initializer "ruby_lsp_rails.setup" do |app|
+        app.config.middleware.insert_after(ActionDispatch::ShowExceptions, RubyLsp::Rails::Middleware)


Out of curiosity, what happens if the application doesn't have ActionDispatch::ShowExceptions?

It'll raise an exception saying there's no ActionDispatch::ShowExceptions to insert after.

But this is one of the key middlewares in Rails. If someone removes it, that's probably not something we'd want to support.

vinistock · 2023-04-20T16:36:42Z

lib/ruby_lsp_rails/middleware.rb

+      sig { params(env: T::Hash[T.untyped, T.untyped]).returns(T::Array[T.untyped]) }
+      def call(env)
+        request = ActionDispatch::Request.new(env)
+        # TODO: improve the model name regex


I agree that we can keep it for the time being. In the future, we may need a simple router just to make the codebase maintainable. Something like

return @app.call(env) unless request.path.start_with?("/ruby_lsp_rails/") endpoint, argument = request.path.delete_prefix("/ruby_lsp_rails/").split("/") case endpoint when "models" # ... when "jobs" # ... end

st0012 added this to the 2023-Q2 milestone Apr 19, 2023

st0012 requested a review from a team as a code owner April 19, 2023 21:51

st0012 self-assigned this Apr 19, 2023

st0012 force-pushed the switch-to-middleware branch from 60107fa to 9416b36 Compare April 19, 2023 21:54

andyw8 reviewed Apr 20, 2023

View reviewed changes

test/lib/middleware_test.rb Show resolved Hide resolved

st0012 force-pushed the switch-to-middleware branch from 9416b36 to e39cff3 Compare April 20, 2023 10:56

st0012 requested a review from andyw8 April 20, 2023 10:57

andyw8 reviewed Apr 20, 2023

View reviewed changes

st0012 force-pushed the switch-to-middleware branch from e39cff3 to 637b911 Compare April 20, 2023 15:19

vinistock approved these changes Apr 20, 2023

View reviewed changes

andyw8 approved these changes Apr 20, 2023

View reviewed changes

st0012 merged commit 912758e into main Apr 20, 2023

st0012 deleted the switch-to-middleware branch April 20, 2023 19:11

st0012 added the bugfix This PR fixes an existing bug label Apr 20, 2023

shopify-shipit bot temporarily deployed to production April 21, 2023 19:13 Inactive

Use middleware instead Rails controller for model query #39

Use middleware instead Rails controller for model query #39

Uh oh!

Conversation

st0012 commented Apr 19, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andyw8 Apr 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andyw8 commented Apr 20, 2023

Uh oh!

st0012 commented Apr 20, 2023

Uh oh!

vinistock left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andyw8 Apr 20, 2023 •

edited

Loading