Skip to content

Use middleware instead Rails controller for model query #39

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 20, 2023
Merged

Use middleware instead Rails controller for model query #39

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Gemfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

sorbet-static (0.5.10782-universal-darwin-22)
sorbet-static (0.5.10782-x86_64-linux)
sorbet-static-and-runtime (0.5.10782)
Expand Down Expand Up @@ -246,6 +247,7 @@ GEM
zeitwerk (2.6.7)

PLATFORMS
arm64-darwin-21
arm64-darwin-22
x86_64-linux

Expand Down
1 change: 0 additions & 1 deletion app/assets/config/ruby_lsp_rails_manifest.js
View file
Open in desktop

This file was deleted.

Empty file removed app/assets/images/rails_ruby_lsp_rails/.keep
View file
Open in desktop
Empty file.
15 changes: 0 additions & 15 deletions app/assets/stylesheets/ruby_lsp_rails/application.css
View file
Open in desktop

This file was deleted.

Empty file removed app/controllers/concerns/.keep
View file
Open in desktop
Empty file.
9 changes: 0 additions & 9 deletions app/controllers/ruby_lsp/rails/application_controller.rb
View file
Open in desktop

This file was deleted.

32 changes: 0 additions & 32 deletions app/controllers/ruby_lsp/rails/models_controller.rb
View file
Open in desktop

This file was deleted.

7 changes: 0 additions & 7 deletions app/jobs/ruby_lsp_rails/application_job.rb
View file
Open in desktop

This file was deleted.

9 changes: 0 additions & 9 deletions app/mailers/ruby_lsp_rails/application_mailer.rb
View file
Open in desktop

This file was deleted.

Empty file removed app/models/concerns/.keep
View file
Open in desktop
Empty file.
10 changes: 0 additions & 10 deletions app/models/ruby_lsp/rails/application_record.rb
View file
Open in desktop

This file was deleted.

15 changes: 0 additions & 15 deletions app/views/layouts/ruby_lsp_rails/application.html.erb
View file
Open in desktop

This file was deleted.

6 changes: 0 additions & 6 deletions config/routes.rb
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion lib/ruby-lsp-rails.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

require "sorbet-runtime"
require "ruby_lsp_rails/version"
require "ruby_lsp_rails/engine"
require "ruby_lsp_rails/railtie"

module RubyLSP
module Rails
Expand Down
30 changes: 0 additions & 30 deletions lib/ruby_lsp_rails/engine.rb
View file
Open in desktop

This file was deleted.

61 changes: 61 additions & 0 deletions lib/ruby_lsp_rails/middleware.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
# typed: strict
# frozen_string_literal: true

module RubyLsp
module Rails
class Middleware
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@andyw8 andyw8 Apr 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

extend T::Sig

PATH_REGEXP = %r{/ruby_lsp_rails/models/(?<model_name>.+)}

sig { params(app: T.untyped).void }
def initialize(app)
@app = app
end

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

match = request.path.match(PATH_REGEXP)

if match
resolve_database_info_from_model(match[:model_name])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

else
@app.call(env)
end
rescue
@app.call(env)
end

private

sig { params(model_name: String).returns(T::Array[T.untyped]) }
def resolve_database_info_from_model(model_name)
const = ActiveSupport::Inflector.safe_constantize(model_name)

if const && const < ActiveRecord::Base
begin
schema_file = ActiveRecord::Tasks::DatabaseTasks.schema_dump_path(const.connection.pool.db_config)
rescue => e
warn("Could not locate schema: #{e.message}")
end

body = JSON.dump({
columns: const.columns.map { |column| [column.name, column.type] },
schema_file: schema_file,
})

[200, { "Content-Type" => "application/json" }, [body]]
else
not_found
end
end

sig { returns(T::Array[T.untyped]) }
def not_found
[404, { "Content-Type" => "text/plain" }, ["Not Found"]]
end
end
end
end
22 changes: 22 additions & 0 deletions lib/ruby_lsp_rails/railtie.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# typed: strict
# frozen_string_literal: true

require "ruby_lsp_rails/middleware"

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


config.after_initialize do |_app|
if defined?(::Rails::Server)
ssl_enable, host, port = ::Rails::Server::Options.new.parse!(ARGV).values_at(:SSLEnable, :Host, :Port)
app_uri = "#{ssl_enable ? "https" : "http"}://#{host}:#{port}"
File.write("#{::Rails.root}/tmp/app_uri.txt", app_uri)
end
end
end
end
end
end
9 changes: 0 additions & 9 deletions sorbet/rbi/shims/actiondispatch.rbi
View file
Open in desktop

This file was deleted.

11 changes: 0 additions & 11 deletions sorbet/rbi/shims/rails.rbi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,3 @@ module Rails
def configure(&block); end
end
end

module RubyLsp
module Rails
class Engine
class << self
sig { returns(ActionDispatch::Routing::RouteSet) }
def routes; end
end
end
end
end
Empty file removed test/controllers/.keep
View file
Open in desktop
Empty file.
8 changes: 3 additions & 5 deletions .../ruby_lsp_rails/models_controller_test.rb → test/lib/middleware_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,8 @@
module RubyLsp
module Rails
class ModelsControllerTest < ActionDispatch::IntegrationTest
T.unsafe(self).include(Engine.routes.url_helpers)

test "GET show returns column information for existing models" do
get model_url(model: "User")
get "/ruby_lsp_rails/models/User"
assert_response(:success)
assert_equal(
{
Expand All @@ -28,12 +26,12 @@ class ModelsControllerTest < ActionDispatch::IntegrationTest
end

test "GET show returns not_found if model doesn't exist" do
get model_url(model: "NonExistentModel")
get "/ruby_lsp_rails/models/Foo"
assert_response(:not_found)
end

test "GET show returns not_found if class is not a model" do
get model_url(model: "ApplicationJob")
get "/ruby_lsp_rails/models/ApplicationJob"
assert_response(:not_found)
end
end
Expand Down