Setting POS as an acronym makes action dispatch mangle POST type detection #8631

Closed
punchh opened this Issue Dec 27, 2012 · 7 comments

Projects

None yet

4 participants

@punchh
punchh commented Dec 27, 2012

We've set POS as an acronym inflection to reflect "Point Of Sale" in our project's inflections file. POS is a major entity in our application with a recent change.

However, post? method now fails because of

HTTP_METHOD_LOOKUP = Hash.new { |h, m| h[m] = m.underscore.to_sym if HTTP_METHODS.include?(m) }

https://github.com/rails/rails/blob/3-2-stable/actionpack/lib/action_dispatch/http/request.rb

I do not think that the above default Hash initializer in HTTP_METHOD_LOOKUP should be relying on underscore for converting to symbols and getting inflections from the project like the above get in the way of its conversion since the lookup to HTTP_METHODS has already returned TRUE.

The method now gets resolved as :pos_t. Ouch!

HTTP_METHOD_LOOKUP = Hash.new{ |h, m| h[m] = m.downcase.gsub(/-/,'_').to_sym if HTTP_METHODS.include?(m) }

The above works although i understand it's mildly less elegant but a lot safer considering the finite list to convert is just above this declaration in HTTP_METHODS.

Failing Test and Patch coming up.

cc @pixeltrix / @carlosantoniodasilva / @josevalim

@asanghi
Contributor
asanghi commented Dec 27, 2012

Just a note above that this problem would not appear on Rails 4 on the master branch because HTTP_METHOD_LOOKUP is eagerly setup when request.rb is loaded. Since this file gets loaded before the inflections from the project, we may not see the problem here.

On Rails 3.2 stable this is a problem because the hash is "lazily" looked up and can be interfered by inflections.

@pixeltrix
Member

@asanghi while your fix does the job I'm thinking that there's a problem with acronym if it converts 'POST' to 'pos_t'. The acronym regex needs to look ahead that the character following the trailing capital letter needs to be a lower case letter, i.e it needs to discern between POST and POSController.

We should still fix the HTTP_METHOD_LOOKUP so that it isn't affected by inflection settings, but can you see if you can tweak the regex to work as I described - thanks.

@asanghi
Contributor
asanghi commented Dec 29, 2012

@pixeltrix thanks for the input. quite a challenge understanding that regex. Perhaps you can guide me a bit?

Looking at the underscore function's code:

1. word.gsub!(/(?:([A-Za-z\d])|^)(#{inflections.acronym_regex})(?=\b|[^a-z])/) { "#{$1}#{$1 && '_'}#{$2.downcase}" }
2. word.gsub!(/([A-Z\d]+)([A-Z][a-z])/,'\1_\2')
3. word.gsub!(/([a-z\d])([A-Z])/,'\1_\2')
4. word.tr!("-", "_")
5. word.downcase!

A lot of magic going on here, most of which i can understand except for the first set employing regex which i dont seem to grasp.

POST => posT (1) => pos_T (3) => pos_t (5)

I tried this (sticking a dot in the positive lookahead matcher but it broke another test.

word.gsub!(/(?:([A-Za-z\d])|^)(#{inflections.acronym_regex})(?=\b|[^a-z].)/) { "#{$1}#{$1 && '_'}#{$2.downcase}" }

A bit lost here, @pixeltrix

@pixeltrix
Member

Sorry @asanghi it turns out that making the existing tests pass and making POST work is an intractable problem. The current tests have IRoRU -> i_ror_u which is indistinguishable by regexp from POST (both have a single trailing capital letter). I'll have to give this some thought as there's quite a few places in the codebase where underscore and camelize are used.

@asanghi
Contributor
asanghi commented Dec 29, 2012

Yup that's the one that failed. IRoRU .

Can we still go ahead with HTTP_METHOD_LOOKUP merge though? Or club with fix for all other possible problems with underscore and camelize?

@asanghi asanghi added a commit to asanghi/rails that referenced this issue Jan 16, 2013
@asanghi asanghi fixes #8631 local inflections from interfereing with HTTP_METHOD_LOOK…
…UP dispatch logic
5f3b40e
@asanghi asanghi added a commit to asanghi/rails that referenced this issue Jan 16, 2013
@asanghi asanghi adding regression test in master for #8631 3c19064
@tenderlove tenderlove added a commit that referenced this issue Jan 17, 2013
@tenderlove tenderlove Merge branch 'master' into jobs
* master: (39 commits)
  Deprecate direct calls to AC::RecordIdentifier.dom_id and dom_class
  clear specific logs when using rake log:clear
  Fix date_select :selected option so you can pass it nil
  document Intercepters in ActionMailer guide
  Don't rely on Hash key's ordering
  Remove warnings: "(...) interpreted as grouped expression"
  adding regression test in master for #8631
  strong parameters exception handling
  Remove header bloat introduced by BestStandardsSupport middleware
  allow :dirs option for .enumerate
  use case statement
  Change the behavior of route defaults
  Add support for other types of routing constraints
  Ensure port is set when passed via the process method
  Raise correct exception now Journey is integrated.
  Update guides/source/active_record_validations.md
  Revert "Merge pull request #8930 from cordawyn/ordered_railties"
  Cleaning up ActiveModel::Dirty tests
  Revert "log at debug level what line caused the redirect_to"
  Improve mysql database tasks handling to ensure we always rescue from an exception
  ...

Conflicts:
	guides/source/action_mailer_basics.md
04c23f2
@rafaelfranca
Member

@pixeltrix Is anything missing here?

@pixeltrix
Member

No - I think we just have to accept that adding POS as an acronym is going to mess up 'POST'.underscore so we just need to be careful about using it where the app's inflections can have an effect.

@pixeltrix pixeltrix closed this Jan 20, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment