Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Planning request migrations #11

Closed
zmievsa opened this issue Sep 6, 2023 · 6 comments
Closed

Planning request migrations #11

zmievsa opened this issue Sep 6, 2023 · 6 comments
Labels
enhancement New feature or request

Comments

@zmievsa
Copy link
Owner

zmievsa commented Sep 6, 2023

No description provided.

@zmievsa zmievsa added the enhancement New feature or request label Sep 6, 2023
@zmievsa zmievsa changed the title Consider adding migrations for requests too Add support for migrations for requests too Sep 10, 2023
@zmievsa
Copy link
Owner Author

zmievsa commented Sep 10, 2023

Migrating requests is a much harder task because there can be so many things different about the requests. There can be a different body, and if it is different -- how do we force fastapi to use a different schema for it? Probably will need to integrate into the app itself.

There can be different params, which is also increasingly hard to maintain.

On a side note.. How do we handle different pagination params on requests? Do we even support such things? Any thoughts, research, tests, or implementation is welcome!

@zmievsa zmievsa changed the title Add support for migrations for requests too Planning request migrations Sep 14, 2023
@zmievsa zmievsa added this to the Request Migrations milestone Sep 14, 2023
@zmievsa
Copy link
Owner Author

zmievsa commented Sep 14, 2023

First, we should consider the types of breaking changes that we can have and order them by frequency:

  1. Any change in a body schema with body schema being a single dependency (POST/PUT/PATCH)
  2. Any change in the list of query params for GET
  3. The alteration of required headers/query params/body params as separate dependencies

(1) and (2) will most likely require separate mechanisms. However, all three and any edge cases can be temporarily handled by allowing users to replace the endpoint with another one with different code.

@zmievsa
Copy link
Owner Author

zmievsa commented Sep 15, 2023

  • Single-param bodies are bodies that consist of a single schema
  • Multi-param bodies consist of multiple schemas/fields such as the ones added with fastapi.Body

Body params

It seems possible to fully handle single-param bodies. We do need to consider the fact that we cannot migrate data as simply as we do with responses. Given versions 1, 2, 3, 4, we receive a 1 request and we have request body migrations from 1 to 2, and from 3 to 4. We cannot migrate request using both (1->2, 3->4) migrations as it can produce incorrect results. So all body migrations must effectively form a chain.

We also have to consider the fact that request schemas do not always change. It would be great if we could remove unchanged schemas from unions. Though it might be entirely unnecessary as full migrations will provide the same results. But it can still be a POSSIBLE FEATURE.

Note that we could optimize serialization and editing of body by adding a request dependency to route, popping it from kwargs, getting body from it, and passing it through migrators. Then finally, parsing it with the resulting schema from latest migrator. If there is already a request dependency, we can simply use it instead.

Query/Header/Cookie params

