grape icon indicating copy to clipboard operation
grape copied to clipboard

After 1.5.0 , 'route_setting :custom , key: :value' is broken with 'mount'.

Open neocoin opened this issue 4 years ago • 5 comments

env

Ruby 3.0.0
Grape 1.5.3
Rails 6.1
      def route_setting(key, value = nil)
        get_or_set :route, key, value
      end

'value' work like frozen hash key. You can reproduce below code.


module V1
  class Status < Grape::API
    desc 'time'
    route_setting :custom, key: :value
    get '/status/time' do
      { time: Time.zone.now }
    end

    desc 'time2'
    route_setting :custom, key: :value
    get '/status/time2' do
      { time: Time.zone.now }
    end
  end
  
  class Mount < Grape::API
    mount ::V1::Status
  end
end

Second , 'route_setting' method calling doesn't work.

[1] pry(main)> ap V1::Status.routes.map{ [_1.path, _1.settings]}
[
    [0] [
        [0] "/:version/status/time(.json)",
        [1] {
            :description => {
                :description => "time"
            },
                 :custom => {
                :key => :value
            }
        }
    ],
    [1] [
        [0] "/:version/status/time2(.json)",
        [1] {
            :description => {
                :description => "time2"
            }
        }
    ]
]
=> nil

Anybody, can explain this situation?

route_setting work fine at ~> 1.4.0 on ruby 2.7

neocoin avatar Apr 06 '21 01:04 neocoin

Probably a bug. Turn it into a failing spec and PR that?

dblock avatar Apr 06 '21 15:04 dblock

I write spec and pr on #2174 . But I don't know a solution yet.

neocoin avatar Apr 06 '21 17:04 neocoin

@dblock

I find problem point and solve with rollback that line. #2174

Fix retaining setup blocks when remounting APIs 416a7e15 Sep 18, 2020

https://github.com/ruby-grape/grape/commit/416a7e15bdfda29cd4f9b335a911bb59a416be60#diff-74e08e6432db25a22b629a2f8031d24d01f8c9435bda532d36e22971d0b1c9a0R33

I give up performance during init. check plz @jylamont

neocoin avatar Apr 16 '21 00:04 neocoin

I was about to open an issue about it, I even created a file to test it.

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "grape", "1.5.3"
end

require "grape"

class BaseAPI < Grape::API
end

class SomeAPI < Grape::API
  route_setting :auth, public: true
  get '/a' do
  end

  route_setting :auth, public: true
  get '/b' do
  end
end

raise 'ERROR ON /a' if SomeAPI.routes.first.settings[:auth] != { public: true }
raise 'ERROR ON /b' if SomeAPI.routes.last.settings[:auth] != { public: true }

BaseAPI.mount SomeAPI

raise 'ERROR ON /a after mount' if SomeAPI.routes.first.settings[:auth] != { public: true }
raise 'ERROR ON /b after mount' if SomeAPI.routes.last.settings[:auth] != { public: true }

stephannv avatar Jun 02 '21 18:06 stephannv

I was about to open an issue about this as well...

I looked around the Grape code and I'm not sure how to actually fix the issue. However, I noticed that adding an empty namespace block actually fixes the issue. If someone wants to create a potential fix then perhaps answering why it works within a namespace might be a good starting point.

Slightly modified reproduction of @stephannv, with grape 1.6.2 and an empty namespace block works:

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "grape", "1.6.2"
end

require "grape"

class BaseAPI < Grape::API
end

class SomeAPI < Grape::API
  route_setting :auth, public: true
  namespace do
    get '/a' do
    end

    route_setting :auth, public: true
    get '/b' do
    end
  end
end

raise 'ERROR ON /a' if SomeAPI.routes.first.settings[:auth] != { public: true }
raise 'ERROR ON /b' if SomeAPI.routes.last.settings[:auth] != { public: true }

BaseAPI.mount SomeAPI

raise 'ERROR ON /a after mount' if SomeAPI.routes.first.settings[:auth] != { public: true }
raise 'ERROR ON /b after mount' if SomeAPI.routes.last.settings[:auth] != { public: true }

madejejej avatar Dec 09 '22 15:12 madejejej