Support paths with different variable names in same path location (#273)

---------

Co-authored-by: Kyle Plump <[email protected]>

Author: dcr8898

lib/hanami/router/leaf.rb

@@ -0,0 +1,42 @@
+# frozen_string_literal: true
+
+require "mustermann/rails"
+
+module Hanami
+  class Router
+    class Leaf
+      # Trie Leaf
+      #
+      # @api private
+      # @since 2.2.0
+      attr_reader :to, :params
+
+      # @api private
+      # @since 2.2.0
+      def initialize(route, to, constraints)
+        @route = route
+        @to = to
+        @constraints = constraints
+        @params = nil
+      end
+
+      # @api private
+      # @since 2.2.0
+      def match(path)
+        match = matcher.match(path)
+
+        @params = match.named_captures if match
+
+        match
+      end
+
+      private
+
+      # @api private
+      # @since 2.2.0
+      def matcher
+        @matcher ||= Mustermann.new(@route, type: :rails, version: "5.0", capture: @constraints)
+      end
+    end
+  end
+end

lib/hanami/router/node.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-require "hanami/router/segment"
+require "hanami/router/leaf"
 
 module Hanami
   class Router
@@ -18,15 +18,14 @@ class Node
       def initialize
         @variable = nil
         @fixed = nil
-        @to = nil
+        @leaves = nil
       end
 
       # @api private
       # @since 2.0.0
-      def put(segment, constraints)
+      def put(segment)
         if variable?(segment)
-          @variable ||= {}
-          @variable[segment_for(segment, constraints)] ||= self.class.new
+          @variable ||= self.class.new
         else
           @fixed ||= {}
           @fixed[segment] ||= self.class.new
@@ -35,35 +34,21 @@ def put(segment, constraints)
 
       # @api private
       # @since 2.0.0
-      #
-      def get(segment) # rubocop:disable Metrics/PerceivedComplexity
-        return unless @variable || @fixed
-
-        captured = nil
-
-        found = @fixed&.fetch(segment, nil)
-        return [found, nil] if found
-
-        @variable&.each do |matcher, node|
-          break if found
-
-          captured = matcher.match(segment)
-          found = node if captured
-        end
-
-        [found, captured&.named_captures]
+      def get(segment)
+        @fixed&.fetch(segment, nil) || @variable
       end
 
       # @api private
       # @since 2.0.0
-      def leaf?
-        @to
+      def leaf!(route, to, constraints)
+        @leaves ||= []
+        @leaves << Leaf.new(route, to, constraints)
       end
 
       # @api private
-      # @since 2.0.0
-      def leaf!(to)
-        @to = to
+      # @since 2.2.0
+      def match(path)
+        @leaves&.find { |leaf| leaf.match(path) }
       end
 
       private
@@ -73,18 +58,6 @@ def leaf!(to)
       def variable?(segment)
         Router::ROUTE_VARIABLE_MATCHER.match?(segment)
       end
-
-      # @api private
-      # @since 2.0.0
-      def segment_for(segment, constraints)
-        Segment.fabricate(segment, **constraints)
-      end
-
-      # @api private
-      # @since 2.0.0
-      def fixed?(matcher)
-        matcher.names.empty?
-      end
     end
   end
 end

lib/hanami/router/trie.rb

@@ -21,33 +21,26 @@ def initialize
 
       # @api private
       # @since 2.0.0
-      def add(path, to, constraints)
+      def add(route, to, constraints)
+        segments = segments_from(route)
         node = @root
-        for_each_segment(path) do |segment|
-          node = node.put(segment, constraints)
+
+        segments.each do |segment|
+          node = node.put(segment)
         end
 
-        node.leaf!(to)
+        node.leaf!(route, to, constraints)
       end
 
       # @api private
       # @since 2.0.0
       def find(path)
+        segments = segments_from(path)
         node = @root
-        params = {}
-
-        for_each_segment(path) do |segment|
-          break unless node
 
-          child, captures = node.get(segment)
-          params.merge!(captures) if captures
-
-          node = child
-        end
+        return unless segments.all? { |segment| node = node.get(segment) }
 
-        return [node.to, params] if node&.leaf?
-
-        nil
+        node.match(path)&.then { |found| [found.to, found.params] }
       end
 
       private
@@ -58,10 +51,11 @@ def find(path)
       private_constant :SEGMENT_SEPARATOR
 
       # @api private
-      # @since 2.0.0
-      def for_each_segment(path, &blk)
+      # @since 2.2.0
+      def segments_from(path)
         _, *segments = path.split(SEGMENT_SEPARATOR)
-        segments.each(&blk)
+
+        segments
       end
     end
   end

spec/integration/hanami/router/recognition_spec.rb

@@ -256,6 +256,38 @@
       end
     end
 
+    describe "variable followed by variable with fixed with different slug names" do
+      let(:router) do
+        described_class.new do
+          get "/:foo",     as: :variable_one, to: RecognitionTestCase.endpoint("variable_one")
+          get "/:bar/baz", as: :variable_two, to: RecognitionTestCase.endpoint("variable_two")
+        end
+      end
+
+      it "recognizes route(s)" do
+        runner.run!([
+          [:variable_one, "/one", {foo: "one"}],
+          [:variable_two, "/two/baz", {bar: "two"}]
+        ])
+      end
+    end
+
+    describe "variable with fixed followed by variable with different slug names" do
+      let(:router) do
+        described_class.new do
+          get "/:bar/baz", as: :variable_two, to: RecognitionTestCase.endpoint("variable_two")
+          get "/:foo",     as: :variable_one, to: RecognitionTestCase.endpoint("variable_one")
+        end
+      end
+
+      it "recognizes route(s)" do
+        runner.run!([
+          [:variable_one, "/one", {foo: "one"}],
+          [:variable_two, "/two/baz", {bar: "two"}]
+        ])
+      end
+    end
+
     describe "relative variable with constraints" do
       let(:router) do
         described_class.new do
@@ -569,20 +601,6 @@
       end
     end
 
-    describe "relative variable with permissive constraint" do
-      let(:router) do
-        described_class.new do
-          get ":test", as: :regex, test: /.*/, to: RecognitionTestCase.endpoint("regex")
-        end
-      end
-
-      it "recognizes route(s)" do
-        runner.run!([
-          [:regex, "/test/", {test: "test"}]
-        ])
-      end
-    end
-
     describe "variable with permissive constraint" do
       let(:router) do
         described_class.new do

spec/integration/hanami/router/scope_spec.rb

@@ -56,7 +56,6 @@
       app = Rack::MockRequest.new(router)
 
       expect(app.request("GET", "/it", lint: true).body).to eq("Root (it)!")
-      expect(app.request("GET", "/it/", lint: true).body).to eq("Root (it)!")
       expect(app.request("GET", "/it/trees", lint: true).body).to eq("Trees (it)!")
     end
 

spec/unit/hanami/router/leaf_spec.rb

@@ -0,0 +1,63 @@
+# frozen_string_literal: true
+
+require "hanami/router/leaf"
+
+RSpec.describe Hanami::Router::Leaf do
+  let(:subject)     { described_class.new(route, to, constraints) }
+  let(:route)       { "/test/route" }
+  let(:to)          { "test proc" }
+  let(:constraints) { {} }
+
+  describe "#initialize" do
+    it "returns a #{described_class} instance" do
+      expect(subject).to be_kind_of(described_class)
+    end
+  end
+
+  describe "#to" do
+    it "returns the endpoint passed as 'to' when initialized" do
+      expect(subject.to).to eq(to)
+    end
+  end
+
+  describe "#match" do
+    context "when path matches route" do
+      let(:matching_path) { route }
+
+      it "returns true" do
+        expect(subject.match(matching_path)).to be_truthy
+      end
+    end
+
+    context "when path doesn't match route" do
+      let(:non_matching_path) { "/bad/path" }
+
+      it "returns true" do
+        expect(subject.match(non_matching_path)).to be_falsey
+      end
+    end
+  end
+
+  describe "#params" do
+    context "without previously calling #match(path)" do
+      it "returns nil" do
+        params = subject.params
+
+        expect(params).to be_nil
+      end
+    end
+
+    context "with variable path" do
+      let(:route)           { "test/:route" }
+      let(:matching_path)   { "test/path" }
+      let(:matching_params) { {"route" => "path"} }
+
+      it "returns captured params" do
+        subject.match(matching_path)
+        params = subject.params
+
+        expect(params).to eq(matching_params)
+      end
+    end
+  end
+end