zmievsa / cadwyn

Production-ready community-driven modern Stripe-like API versioning in FastAPI
https://docs.cadwyn.dev/
MIT License
184 stars 24 forks source link

Documentation for migrating schemas that are nested external to the head package #191

Open gabloe opened 1 month ago

gabloe commented 1 month ago

Is your feature request related to a problem? Please describe. I am evaluating Cadwyn as an option for my API to enable versioning. Many of my application routes use a paginated response model using FastAPI-Pagination, and in these cases the response model is wrapped with / nested in a "Page" model that exists outside of my head schemas package. In my route I will query for data using Beanie, and then project the response.

Consider the below demo project:

demo.schemas.head.customer.py:

def CustomerBase(BaseModel):
   name: str = Field(...)
   address: str = Field(...)

def CustomerIn(CustomerBase):
   pass

def CustomerOut(CustomerBase):
   pass

demo.routes.customer.py:

from demo.models import Customer    # Beanie document model
from demo.exceptions import CustomerNotFoundError
from demo.schemas.head import CustomerIn, CustomerOut
from cadwyn import VersionedAPIRouter
from beanie import PydanticObjectId
from fastapi_pagination import Page

router = VersionedAPIRouter(
    prefix="/customer",
    tags=["customer"],
)

@router.get(
    "/",
    response_model=Page[CustomerOut],
    description="List customers.",
)
async def list_customers(
    customer: CustomerIn,
):
    return await paginate(Customer)

@router.get(
    "/{id}",
    response_model=CustomerOut,
    description="Get customers by ID.",
)
async def get_customer(
    id: PydanticObjectId,
):
    cus = await Customer.get(id)
    if cus is None:
       raise CustomerNotFoundError()
    return customer.model_dump()

I wanted to get an understanding about version migrations so I made a very simple change to the CustomerBase schema by adding an address field, and I wanted to test whether I could enforce that field to be present only in newer API versions. I added a address field to my CustomerBase in the head package, and added a version change to specify that the field did not previously exist.

demo.versions.v2024_05_31.py:

from cadwyn.structure import VersionChange, schema
from demo.schemas.head import CustomerBase

class AddressDidntExist(VersionChange):
    description = "..."
    instructions_to_migrate_to_previous_version = (
        schema(CustomerBase).field("address").didnt_exist,
    )

This works fine for the get_customer route. In the older API version the address is not present in the response, and in the newer version the address is present in the response. But, the list_customers route which uses a wrapped response model includes the address field in both versions.

My assumption here is that because my version change migration is applied to CustomerBase and not Page[CustomerBase] the migration is not getting applied.

The paginated response in the old version looks like:

{
   "items": [
      {
         "id": "665909337b970ec73402d0ed",
         "name": "John Doe",
         "address": "123 Cherry Lane"
      }
   ],
   "total": 1,
   "page": 1,
   "size": 10,
   "pages": 1
}

And when using the get_customer route (non-paginated) I see the expected response after migration.

{
   "id": "665909337b970ec73402d0ed",
   "name": "John Doe"
}

My question is whether this type of migration is supported with Cadwyn? Is it possible to provide migration instructions for schemas that are wrapped with another schema (in this case the Page) which exist outside of my head package?

zmievsa commented 1 month ago

I am afraid that the migration of internal schemas within external schemas is not supported yet. In fact, migrating generic response models might also be hard right now. Additionally, adding an address to the response is not a breaking change so I would recommend to just add it into all versions.

I am currently drafting a feature that would add support for external schemas but it won't be ready any time soon, sadly. I recommend implementing those classes internally

gabloe commented 1 month ago

Ok, got it. Yeah I know it’s not a breaking change. I was just doing a very simple test to evaluate the capabilities of Cadwyn.

zmievsa commented 1 month ago

I'll try to push it forward in my backlog to speed up the process

zmievsa commented 3 weeks ago

It's the second thing in my backlog. So if you are willing to wait a little bit, I think it will actually happen relatively soon