[go_router_builder] Report duplicate route paths at build time - #12399
Open
brianegan wants to merge 11 commits into
Open
[go_router_builder] Report duplicate route paths at build time#12399brianegan wants to merge 11 commits into
brianegan wants to merge 11 commits into
Conversation
Adds validation to the code generator that detects conflicting route paths among sibling routes, including across StatefulShellRoute branches and through transparent shell route containers. Paths are normalized so that structurally equivalent patterns with different parameter names (e.g. `meal/:id` vs `meal/:mealId`) are correctly flagged as conflicts.
Duplicate route paths are now reported as build warnings by default, since they are legal at runtime: go_router matches the first route that fits, so the later route is unreachable rather than invalid. The new `duplicate_route_paths` builder option raises them to build errors with `error`, or silences them with `ignore`. Warning mode changes two things about the walk. Every conflict is reported rather than only the first, so a build does not reveal them one at a time. And the walk no longer descends into shell routes, which own no URL namespace, so each level is visited once instead of printing nested conflicts twice. Widens what the check covers: * Relative routes were invisible, because `RelativeGoRouteConfig` does not extend `GoRouteConfig`. The collector now keys on `_GoRouteMixin`, which both classes carry. * Routes declared by separate annotations in the same file were never compared, nor was a top-level route's own path. Validation moved up to the generator, the only place that sees every annotation in a library, and treats the top-level routes as siblings of the root. The golden harness gained two optional companion files per input: `<name>.dart.options` holds JSON builder options, and `<name>.dart.warnings` holds expected warnings, with an empty file asserting silence. Also drops the duplicate `DoubleRoute` and `DoubleExtensionRoute` registrations from the example, which the check correctly flags. Both were unreachable, so only the dead registration leaves the generated output.
The docs claimed the later of two routes sharing a path is unreachable. That holds for a leaf, but go_router backtracks: when a route matches only a prefix and none of its children complete the URL, matching moves on to the next sibling. So the accurate statement is narrower. Two *different* route classes sharing a path means navigating to the second class's location lands on the first class's page. One route class declared twice with different children is sound, and groups children by feature area. Both shapes stay reported, because the builder cannot tell a deliberate grouping from an accidental duplicate, and the severity option already lets the author decide. Adds a test input pinning that, so the grouping shape is not later carved out into a silent exemption.
Two teams each extending `/cart` with their own children cannot share one route class, because a `@TypedGoRoute` annotation has to sit on the class it names, so that class can be annotated only once. They are left declaring separate routes at the same path, which works for the children and leaves only the parent URL ambiguous. Documents the shape, the absolute-path alternative that avoids it, and when that alternative does not apply. Also states two limits that were easy to miss. Routes in different files are never compared, so the two-team shape usually goes unreported. And the severity is package-wide with no per-route escape hatch, so `error` fails the builds of the sound shapes too.
The two-team example was confusing and rested on a shaky premise. Splitting a route table across files is not supported anyway, per flutter/flutter#122258, so the scenario it described is not one anyone can reach today. Replaces it with the scope that actually holds. Routes are compared across a whole library, `part` files included, so the common workaround of splitting a large route table across parts hides nothing. Verified by declaring a colliding route in a part file of the example and watching it get reported.
Three fixes surfaced by running the repository tooling. The README's grouping example was the only Dart code block not managed by code-excerpt, which fails `validate`. Sourcing it from `example/lib/readme_excerpts.dart` is not an option, since the builder processes that file and the example is a deliberate duplicate, so it would warn on every example build. The pattern describes fine in prose, so the block is gone and the paragraph absorbed it. The CHANGELOG said the check compares annotations "in the same file". The scope is the library, `part` files included. Applies the repo formatter to two test inputs, one of which was already misformatted before this branch.
The message named the class on both sides of an "and", so a single class declared twice read as though two classes were involved: Duplicate route path detected: "double-route/:requiredDoubleField" from DoubleRoute and "double-route/:requiredDoubleField" from DoubleRoute both match the same URL pattern. Now: Duplicate route path detected: DoubleRoute is declared more than once at "double-route/:requiredDoubleField". Only that pairing is reworded. Any other pair keeps naming both sides, which stays clear even when the class repeats, because the two paths differ.
Swaps the meal and drink routes for product and variant, and the README's `meal/:id` illustration for `product/:id`. Same coverage, a domain more readers will recognize.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
github-actions
Bot
added
p: go_router_builder
triage-framework
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces duplicate route path detection to go_router_builder. It adds a new duplicate_route_paths builder option to configure how duplicate paths are reported (as warnings, errors, or ignored). The implementation includes updates to the generator, a new DuplicatePathSeverity enum, and comprehensive test cases. Feedback on the changes suggests extracting a regular expression in _normalizePath to a reusable constant to avoid compiling it on every invocation.
…ePath `_normalizePath` runs once per route, so building the `RegExp` inside it recompiled the pattern on every call. Lifts it to a top-level final, which Dart initializes lazily and therefore compiles once. Addresses review feedback on flutter#12399.
brianegan
added a commit
to brianegan/packages
that referenced
this pull request
Aug 8, 2026
…ePath `_normalizePath` runs once per route, so building the `RegExp` inside it recompiled the pattern on every call. Lifts it to a top-level final, which Dart initializes lazily and therefore compiles once. Addresses review feedback on flutter#12399. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
brianegan
force-pushed
the
feature/error-on-conflicts
branch
from
August 8, 2026 10:23
00799ee to
9a5cc47
Compare
Review of the previous approach found three shapes it got wrong. All three came from comparing the path each route declares, level by level, rather than the URL the route resolves to. Missed: a child path repeated across two declarations of one parent. The two declarations are sound on their own, but their children share a URL namespace, and the walk checked them in two isolated ones. So the README blessed a shape that could hide the exact dead route this feature exists to catch. Missed: a multi-segment path against the equivalent nesting. A route at `section/detail` resolves where a route at `section` holding a child at `detail` does, but the two sit at different depths and were never compared. Missed: paths differing only in casing. A route with `caseSensitive: false` matches any casing, so it shadows a later route equal to it modulo case. Two case sensitive routes differing in case match different URLs, so folding unconditionally would report routes that do not collide. Both directions are now pinned by a test. Routes are now compared by their whole accumulated pattern, which drops the shell transparency special case, since a shell contributes nothing to the paths beneath it. Reports name the full URL too, which is what a reader needs in order to fix one. Also moves parameter normalization to `path_utils.dart` beside the pattern that defines what a parameter is, and reuses that pattern, so a colon inside a parameter's regex constraint is no longer mistaken for a second parameter. Test infrastructure: * `.warnings` files now assert silence by default, so an input that starts logging a warning fails until the warning is declared. Previously only the two inputs with the file asserted anything. * The message comparison had its arguments reversed, which labelled the diff backwards on failure and cost real time to read. * Raises the per-case timeout from 100 seconds to 5 minutes. Each case resolves against real SDK sources, and 100 seconds proved short enough to fail spuriously on a loaded machine, which reads as a test failure rather than a timeout. * Adds unit tests for the option parsing, including the rejected value, which the golden harness cannot reach. Addresses review feedback on flutter#12399.
The section had grown to explain every case twice, once in a bulleted reference list and again in prose. Cuts it by roughly half, leads with what goes wrong rather than with how the comparison works, and moves the config snippet ahead of the reasoning so the common need is answered first.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds build-time detection of routes that resolve to the same URL pattern.
go_routermatches the first route that fits, so a duplicated path silently makes the later route's page unreachable, andLaterRoute().go(context)navigates you toEarlierRoute's page instead. Today nothing surfaces that until you run into the issue at runtime.Reported as a warning by default, and new
duplicate_route_pathsbuilder option acceptswarning(the default),error, andignore, so teams whose duplicates are mistakes can fail the build instead.Before,
go_router_builder's own example app built clean despite two duplicate registrations:After:
That duplicate was real.
DoubleRoutehad been registered twice since #2395 (2022) andDoubleExtensionRoutesince #9458 (2025). Both were unreachable, and this PR removes them. The only change to the generated output is the dead route entries.What competes for a URL namespace:
product/:idandproduct/:productIdare one pattern.StatefulShellRoutebranches, since those own no path of their own and their children compete with the routes around them./home/details/editand/home/editstay distinct.$appRoutescollects them into one list. Scope is the library,partfiles included.Testing: five new golden inputs in
test_inputs/cover sibling duplicates, parameter-name-only differences,StatefulShellRoutebranches, relative routes, cross-annotation duplicates, and the sound same-class grouping case. The golden harness gained two optional companion files per input, documented in the README:<name>.dart.optionssupplies JSON builder options so each severity is exercised through the real option-parsing path, and<name>.dart.warningsasserts logged warnings, with an empty file asserting silence.Fixes flutter/flutter#190741
Pre-Review Checklist
[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2