chore: Improve parameter definition matching

Author: codekeyz

packages/spanner/lib/src/parametric/definition.dart

@@ -4,15 +4,13 @@ import 'utils.dart';
 
 typedef ParamAndValue = ({String name, String? value});
 
-SingleParameterDefn _singleParamDefn(RegExpMatch m, [String? nextPart]) =>
-    SingleParameterDefn._(
+SingleParameterDefn _singleParamDefn(RegExpMatch m) => SingleParameterDefn._(
       m.group(2)!,
       prefix: m.group(1)?.nullIfEmpty,
       suffix: m.group(3)?.nullIfEmpty,
-      nextPart: nextPart,
     );
 
-ParameterDefinition buildParamDefinition(String part, [String? nextPart]) {
+ParameterDefinition buildParamDefinition(String part) {
   if (closeDoorParametricRegex.hasMatch(part)) {
     throw ArgumentError.value(
         part, null, 'Parameter definition is invalid. Close door neighbors');
@@ -24,13 +22,10 @@ ParameterDefinition buildParamDefinition(String part, [String? nextPart]) {
   }
 
   if (matches.length == 1) {
-    return _singleParamDefn(matches.first, nextPart);
+    return _singleParamDefn(matches.first);
   }
 
-  return CompositeParameterDefinition._(
-    matches.map(_singleParamDefn),
-    nextPart,
-  );
+  return CompositeParameterDefinition._(matches.map(_singleParamDefn));
 }
 
 abstract class ParameterDefinition implements HandlerStore {
@@ -42,9 +37,6 @@ abstract class ParameterDefinition implements HandlerStore {
 
   bool get terminal;
 
-  /// Next route part included so we can match early if its a static next part
-  String? get nextPart;
-
   bool matches(String route, {bool caseSensitive = false});
 
   void resolveParams(
@@ -61,14 +53,11 @@ class SingleParameterDefn extends ParameterDefinition with HandlerStoreMixin {
   final String? prefix;
   final String? suffix;
 
-  @override
-  final String? nextPart;
-
   @override
   final String templateStr;
 
   @override
-  String get key => 'prefix=$prefix&suffix=$suffix&terminal=$terminal';
+  String get key => 'prefix=$prefix&suffix=$suffix';
 
   bool _terminal;
 
@@ -77,20 +66,19 @@ class SingleParameterDefn extends ParameterDefinition with HandlerStoreMixin {
 
   @override
   bool matches(String route, {bool caseSensitive = false}) {
-    return matchPattern(
-          route,
-          prefix ?? '',
-          suffix ?? '',
-          caseSensitive,
-        ) !=
-        null;
+    final match = matchPattern(
+      route,
+      prefix ?? '',
+      suffix ?? '',
+      caseSensitive,
+    );
+    return match != null;
   }
 
   SingleParameterDefn._(
     this.name, {
     this.prefix,
     this.suffix,
-    this.nextPart,
   })  : templateStr =
             buildTemplateString(name: name, prefix: prefix, suffix: suffix),
         _terminal = false;
@@ -119,11 +107,7 @@ class CompositeParameterDefinition extends ParameterDefinition
   final Iterable<SingleParameterDefn> parts;
   final SingleParameterDefn _maybeTerminalPart;
 
-  @override
-  final String? nextPart;
-
-  CompositeParameterDefinition._(this.parts, [this.nextPart])
-      : _maybeTerminalPart = parts.last;
+  CompositeParameterDefinition._(this.parts) : _maybeTerminalPart = parts.last;
 
   @override
   String get templateStr => parts.map((e) => e.templateStr).join();

packages/spanner/lib/src/tree/node.dart

@@ -140,17 +140,26 @@ class ParametricNode extends Node {
 
   ParametricNode(HTTPMethod method, ParameterDefinition defn)
       : _definitionsMap = {} {
-    addNewDefinition(method, defn);
+    addNewDefinition(method, defn, false);
   }
 
-  void addNewDefinition(HTTPMethod method, ParameterDefinition defn) {
+  void addNewDefinition(
+    HTTPMethod method,
+    ParameterDefinition defn,
+    bool terminal,
+  ) {
     var definitions = _definitionsMap[method];
     if (definitions == null) {
       definitions = [];
       _definitionsMap[method] = definitions;
     }
 
     if (definitions.isNotEmpty) {
+      /// At this point, terminal in [defn] will always be false since
+      /// terminals are only set after we've added a route to the definition.
+      ///
+      /// So we compare key without terminal and then manually check with the
+      /// [terminal] value we have right now.
       final existing = definitions.firstWhereOrNull((e) => e.key == defn.key);
       if (existing != null) {
         if (existing.name != defn.name) {
@@ -162,13 +171,8 @@ class ParametricNode extends Node {
           );
         }
 
-        if (existing.terminal && defn.terminal) {
-          throw ArgumentError(
-            'Route already exists${[
-              ' - ${existing.templateStr}',
-              ' - ${defn.templateStr}',
-            ].join('\n')}',
-          );
+        if (existing.terminal && terminal) {
+          throw ArgumentError('Route entry already exists');
         }
 
         return;
@@ -191,19 +195,9 @@ class ParametricNode extends Node {
     String? nextPart,
   }) {
     return _definitionsMap[method]?.firstWhereOrNull(
-      (e) {
-        if (nextPart != null && e.nextPart != null && e.nextPart!.isStatic) {
-          final match = caseSensitive
-              ? e.nextPart == nextPart
-              : e.nextPart!.toLowerCase() == nextPart.toLowerCase();
-          if (match) return true;
-        }
-
-        return (terminal
-                ? (e.terminal && e.hasMethod(method))
-                : e.terminal == terminal) &&
-            e.matches(part, caseSensitive: caseSensitive);
-      },
+      (e) =>
+          (!terminal || (e.terminal && e.hasMethod(method))) &&
+          e.matches(part, caseSensitive: caseSensitive),
     );
   }
 }

packages/spanner/lib/src/tree/tree.dart

@@ -146,7 +146,7 @@ class Spanner {
       return node.addChildAndReturn(WildcardNode.key, WildcardNode());
     }
 
-    final defn = buildParamDefinition(routePart, nextPart);
+    final defn = buildParamDefinition(routePart);
     final paramNode = node.paramNode;
 
     if (paramNode == null) {
@@ -157,7 +157,7 @@ class Spanner {
       return nextPart == null ? defn : newNode;
     }
 
-    paramNode.addNewDefinition(method, defn);
+    paramNode.addNewDefinition(method, defn, nextPart == null);
 
     return nextPart == null
         ? defn
@@ -218,13 +218,7 @@ class Spanner {
       /// use wildcard if we have any registered
       if ((childNode == null && parametricNode == null) ||
           (childNode == null && definition == null)) {
-        if (wildcardNode != null) {
-          resolvedParams.add((
-            name: WildcardNode.key,
-            value: routeSegments.sublist(i).join('/'),
-          ));
-          rootNode = wildcardNode;
-        }
+        if (wildcardNode != null) rootNode = wildcardNode;
         break;
       }
 

packages/spanner/test/parametric_test.dart

@@ -45,7 +45,21 @@ void main() {
           ..addRoute(HTTPMethod.GET, '/user', null)
           ..addRoute(HTTPMethod.GET, '/user', null);
 
-        final exception = runSyncAndReturnException<ArgumentError>(router);
+        router2() => Spanner()
+          ..addRoute(HTTPMethod.GET, '/<lang>/item/<id>', null)
+          ..addRoute(HTTPMethod.GET, '/<lang>/item/<id>', null);
+
+        router3() => Spanner()
+          ..addRoute(HTTPMethod.GET, '/<lang>/item/chima<id>hello', null)
+          ..addRoute(HTTPMethod.GET, '/<lang>/item/chima<id>hello', null);
+
+        var exception = runSyncAndReturnException<ArgumentError>(router);
+        expect(exception.message, contains('Route entry already exists'));
+
+        exception = runSyncAndReturnException<ArgumentError>(router2);
+        expect(exception.message, contains('Route entry already exists'));
+
+        exception = runSyncAndReturnException<ArgumentError>(router3);
         expect(exception.message, contains('Route entry already exists'));
       });
     });
@@ -95,17 +109,16 @@ void main() {
     test('contain param and wildcard together', () {
       final router = Spanner()
         ..addRoute(HTTPMethod.GET, '/<lang>/item/<id>', null)
-        ..addRoute(HTTPMethod.GET, '/<lang>/item/*', null);
+        ..addRoute(HTTPMethod.GET, '/<lang>/item/*', #wild);
 
       expect(
         router.lookup(HTTPMethod.GET, '/fr/item/12345'),
         havingParameters({'lang': 'fr', 'id': '12345'}),
       );
 
-      expect(
-        router.lookup(HTTPMethod.GET, '/fr/item/12345/edit'),
-        havingParameters({'lang': 'fr', '*': '12345/edit'}),
-      );
+      final result = router.lookup(HTTPMethod.GET, '/fr/item/12345/edit');
+      expect(result?.values, [#wild]);
+      expect(result?.params, {'lang': 'fr', 'id': '12345'});
     });
   });
 }