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

Allow to create request scopes outside of the middleware #12

Merged
merged 1 commit into from
May 19, 2023

Conversation

soldag
Copy link
Contributor

@soldag soldag commented May 15, 2023

I started to implement something very similar to this project on my own, but was very happy to find this nice little library, which almost does all the stuff I need in order to use injector in my FastAPI application. So thanks for the work first of all!

However, it's lacking one particular feature: It would be great if there's a possibility to create request scopes also outside of the middleware. Background of this is that in my application I need to process messages I get from a message queue. Same as for requests it would be nice if it's possible to scope the lifetime of services to the handling of a single message. I could imagine there are more, similar cases where this might be helpful, for example background tasks (i.e. celery) or cronjobs. For sure I could write my own scope for this, but then I would more or less need to duplicate all of your work, so I thought it would make more sense to integrate it directly into your library.

To achieve this, I moved the logic of assigning request ids from the InjectorMiddleware to a new class called RequestScopeFactory, which allows create request scopes also outside the middleware. This could then be used for example like:

class MessageHandler:
    def __init__(self, request_scope_factory: Inject[RequestScopeFactory]) -> None:
        self.request_scope_factory = request_scope_factory

    async def handle(self, message: Any) -> None:
        with self.request_scope_factory.create_scope():
            # process message

What do you think about this?

@matyasrichter
Copy link
Owner

Hi Sören!
Sorry for the delay.

This looks great, thanks for the contribution! Looks like CI is broken, but that seems unrelated to your changes. I will merge your PR when I figure out what's wrong with it, but I'm a bit busy at the moment.

@soldag
Copy link
Contributor Author

soldag commented May 19, 2023

I actually had the same problem also when I wanted to run pre-commit locally. Looks like the issue can be fixed by updating isort (home-assistant/core#86892 (comment)). It now works for me. I pushed the change to my branch so once you approve the workflow execution we'll see if it also fixes CI.

@matyasrichter
Copy link
Owner

I will fix things up in #13 so that you can rebase your branch.

@matyasrichter matyasrichter merged commit e2e4488 into matyasrichter:main May 19, 2023
10 checks passed
@matyasrichter
Copy link
Owner

I've merged this and created a new release, https://github.com/matyasrichter/fastapi-injector/releases/tag/0.5.0. Thanks for your PR!

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.

None yet

2 participants