-
Notifications
You must be signed in to change notification settings - Fork 15
refactor: No normalized path interning #375
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,6 +49,10 @@ final class _TrieNode<T> { | |
|
|
||
| /// Indicates if this node holds a single value | ||
| bool get isSingle => _hasNoChildren && value != null; | ||
|
|
||
| /// True if this node has both a literal child and a dynamic segment, so a | ||
| /// lookup here may have to backtrack from the literal to the dynamic branch. | ||
| bool get _isAmbiguous => children.isNotEmpty && dynamicSegment != null; | ||
| } | ||
|
|
||
| sealed class _DynamicSegment<T> { | ||
|
|
@@ -60,7 +64,10 @@ sealed class _DynamicSegment<T> { | |
| final class _Parameter<T> extends _DynamicSegment<T> { | ||
| final String name; | ||
|
|
||
| _Parameter(this.name); | ||
| /// The parameter [name] as a [Symbol], precomputed for use during lookup. | ||
| final Symbol symbol; | ||
|
|
||
| _Parameter(this.name) : symbol = Symbol(name); | ||
| } | ||
|
|
||
| final class _Wildcard<T> extends _DynamicSegment<T> {} | ||
|
|
@@ -75,6 +82,14 @@ final class PathTrie<T extends Object> { | |
| // Note: not final since we update in attach | ||
| var _root = _TrieNode<T>(); | ||
|
|
||
| /// True if any node has both literal children and a dynamic segment, the only | ||
| /// case where a lookup can match a literal, fail deeper, and need the dynamic | ||
| /// branch. Set during registration and never cleared, so it may over-report. | ||
| bool _needsBacktracking = false; | ||
|
|
||
| /// Whether lookups on this trie require backtracking. See [_needsBacktracking]. | ||
| bool get needsBacktracking => _needsBacktracking; | ||
|
|
||
| /// Adds a path and its associated value to the trie. | ||
| /// | ||
| /// The [normalizedPath] is expected to be pre-normalized (e.g., using | ||
|
|
@@ -258,7 +273,8 @@ final class PathTrie<T extends Object> { | |
|
|
||
| for (int i = 0; i < segments.length; i++) { | ||
| final segment = segments[i]; | ||
| final dynamicSegment = currentNode.dynamicSegment; | ||
| final node = currentNode; // node this segment is added to | ||
| final dynamicSegment = node.dynamicSegment; | ||
|
|
||
| if (segment.startsWith('**')) { | ||
| // Handle tail segment | ||
|
|
@@ -312,6 +328,8 @@ final class PathTrie<T extends Object> { | |
| () => _TrieNode<T>(), | ||
| ); | ||
| } | ||
|
|
||
| if (node._isAmbiguous) _needsBacktracking = true; | ||
| } | ||
| return currentNode; | ||
| } | ||
|
|
@@ -376,23 +394,35 @@ final class PathTrie<T extends Object> { | |
| : (final v) => parentMap(childMap(v)); | ||
| } | ||
| currentNode.children.addAll(node.children); | ||
|
|
||
| _needsBacktracking = | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I ran into what looks like a stale-flag case with final sub = router.group('/api');
sub.get('/users/x', 1);
sub.get('/users/:id/y', 2);
router.lookup(Method.get, '/api/users/x/y'); // PathMiss — matches {id: x} on main
|
||
| _needsBacktracking || | ||
| trie._needsBacktracking || | ||
| currentNode._isAmbiguous; | ||
|
|
||
| trie._root = consume ? _TrieNode() : currentNode; | ||
| } | ||
|
|
||
| /// Looks up a [normalizedPath] in the trie and extracts parameters. | ||
| /// | ||
| /// Literal segments are prioritized over parameters during matching. | ||
| /// If [backtrack] is set (default), then the search is allowed to use | ||
| /// backtracking. | ||
| /// Literal segments are prioritized over parameters. Backtracking runs only | ||
| /// when [backtrack] is true and the table [needsBacktracking]; otherwise a | ||
| /// faster non-backtracking walk is used. Passing `backtrack: false` lets a | ||
| /// literal shadow an overlapping parameter at the same level. | ||
| /// | ||
| /// Returns a [TrieMatch] containing the associated value and extracted | ||
| /// parameters if a matching path is found, otherwise returns `null`. | ||
| /// Returns a [TrieMatch] if a matching path is found, otherwise `null`. | ||
| TrieMatch<T>? lookup( | ||
| final NormalizedPath normalizedPath, { | ||
| final bool backtrack = true, | ||
| }) { | ||
| return backtrack | ||
| ? _lookupRecursive(_root, normalizedPath, 0, _root.map, const {}) | ||
| return backtrack && _needsBacktracking | ||
| ? _lookupRecursive( | ||
| _root, | ||
| normalizedPath, | ||
| 0, | ||
| _root.map, | ||
| <Symbol, String>{}, | ||
| ) | ||
| : _lookupIterative(normalizedPath); | ||
| } | ||
|
|
||
|
|
@@ -432,17 +462,14 @@ final class PathTrie<T extends Object> { | |
|
|
||
| final segment = segments[index]; | ||
|
|
||
| TrieMatch<T>? next(_TrieNode<T> node, final T Function(T)? map) => | ||
| _lookupRecursive(node, normalizedPath, index + 1, map, parameters); | ||
|
|
||
| // Try literal match first | ||
| final child = node.children[segment]; | ||
| if (child != null) { | ||
| final newMap = _composeMap(currentMap, child.map); | ||
| final result = _lookupRecursive( | ||
| child, | ||
| normalizedPath, | ||
| index + 1, | ||
| newMap, | ||
| parameters, | ||
| ); | ||
| final result = next(child, newMap); | ||
| if (result != null) return result; | ||
| // Fall through to try dynamic segment | ||
| } | ||
|
|
@@ -452,9 +479,6 @@ final class PathTrie<T extends Object> { | |
| if (dynamicSegment != null) { | ||
| final dynamicNode = dynamicSegment.node; | ||
| final newMap = _composeMap(currentMap, dynamicNode.map); | ||
| final newParams = dynamicSegment is _Parameter<T> | ||
| ? {...parameters, Symbol(dynamicSegment.name): segment} | ||
| : parameters; | ||
|
|
||
| if (dynamicSegment is _Tail<T>) { | ||
| // Tail matches: check for value at this position | ||
|
|
@@ -463,19 +487,18 @@ final class PathTrie<T extends Object> { | |
| value = newMap?.call(value) ?? value; | ||
| return TrieMatch( | ||
| value, | ||
| newParams, | ||
| parameters, | ||
| normalizedPath.subPath(0, index), | ||
| normalizedPath.subPath(index), | ||
| ); | ||
| } | ||
| } else if (dynamicSegment is _Parameter<T>) { | ||
| parameters[dynamicSegment.symbol] = segment; | ||
| final result = next(dynamicNode, newMap); | ||
| if (result != null) return result; | ||
| parameters.remove(dynamicSegment.symbol); // backtrack | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like this drops the outer binding when the same param name is bound at two levels and the inner branch fails: final trie = PathTrie<int>()
..add(NormalizedPath('/:x/lit/:x/a'), 1)
..add(NormalizedPath('/:x/:y/c'), 2);
trie.lookup(NormalizedPath('/v/lit/c')); // params = {y: lit} — x is goneOn main this returns {x: v, y: lit} since the per-level copy kept the outer value. Probably a good place to add a test as well. |
||
| } else { | ||
| return _lookupRecursive( | ||
| dynamicNode, | ||
| normalizedPath, | ||
| index + 1, | ||
| newMap, | ||
| newParams, | ||
| ); | ||
| return next(dynamicNode, newMap); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -492,39 +515,30 @@ final class PathTrie<T extends Object> { | |
| return (final v) => outer(inner(v)); | ||
| } | ||
|
|
||
| // coverage:ignore-start | ||
| // ignore: unused_element | ||
| /// Non-backtracking lookup, valid only when [needsBacktracking] is false: | ||
| /// commits to a literal match if present, else takes the single dynamic branch. | ||
| TrieMatch<T>? _lookupIterative(final NormalizedPath normalizedPath) { | ||
| final segments = normalizedPath.segments; | ||
| final parameters = <Symbol, String>{}; | ||
|
|
||
| var currentNode = _root; | ||
| var currentMap = currentNode.map; | ||
|
|
||
| // Helper function to update combinedMap when descending the trie | ||
| void updateMap() { | ||
| final cm = currentMap; | ||
| final m = currentNode.map; | ||
| currentMap = cm == null | ||
| ? m // may also be null | ||
| : (m == null ? cm : (final v) => cm(m(v))); // compose map function | ||
| } | ||
|
|
||
| int i = 0; | ||
| for (; i < segments.length; i++) { | ||
| final segment = segments[i]; | ||
| final child = currentNode.children[segment]; | ||
| if (child != null) { | ||
| // Prioritize literal match | ||
| currentNode = child; | ||
| updateMap(); | ||
| currentMap = _composeMap(currentMap, currentNode.map); | ||
| } else { | ||
| final dynamicSegment = currentNode.dynamicSegment; | ||
| if (dynamicSegment == null) return null; // no match | ||
| currentNode = dynamicSegment.node; | ||
| updateMap(); | ||
| currentMap = _composeMap(currentMap, currentNode.map); | ||
| if (dynamicSegment case final _Parameter<T> parameter) { | ||
| parameters[Symbol(parameter.name)] = segment; | ||
| parameters[parameter.symbol] = segment; | ||
| } | ||
| if (dynamicSegment is _Tail<T>) break; // possible early match | ||
| } | ||
|
|
@@ -542,15 +556,14 @@ final class PathTrie<T extends Object> { | |
| if (dynamicSegment is _Tail<T>) { | ||
| currentNode = dynamicSegment.node; | ||
| value = currentNode.value; | ||
| updateMap(); | ||
| currentMap = _composeMap(currentMap, currentNode.map); | ||
| } | ||
| } | ||
|
|
||
| if (value == null) return null; | ||
| value = currentMap?.call(value) ?? value; | ||
| return TrieMatch(value, parameters, matchedPath, remainingPath); | ||
| } | ||
| // coverage:ignore-end | ||
|
|
||
| /// Returns true if the path trie has no routes. | ||
| bool get isEmpty => _root.isEmpty; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is
NoCachestill needed? I couldn't find any consumers now thatNormalizedPath.internedis gone, so there doesn't seem to be anything to plug it into.