ruby-grape / grape / 33686809238

97%

master: 97%

Build Type

Pull #2889

github

Committed by ericproulx

Commit Message

Constrain the version capture with a Regexp instead of an Array

`Pattern#extract_capture` hands the declared versions to Mustermann as the
Array of Strings they arrive in:

    requirements.merge(version: map_str(version))

Mustermann uses `capture:` for two things: shaping the regexp, and deriving
*param converters*. The second is the problem. `AST::ParamScanner::Capture`
only knows how to build a converter from a Class or a Symbol, so for an Array
of plain Strings every entry contributes nothing — and it still hands one back:

    entries = capture.map { |item| converter(item) }.compact   # => []
    converter = ->(string) do
      _, c = entries.find { |r, _| r.match?(string) }          # => nil
      c&.call(string) || string                                # => string
    end

An identity function, registered for `version` on every path-versioned route.

Its cost is not the call. A non-empty converter table makes
`AST::Pattern#identity_params?` false forever, and that is the predicate
`RegexpBased#build_params` consults to decide whether it may hand back the
capture Hash as it came:

    if @simple_captures
      params = match.named_captures
      return params if params.empty? || identity_params?(params)
      params.each_with_object({}) { |(k, v), h| h[k] = map_param(k, v) }

So every request on the route rebuilt the Hash through `map_param` and called
the do-nothing lambda on each value — `Route#params_for` runs this on every
matched request. Passing `Regexp.union(map_str(version))` registers no
converter at all, and produces a simpler regexp into the bargain:

    array:   (?<version>(?-mix:(?-mix:(?:v|%76)(?:1|%31))|(?-mix:(?:v|%76)(?:2|%32))))
    regexp:  (?<version>(?-mix:v1|v2))

`Regexp.union` escapes its arguments, so versions carrying regexp
metacharacters (`v1.0`, `v2+beta`) still match literally.

One behavioural difference, visible in those two lines: Mustermann expands an
Array entry the way it expands a path literal, so each cha... (continued)

Pull Request Pull Request #2889: Constrain the version capture with a Regexp instead of an Array

Coverage Stats

1239 of 1334 branches covered (92.88%)

Branch coverage included in aggregate %.

17 of 17 new or added lines in 4 files covered. (100.0%)

1 existing line in 1 file now uncovered.

3925 of 3993 relevant lines covered (98.3%)

27527.7 hits per line