Git Product home page Git Product logo

Comments (10)

michaeltoohig avatar michaeltoohig commented on August 22, 2024 1

Here is a single file example. It does not cause the issue when tested with sqlite, just postgres. I included the fastapi-users setup as well since that's what I'm working with but I believe it can be ignored from my quick test commenting out code related to it but just in case I left it here.

import databases
import sqlalchemy
from fastapi import FastAPI, Request
from fastapi_crudrouter import DatabasesCRUDRouter
from fastapi_users import FastAPIUsers, models
from fastapi_users.authentication import JWTAuthentication
from fastapi_users.db import SQLAlchemyBaseUserTable, SQLAlchemyUserDatabase
from sqlalchemy.ext.declarative import DeclarativeMeta, declarative_base
from sqlalchemy.orm import sessionmaker
from sqlalchemy import Column, Integer, String
from pydantic import BaseModel

DATABASE_URL = "postgresql://test:test@localhost/test"
SECRET = "SECRET"

app = FastAPI()


#
## SCHEMAS
#


class User(models.BaseUser):
    pass

class UserCreate(models.BaseUserCreate):
    pass

class UserUpdate(User, models.BaseUserUpdate):
    pass

class UserDB(User, models.BaseUserDB):
    pass


class ThingCreate(BaseModel):
    name: str

class Thing(ThingCreate):
    id: int


#
## PERSISTENCE
#


database = databases.Database(DATABASE_URL)
Base: DeclarativeMeta = declarative_base()


class UserTable(Base, SQLAlchemyBaseUserTable):
    pass

class ThingTable(Base):
    __tablename__ = 'things'
    id = Column(Integer, primary_key=True)
    name = Column(String, nullable=False)

engine = sqlalchemy.create_engine(DATABASE_URL, pool_pre_ping=True)
SessionLocal = sessionmaker(autocommit=False, autoflush=False, bind=engine)
Base.metadata.create_all(engine)


#
## FASTAPI USERS
#


users = UserTable.__table__
user_db = SQLAlchemyUserDatabase(UserDB, database, users)


def on_after_register(user: UserDB, request: Request):
    print(f"User {user.id} has registered.")


def on_after_forgot_password(user: UserDB, token: str, request: Request):
    print(f"User {user.id} has forgot their password. Reset token: {token}")


def after_verification_request(user: UserDB, token: str, request: Request):
    print(f"Verification requested for user {user.id}. Verification token: {token}")


jwt_authentication = JWTAuthentication(
    secret=SECRET, lifetime_seconds=3600, tokenUrl="auth/jwt/login"
)


fastapi_users = FastAPIUsers(
    user_db,
    [jwt_authentication],
    User,
    UserCreate,
    UserUpdate,
    UserDB,
)
app.include_router(
    fastapi_users.get_auth_router(jwt_authentication), prefix="/auth/jwt", tags=["auth"]
)
app.include_router(
    fastapi_users.get_register_router(on_after_register), prefix="/auth", tags=["auth"]
)
app.include_router(
    fastapi_users.get_reset_password_router(
        SECRET, after_forgot_password=on_after_forgot_password
    ),
    prefix="/auth",
    tags=["auth"],
)
app.include_router(
    fastapi_users.get_verify_router(
        SECRET, after_verification_request=after_verification_request
    ),
    prefix="/auth",
    tags=["auth"],
)
app.include_router(fastapi_users.get_users_router(), prefix="/users", tags=["users"])


@app.on_event("startup")
async def startup():
    await database.connect()


@app.on_event("shutdown")
async def shutdown():
    await database.disconnect()


#
## FASTAPI CRUDROUTER
#


thing_router = DatabasesCRUDRouter(
    schema=Thing,
    create_schema=ThingCreate,
    table=ThingTable.__table__,
    database=database,
    delete_all_route=False,
)
app.include_router(thing_router)

And the output of my project requirements

aioredis==2.0.0
aiosqlite==0.17.0
alembic==1.6.5
arq==0.21
asgiref==3.4.1
async-timeout==3.0.1
asyncpg==0.24.0
bcrypt==3.2.0
cffi==1.14.6
click==8.0.1
databases==0.4.3
deprecated==1.2.12
dnspython==2.1.0
email-validator==1.1.3
fastapi==0.68.0
fastapi-crudrouter==0.8.1
fastapi-users==6.1.2
h11==0.12.0
idna==3.2
makefun==1.11.3
mako==1.1.4
markupsafe==2.0.1
passlib==1.7.4
psycopg2-binary==2.9.1
pycparser==2.20
pydantic==1.8.2
pyjwt==2.1.0
python-dateutil==2.8.2
python-editor==1.0.4
python-multipart==0.0.5
redis==3.5.3
six==1.16.0
sqlalchemy==1.3.24
starlette==0.14.2
tenacity==8.0.1
typing-extensions==3.10.0.0
uvicorn==0.15.0
wrapt==1.12.1

