Skip to content
This repository has been archived by the owner on Sep 13, 2017. It is now read-only.

Commit

Permalink
* lib/journey/formatter.rb: reject non-truthy parameters parts. Fixes…
Browse files Browse the repository at this point in the history 
… rails ticket #4587
  • Loading branch information
@tenderlove
tenderlove committed Feb 15, 2012
1 parent 01c50c6 commit 70d101b
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 1 deletion.
5 changes: 5 additions & 0 deletions CHANGELOG.rdoc
View file
@@ -1,3 +1,8 @@
Wed Feb 15 11:49:41 2012 Aaron Patterson <aaron@tenderlovemaking.com>

* lib/journey/formatter.rb: reject non-truthy parameters parts.
Fixes rails ticket #4587

Mon Jan 23 17:07:53 2012 Aaron Patterson <aaron@tenderlovemaking.com>

* Added symbol? and literal? predicate methods to the ast nodes for
Expand Down
2 changes: 1 addition & 1 deletion lib/journey/formatter.rb
View file
Expand Up @@ -104,7 +104,7 @@ def verify_required_parts! route, parts
if tests.key? key
/\A#{tests[key]}\Z/ === parts[key]
else
true
parts.fetch(key) { false }
end
}
end
Expand Down

23 comments on commit 70d101b

@jonnii
Copy link

@jonnii jonnii commented on 70d101b Feb 21, 2012

Choose a reason for hiding this comment

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

There's something in this commit that's breaking my routes. I have something like:

resources :sightings do
resources :responses
end

I'm not sure exactly how to write a failing test for journey, but I'll give it a shot.

@jonnii
Copy link

@jonnii jonnii commented on 70d101b Feb 21, 2012

Choose a reason for hiding this comment

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

Not sure if this helps, but the routes look in rake routes, and the link_to helper generates the right url, it's just that the request doesn't get routed to the controller.

@jaredbeck
Copy link

Choose a reason for hiding this comment

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

Do you get ActionController::RoutingError: No route matches? I was getting that after upgrading to journey 1.0.2. I was mistakenly passing nil arguments to route helpers.

@jonnii
Copy link

@jonnii jonnii commented on 70d101b Feb 21, 2012

Choose a reason for hiding this comment

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

I get:

