Raise when adding a duplicate route (#42)

spec/integration_spec.cr

@@ -6,9 +6,9 @@ describe LuckyRouter do
 
     # Here to makes sure things run super fast even with lots of routes
     1000.times do
-      router.add("put", "#{(rand * 100).to_i}", :fake_show)
-      router.add("get", "#{(rand * 100).to_i}/edit", :fake_edit)
-      router.add("get", "#{(rand * 100).to_i}/new/edit", :fake_new_edit)
+      router.add("put", "#{UUID.random}", :fake_show)
+      router.add("get", "#{UUID.random}/edit", :fake_edit)
+      router.add("get", "#{UUID.random}/new/edit", :fake_new_edit)
     end
 
     router.add("get", "/:organization", :organization)
@@ -195,31 +195,85 @@ describe LuckyRouter do
   end
 
   describe "route with trailing slash" do
-    router = LuckyRouter::Matcher(Symbol).new
-    router.add("get", "/users/:id", :show)
-
     context "is defined with a trailing slash" do
-      router.add("get", "/users/", :index)
-
       it "should treat it as a index route when called without a trailing slash" do
+        router = LuckyRouter::Matcher(Symbol).new
+        router.add("get", "/users/", :index)
         router.match!("get", "/users").payload.should eq :index
       end
 
       it "should treat it as a index route when called with a trailing slash" do
+        router = LuckyRouter::Matcher(Symbol).new
+        router.add("get", "/users/", :index)
         router.match!("get", "/users/").payload.should eq :index
       end
     end
 
     context "is defined without a trailing slash" do
-      router.add("get", "/users", :index)
-
       it "should treat it as a index route when called without a trailing slash" do
+        router = LuckyRouter::Matcher(Symbol).new
+        router.add("get", "/users", :index)
         router.match!("get", "/users").payload.should eq :index
       end
 
       it "should treat it as a index route when called with a trailing slash" do
+        router = LuckyRouter::Matcher(Symbol).new
+        router.add("get", "/users", :index)
         router.match!("get", "/users/").payload.should eq :index
       end
     end
   end
+
+  describe "duplicate route checking" do
+    it "raises on normal duplicate" do
+      router = LuckyRouter::Matcher(Symbol).new
+      router.add("get", "/posts/something", :post_index)
+
+      expect_raises LuckyRouter::DuplicateRouteError do
+        router.add("get", "/posts/something", :other_post_index)
+      end
+    end
+
+    it "does not raise on duplicate path with different methods" do
+      router = LuckyRouter::Matcher(Symbol).new
+      router.add("get", "/posts/something", :post_index)
+      router.add("post", "/posts/something", :create_something)
+    end
+
+    it "raises even if path variable is different" do
+      router = LuckyRouter::Matcher(Symbol).new
+      router.add("get", "/posts/:id", :post_show)
+
+      expect_raises LuckyRouter::DuplicateRouteError do
+        router.add("get", "/posts/:post_id", :other_post_show)
+      end
+    end
+
+    it "raises on optional paths when pre-existing path does not have optional part" do
+      router = LuckyRouter::Matcher(Symbol).new
+      router.add("get", "/posts", :post_index)
+
+      expect_raises LuckyRouter::DuplicateRouteError do
+        router.add("get", "/posts/?something", :post_something)
+      end
+    end
+
+    it "raises on optional paths when pre-existing path has optional part" do
+      router = LuckyRouter::Matcher(Symbol).new
+      router.add("get", "/posts/something", :post_something)
+
+      expect_raises LuckyRouter::DuplicateRouteError do
+        router.add("get", "/posts/?something", :post_something_maybe)
+      end
+    end
+
+    it "raises on glob routes if path without glob matches pre-existing" do
+      router = LuckyRouter::Matcher(Symbol).new
+      router.add("get", "/posts", :post_index)
+
+      expect_raises LuckyRouter::DuplicateRouteError do
+        router.add("get", "/posts/*", :post_glob)
+      end
+    end
+  end
 end

spec/path_normalizer_spec.cr

@@ -0,0 +1,37 @@
+require "./spec_helper"
+
+describe LuckyRouter::PathNormalizer do
+  describe ".normalize" do
+    it "turns regular path parts into slash delimitted string" do
+      path_parts = LuckyRouter::PathPart.split_path("/api/v1/users")
+
+      result = LuckyRouter::PathNormalizer.normalize(path_parts)
+
+      result.should eq("/api/v1/users")
+    end
+
+    it "gives path variables a generic name" do
+      path_parts = LuckyRouter::PathPart.split_path("/users/:id")
+
+      result = LuckyRouter::PathNormalizer.normalize(path_parts)
+
+      result.should eq("/users/:path_variable")
+    end
+
+    it "removes question mark from optional path" do
+      path_parts = LuckyRouter::PathPart.split_path("/users/?name")
+
+      result = LuckyRouter::PathNormalizer.normalize(path_parts)
+
+      result.should eq("/users/name")
+    end
+
+    it "removes question mark and gives generic name to optional path variables" do
+      path_parts = LuckyRouter::PathPart.split_path("/users/?:name")
+
+      result = LuckyRouter::PathNormalizer.normalize(path_parts)
+
+      result.should eq("/users/:path_variable")
+    end
+  end
+end

spec/spec_helper.cr

@@ -1,2 +1,3 @@
 require "spec"
 require "../src/lucky_router"
+require "uuid"

src/lucky_router/errors.cr

@@ -10,4 +10,10 @@ module LuckyRouter
       super "Tried to define a glob as `#{glob}`, but it is invalid. Globs must be defined like `*` or given a name like `*:name`."
     end
   end
+
+  class DuplicateRouteError < LuckyRouterError
+    def initialize(method, path)
+      super "Router already contains a route matching a #{method.upcase} request to `#{path}`."
+    end
+  end
 end

src/lucky_router/matcher.cr

@@ -18,6 +18,7 @@
 class LuckyRouter::Matcher(T)
   # starting point from which all fragments are located
   getter root = Fragment(T).new(path_part: PathPart.new(""))
+  getter normalized_paths = Set(String).new
 
   def add(method : String, path : String, payload : T)
     all_path_parts = PathPart.split_path(path)
@@ -30,25 +31,33 @@ class LuckyRouter::Matcher(T)
 
     path_without_optional_params = all_path_parts.reject(&.optional?)
 
-    process_and_add_path(method, path_without_optional_params, payload)
+    process_and_add_path(method, path_without_optional_params, payload, path)
     optional_parts.each do |optional_part|
       path_without_optional_params << optional_part
-      process_and_add_path(method, path_without_optional_params, payload)
+      process_and_add_path(method, path_without_optional_params, payload, path)
     end
     if glob_part
       path_without_optional_params << glob_part
-      process_and_add_path(method, path_without_optional_params, payload)
+      process_and_add_path(method, path_without_optional_params, payload, path)
     end
   end
 
-  private def process_and_add_path(method : String, parts : Array(PathPart), payload : T)
+  private def process_and_add_path(method : String, parts : Array(PathPart), payload : T, path : String)
     if method.downcase == "get"
       root.process_parts(parts, "head", payload)
     end
 
+    duplicate_check(method, parts, path)
+
     root.process_parts(parts, method, payload)
   end
 
+  private def duplicate_check(method : String, parts : Array(PathPart), path : String)
+    normalized_path = method.downcase + PathNormalizer.normalize(parts)
+    raise DuplicateRouteError.new(method, path) if normalized_paths.includes?(normalized_path)
+    normalized_paths << normalized_path
+  end
+
   def match(method : String, path_to_match : String) : Match(T)?
     parts = path_to_match.split('/')
     parts.pop if parts.last.blank?

src/lucky_router/path_normalizer.cr

@@ -0,0 +1,11 @@
+class LuckyRouter::PathNormalizer
+  DEFAULT_PATH_VARIABLE_NAME = ":path_variable"
+
+  def self.normalize(path_parts : Array(PathPart)) : String
+    path_parts.map { |path_part| normalize(path_part) }.join('/')
+  end
+
+  private def self.normalize(path_part : PathPart) : String
+    path_part.path_variable? ? DEFAULT_PATH_VARIABLE_NAME : path_part.name
+  end
+end