Rest Controller wildcard registration (#46487)

Registering two different http methods on the same path using different wildcard names would result in the last wildcard name being active only. Now throw an exception instead. Closes #46482
elastic · Sep 9, 2019 · 9fce5a9 · 9fce5a9
1 parent e21deae
commit 9fce5a9
Show file tree

Hide file tree

Showing 3 changed files with 29 additions and 6 deletions.
diff --git a/server/src/main/java/org/elasticsearch/common/path/PathTrie.java b/server/src/main/java/org/elasticsearch/common/path/PathTrie.java
@@ -96,7 +96,12 @@ private TrieNode(String key, T value, String wildcard) {
 
         private void updateKeyWithNamedWildcard(String key) {
             this.key = key;
-            namedWildcard = key.substring(key.indexOf('{') + 1, key.indexOf('}'));
+            String newNamedWildcard = key.substring(key.indexOf('{') + 1, key.indexOf('}'));
+            if (namedWildcard != null && newNamedWildcard.equals(namedWildcard) == false) {
+                throw new IllegalArgumentException("Trying to use conflicting wildcard names for same path: "
+                    + namedWildcard + " and " + newNamedWildcard);
+            }
+            namedWildcard = newNamedWildcard;
         }
 
         private void addInnerChild(String key, TrieNode child) {

diff --git a/server/src/test/java/org/elasticsearch/common/path/PathTrieTests.java b/server/src/test/java/org/elasticsearch/common/path/PathTrieTests.java
@@ -19,6 +19,7 @@
 
 package org.elasticsearch.common.path;
 
+import org.elasticsearch.common.path.PathTrie.TrieMatchingMode;
 import org.elasticsearch.rest.RestUtils;
 import org.elasticsearch.test.ESTestCase;
 
@@ -29,8 +30,6 @@
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.nullValue;
 
-import org.elasticsearch.common.path.PathTrie.TrieMatchingMode;
-
 public class PathTrieTests extends ESTestCase {
 
     public static final PathTrie.Decoder NO_DECODER = new PathTrie.Decoder() {
@@ -122,13 +121,13 @@ public void testWildcardMatchingModes() {
         PathTrie<String> trie = new PathTrie<>(NO_DECODER);
         trie.insert("{testA}", "test1");
         trie.insert("{testA}/{testB}", "test2");
-        trie.insert("a/{testA}", "test3");
+        trie.insert("a/{testB}", "test3");
         trie.insert("{testA}/b", "test4");
         trie.insert("{testA}/b/c", "test5");
         trie.insert("a/{testB}/c", "test6");
         trie.insert("a/b/{testC}", "test7");
         trie.insert("{testA}/b/{testB}", "test8");
-        trie.insert("x/{testA}/z", "test9");
+        trie.insert("x/{testB}/z", "test9");
         trie.insert("{testA}/{testB}/{testC}", "test10");
 
         Map<String, String> params = new HashMap<>();
@@ -196,7 +195,7 @@ public void testExplicitMatchingMode() {
         trie.insert("a", "test2");
         trie.insert("{testA}/{testB}", "test3");
         trie.insert("a/{testB}", "test4");
-        trie.insert("{testB}/b", "test5");
+        trie.insert("{testA}/b", "test5");
         trie.insert("a/b", "test6");
         trie.insert("{testA}/b/{testB}", "test7");
         trie.insert("x/{testA}/z", "test8");

diff --git a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java
@@ -56,6 +56,7 @@
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicReference;
+import java.util.stream.Collectors;
 
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
@@ -175,6 +176,24 @@ public void testRegisterWithDeprecatedHandler() {
         verify(controller).registerAsDeprecatedHandler(deprecatedMethod, deprecatedPath, handler, deprecationMessage, logger);
     }
 
+    public void testRegisterSecondMethodWithDifferentNamedWildcard() {
+        final RestController restController = new RestController(null, null, null, circuitBreakerService, usageService);
+
+        RestRequest.Method firstMethod = randomFrom(RestRequest.Method.values());
+        RestRequest.Method secondMethod =
+            randomFrom(Arrays.stream(RestRequest.Method.values()).filter(m -> m != firstMethod).collect(Collectors.toList()));
+
+        final String path = "/_" + randomAlphaOfLengthBetween(1, 6);
+
+        RestHandler handler = mock(RestHandler.class);
+        restController.registerHandler(firstMethod, path + "/{wildcard1}", handler);
+
+        IllegalArgumentException exception = expectThrows(IllegalArgumentException.class,
+            () -> restController.registerHandler(secondMethod, path + "/{wildcard2}", handler));
+
+        assertThat(exception.getMessage(), equalTo("Trying to use conflicting wildcard names for same path: wildcard1 and wildcard2"));
+    }
+
     public void testRestHandlerWrapper() throws Exception {
         AtomicBoolean handlerCalled = new AtomicBoolean(false);
         AtomicBoolean wrapperCalled = new AtomicBoolean(false);