document 404 response

[sondrelg]

Closes #103, #102

PR Includes 404 responses for the retrieve, update, and delete endpoints.

Context

I started working on this thinking that crudrouter manually documented the 200 and 422 response, but I've come to suspect that this is built-into FastAPI - is that right?

Passing responses to fastapi.routing.add_api_route seems to satisfy the wip test I created. I also tested with

Left to do

Before finalizing, I was hoping to run the test suite here @awtkns - I had some trouble setting up some of the test requirements locally 👏 And please let me know if you have any suggestions for improvement.

And if you have any ideas about where to include tests, that would be great! So far I just put a wip test in test_openapi_schema.py which seemed fitting.

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/flortz/fastapi-crudrouter/Cm1xoiicEYtMrFQ9H6h6wzQ5XsoN
✅ Preview: https://fastapi-crudrouter-git-fork-sondrelg-sondrelg-doc-96371c-flortz.vercel.app

[awtkns]

Awesome thanks so much for this! I am also tempted to include schemathesis in the test suite to validate the correctness of the generated OpenApi spec. This will be merged and released sometime over the next couple days,

[awtkns]

Not needed if done as described below 😄

[awtkns]

Suggested change

	responses=RESPONSES,
	responses=[NOT_FOUND],

Minor: It may be cleaner to add the field responses: [HTTPException] = None to the _add_api_route. In the _add_api_route function. You could then extract the status code / detail of the exception, reducing code duplication.

[sondrelg]

This won't work unfortunately - the responses must be a mapping, so we would need something like a dictionary merge. But I guess we could also just define a RESPONSES to contain everything needed.

What would you prefer? 🙂

The type needs to be Optional[Dict[Union[int, str], Dict[str, Any]]] and it fails with TypeError: 'list' object is not a mapping if you try to pass a list.

[sondrelg]

I was imagining either

NOT_FOUND_RESPONSE = {"404": {"detail": "Item not found"}}
FORBIDDEN_RESPONSE = {"403": {"detail": "Forbidden"}}
INTERNAL_SERVER_ERROR_RESPONSE = {"500": {"detail": "Server error"}}
RESPONSES = NOT_FOUND_RESPONSE | FORBIDDEN_RESPONSE | INTERNAL_SERVER_ERROR_RESPONSE

or just

RESPONSES = {
    "404": {"detail": "Item not found"}, 
    "403": {"detail": "Forbidden"}, 
    "500": {"detail": "Server error"}
}

[sondrelg]

I implemented the last one in eb489bb, but let me know if you prefer something else 👏

[Stranger6667]

Hi there :)

I am also tempted to include schemathesis in the test suite to validate the correctness of the generated OpenApi spec.

Awesome! Let me know if you'll need any adjustments on the Schemathesis side for your use case :)

[sondrelg]

I think this is done btw @awtkns 🙂 Let me know if you want any other changes implemented 👍

[awtkns]

Thanks for this @sondrelg 🎉

Changes will be released to version 0.8.3 on merge.

[sondrelg]

@awtkns I looked at this a little more, and I think my implementation here might be a little bit off. If you're planning to implement schemathesis I'm sure you'll spot it, but leaving this here in case it's helpful.

Looking through these docs, it looks like this is the way to specify responses:

class NotFoundModel(BaseModel):
    detail: str


@router.get('/{item_id}/', status_code=200, response_model=Model, responses={'404': {'model': NotFoundModel}})
async def retrieve(item_id: int) -> Model:
    try:
        return await Model.objects.get(id=item_id)
    except NoMatch:
        raise HTTPException(status_code=404, detail="Item not found")

In other words, I think this PR fixed one schemathesis issue: the missing response code, but didn't fully resolve it, since the model isn't correctly specified.

[sondrelg]

Not sure how helpful this is, since it's not 1:1 with the crudrouter implementation, but here's a full example of most of the crudrouter endpoints, using ormar, after adjusting them so they pass schemathesis tests:

import logging
from typing import Optional

from asyncpg import UniqueViolationError, CharacterNotInRepertoireError
from fastapi import APIRouter, HTTPException
from ormar import NoMatch
from pydantic import BaseModel

from payments.models import Author

logger = logging.getLogger(__name__)

author_router = APIRouter(prefix='/authors')


class HTTPExceptionMessage(BaseModel):
    detail: str


responses = {
    'not_found': {'404': {'model': HTTPExceptionMessage}},
    'validation_error': {'422': {'model': HTTPExceptionMessage}}
}


@author_router.get('/', status_code=200, response_model=list[Author])
async def get(skip: Optional[int] = 0, limit: Optional[int] = None) -> list[Author]:
    qs = Author.objects.filter()
    if skip:
        qs.offset(skip)
    if limit:
        qs.limit(limit)
    return await qs.all()


AuthorCreateModel = Author.get_pydantic(exclude={'id', 'books'})


@author_router.post('/', status_code=201, response_model=Author, responses=responses['validation_error'])
async def create(author: AuthorCreateModel) -> Author:
    try:
        return await Author.objects.create(**author.dict())
    except UniqueViolationError:
        raise HTTPException(422, detail="Name is taken")
    except CharacterNotInRepertoireError:
        raise HTTPException(422, detail="Invalid encoding")


@author_router.delete('/', status_code=204)
async def delete_all() -> None:
    authors = await Author.objects.all()
    for author in authors:
        await author.delete()


@author_router.get('/{item_id}/', status_code=200, response_model=Author, responses=responses["not_found"])
async def retrieve(item_id: int) -> Author:
    try:
        return await Author.objects.get(id=item_id)
    except NoMatch:
        raise HTTPException(status_code=404, detail="Item not found")


AuthorUpdateModel = Author.get_pydantic(exclude={'id', 'books'})


@author_router.put('/{item_id}/', status_code=200, response_model=Author, responses=responses['not_found'])
async def update(item_id: int, updated_author: AuthorUpdateModel) -> Author:
    try:
        author = await Author.objects.get(id=item_id)
    except NoMatch:
        raise HTTPException(status_code=404, detail="Item not found")

    await author.update(**updated_author.dict(exclude_unset=True))
    return author


@author_router.delete('/{item_id}/', status_code=204, responses=responses['not_found'])
async def delete(item_id: int) -> None:
    try:
        author = await Author.objects.get(id=item_id)
        await author.delete()
    except NoMatch:
        raise HTTPException(status_code=404, detail="Item not found")

[awtkns]

Thanks again for the detailed write up @sondrelg! I've created an issue to track this in #105.

Have you written any additional code to setup Schemathesis for testing against the file above? Or are you just doing it from the cli?

[sondrelg]

Just from the CLI for now 👍