Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

dependency_overrides requires unnecessary arguments with Protocol/ABC in Swagger #4118

Closed
9 tasks done
GLEF1X opened this issue Oct 31, 2021 · 11 comments
Closed
9 tasks done

Comments

@GLEF1X
Copy link

GLEF1X commented Oct 31, 2021

First Check

  • I added a very descriptive title to this issue.
  • I used the GitHub search to find a similar issue and didn't find it.
  • I searched the FastAPI documentation, with the integrated search.
  • I already searched in Google "How to X in FastAPI" and didn't find any information.
  • I already read and followed all the tutorial in the docs and didn't find an answer.
  • I already checked if it is not related to FastAPI but to Pydantic.
  • I already checked if it is not related to FastAPI but to Swagger UI.
  • I already checked if it is not related to FastAPI but to ReDoc.

Commit to Help

  • I commit to help with one of those options 👆

Example Code

from __future__ import annotations

import dataclasses
from typing import Protocol

import uvicorn
from fastapi import FastAPI, Depends, Response

app = FastAPI()


@dataclasses.dataclass()
class Foo:
    id: int


class BaseRepo(Protocol):
    def get_foo(self, some_id: int) -> Foo: ...


class ConcreteRepo:
    def get_foo(self, some_id: int) -> Foo:
        return Foo(id=some_id)


@app.get("/hello/world")
async def view(repo: BaseRepo = Depends(BaseRepo)):
    return Response(200)


app.dependency_overrides[BaseRepo] = lambda: ConcreteRepo()  # also without lambda works incorrect

if __name__ == '__main__':
    uvicorn.run(app)

Description

  • Open /docs route and see that API method requires *args and **kwargs for request (probably from metaclass of Protocol)

Operating System

Linux

Operating System Details

No response

FastAPI Version

0.70.0

Python Version

3.9.7

Additional Context

No response

@GLEF1X GLEF1X added the question Question or problem label Oct 31, 2021
GLEF1X added a commit to GLEF1X/fastapi that referenced this issue Oct 31, 2021
GLEF1X added a commit to GLEF1X/fastapi that referenced this issue Oct 31, 2021
@GLEF1X
Copy link
Author

GLEF1X commented Oct 31, 2021

#4119

@marple-git
Copy link

👌

@Tishka17
Copy link

Is there any plans to fix this?

Dependency injection feature is totally unusable because of this:

  • We cannot use classes with __init__ as a dependency, because all init args are shown in swagger, though they are have no relation to API calls
  • We cannot use Protocol as a dependency as well, because random args and kwargs appear in swgger. Though, protocols cannot have instances

There is no way to use types as a dependencies, only possible solution is to create stub functions which looks very dirty.

@MaksimZayats
Copy link

This library probably solves this problem
https://github.com/MaximZayats/fastapi-better-di

@GLEF1X
Copy link
Author

GLEF1X commented Apr 18, 2022

This library probably solves this problem https://github.com/MaximZayats/fastapi-better-di

It's not a quite great solution of this issue due to what this lib actually does. Patching FastAPI is an implicit spike to overcome FastAPI DI's flaw. I think it would be better to apply my brief PR #4119

@SebastianKrings
Copy link

would this also allow to override dependencies taking arguments?
like this:

`@app.get("/hello/world")
async def view(repo: BaseRepo = Depends(BaseRepo("anyArg"))):
return Response(200)

app.dependency_overrides[BaseRepo] = lambda: ConcreteRepo() #override even without giving an argument`

@tiangolo
Copy link
Owner

Hey there! I see that in your examples BaseRepo and ConcreteRepo don't have an __init__ method, so they don't require other dependencies, don't require data from the request, don't integrate with OpenAPI or OAuth2. Why do you want to put them in dependencies instead of creating the instances in the code?

Using things in dependencies is normally for when there's a reason to use them, because otherwise, it's simpler and more explicit to call the code directly in your function.

I want to understand your use case to see why it would make sense to support this, to achieve what, and see if there's another way to do it.

@GLEF1X
Copy link
Author

GLEF1X commented Aug 26, 2022

Hi, @tiangolo! Thank you for your response! Example with BaseRepo and ConcreteRepo is simplified and was made just to show that the problem exists.

I believe, usage of DI using dependency_overrides is more farsighted and for big applications extremely useful, because it follows DI conception and gains all advantages of this approach.
From wiki:

In software engineering, dependency injection is a design pattern in which an object or function receives other objects or functions that it depends on. A form of inversion of control, dependency injection aims to separate the concerns of constructing objects and using them, leading to loosely coupled programs.[1][2][3] The pattern ensures that an object or function which wants to use a given service should not have to know how to construct those services. Instead, the receiving 'client' (object or function) is provided with its dependencies by external code (an 'injector'), which it is not aware of.[4] Dependency injection helps by making implicit dependencies explicit and helps solve the following problems:[5]