No route matches {:action=>"show", :controller=>"responses", :sighting_id=>#<Sighting id: 101, ... other attributes go here some of which are nil... >}

@jaredbeck
Copy link

Choose a reason for hiding this comment

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

If I understand Aaron's commit message correctly, all of your falsey arguments will be ignored (nil is falsey).

@jonnii
Copy link

@jonnii jonnii commented on 70d101b Feb 21, 2012

Choose a reason for hiding this comment

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

I'm not sure what's going on, but what's weird is that it's using the instance of the model from the route... the urls generate just fine but when you actually follow the link you get the no route matches exception. Changing to something like = link_to 'foo', new_sighting_response(@sighting.id) doesn't fix it.

@pixeltrix
Copy link

Choose a reason for hiding this comment

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

@jonnii are you sure that the url gets generated correctly? The 'No route matches' with a model instance for :sighting_id seems to be a generation error - what happens if you try to generate the route in the console?

@jonnii
Copy link

@jonnii jonnii commented on 70d101b Feb 21, 2012

Choose a reason for hiding this comment

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

I'm absolutely sure. I'll test the route generation when I get home tonight.

@pixeltrix
Copy link

Choose a reason for hiding this comment

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

I can't seem to reproduce this - at @jaredbeck said the only way I can get this error is by passing nil to sighting_response_path, e.g:

<%= link_to 'Response', sighting_response_path(sighting, nil) %>

@jonnii
Copy link

@jonnii jonnii commented on 70d101b Feb 21, 2012

Choose a reason for hiding this comment

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

Like i said, that works fine. The problem is when you actually hit the link and you get a no route matches.

@pixeltrix
Copy link

Choose a reason for hiding this comment

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

Can you create a repo with a simple failing app - I've created one using the models above and it appears to work.

@tenderlove
Copy link
Member Author

Choose a reason for hiding this comment

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

@jonnii when you get a chance, can you post the routes you have, along with the URL you're hitting? Thanks.

@cgunther
Copy link

Choose a reason for hiding this comment

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

I'm seeing a similar regression after upgrading from v1.0.1 to v1.0.2, however it only affects my tests

ActionController::RoutingError:
No route matches {:action=>"show", :controller=>"accessory/items", :id=>#<Accessory::Item id: 1, ...>}

The line triggering that is:

redirect_to accessory_item_path(@item)

Clipped rake routes:

accessory_item GET    /accessory/items/:id(.:format)                                  accessory/items#show

Clipped routes.rb

namespace :accessory do
  resources :items, only: [:show, :new, :create, :edit, :update] do
    get 'activate', on: :member
  end
end

I have the #find calls stubbed out in this test to avoid hitting the database and it returns un-persisted model instances, which #to_param returns nil on.

I realize that my tests may have been falsely passing before this v1.0.2 update, but it is a regression nevertheless and wanted to get the official opinion on it.

@tenderlove
Copy link
Member Author

Choose a reason for hiding this comment

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

@cgunther did this route work on Rails 3.1?

I'm happy to revert this commit, but it did fix a regression from 3.1.x. 😢

@cgunther
Copy link

Choose a reason for hiding this comment

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

This app was on 3.2 from the start.

I wouldn't revert just on my case as only tests break for me, not the actual app code. I'll gladly accept that v1.0.2 is a bugfix and that my tests were falsely passing and need changing. I just wanted to toss in my experiences.

@pixeltrix
Copy link

Choose a reason for hiding this comment

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

@tenderlove I wouldn't revert - I'm pretty sure everyone of these errors is due to passing nil parameters to url helpers so all we are doing is exposing existing bugs which previously would've linked to an index page.

What's unhelpful though is that there's no backtrace to the error which makes it look as though it's a recognition error rather than a generation error - ideally it would show up as a TemplateError which would give some idea where the problem was.

@Dinuz
Copy link

@Dinuz Dinuz commented on 70d101b Feb 23, 2012

Choose a reason for hiding this comment

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

@pixeltrix We were chatting on I believe the same issue on another post (my mistake that I didn't look in this post before). I have the same identical error, and the error is not fixed avoiding to pass nil parameters to the url (the url is correctly generated, but when you hit it, then the no routes error come out - I ran rake routes, and my routes are right).

In my case this happen on models for which I didn't use the before_create method in the model (then empty models).

@cgunther
Copy link

Choose a reason for hiding this comment

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

@Dinuz are the models you're passing to the URL helper persisted, or are they unsaved objects?

That was the root of my error, I was passing unsaved models in my tests to the URL helpers, which when #to_param is called on them, returned nil.

@Dinuz
Copy link

@Dinuz Dinuz commented on 70d101b Feb 23, 2012

Choose a reason for hiding this comment

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

Good point....I solved the issue too, and just now I am reading your observation.

The models were unsaved object:))

I believe that the new version just exposed a bug in my code...and was hard to figure out where it was located, because in my case was in a partial of the header.

PS. My app is also in 3.2

@zacksiri
Copy link

Choose a reason for hiding this comment

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

yeah i am having the same issue.

@cgunther
Copy link

Choose a reason for hiding this comment

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

@artellectual are you sure you're not mistakenly passing nil or something that evaluates to nil to a URL helper somewhere?

Watch out for passing unsaved model objects to URL helpers, even though they are not nil themselves, calling #to_param on them returns nil which was the cause of my problems.

@Dinuz
Copy link

@Dinuz Dinuz commented on 70d101b Feb 24, 2012

Choose a reason for hiding this comment

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

@cgunther you got it!!! That was my problem, and I believe is the same happening to @artellectual

@zacksiri
Copy link

Choose a reason for hiding this comment

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

ok manage to resolve this issue as well, thx for the help!

Please sign in to comment.