Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughSubRouter's constructor is refactored to remove the file/name parameter, now accepting only prefix and dependencies. Correspondingly, BaseRobyn initialization is simplified while Robyn assumes responsibility for OpenAPI setup and dev-mode handling. All documentation and integration tests are updated to reflect the new API. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 5❌ Failed checks (5 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
robyn/__init__.py (2)
79-95:⚠️ Potential issue | 🟠 MajorFix mutable default argument shared across instances:
DependencyMap()
dependencies: DependencyMap = DependencyMap()creates a shared default instance across all callers who don't pass an explicit value. This pattern appears inBaseRobyn.__init__(line 79),Robyn.__init__(line 557), andSubRouter.__init__(line 691).Since
DependencyMaphas mutating methods (add_router_dependency,add_global_dependency,merge_dependencies) that are actively called in the codebase, multiple instances will inadvertently share and mutate the sameDependencyMapif created without explicitly passing a value. This causes state to leak across independent instances.Use
Noneas default and instantiate inside the method:def __init__(self, dependencies: DependencyMap = None) -> None: self.dependencies = dependencies if dependencies is not None else DependencyMap()
496-497:⚠️ Potential issue | 🔴 Critical
include_routeronBaseRobynreferences attributes only defined onRobyn—will crash onSubRouter.
self.configandself.openapiare only initialized inRobyn.__init__, not inBaseRobyn.__init__. Sinceinclude_routeris defined onBaseRobyn(line 480), calling it on aSubRouterinstance will raiseAttributeErrorwhen accessingself.configat line 496.Additionally, line 497 has a separate bug:
self.openapi.add_subrouter_paths(self.openapi)passesself's OpenAPI object to its own method instead of the router being included. The parameter should be the incoming router's OpenAPI data (or relevant paths). SinceSubRouterhas noopenapiattribute at all, this logic should not live onBaseRobyn—it belongs only inRobyn.include_router.Proposed fix
Move OpenAPI handling to
Robyn.include_routerby overriding the method, or guard attribute access withhasattr:- if not self.config.disable_openapi and self.openapi is not None: - self.openapi.add_subrouter_paths(self.openapi) + if hasattr(self, "config") and not self.config.disable_openapi and hasattr(self, "openapi") and self.openapi is not None: + self.openapi.add_subrouter_paths(router.router)Or better: override
include_routerinRobynto handle OpenAPI integration separately, keepingBaseRobyn.include_routerfocused on route merging.
🧹 Nitpick comments (1)
robyn/__init__.py (1)
583-607: Minor inconsistency inROBYN_CLIenv var handling.Line 569 uses
bool(os.environ.get("ROBYN_CLI", False))while line 600 usesos.getenv("ROBYN_CLI", False)directly. Both treat any truthy string as enabled, but the inconsistent patterns make the intent less clear. Consider unifying to a single helper or consistent idiom.
1 participant
Description
This PR fixes #1189
Summary
This PR does a much needed refactor in subrouter
PR Checklist
Please ensure that:
Pre-Commit Instructions:
Summary by CodeRabbit
Release Notes
Refactor
Documentation