from fastapi-crudrouter.

awtkns avatar awtkns commented on August 22, 2024 1

@michaeltoohig thanks for the code. I was able to get the tests failing. Hoping to implement the fetch and delete in separate transactions. This should resolve you issue.

from fastapi-crudrouter.

awtkns avatar awtkns commented on August 22, 2024

@michaeltoohig thanks for reporting this, would you be to provide me with a minable runnable example?

from fastapi-crudrouter.

awtkns avatar awtkns commented on August 22, 2024

I added a test in #96 which disables the delete_all route while keeping the delete_one route enabled. It passes, I am thinking I could be due to something in your dependancy.

from fastapi-crudrouter.

michaeltoohig avatar michaeltoohig commented on August 22, 2024

Sorry, I didn't make a reproducible example earlier. Either way, I looked further into to confirm the issue is not the dependency. I tested by both removing the dependency all together and by overriding the endpoint.

def _delete_one(self, *args: Any, **kwargs: Any) -> CALLABLE:
        async def route(item_id: self._pk_type) -> Model:  # type: ignore
            query = self.table.delete().where(self._pk_col == item_id)

            row = await self._get_one()(item_id)  # <--- here is the source of the 404
            rid = await self.db.execute(query=query)

            if rid:
                return row
            else:
                raise NOT_FOUND

        return route

In the _delete_one endpoint the above marked line is the point that raises the 404 HTTPException. I paused at the line before and in my database the row has already been removed. So now I need to look more into either my database config (standard postgres 13 docker image), or project config is committing the delete too early.

database = databases.Database(settings.SQLALCHEMY_DATABASE_URI)
engine = create_engine(
    settings.SQLALCHEMY_DATABASE_URI,
    pool_pre_ping=True,
)
SessionLocal = sessionmaker(autocommit=False, autoflush=False, bind=engine)

So I think the issue is database does not have autoflush=False or equivalent, which is used by the CRUDRouter. In a similar looking issue (I may be wrong) encode/databases#219 the databases project passes blame down to asyncpg.

from fastapi-crudrouter.

awtkns avatar awtkns commented on August 22, 2024

@michaeltoohig thanks for the detailed response, this certainly sounds like a bug, Assuming the id is in the database, it should be returned on the line row = await self._get_one()(item_id). We do not even execute the delete till after, so this is certainly strange.

async def route(item_id: self._pk_type) -> Model:  # type: ignore
  query = self.table.delete().where(self._pk_col == item_id)  # <--- Here we are just building the query to execute later
  
  row = await self._get_one()(item_id) <- getting the object
  rid = await self.db.execute(query=query)  # <--- The object get removed here
  
  if rid:
    return row
  else:
    raise NOT_FOUND

By chance do you have a single file that I could run to reproduce the issue? From there I may able to implement a fix.

from fastapi-crudrouter.

awtkns avatar awtkns commented on August 22, 2024

Also mentioned here: kvesteri/sqlalchemy-continuum#259

from fastapi-crudrouter.

awtkns avatar awtkns commented on August 22, 2024

@michaeltoohig after much digging, the issue seems to stem from this: tiangolo/fastapi#1369 (answered here: tiangolo/fastapi#1369 (comment)). I am also not an expert on encode/databases but you maybe be also to fix it using this.

I am also implementing a fix for it natively in crudrouter. You can test out a sample of it with:

pip3 install wheel
pip3 install git+https://github.com/awtkns/fastapi-crudrouter.git@86_asyncpg_databases

Let me know if that works for you. πŸ˜„

from fastapi-crudrouter.

michaeltoohig avatar michaeltoohig commented on August 22, 2024

That does work, now I get the 200 response as expected.

Much appreciated!

from fastapi-crudrouter.

awtkns avatar awtkns commented on August 22, 2024

Reopened until fix is merged.

from fastapi-crudrouter.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    πŸ–– Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. πŸ“ŠπŸ“ˆπŸŽ‰

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❀️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.