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

route params changing after saving file #631

Closed
mwmitchell opened this Issue Aug 10, 2011 · 25 comments

Comments

Projects
None yet
6 participants
@mwmitchell
Contributor

mwmitchell commented Aug 10, 2011

I have this:

get "/page/:page" do
params.inspect
end

I start up padrino via "padrino start", then hit this in my browser:

http://localhost:3000/page/2

I get this:

{:page=>"2"}

If I go back to the file, and change it to this:

get "/page/:page" do

params.inspect
end

(notice, I'm only adding a space before the params.inspect) - then hit the browser again at http://localhost:3000/page/2, I get this:

{:page=>["page", "2"]}

This seems like a code reloading issue?

@ghost ghost assigned joshbuddy Aug 10, 2011

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Aug 10, 2011

Member

Thanks for the report, we are working on fixing this soon and pushing a patch release.

Member

nesquena commented Aug 10, 2011

Thanks for the report, we are working on fixing this soon and pushing a patch release.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Aug 10, 2011

Member

You can always disable reload in the interim if its blocking development

Member

nesquena commented Aug 10, 2011

You can always disable reload in the interim if its blocking development

@joshbuddy

This comment has been minimized.

Show comment
Hide comment
@joshbuddy

joshbuddy Aug 11, 2011

Contributor

I've added a test for this: 4d7069e

Contributor

joshbuddy commented Aug 11, 2011

I've added a test for this: 4d7069e

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Aug 11, 2011

Member

Thanks, does the test pass for both of these? Why is this issue coming up? Is it the development reloader @DAddYE. Perhaps we should make the change soon to fork_reloader by default for non windows? (and extract current reloader to a gem)

Member

nesquena commented Aug 11, 2011

Thanks, does the test pass for both of these? Why is this issue coming up? Is it the development reloader @DAddYE. Perhaps we should make the change soon to fork_reloader by default for non windows? (and extract current reloader to a gem)

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Aug 12, 2011

Member

I think we reload a class that don't need that.

Those that have this problem can gist me:

puts Padrino::Reloader.exclude_constants
puts Padrino::Reloader.include_constants

Add these lines in your controller and copy the result only when reloading was wrong.

Thanks so much!

Member

DAddYE commented Aug 12, 2011

I think we reload a class that don't need that.

Those that have this problem can gist me:

puts Padrino::Reloader.exclude_constants
puts Padrino::Reloader.include_constants

Add these lines in your controller and copy the result only when reloading was wrong.

Thanks so much!

@minikomi

This comment has been minimized.

Show comment
Hide comment
@minikomi

minikomi Aug 16, 2011

Contributor

https://gist.github.com/1148389 Here's mine.. happens after saving, same pattern.. becomes nested array in hash rather than straight hash.

Contributor

minikomi commented Aug 16, 2011

https://gist.github.com/1148389 Here's mine.. happens after saving, same pattern.. becomes nested array in hash rather than straight hash.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Aug 16, 2011

Member

Interesting, out of curiosity I wonder if this was introduced into 0.10.1 or if has been around since 0.10?

Member

nesquena commented Aug 16, 2011

Interesting, out of curiosity I wonder if this was introduced into 0.10.1 or if has been around since 0.10?

@minikomi

This comment has been minimized.

Show comment
Hide comment
@minikomi

minikomi Aug 16, 2011

Contributor

From what I can tell it snuck in with 10.1... maybe the difference is in http_router versions?

Contributor

minikomi commented Aug 16, 2011

From what I can tell it snuck in with 10.1... maybe the difference is in http_router versions?

@minikomi

This comment has been minimized.

Show comment
Hide comment
@minikomi

minikomi Aug 19, 2011

Contributor

For what it's worth, editing helpers also causes this to happen.

Contributor

minikomi commented Aug 19, 2011

For what it's worth, editing helpers also causes this to happen.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Aug 19, 2011

Member

Yeah the reloader didn't change much in this version. I wonder what caused this to appear... @DAddYE @joshbuddy

Member

nesquena commented Aug 19, 2011

Yeah the reloader didn't change much in this version. I wonder what caused this to appear... @DAddYE @joshbuddy

@minikomi

This comment has been minimized.

Show comment
Hide comment
@minikomi

minikomi Aug 19, 2011

Contributor

Could it be:

Gem padrino-core-0.10.0: http_router (~> 0.8.10)
Gem padrino-core-0.10.1: http_router (~> 0.9.4)

Edit: No, I just rolled back the gem version used by core to try it.. still get the same error.

Contributor

minikomi commented Aug 19, 2011

Could it be:

Gem padrino-core-0.10.0: http_router (~> 0.8.10)
Gem padrino-core-0.10.1: http_router (~> 0.9.4)

Edit: No, I just rolled back the gem version used by core to try it.. still get the same error.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Aug 19, 2011

Member

Interesting, so you don't get this in padrino 0.10.0. But you do get this in 0.10.1 using http_router 0.8.10? Is that right?

Member

nesquena commented Aug 19, 2011

Interesting, so you don't get this in padrino 0.10.0. But you do get this in 0.10.1 using http_router 0.8.10? Is that right?

@minikomi

This comment has been minimized.

Show comment
Hide comment
@minikomi

minikomi Aug 19, 2011

Contributor

That's right.

Contributor

minikomi commented Aug 19, 2011

That's right.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Aug 19, 2011

Member

Ok thanks for experimenting and reporting back. So it isn't changes to http_router. It must be something between 0.10.0 and 0.10.1...

Member

nesquena commented Aug 19, 2011

Ok thanks for experimenting and reporting back. So it isn't changes to http_router. It must be something between 0.10.0 and 0.10.1...

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Aug 19, 2011

Member

0.10.0...0.10.1 is a comprehensive diff. Was trying to spot changes to the reloader...so far no luck (perhaps 0.10.0...0.10.1diff-21)

Member

nesquena commented Aug 19, 2011

0.10.0...0.10.1 is a comprehensive diff. Was trying to spot changes to the reloader...so far no luck (perhaps 0.10.0...0.10.1diff-21)

@minikomi

This comment has been minimized.

Show comment
Hide comment
@minikomi

minikomi Aug 19, 2011

Contributor

How about in the routing.rb in core/application?

Contributor

minikomi commented Aug 19, 2011

How about in the routing.rb in core/application?

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Aug 19, 2011

Member

Yeah that's by far the most likely culprit although not clear to me what about it would introduce this behavior...

Member

nesquena commented Aug 19, 2011

Yeah that's by far the most likely culprit although not clear to me what about it would introduce this behavior...

@minikomi

This comment has been minimized.

Show comment
Hide comment
@minikomi

minikomi Aug 19, 2011

Contributor

I'm going to step back through versions to see where the bug came in..

Contributor

minikomi commented Aug 19, 2011

I'm going to step back through versions to see where the bug came in..

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Aug 19, 2011

Member

Awesome man, I really appreciate you helping us investigate this

Member

nesquena commented Aug 19, 2011

Awesome man, I really appreciate you helping us investigate this

@minikomi

This comment has been minimized.

Show comment
Hide comment
@minikomi

minikomi Aug 19, 2011

Contributor

Oops.. made a mistake sorry, when called with a "Sinatra style" route, it didn't work.. back to square one.. :(

Contributor

minikomi commented Aug 19, 2011

Oops.. made a mistake sorry, when called with a "Sinatra style" route, it didn't work.. back to square one.. :(

@minikomi

This comment has been minimized.

Show comment
Hide comment
@minikomi

minikomi Aug 19, 2011

Contributor

It's changing before it enters the process_destination_path(path, env) method.. this sets @params to env['router.params'], but outputting env['router.params'] right at the start of this method still shows the change from object to object with nested array..

Edit: Spent a good amount of time trying to find what was triggering the change to no avail.. sorry. Whatever is responsible for generating request.params seems to be the place to look..

Contributor

minikomi commented Aug 19, 2011

It's changing before it enters the process_destination_path(path, env) method.. this sets @params to env['router.params'], but outputting env['router.params'] right at the start of this method still shows the change from object to object with nested array..

Edit: Spent a good amount of time trying to find what was triggering the change to no avail.. sorry. Whatever is responsible for generating request.params seems to be the place to look..

@chromaticbum

This comment has been minimized.

Show comment
Hide comment
@chromaticbum

chromaticbum Aug 30, 2011

Will fork and send pull request soon.

Issue was caused by matchers being added to http_routing in a different order at reload time than at load time.
This could be an issue with http_router gem also, the culprit line is this in HttpRouter::Node::Glob
"request.params << (globbed_params#{id} = [])

this turns request.params into an array with an array inside of it.
This line is never reached if the matchers are added in the order they are added at load time. The lookup node is reached instead, which seems to handle params properly.

Fix is to make reload! load the user-defined routes before the default routes so that the lookup_node code is reached before the glob code.

Change

  def reload!
    logger.devel "Reloading #{self}"
    @_dependencies = nil # Reset dependencies
    reset! # Reset sinatra app
    reset_routes! # Remove all existing user-defined application routes
    Padrino.require_dependencies(self.app_file, :force => true) # Reload the app file
    require_dependencies # Reload dependencies
    register_initializers # Reload our middlewares
    default_filters! # Reload filters
    default_errors!  # Reload our errors
    I18n.reload! if defined?(I18n) # Reload also our translations
  end

to

def reload!
logger.devel "Reloading #{self}"
@_dependencies = nil # Reset dependencies
reset! # Reset sinatra app
reset_router! # Remove all existing user-defined application routes
Padrino.require_dependencies(self.app_file, :force => true) # Reload the app file
require_dependencies # Reload dependencies
default_routes!
register_initializers # Reload our middlewares
default_filters! # Reload filters
default_errors! # Reload our errors
I18n.reload! if defined?(I18n) # Reload also our translations
end

chromaticbum commented Aug 30, 2011

Will fork and send pull request soon.

Issue was caused by matchers being added to http_routing in a different order at reload time than at load time.
This could be an issue with http_router gem also, the culprit line is this in HttpRouter::Node::Glob
"request.params << (globbed_params#{id} = [])

this turns request.params into an array with an array inside of it.
This line is never reached if the matchers are added in the order they are added at load time. The lookup node is reached instead, which seems to handle params properly.

Fix is to make reload! load the user-defined routes before the default routes so that the lookup_node code is reached before the glob code.

Change

  def reload!
    logger.devel "Reloading #{self}"
    @_dependencies = nil # Reset dependencies
    reset! # Reset sinatra app
    reset_routes! # Remove all existing user-defined application routes
    Padrino.require_dependencies(self.app_file, :force => true) # Reload the app file
    require_dependencies # Reload dependencies
    register_initializers # Reload our middlewares
    default_filters! # Reload filters
    default_errors!  # Reload our errors
    I18n.reload! if defined?(I18n) # Reload also our translations
  end

to

def reload!
logger.devel "Reloading #{self}"
@_dependencies = nil # Reset dependencies
reset! # Reset sinatra app
reset_router! # Remove all existing user-defined application routes
Padrino.require_dependencies(self.app_file, :force => true) # Reload the app file
require_dependencies # Reload dependencies
default_routes!
register_initializers # Reload our middlewares
default_filters! # Reload filters
default_errors! # Reload our errors
I18n.reload! if defined?(I18n) # Reload also our translations
end

chromaticbum pushed a commit to chromaticbum/padrino-framework that referenced this issue Aug 30, 2011

DAddYE added a commit that referenced this issue Aug 30, 2011

chromaticbum pushed a commit to chromaticbum/padrino-framework that referenced this issue Aug 30, 2011

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Aug 30, 2011

Member

Can you guys confirm me that this problem gone away? If not reopen! Thanks

Member

DAddYE commented Aug 30, 2011

Can you guys confirm me that this problem gone away? If not reopen! Thanks

@DAddYE DAddYE closed this Aug 30, 2011

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Aug 30, 2011

Member

Obviously thanks so so much to @chromaticbum, awesome work!

Member

DAddYE commented Aug 30, 2011

Obviously thanks so so much to @chromaticbum, awesome work!

@minikomi

This comment has been minimized.

Show comment
Hide comment
@minikomi

minikomi Aug 31, 2011

Contributor

Great work @chromaticbum. Thanks a bunch!

Contributor

minikomi commented Aug 31, 2011

Great work @chromaticbum. Thanks a bunch!

xavierRiley added a commit to xavierRiley/padrino-framework that referenced this issue Sep 5, 2011

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