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.

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

Jump to bottom

@params are reset to pre-filter state if there are parameters in the filter's path pattern #1567

Closed
DannyBen opened this issue Sep 12, 2019 · 2 comments · Fixed by #1569
Closed

@params are reset to pre-filter state if there are parameters in the filter's path pattern #1567

DannyBen opened this issue Sep 12, 2019 · 2 comments · Fixed by #1569
Labels
bug

Comments

@DannyBen
Copy link

Hello,

I am fairly sure this is a bug and not an intended behavior. I may be wrong though.

When amending @params from before blocks, a different behavior is observed when the before filter uses parameters in the path.

This was initially discussed in this StackOverflow question.

Reproduction steps

Create test.rb

# test.rb
require 'sinatra'
require 'sinatra/reloader'

set :bind, '0.0.0.0'
set :port, 3000

before            { logger.info "before 1" ; @params['before-1'] = 'yes' }
before('/hi')     { logger.info "before 2" ; @params['before-2'] = 'yes' }
before('/:id/hi') { logger.info "before 3" ; @params['before-3'] = 'yes' }

get('/')       { params.to_json }  # pass
get('/hi')     { params.to_json }  # pass
get('/:id/hi') { params.to_json }  # FAIL

Run it:

$ ruby test.rb

Then access it with these three calls:

$ curl localhost:3000/
$ curl localhost:3000/hi
$ curl localhost:3000/123/hi

Actual behavior

The before filters append values to the @params hash properly, except in the last case:

$ curl localhost:3000/
{"before-1":"yes"}

$ curl localhost:3000/hi
{"before-1":"yes","before-2":"yes"}

$ curl localhost:3000/123/hi
{"before-1":"yes","id":"123"}

Expected behavior

Expecting to have the third before filter work as well, and set the value for @params['before-3'].

$ curl localhost:3000/123/hi
{"before-1":"yes","id":"123", "before-3":"yes"}

Additional information

As was pointed out in Amadan's answer on StackOverflow, the two places in the code that play a part in this are:

sinatra/lib/sinatra/base.rb

Line 1022 in b008189

original, @params = @params, @params.merge(params) if params.any?

and

sinatra/lib/sinatra/base.rb

Line 1041 in b008189

@params = original if original

@jkowens
Copy link
Member

jkowens commented Sep 13, 2019

I agree, this looks like a bug. We need to remove routing params from the @params hash without losing other key/value pairs that could have been added.

@jkowens jkowens added the bug label Sep 13, 2019
@jkowens jkowens mentioned this issue Oct 6, 2019
Merged
@horaciob horaciob mentioned this issue Oct 26, 2019
Merged
@horaciob
Copy link
Contributor

horaciob commented Oct 26, 2019 •
edited
Loading

Hi there I've added a test for this scenario, @jkowens PR makes test run successfully.

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Mar 20, 2020
www/ruby-sinatra: update to 2.0.8.1
610f760 
Update ruby-sinatra to 2.0.8.1.


## 2.0.8.1 / 2020-01-02

* Allow multiple hashes to be passed in `merge` and `merge!` for `Sinatra::IndifferentHash` [#1572](sinatra/sinatra#1572) by Shota Iguchi

## 2.0.8 / 2020-01-01

* Lookup Tilt class for template engine without loading files [#1558](sinatra/sinatra#1558). Fixes [#1172](sinatra/sinatra#1172) by Jordan Owens

* Add request info in NotFound exception [#1566](sinatra/sinatra#1566) by Stefan Sundin

* Add `.yaml` support in `Sinatra::Contrib::ConfigFile` [#1564](sinatra/sinatra#1564). Fixes [#1563](sinatra/sinatra#1563) by Emerson Manabu Araki

* Remove only routing parameters from @params hash [#1569](sinatra/sinatra#1569). Fixes [#1567](sinatra/sinatra#1567) by Jordan Owens, Horacio

* Support `capture` and `content_for` with Hamlit [#1580](sinatra/sinatra#1580) by Takashi Kokubun

* Eliminate warnings of keyword parameter for Ruby 2.7.0 [#1581](sinatra/sinatra#1581) by Osamtimizer

## 2.0.7 / 2019-08-22

* Fix a regression [#1560](sinatra/sinatra#1560) by Kunpei Sakai

## 2.0.6 / 2019-08-21

* Fix an issue setting environment from command line option [#1547](sinatra/sinatra#1547), [#1554](sinatra/sinatra#1554) by Jordan Owens, Kunpei Sakai

* Support pandoc as a new markdown renderer [#1533](sinatra/sinatra#1533) by Vasiliy

* Remove outdated code for tilt 1.x [#1532](sinatra/sinatra#1532) by Vasiliy

* Remove an extra logic for `force_encoding` [#1527](sinatra/sinatra#1527) by Jordan Owens

* Avoid multiple errors even if `params` contains special values [#1526](sinatra/sinatra#1527) by Kunpei Sakai

* Support `bundler/inline` with `require 'sinatra'` integration [#1520](sinatra/sinatra#1520) by Kunpei Sakai

* Avoid `TypeError` when params contain a key without a value on Ruby < 2.4 [#1516](sinatra/sinatra#1516) by Samuel Giddins

* Improve development support and documentation and source code by  Olle Jonsson, Basavanagowda Kanur, Yuki MINAMIYA
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove only routing parameters from @params hash jkowens/sinatra
3 participants
@horaciob @jkowens @DannyBen