Journey scanner fails to recognise sub-delims pchars

[pixeltrix]

The scanner in Journey fails to recognise routes that use literals from the sub-delims section of RFC 3986

# config/routes.rb
get '/boom!', to: 'boom#bang' as: :boom_bang

# app/controllers/boom_controller.rb
class BoomController < ApplicationController
  def bang; render text: 'bang!'; end
end

>> app.get('/boom!')
=> 404

The simple solution is to add the additional chars to the regexp in the scanner but there's the issue that : and () are in the sub-delims so we should probably allow their use via escaping and to do that I think we need to change the behaviour of a :LITERAL node.

@tenderlove any comments/suggestions?

[tenderlove]

The simple solution is to add the additional chars to the regexp in the scanner but there's the issue that : and () are in the sub-delims so we should probably allow their use via escaping and to do that I think we need to change the behaviour of a :LITERAL node.

Yep. We just need to change the scanner.

[Bounga]

Just created a PR to fix this issue in Journey. The "(" and ")" cases are not handled since I was not sure of how to do it nicely.

[Bounga]

PR now provide compatibility with all sub-delims listed in RFC

[heathd]

Hello,

we have noticed that this change has lead to a change in behaviour of routing. It's a bit of an edge case which wasn't documented, so I'm just noting this here in case others encounter the same issue.

in rails 4.1.11 the route '/page*foo' is tokenised as [[:SLASH, "/"], [:LITERAL, "page"], [:STAR, "*foo”]]
wheras in rails 4.2.3 the same route is tokenised as [[:SLASH, '/'], [:LITERAL, 'page*foo']]

Therefore it's no longer possible to use the 'wildcard segment' matching except at the start of a url or a / path separator.

We are relying on this behaviour in a current rails 4.1.11 app, you can see the routing code here https://github.com/alphagov/content-store/blob/master/config/routes.rb

Since this functionality was undocumented, I don't feel like this should be considered a regression.

However, I wonder if it would be valuable to characterise this behaviour in a test case. I'd be happy to submit a PR for that if others think it would be a good idea?

Thanks

David

[pixeltrix]

@heathd no, I'd still consider this a regression how ever much of an edge case this is - I'll have a look at it.

[heathd]

@pixeltrix ok thanks