Conversation
Greptile SummaryThis PR adds a duplicate-registration guard to Key changes:
Minor concern:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["setup_dynamic_args(args)"] --> B["Filter: remove args already\nregistered as DynamicRouteVar"]
B --> C{args empty\nafter filter?}
C -- Yes --> D["return (no-op)"]
C -- No --> E["_check_overwritten_dynamic_args(args)"]
E --> F["For each remaining arg"]
F --> G{arg type?}
G -- SINGLE --> H["argsingle_factory(param)\n→ returns str from router.params"]
G -- LIST --> I["arglist_factory(param)\n→ returns list[str] from router.params"]
G -- other --> J["skip / continue"]
H --> K["Create DynamicRouteVar"]
I --> K
K --> L["setattr(cls, param, dynamic_var)"]
L --> M["Update cls.computed_vars,\ncls.vars, inherited vars"]
Last reviewed commit: "skip duplicate Dynam..." |
| # Skip dynamic args that have already been registered by a previous route. | ||
| args = { | ||
| k: v | ||
| for k, v in args.items() | ||
| if not ( | ||
| (computed_var := cls.computed_vars.get(k)) is not None | ||
| and isinstance(computed_var, DynamicRouteVar) | ||
| ) | ||
| } |
There was a problem hiding this comment.
Silent type mismatch when same arg registered with different types
The filter skips any arg already registered as a DynamicRouteVar, regardless of the arg's type (SINGLE vs LIST). If two routes reuse the same parameter name but with different types — e.g., /users/[id] (SINGLE) and /posts/[...id] (LIST) — the first registration always wins silently, causing the wrong accessor (string vs. list) to be used for the second route.
Consider logging a warning when the incoming v (arg type) doesn't match the type that was used when the existing DynamicRouteVar was created, so developers are alerted to the mismatch rather than experiencing subtle runtime bugs:
# Skip dynamic args that have already been registered by a previous route.
args_to_skip = {}
new_args = {}
for k, v in args.items():
existing = cls.computed_vars.get(k)
if existing is not None and isinstance(existing, DynamicRouteVar):
args_to_skip[k] = v
else:
new_args[k] = v
if args_to_skip:
console.warn(
f"Dynamic route args {list(args_to_skip)} are already registered and will be skipped."
)
args = new_argsThis is a low-likelihood edge case (same arg name with different types), but the silent failure mode is worth guarding against.
No description provided.