Fast pattern compilation skips duplicate name validation

[rkh]

This is a regression introduced by performance improvements.

Example code:

Mustermann.new("/users/:id/posts/:id").params("/users/1/posts/2")

Expected result (happening on Mustermann 3.1):

Mustermann::CompileError: can't use the same capture name twice: "/users/:id/posts/:id"

Actual result (Mustermann 4.0.0):

{"id" => "2"}

[cf-ka]

First, thanks for the new release of Mustermann, the benchmark result from the Blog post looks great. :D

I'm not so deep into the mustermann code and have not much experience in how mustermann work to 100%, but I have tried to find a solution for the bug. (more or less)

After try to investigate the bug I have created a naive solution.

What I have find out is that if the fast_pattern is use it will never use the Validation methods like in pattern.rb that are calls in the 'to_ast' method.
And I have never got trigger the method 'to_ast' from fast_pattern.rb.

The only solution that I have found for me was to check the string after it's split in tokens, if in the Tokens Array cap contains the same symbol/string twice.

ast/fast_pattern.rb

def fast_compile
        tokens = @string.scan(SEGMENT_SCAN)

        # https://github.com/sinatra/mustermann/issues/162
        # Try to check the tokens cap if it's exists twice
        # only a workaround !?
        token_caps = tokens.filter_map { |(_, cap, _)| cap }
        duplicate_cap = token_caps.find { |name| token_caps.count(name) > 1 }
        raise CompileError, "can't use the same capture name twice: '#{duplicate_cap}'" if duplicate_cap
        
        src = String.new
        tokens.each_with_index do |(sep, cap, chars), i|
          if sep
            src << '\\/'
          elsif cap
            # Mirror the compiler: wrap in atomic group when the next token is a separator.
            if tokens[i + 1]&.first
              src << "(?<#{cap[1..]}>(?>[^/\\?#]+))"
            else
              src << "(?<#{cap[1..]}>[^/\\?#]+)"
            end
          else
            chars.each_char { |c| src << ENCODED[c] }
          end
        end
        Regexp.new(src)
      end

Result:

irb(main):001> require "mustermann"
=> true
irb(main):002> Mustermann.new("/")
=> #<Mustermann::Sinatra:"/">
irb(main):003> Mustermann.new("/users")
=> #<Mustermann::Sinatra:"/users">
irb(main):004> Mustermann.new("/users/:foo")
=> #<Mustermann::Sinatra:"/users/:foo">
irb(main):005> Mustermann.new("/users/:foo/name/:bar")
=> #<Mustermann::Sinatra:"/users/:foo/name/:bar">
irb(main):006> Mustermann.new("/users/:foo/name/:foo")
~/.gem/ruby/4.0.5/gems/mustermann-4.0.0/lib/mustermann/ast/fast_pattern.rb:79:in 'Mustermann::AST::FastPattern#fast_compile': can't use the same capture name twice: ':foo' (Mustermann::CompileError)

I don't know is this is helpful, but I have try it. :)

[rkh]

Thanks. That indeed solves the specific issue. Great work taking a stab at this! Mustermann is not the most straightforward.

My thinking was to abstract out capture validations into something that can work on a list of capture names instead, so we don't have two sources of authority for what needs validating.

Something along the lines of (not tested):

mustermann/validator

# frozen_string_literal: true

module Mustermann
  # Validates capture names.
  # @!visibility private
  class Validator
    # @param names [Array<String>] List of capture names to validate
    # @raise (see #check_name)
    # @see #check_name
    def self.validate(names, **) = names.each_with_object(new) { _1.check_name(_2, **) }

    def initialize = @names = Set.new

    # @param name [String] A capture name to validate
    # @param forbidden [Array<String>] List of forbidden capture names
    # @raise [Mustermann::CompileError] if name is not acceptable
    def check_name(name, forbidden: EMPTY_ARRAY)
      raise CompileError, "capture name can't be empty" if name.nil? or name.empty?
      raise CompileError, "capture name must start with underscore or lower case letter" unless name =~ /^[a-z_]/
      raise CompileError, "capture name can't be #{name}" if Array(forbidden).include? name
      raise CompileError, "can't use the same capture name twice" if names.include? name
      names << name
    end
  end
end

mustermann/ast/validation

# frozen_string_literal: true
require 'mustermann/ast/translator'
require 'mustermann/validator'

module Mustermann
  module AST
    # Run {Mustermann::Validator} for a given AST.
    #
    # Internally a poor man's visitor (abusing translator to not have to implement a visitor).
    # @!visibility private
    class Validation < Translator
      # Runs validations.
      #
      # @param [Mustermann::AST::Node] ast to be validated
      # @return [Mustermann::AST::Node] the validated ast
      # @raise [Mustermann::AST::CompileError] if validation fails
      def self.validate(ast)
        new.translate(ast)
        ast
      end

      translate(Object, :splat) {}
      translate(:node) { t(payload) }
      translate(Array) { each { |p| t(p)} }
      translate(:capture) { t.validator.check_name(name, forbidden: ['captures', 'splat'])}
      translate(:variable, :named_splat) { t.validator.check_name(name, forbidden: 'captures')}

      # @return [Array<String>] list of capture names in tree
      # @!visibility private
      def validator = @validator ||= Mustermann::Validator.new
    end
  end
end

Then fast_compile could call Validator.validate(names, forbidden: ['captures', 'splat'])

By using Set internally, it also improves the name check complexity from O(n²) to O(n), though I doubt that matters much as the list of captures would be short under normal usage.