Skip to content
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

[Feature Request] Warn when multiple route names are the same #2524

Closed
BenJeau opened this issue Aug 9, 2022 · 4 comments · Fixed by #2525
Closed

[Feature Request] Warn when multiple route names are the same #2524

BenJeau opened this issue Aug 9, 2022 · 4 comments · Fixed by #2525

Comments

@BenJeau
Copy link

BenJeau commented Aug 9, 2022

Is your feature request related to a problem? Please describe your use case.
Just spent a good amount of time trying to figure why injections were not working on certain routes, after digging the code a bit more, it seemed like when the route names are the same, the injection will overwrite the value in the SignatureRegistry

Describe the solution you'd like
Something like this, showing which route names are already in the registry and maybe also mentioning which route have injections (although that last one would be for debugging purposes, maybe have a toggle?):

[2022-08-08 21:17:46 -0400] [80724] [DEBUG] Dispatching signal: server.init.after
[2022-08-08 21:23:19 -0400] [81202] [DEBUG] Injecting into route 'manager.cases.get_cases' {'user': (<class 'src.utils.User'>, <Constructor(func=create)>)} 
[2022-08-08 21:23:19 -0400] [81202] [WARNING] Following route has already been injected: 'ioc_manager.templates.get_rule_filters'
[2022-08-08 21:23:19 -0400] [81202] [WARNING] Following route has already been injected: 'ioc_manager.cases.get_case_iocs'
[2022-08-08 21:23:19 -0400] [81202] [WARNING] Following route has already been injected: 'ioc_manager.cases.get_case_iocs'
[2022-08-08 21:17:46 -0400] [80724] [INFO] Starting worker [80724]

For the warning, I've just added a simple check here:

https://github.com/sanic-org/sanic-ext/blob/b65cd9f28c8d1e6ee0aad91685bfb84fc4925ac6/sanic_ext/extensions/injection/registry.py#L54-L59

With

    def register(
        self,
        route_name: str,
        injections: Dict[str, Tuple[Type, Optional[Callable[..., Any]]]],
    ) -> None:
        if route_name in self._registry:
            logger.warning(f"Following route has already been injected: '{route_name}'")
        self._registry[route_name] = injections
@BenJeau
Copy link
Author

BenJeau commented Aug 9, 2022

Not sure if this should be a part of sanic_ext or sanic

@BenJeau
Copy link
Author

BenJeau commented Aug 9, 2022

Another way would be to list, at the beginning when starting the server, all the routes and their route names, and then warning on the duplicate ones (I would personally love this since you sometimes don't know which routes/handlers actually get registered when you have a big codebase with nested blueprints) - unless that's already a feature

@ahopkins
Copy link
Member

ahopkins commented Aug 9, 2022

This is a known thing. We will probably start with a warning and eventually raise an error for duplicate named routes. It will probably be a part of sanic-routing in finalize.

@ahopkins ahopkins transferred this issue from sanic-org/sanic-ext Aug 9, 2022
@ahopkins ahopkins transferred this issue from sanic-org/sanic-routing Aug 9, 2022
@ahopkins
Copy link
Member

ahopkins commented Aug 9, 2022

I am going to solve this in app startup so we can provide a deprecation notice consistently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants