search

igorbenav / fastcrud

FastCRUD is a Python package for FastAPI, offering robust async CRUD operations and flexible endpoint creation utilities.
MIT License
530 stars 32 forks source link

FastCRUD class docs don't match signature #68

Closed slaarti closed 1 month ago

slaarti commented 1 month ago

Describe the bug or question

According to the documentation, the FastCRUD class takes optional create, update, and delete schemas as arguments, but this doesn't make sense according to the calling signature for FastCRUD.__init__() and indeed it doesn't seem to work in practice.

(As a secondary but related matter, the documentation references a bunch of example model/schema classes that are never fully defined anywhere, including the code and tests, which made figuring out how to articulate this fairly tricky. If what the docs say is supposed to work, it's something that probably needs some actual tests written for to confirm.)

To Reproduce

I had to do a little fudging using the Item model/schema classes to get a set that'd work for the User model referenced in the docs, but it seems to me like this:

from fastcrud import FastCRUD
from pydantic import BaseModel
from sqlalchemy import Boolean, Column, DateTime, Integer, String
from sqlalchemy.orm import DeclarativeBase

class Base(DeclarativeBase):
    pass

class User(Base):
    __tablename__ = "user"
    id = Column(Integer, primary_key=True)
    name = Column(String)
    archived = Column(Boolean, default=False)
    archived_at = Column(DateTime)

class UserCreateSchema(BaseModel):
    name: str

class UserUpdateSchema(BaseModel):
    name: str

should work for this (Example 6 from the page listed above, in turn taken from the FastCRUD class doc string), and yet:

>>> custom_user_crud = FastCRUD(User, UserCreateSchema, UserUpdateSchema, is_deleted_column="archived", deleted_at_column="archived_at")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: FastCRUD.__init__() got multiple values for argument 'is_deleted_column'

For that matter, Example 1 seems like it works, but the case of it "working" is actually a typing violation:

>>> user_crud = FastCRUD(User, UserCreateSchema, UserUpdateSchema)
>>> user_crud.is_deleted_column
<class '__main__.UserCreateSchema'>
>>> user_crud.deleted_at_column
<class '__main__.UserUpdateSchema'>
igorbenav commented 1 month ago

Hey, @slaarti, thanks for the issue! I'll fix it. I believe I basically took stuff from the written tests for the docs, so it should work, but version changes and human errors mess everything up.

slaarti commented 1 month ago

Okay, well, I went through all of the docs/, fastcrud/, and tests/ directories, and compiled all of the tables and schemas that were referenced (sometimes in multiple places, but I just noted where I first saw them) and/or defined. Sometimes there are not-quite-duplicates, like UserCreateSchema and CreateUserSchema, which I noted down separately but should probably be unified somehow.

Item

Order

Customer

Project

Participant

User

Tier

Tier and TierModel are nearly identical, except that TierModel has a field relating it to the ModelTest table; are they supposed to be the same? There's also a TierSchemaTest in the conftest.py files; should TierSchema be renamed to this?

Department

Task

MyModel

Role

Product

Comment

ModelTest

Booking

Customer

OtherModel

igorbenav commented 1 month ago

That is really great work, thank you so much! Do you want to also fix stuff and make a pr?

slaarti commented 1 month ago

Uhhh, that depends on the specific stuff. Like, if it's "put together class defs for the models/schemas that currently lack them based on what the docs/code use them for -- IMO ideally in the process cleaning up stuff like there being both (as an example) ProductCreateSchema and CreateProductSchema -- and putting them somewhere that can be referenced" ? Sure, I can at least take a crack at that.

However, none of this really resolves the problem that sparked this issue, which is that all of the examples in the FastCRUD class docstring rely on invoking the class in ways that don't make sense given the actual code, which is something I have no idea what to do about. You're probably going to have to handle that one yourself, since I assume you had some kind of intention/plan with that.

igorbenav commented 1 month ago

Yeah, sure, I can fix the docstring examples. I can also fix the docs, no problem, just wondering if you'd like to do it.

slaarti commented 1 month ago

I'm down to split the work: I can put together the example models/schemas and clean up the docs accordingly, while you fix the docstrings? If that works for you, I can create a separate issue that can be assigned to me for my part, while this one remains for yours.

igorbenav commented 1 month ago

Sounds good to me! Thanks

igorbenav commented 1 month ago 
>>> user_crud = FastCRUD(User, UserCreateSchema, UserUpdateSchema)
>>> user_crud.is_deleted_column
<class '__main__.UserCreateSchema'>
>>> user_crud.deleted_at_column
<class '__main__.UserUpdateSchema'>

The issue here is it should be something like this:

CRUDUser = FastCRUD[User, UserCreateInternal, UserUpdate, UserUpdateInternal, UserDelete]
crud_users = CRUDUser(User)

And it would work. This is useful for typing, but indeed it's wrong in the way I wrote it. I'm fixing the docstring stuff

slaarti commented 1 month ago

Aha, that does make a lot more sense, yes. And I see your commits with the fix. Thank you!