Skip to content

Implemented 405 response instead of 404, resolves #25. #37

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 8 additions & 1 deletion lib/flame/dispatcher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,13 @@ def config
@app_class.config
end

## Available routes endpoint
def available_endpoint
return @available_endpoint if defined? @available_endpoint
@available_endpoint =
@app_class.router.routes.endpoint(*request.path.parts)
end

## Build a path to the given controller and action, with any expected params
##
## @param ctrl [Flame::Controller] class of controller
Expand Down Expand Up @@ -155,7 +162,7 @@ def cached_tilts
## Return response if HTTP-method is OPTIONS
def try_options
return unless request.http_method == :OPTIONS
allow = @app_class.router.routes.dig(request.path)&.allow
allow = available_endpoint&.allow
halt 404 unless allow
response.headers['Allow'] = allow
end
Expand Down
9 changes: 6 additions & 3 deletions lib/flame/dispatcher/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,13 @@ module Routes

## Find route and try execute it
def try_route
route = @app_class.router.find_route(request.path, request.http_method)
return nil unless route
http_method = request.http_method
http_method = :GET if http_method == :HEAD
return nil unless available_endpoint
route = available_endpoint[http_method]
halt(405, nil, 'Allow' => available_endpoint.allow) unless route
status 200
execute_route(route)
execute_route route
true
end

Expand Down
2 changes: 1 addition & 1 deletion lib/flame/path.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ class Path
## @param parts [Array<String, Flame::Path>] parts of expected path
## @return [Flame::Path] path from parts
def self.merge(*parts)
parts.join('/').gsub(%r{\/{2,}}, '/')
parts.join('/').gsub(%r|/{2,}|, '/')
end

def initialize(*paths)
Expand Down
22 changes: 3 additions & 19 deletions lib/flame/router.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,30 +36,14 @@ def add_controller(ctrl, path = nil, &block)

using GorillaPatch::DigEmpty

## Find route by any attributes
## @param path [Flame::Path, String] path for route search
## @param http_method [Symbol, nil] HTTP-method
## @return [Flame::Route, nil] return the found route, otherwise `nil`
def find_route(path, http_method = nil)
path = Flame::Path.new(path) unless path.is_a?(Flame::Path)
endpoint = routes.dig(*path.parts)&.dig_through_opt_args
return unless endpoint
http_method = :GET if http_method == :HEAD
## For `find_nearest_route` with any method
route = http_method ? endpoint[http_method] : endpoint.values.first
route if route.is_a? Flame::Router::Route
end

## Find the nearest route by path
## @param path [Flame::Path] path for route finding
## @return [Flame::Route, nil] return the found nearest route or `nil`
def find_nearest_route(path)
path = Flame::Path.new(path) if path.is_a? String
path_parts = path.parts.dup
path_parts = Flame::Path.new(path).parts.dup
while path_parts.size >= 0
route = find_route Flame::Path.new(*path_parts)
break if route || path_parts.empty?
path_parts.pop
route = routes.endpoint(*path_parts)&.values&.first
break if route&.is_a?(Flame::Router::Route) || path_parts.pop.nil?
end
route.is_a?(Flame::Router::Routes) ? route[nil] : route
end
Expand Down
6 changes: 6 additions & 0 deletions lib/flame/router/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,12 @@ def allow
methods.push(:OPTIONS).join(', ')
end

## Move like '#dig' with '#dig_through_opt_args'
## @param path_parts [Array<String, Flame::Path, Flame::Path::Part>]
def endpoint(*path_parts)
dig(*path_parts)&.dig_through_opt_args
end

private

%i[req opt].each do |type|
Expand Down
16 changes: 16 additions & 0 deletions spec/unit/dispatcher_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ def hello(name)
"Hello, #{name}!"
end

def baz(var = nil)
"Hello, #{var}!"
end

def test
'Route content'
end
Expand Down Expand Up @@ -128,6 +132,12 @@ class DispatcherApplication < Flame::Application
respond.body.should.equal []
end

it 'should return 405 for not allowed HTTP-method with Allow header' do
respond = @init.call(method: 'POST').run!.last
respond.headers['Allow'].should.equal 'GET, OPTIONS'
respond.status.should.equal 405
end

describe 'OPTIONS HTTP-method' do
before do
@respond = @init.call(method: 'OPTIONS').run!.last
Expand Down Expand Up @@ -158,6 +168,12 @@ class DispatcherApplication < Flame::Application
respond = dispatcher.run!.last
respond.headers.key?('Allow').should.be.false
end

should 'return `Allow` header for route with optional parameters' do
dispatcher = @init.call(method: 'OPTIONS', path: '/baz')
respond = dispatcher.run!.last
respond.headers.key?('Allow').should.be.true
end
end
end

Expand Down
14 changes: 14 additions & 0 deletions spec/unit/router/routes_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -102,4 +102,18 @@
@routes['foo']['bar'].allow.should.be.nil
end
end

describe '#endpoint' do
should 'return nested routes from path' do
routes = @init.call(['/foo', '/:?var', '/bar'])
routes.endpoint('/foo').should.equal routes['foo'][':?var']
routes.endpoint('/foo/some').should.equal routes['foo'][':?var']
routes.endpoint('/foo/some/bar')
.should.equal routes['foo'][':?var']['bar']
end

should 'return nil for not-existing path' do
@routes.endpoint('/foo/baz').should.be.nil
end
end
end
52 changes: 0 additions & 52 deletions spec/unit/router_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -266,58 +266,6 @@ class RouterApplication < Flame::Application
end
end

describe '#find_route' do
it 'should receive path as String' do
@router.add_controller RouterRESTController
-> { @router.find_route('/router_rest/42', :PUT) }
.should.not.raise(NoMethodError)
end

it 'should return route by path and HTTP-method' do
@router.add_controller RouterRESTController
@router.find_route('/router_rest/42', :PUT)
.should.equal Flame::Router::Route.new(
RouterRESTController, :update
)
end

it 'should return route by path without optional argument' do
@router.add_controller RouterController
@router.find_route('/router/foo/bar/baz', :GET)
.should.equal Flame::Router::Route.new(
RouterController, :foo
)
end

it 'should return route by path without multiple optional arguments' do
@router.add_controller RouterController
@router.find_route('/router/foo/bar/baz', :GET)
.should.equal Flame::Router::Route.new(
RouterController, :foo
)
end

it 'should return route by HEAD HTTP-request instead of GET' do
@router.add_controller RouterRESTController
@router.find_route('/router_rest', :HEAD)
.should.equal Flame::Router::Route.new(
RouterRESTController, :index
)
end

it 'should return nil for not existing route' do
@router.add_controller RouterRESTController
@router.find_route(action: :foo)
.should.equal nil
end

it 'should return nil by path without required argument' do
@router.add_controller RouterController
@router.find_route(path: '/router/foo/bar')
.should.equal nil
end
end

Copy link
Owner

Choose a reason for hiding this comment

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

Add tests for Routes#endpoint, please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

describe '#find_nearest_route' do
it 'should return route by path' do
@router.add_controller RouterController
Expand Down