How can a class be independent from the creation of the objects it depends on?

  • How can an application, and the objects it uses support different configurations?
  • How can the behavior of a piece of code be changed without editing it directly?
  • Fundamentally, dependency injection consists of passing parameters to a method.[6]

Because the client does not build or find the service itself, it typically only needs to declare the interfaces of the services it uses, rather than their concrete implementations. This makes it easier to change which services are actually used at runtime, especially in statically-typed languages where changing the underlying objects would otherwise require re-compiling the source code.

I want to emphasize this:

The pattern ensures that an object or function which wants to use a given service should not have to know how to construct those services.

In practice approach with using Depends with dependency_overrides makes code much more cleaner. For instance, I need to pass a UserRepository instance to the request handler. In main function, where app is being created I do something like this - in this example i use a workaround(empty classes) for this issue, but with fix of this issue it would be just an interface(abc class or protocol or just a plain class). So in request handler now I can just use Depends to get instance of UserRepository - code example. If I want to add caching or logging to UserRepository I won't touch the request handler's code. Instead of it, I will implement the Decorator pattern and just change the code of dependency_overrides.

Maybe you will ask why it's better than something like the get_db. It's not bad approach, but it violates the DI philosophy.

def get_db():
    session = session_maker()
    try:
        yield session
    finally:
        session.close()
 
@app.get("/")
async def handler(db: Session = Depends(get_db)):
    return {"message": "Hello World"}

In this example, the handler knows about the factory of dependency and strongly tied to it(the module with handler must import it).

@github-actions github-actions bot removed the answered label Aug 26, 2022
@JarroVGIT
Copy link
Contributor

It's not bad approach, but it violates the DI philosophy.

But why is that? To re-quote what you wanted to emphasise:

The pattern ensures that an object or function which wants to use a given service should not have to know how to construct those services.

Having a signature of async def handler(db: Session = Depends(get_db)): complies with that statement, no? The handler function here still has no notion of the construction of a Session object, only how to get an instance of it (so yes, it knows the factory). The argument the handler now has a strongly tied because it needs to import it now is not very strong imho, because in the alternative it would need to import BaseRepo instead.

I can see this making sense when you, on runtime, want to determine what implementation of BaseRepo must be passed to the path operation function, but that is also not possible with the proposed solution (as it would override all cases of BaseRepo).

It is an interesting idea though!

@GLEF1X
Copy link
Author

GLEF1X commented Sep 27, 2022

Hello, I've found related issue #617 with the same problem but with a more complicated solution. With dependency_overrides the problem of this issue can be solved easily.

For instance this:

from fastapi import FastAPI, Depends
from sqlalchemy import create_engine
from sqlalchemy.orm import sessionmaker, Session

app = FastAPI()
engine = create_engine("sqlite://")
session_maker = sessionmaker()


def get_db_session():
    session = session_maker()
    try:
        yield session
    finally:
        session.close()


@app.get("/")
async def root(session: Session = Depends(get_db_session)):
    pass

Can be written this way with dependency_overrides:

import uvicorn
from fastapi import FastAPI, Depends
from sqlalchemy import create_engine
from sqlalchemy.orm import sessionmaker, Session


def provide_session(session_maker: sessionmaker):
    def inner():
        session = session_maker()
        try:
            yield session
        finally:
            session.close()

    return inner


def create_app():
    app = FastAPI()

    engine = create_engine("sqlite://", echo=True, echo_pool=True)
    session_maker = sessionmaker(engine)
    app.dependency_overrides[Session] = provide_session(session_maker)
    return app


app = create_app()


@app.get("/")
async def root(session: Session = Depends(Session)):
    print(session)


uvicorn.run(app)

No global variables - fewer problems

Another related issue: #4183

@jbmusso
Copy link

jbmusso commented Nov 16, 2022

I'm having the same issue as @GLEF1X and I can confirm the fix in #4119 solves this issue.

The above solution is how I supposed FastAPI would work:

  • args and kwargs should not appear in Swagger when using Protocol
  • I'd rather specify interfaces as Protocol in route handler rather than concrete implementations (which needs to be imported at the module-level, only to be overridden later in tests)

@GLEF1X 's solution is rather elegant and I believe is similar to several dependency injection approaches in other languages.

Roughly, I'm following a clean architecture / hexagonal architecture approach and only rely on interfaces (Protocol) for specifying dependencies.

edit: sorry, I hit shift+enter too quickly

Repository owner locked and limited conversation to collaborators Feb 28, 2023
@tiangolo tiangolo converted this issue into discussion #8668 Feb 28, 2023

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Projects
None yet
Development

No branches or pull requests

8 participants