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

Unexpected param parsing with optional url parameters #1363

Open
lvonk opened this Issue Nov 10, 2017 · 5 comments

Comments

Projects
None yet
2 participants
@lvonk
Contributor

lvonk commented Nov 10, 2017

While investigating #1361 I found the following which I think is a separate issue. Since this issue also occurs in Sinatra 1.4.8 I don't know if this is by design or not, but imho it leads to unexpected behaviour.

I think it boils down to the fact that Sinatra should let the most specific route match do the parameter parsing and use those parsed values for the filters (like before). Not doing so will cause possible different param values for the same param in filters (before) and the verbs (get, post etc)

Testcase:

require 'minitest/autorun'
require 'sinatra/base'
require 'rack/test'

VALUES = {}

class App < Sinatra::Base

  before '/foo/:bar/?:baz?' do
    VALUES[:before] = params[:baz]
  end

  get '/foo/:bar/?:baz?/1' do
    VALUES[:get] = params[:baz]
  end
end

class NestedNamespacesTest < Minitest::Test
  include Rack::Test::Methods

  def app
    App
  end

  def test_same_values
    get "/foo/2017/1"
    assert_equal VALUES[:before], VALUES[:get]
  end
end

This test will fail, meaning that in the before filter baz yields to 1 but in the actual route it is nil.

@namusyaka

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Nov 11, 2017

Member

First of all, you should use (/:bar)? as an optional parameter instead of :bar/?.
The captured name depends on the context. Therefore, I think the behavior is not a bug.

Member

namusyaka commented Nov 11, 2017

First of all, you should use (/:bar)? as an optional parameter instead of :bar/?.
The captured name depends on the context. Therefore, I think the behavior is not a bug.

@lvonk

This comment has been minimized.

Show comment
Hide comment
@lvonk

lvonk Nov 12, 2017

Contributor

Hi @namusyaka thanks for replying. The optional parameter is baz in the example, not bar. '/foo/:bar/?:baz?' (my apologies for the confusing names in the example). Both ways of defining an optional parameter seem to work the same /?:baz? versus (/?:baz)?. Changing to (/?:baz)? does not result in a different outcome.

The captured name depends on the context

I would argue the context is the most specific route. In fact that is also what the readme states (or at least that is how I interpret it):

Before filters are evaluated before each request within the same context as the routes will be and can modify the request and response.

Bug or not, IMHO it makes most sense that filters work from within that context. I can't think of a context or situation where I do want param values to differ between filter and route?

Contributor

lvonk commented Nov 12, 2017

Hi @namusyaka thanks for replying. The optional parameter is baz in the example, not bar. '/foo/:bar/?:baz?' (my apologies for the confusing names in the example). Both ways of defining an optional parameter seem to work the same /?:baz? versus (/?:baz)?. Changing to (/?:baz)? does not result in a different outcome.

The captured name depends on the context

I would argue the context is the most specific route. In fact that is also what the readme states (or at least that is how I interpret it):

Before filters are evaluated before each request within the same context as the routes will be and can modify the request and response.

Bug or not, IMHO it makes most sense that filters work from within that context. I can't think of a context or situation where I do want param values to differ between filter and route?

@namusyaka

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Nov 12, 2017

Member

The optional parameter is baz in the example, not bar. '/foo/:bar/?:baz?' (my apologies for the confusing names in the example). Both ways of defining an optional parameter seem to work the same /?:baz? versus (/?:baz)?. Changing to (/?:baz)? does not result in a different outcome.

Sorry for your confusing. I wanted to say that the optional parameter should be (/:foo)?, please see mustermann's readme.

Bug or not, IMHO it makes most sense that filters work from within that context. I can't think of a context or situation where I do want param values to differ between filter and route?

Actually the patterns in the route and the filter are not necessarily closely related.
This behavior is established.

Member

namusyaka commented Nov 12, 2017

The optional parameter is baz in the example, not bar. '/foo/:bar/?:baz?' (my apologies for the confusing names in the example). Both ways of defining an optional parameter seem to work the same /?:baz? versus (/?:baz)?. Changing to (/?:baz)? does not result in a different outcome.

Sorry for your confusing. I wanted to say that the optional parameter should be (/:foo)?, please see mustermann's readme.

Bug or not, IMHO it makes most sense that filters work from within that context. I can't think of a context or situation where I do want param values to differ between filter and route?

Actually the patterns in the route and the filter are not necessarily closely related.
This behavior is established.

@lvonk

This comment has been minimized.

Show comment
Hide comment
@lvonk

lvonk Nov 12, 2017

Contributor

Actually the patterns in the route and the filter are not necessarily closely related.
This behavior is established.

Hmm okay. I did not consider that. It still feels strange though to have a param's value differ in a filter and route in a single request...

Well anyway if this is the way it should work, than indeed it is not a bug. Thanks for taking the time to look into this! Feel free to close this issue if you consider it handled.

Contributor

lvonk commented Nov 12, 2017

Actually the patterns in the route and the filter are not necessarily closely related.
This behavior is established.

Hmm okay. I did not consider that. It still feels strange though to have a param's value differ in a filter and route in a single request...

Well anyway if this is the way it should work, than indeed it is not a bug. Thanks for taking the time to look into this! Feel free to close this issue if you consider it handled.

@namusyaka

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Nov 12, 2017

Member

I think this is not a bug, but I'd like to keep this issue for discussing the API-design.
@lvonk Thanks for your report.

cc @zzak

Member

namusyaka commented Nov 12, 2017

I think this is not a bug, but I'd like to keep this issue for discussing the API-design.
@lvonk Thanks for your report.

cc @zzak

@namusyaka namusyaka added this to the Beyond milestone Feb 6, 2018

@namusyaka namusyaka modified the milestones: Beyond, v2.1.0 Feb 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment