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

The purpose of hanami in this gem? #188

Closed
pat opened this issue Aug 10, 2023 · 10 comments
Closed

The purpose of hanami in this gem? #188

pat opened this issue Aug 10, 2023 · 10 comments

Comments

@pat
Copy link

pat commented Aug 10, 2023

Hi there 👋🏻

We're considering using openapi_first in a Rails app - particularly given the support for OpenAPI v3.1 - but I'm wondering why hanami-router is a dependency for the validation process? To be clear: I'm a fan of hanami, and use it in other projects - it's just that I don't want to bring in a tonne of new indirect dependencies for such a focused need, and I don't immediately grok the purpose of it here. If you have the time to enlighten me, that'd be much appreciated :)

@ahx
Copy link
Owner

ahx commented Sep 19, 2023

Hi @pat. Sorry for getting back to you so late. I did not see the message :/.

This is a very good question. openapi_first uses hanami-router for find the OpenApi operation for the current request. hanami-router implements a trie algorithm which makes this super efficient.
Please don't hesitate to raise any concerns or general feedback here if you have any thoughts about this.

About OpenAPI 3.1 support: json_schemer supports OpenAPI 3.0 and 3.1 since version 2.0 and I am about to update openapi_first to use that version. So we should be able to support all nuances of 3.1

@SalvatoreT
Copy link

I'm also struggling with all the dependencies pulled in just for the one usage of hanami-router. I think it should be possible to remove the dependency.

I started on a spike but ran out of time to have it handle the 404/405 and other non-happy cases.

diff --git a/lib/openapi_first/router.rb b/lib/openapi_first/router.rb
index 6da7c44..26f6dcc 100644
--- a/lib/openapi_first/router.rb
+++ b/lib/openapi_first/router.rb
@@ -2,7 +2,6 @@
 
 require 'rack'
 require 'multi_json'
-require 'hanami/router'
 require_relative 'body_parser_middleware'
 
 module OpenapiFirst
@@ -77,23 +76,31 @@ module OpenapiFirst
     end
 
     def build_router(operations)
-      router = Hanami::Router.new.tap do |r|
-        operations.each do |operation|
-          normalized_path = operation.path.gsub('{', ':').gsub('}', '')
-          r.public_send(
-            operation.method,
-            normalized_path,
-            to: build_route(operation)
-          )
-        end
+      @routes = {}
+      operations.each do |operation|
+        normalized_path = operation.path.gsub('{', ':').gsub('}', '')
+        @routes[[operation.method.downcase.to_sym, normalized_path]] = build_route(operation)
       end
+
+      route_request_lambda = lambda { |env| route_request(env) }
+
       raise_error = @raise
       Rack::Builder.app do
-        use(BodyParserMiddleware, raise_error:)
-        run router
+        use(BodyParserMiddleware, raise_error: raise_error)
+        run route_request_lambda
       end
     end
 
+    def route_request(env)
+      req = Rack::Request.new(env)
+      key = [req.request_method.downcase.to_sym, req.path]
+      route = @routes[key]
+      return route.call(env) if route
+
+      raise_error(env) if @raise
+      @app.call(env) if @not_found == :continue
+    end
+
     def build_route(operation)
       lambda do |env|
         env[OPERATION] = operation

@ahx
Copy link
Owner

ahx commented Oct 27, 2023

@SalvatoreT I have not tried the patch yet, but that should not work for paths with path parameters, or does it?
Having a request path like /pets/42/food should match a /pets/{id}/food.

@SalvatoreT
Copy link

Probably not! It's half baked, for sure. I'm more trying to point towards a direction than land a specific implementation.

@ahx
Copy link
Owner

ahx commented Oct 27, 2023

Thanks for the reply @SalvatoreT. I will try to find a solution that does not need Hanami-router @pat, but using a trie algorithm like hanami-router does makes sense to me.
I will trie try to come up with a simpler (naive) version and try to measure how that impacts performance.

@ahx
Copy link
Owner

ahx commented Nov 14, 2023

I will remove hanami-router in #197

@ahx ahx closed this as completed Nov 14, 2023
@pat
Copy link
Author

pat commented Nov 15, 2023

Thanks @ahx, greatly appreciate you going to the effort of reworking things!

@SalvatoreT
Copy link

Yes, thank you!

@ahx
Copy link
Owner

ahx commented Jan 8, 2024

I have released 1.0 the other day and things got much better. hanami-router was removed and also Router is gone. I would love to get your feedback on this if you find the time.

@SalvatoreT
Copy link

I just started leave, so I won't be able to take a swing for a bit. 😓

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

No branches or pull requests

3 participants