fastapi-utils icon indicating copy to clipboard operation
fastapi-utils copied to clipboard

Added the base_route and crud_base

Open nyejon opened this issue 4 years ago • 5 comments

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 avatar Mar 06 '20 20:03 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?

nyejon avatar Mar 06 '20 21:03 nyejon

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.

dmontagu avatar Mar 07 '20 06:03 dmontagu

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 avatar Mar 07 '20 11:03 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 avatar Mar 07 '20 11:03 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?

nyejon avatar Apr 05 '20 07:04 nyejon