Added the base_route and crud_base

[nyejon]

Hi @dmontagu!

This is my initial attempt at defining both the base_route and the crud_base.

Both of these can be used to start a CRUD API
and adds filtering and sorting to the read_many endpoint

Let me know your thoughts and then I will revise and add tests etc.

I'm not sure how to add packages.. Just did poetry add but seems to have changed a few things that shouldn't have been changed.

[nyejon]

I'm a bit confused how to solve the mypy issues with the

"(got "Base", expected "ResponseModelType")"

I thought orm_mode would try to accommodate this?

[dmontagu]

I think this should probably raise some kind of validation error if invalid filter fields are specified (i.e., kwargs contains things that aren't used).

In general, I really prefer to avoid using **kwargs as an input for type safety considerations, but it feels like it may be appropriate here. But I think we should add some validation of the specified filter field values.

[dmontagu]

Is there a benefit to this being a dependency rather than just a class var? That is to say, why not just use

    ...
    filter_fields: typing.ClassVar[List[str]] = []

and then you can override the list in a subclass if desired? Also, if this approach makes sense, it may be better to use a tuple rather than a list for this to ensure immutability.

I think that approach should work fine, and would have a bit less overhead too. And you could still override with a dependency if you wanted to. (That could maybe go into docs.)

[nyejon]

Oh that's exactly what I was looking for. Didn't know how to stop the class variables showing up in the openapi spec

[dmontagu]

Do you think it would make sense to change the name here to CRUDRoute rather than BaseRoute? BaseRoute seems a little overly generic to me.

[dmontagu]

Any chance we could put this file (crud_base.py) and (base_route.py) into a crud subfolder/package? It could be fastapi_utils.crud.base and fastapi_utils.crud.route.

[nyejon]

Sure

[dmontagu]

I think it should be possible for the user to specify their own base class, not the one provided by the library. Otherwise I think it would be difficult/impossible to override things like the table naming scheme or similar.

Moreover, it could add some annoyances when using alembic for migrations.

I'm not sure how hard it would be to refactor to allow a user-specified Base, but I think we should try to support that if possible.

[dmontagu]

As noted above, for multiple reasons I would much prefer if it were possible for the user to provide their own declarative_base(), rather than being forced into using the one defined in this module.

I think it may be challenging to get the type hinting right, but I think the functionality benefits are worth the associated challenge with the ModelType TypeVar.

I suspect even in the worst case there's probably something we can do with an if TYPE_CHECKING: block to make it work nicely for most/all cases.

[dmontagu]

This looks great! There are some changes I think we should make before merging but I think it shouldn't be too hard to get it there.

Don't worry too much about the mypy / poetry issues for now if you run into trouble -- I can help figure them out once the above comments are addressed.

You might want to rebase on top of the latest commit to master since I updated various dependencies there (and you'll probably get merge conflicts with the lock file; you can just ignore any conflicts and regenerate it from the pyproject.toml).

The change you made to pyproject.toml is fine, though I think both sqlalchemy and sqlalchemy-filters should both be made optional (since there is some functionality provided by this library that requires neither). You should be able to run make lock && make develop to update the lock file.

Since the lockfile is only used during development I don't mind if it gets updated frequently. It would be more of a problem if lots of people were simultaneously making PRs, but I don't think we need to worry about it for now.

[nyejon]

Would you know of a way to register routes without needing to override every route method?

This kinda needs the new Starlette way of registering routes rather than the decorator method used in FastAPI?

[nyejon]

Another consideration, on the get_many method it is often necessary to return some additional metadata to indicate how many records there are.

This should then be more than just a List[ResponseModel]

For example, in Django Rest Framework they return the result for get many like this:

{
    "count": 1,
    "next": null,
    "previous": null,
    "results": [
        {
            "url": "https://restframework.herokuapp.com/users/1/",
            "id": 1,
            "username": "admin",
            "snippets": [
                "https://restframework.herokuapp.com/snippets/1/"
            ]
        }
    ]
}

I would propose then using a separate MultiSchemaType that could be defined with a default, but could then be overridden by the user if they want to change their API format.

The problem I have now is in the CRUDBase returning query.all(), when we actually might need some additional data like the count. I could create a separate return type for this too?

[nyejon]

Do you know how to get mypy to detect the crud_base would have the get_many, get, delete methods?

[nyejon]

Hi @dmontagu

I would like to continue working on this now that I have a bit more time again.

I am a bit stuck with how we could add all of the base routes to the fastapi router.

As it currently stands, the only way I can see is that a user would have to manually add each of the routes with the router.get(), router.post etc decorator.

Do you know how I could get around this? It would be nice if the class could have a router property, that could be added to the parent router like usual with app.include_router(base_routes.router) or similar...

Edit:

Maybe I'm missing something here with how to develop for fastapi. Do you use graphql for CRUD stuff and then the REST part for other things? Do you add pydantic validation to GraphQL mutations?