There are no single-param form of these types of params. So there are only two ways to modify/migrate them:

  1. Come up with a general-purpose approach for doing so (not even sure if it's possible without an enormous overcomplication). This would also make it possible to handle single-param bodies gracefully
  2. Make an editing interface strictly for a small set of overly simplistic situations

Forms

No idea how to handle them. Need help and research here!

Alternative

The following approach would allow us to handle all request cases at once:

from fastapi import Query, Body, Header

router = VersionedAPIRouter()

@router.post("/test")
async def test(a: str = Query(), b: str = Body(), c: str = Header()):
    ...

class ChangeAddressToList(VersionChange):
    description = "..."

    @convert_request_to_next_version_for("/test", ["POST"])
    def change_header_and_body(cls, b: str, c: str):
        # do anything with b and c here
        return b, c

It will not allow us to add or remove params but it will allow for modifying any kinds of arguments. Though there should probably be a separate way to migrate body json separately to optimize it.
So this is not a fully-fledged solution too :(

@zmievsa
Copy link
Owner Author

zmievsa commented Sep 16, 2023

I might've figured this out. Problems and solutions for them. Big thanks to @emilskra for guidance.

Request and response migrations have vastly different tooling

So far we have been viewing request and response migrations as vastly different because request migrations much more often affect business logic and because fastapi's handling of requests is vastly different. However, it makes a lot more sense to unify approaches for them. So I present the redesign of how we could write request and response migrations. It gives us a lot more functionality and is a lot easier to understand for the users.

def migration(response):
    response.body["hello"] = True
    response.headers["gamarjoba"] = request.headers.pop("hi")
    response.status_code = 200


def request_migration(request):
    request.body["hello"] = True
    request.headers["gamarjoba"] = request.headers.pop("hi")
    request.query["hello"] = "world"

So in essence we give our users ability to fine-tune how they handle responses and we give our users ability to migrate requests in the same manner. In order to do this for requests, we will have to marshall request.body from pydantic on the first attribute access and then unmarshall it back after we are done with all request migrations. This will also remove the need to have the nasty union schemas.

However, let's say a new version doesn't have some of the fields of the old version. How would we handle that? How do we use that field after a migrated body has been unmarshalled? We add a _-prefixed attribute that is marked as PrivateAttr with a sane default for new version models and delete it in the old version. The prefix will hide the attribute from openapi. For example, if client_id is only passed in an old request model, we add a _migrated_client_id field to the new model and use it from business logic.

It also makes sense to replace "had_property" with "property.was".

Replacing individual params (query, header, cookie)

endpoint("/v1/users/{user_id}", ["GET"]).dependency("param_name").was(Query()) # 1
endpoint("/v1/users/{user_id}", ["GET"]).dependency("param_name").was(Depends(get_param_dependency)) # 2

@endpoint("/v1/users/{user_id}", ["GET"]).dependency("param_name").was # 3
def apply_some_changes_to_dependency():
    def any_name_here(anything_here: str = Query()):
        # Do any actions with the dependency here
        return anything_here


    return Depends(any_name_here)
  1. (accepts an instance of Header/Query/Cookie) Allows you to change the type (header/query/cookie) or config of any dependency. Body should not be supported!
  2. (accepts an instance of Depends) Allows you to change some dependency entirely to another function. It's useful when you want to change dependency's logic without affecting your route. However, sometimes type hints of the dependency cause circular imports from the business logic which is why we have Path 3.
  3. (accepts a callable that returns a callable) Is same as 2 except that it allows you to import things after the migration has been defined so it solves the problem of circular imports. It's needed extremely rarely.

You can use APIRoute.dependency_overrides_provider for 3. Not sure if you should though...

Rejected solutions

@dependency("api.v1.users:some_dependency").was
def anything_here():
   ...

It is really implicit and prone to errors. It doesn't really give us anything in terms of functionality over the solutions above so I decided that it was a bad idea to include it.

Changing logic for handling params or removing the param completely

At this point it makes a lot more sense to just replace the route function.

@zmievsa
Copy link
Owner Author

zmievsa commented Oct 8, 2023

I have realized something that solves all problems with request unions and migrations of requests:

We don't migrate our responses from latest version to all versions and we don't migrate our requests to latest version. We migrate our responses from our internal data representation to any version and we migrate our requests from any version to our internal representation. And our internal representations contain unions of all data.

That way we will be able to throw out unions dir, FillablePrivateAttr, and schema(...).property while providing our users with a more coherent and full interface and gaining a request-response parity in terms of how we handle them.

This also means that many tasks related to preventing the generation of schemas are no longer necessary.

Note, however, that internal representations shouldn't generally lie within versioned schemas. They are essentially unversioned.

@zmievsa
Copy link
Owner Author

zmievsa commented Oct 8, 2023

Implemented. Yes, it really is as great and as simple as I expected -- even simpler. This officially concludes our request migration planning as all necessary features have either been planned or implemented.

@zmievsa zmievsa closed this as completed Oct 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant