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

Move handler execution into a separate class method #36

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

LeafyLappa
Copy link

The change allows consumers of the library to modify exactly how handler resource methods are called. My case is dependency injection (with lagom):

class ResourceWithDependencyInjection(Resource):
    dependency_container: Container

    @classmethod
    async def execute_handler(cls, request, request_context, handler_name, *args, **kwargs):
        resource = cls(request, request_context, *args, **kwargs)
        handler = getattr(resource, handler_name, None)
        handler_with_deps = cls.dependency_container.partial(handler)
        response = await handler_with_deps(*args, **kwargs)
        return response

    @classmethod
    def register_routes(cls, app, base_path: str = '', container: Container = Container()):
        cls.dependency_container = container
        super().register_routes(app, base_path)

Regardless, it's a nice refactor in the spirit of SRP.

@vladmunteanu
Copy link
Owner

This is a nice proposal @LeafyLappa, thanks for the PR!
I'm going to merge this, but I would really appreciate if you had some time to write a brief section in the documentation about dependency injection and how you use it.
Although I'm personally not a heavy user of dependency injection, I can definitely understand why people accustomed with FastAPI or other frameworks would be happy to have this functionality here.

@LeafyLappa
Copy link
Author

This PR does not add dependency injection per se, it only allowed me to write a simple (likely not at all scalable, extensible or even thread safe) integration for lagom and a rather quirky one at how it does its thing... storing the dependency container inside a class field, freely accessible from inside the handler. I could publish it, I feel its place would be at https://lagom-di.readthedocs.io/en/latest/framework_integrations though, and then it could be mentioned in the documentation of this project!

@vladmunteanu
Copy link
Owner

@LeafyLappa

This PR does not add dependency injection per se

Yes, that was clear.

Well, no rush with this, but I think it would be cool to see an example nonetheless.

@vladmunteanu
Copy link
Owner

@LeafyLappa The test suite found some extra white spaces in there apparently.

@LeafyLappa
Copy link
Author

Please run it again, the last commit should fix the issue.

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

Successfully merging this pull request may close these issues.

2 participants