neoclient / neoclient

:rocket: Fast API Clients for Python
https://pypi.org/project/neoclient/
MIT License
1 stars 0 forks source link

:bug: Mutations to Path Params Lost #171

Open tombulled opened 1 year ago

tombulled commented 1 year ago
from typing import MutableMapping

from neoclient import PathParams, get, request_depends

def add_path_params(path_params: MutableMapping[str, str] = PathParams()) -> None:
    path_params.update({"endpoint": "get"})

@request_depends(add_path_params)
@get("https://httpbin.org/{endpoint}")
def request():
    ...
>>> request()
╭─────────────────────────────── Traceback (most recent call last) ────────────────────────────────╮
│ <stdin>:1 in <module>                                                                            │
│                                                                                                  │
│ /home/user/Documents/git/neoclient/neoclient/neoclient/operation.py:140 in wrapper               │
│                                                                                                  │
│   137 │   │   │   │   # Read off `self` or `cls`                                                 │
│   138 │   │   │   │   _, *args = args  # type: ignore                                            │
│   139 │   │   │                                                                                  │
│ ❱ 140 │   │   │   return self(*args, **kwargs)                                                   │
│   141 │   │                                                                                      │
│   142 │   │   set_operation(wrapper, self)                                                       │
│   143                                                                                            │
│                                                                                                  │
│ /home/user/Documents/git/neoclient/neoclient/neoclient/operation.py:84 in __call__               │
│                                                                                                  │
│    81 │   │   │   resolve_request(request_dependency, pre_request)                               │
│    82 │   │                                                                                      │
│    83 │   │   # Validate the pre-request (e.g. to ensure no path params have been missed)        │
│ ❱  84 │   │   pre_request.validate()                                                             │
│    85 │   │                                                                                      │
│    86 │   │   request: Request = pre_request.build_request(client)                               │
│    87                                                                                            │
│                                                                                                  │
│ /home/user/Documents/git/neoclient/neoclient/neoclient/models.py:524 in validate                 │
│                                                                                                  │
│   521 │   │                                                                                      │
│   522 │   │   # Validate path params are correct                                                 │
│   523 │   │   if expected_path_params != actual_path_params:                                     │
│ ❱ 524 │   │   │   raise IncompatiblePathParameters(                                              │
│   525 │   │   │   │   f"Expected {tuple(expected_path_params)}, got {tuple(actual_path_params)   │
│   526 │   │   │   )                                                                              │
│   527                                                                                            │
╰──────────────────────────────────────────────────────────────────────────────────────────────────╯
IncompatiblePathParameters: Expected ('endpoint',), got ()
tombulled commented 1 year ago

This is the source of the bug:

from typing import Mapping
from pydantic import BaseModel

class Request(BaseModel):
    path_params: Mapping[str, str]

path_params: Mapping[str, str] = {"name": "sam", "age": "43"}

request: Request = Request(path_params=path_params)
>>> id(path_params)
140541875083008
>>> id(request.path_params)
140541856807808
>>> request.path_params is path_params
False

Pydantic is shallow/deep copying the path params. As a Pydantic model is built and instantiated every time resolution happens (e.g. for each request dependency), a fresh copy of the path params is provided each time, so mutations are immediately lost

tombulled commented 1 year ago

This bug is slightly more complicated than previously thought & affects more than just path params.

tombulled commented 1 year ago

There's a copy_on_model_validation config option which defaults to "shallow". Setting it to "none" should fix this issue.

Ref: https://docs.pydantic.dev/1.10/usage/model_config/

tombulled commented 1 year ago

Much time has been spent trying to crack this one, to no avail. I think pydantic is using dict(v) to "validate" the path params under the hood, which naturally creates a copy of the data.

This could be accepted as the nature of the beast, and if you want to modify request attributes, you have to depend upon PreRequest, however that feels less than ideal.

tombulled commented 1 year ago

Could investigate moving to Pydantic V2 and using strict mode?

tombulled commented 1 year ago

Type coercion is wanted during response resolution, but not wanted during request resolution (aka. composition)

tombulled commented 1 year ago

Proven to also affect headers:

from typing import MutableMapping

from neoclient import Headers, get, request_depends

def add_headers(headers: MutableMapping[str, str] = Headers()) -> None:
    headers.update({"name": "sam"})

@request_depends(add_headers)
@get("https://httpbin.org/headers")
def request():
    ...
>>> request()
{
  'headers': {
    'Accept': '*/*',
    'Accept-Encoding': 'gzip, deflate',
    'Host': 'httpbin.org',
    'User-Agent': 'neoclient/0.1.55'
  }
}
tombulled commented 1 year ago

After some experimentation with Pydantic V2, it appears that this can be mitigated by leveraging WrapValidator

from typing import Mapping, Annotated
from pydantic import BaseModel, ConfigDict, WrapValidator

class Headers(dict):
    pass

def validate_headers(v, handler):
    if isinstance(v, Headers):
        return v

    return handler(v)

ValidatedHeaders = Annotated[Mapping[str, str], WrapValidator(validate_headers)]

class Request(BaseModel):
    model_config = ConfigDict(strict=True)

    headers: ValidatedHeaders

raw_headers: Mapping[str, str] = {"name": "sam", "age": "43"}
headers: Headers = Headers(raw_headers)

request: Request = Request(headers=headers)
request2: Request = Request(headers=raw_headers)

print(id(headers))
print(id(request.headers))
print(id(request2.headers))

assert request.headers is headers
tombulled commented 1 year ago

Soft blocked by #177