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

testing pydantic v1.10.0a2 #401

Closed

Conversation

samuelcolvin
Copy link
Contributor

TO NOT MERGE

This is pull request is just to test pydantic v1.10, see pydantic/pydantic#4359

@samuelcolvin
Copy link
Contributor Author

Hi, please can you kick off tests so I can check if v1.10 will break anything.

@Goldziher
Copy link
Contributor

Hi, please can you kick off tests so I can check if v1.10 will break anything.

sure thing.

@Goldziher
Copy link
Contributor

we actually have a pending issue for this --> #394

@Goldziher
Copy link
Contributor

are you certain it installed? because the lock file has not been modified.

@samuelcolvin
Copy link
Contributor Author

Oh, 🤦 I was working through quite a few libraries.

@samuelcolvin
Copy link
Contributor Author

hopefully that's working, can you kick off tests.

BTW, the project will be much more attractive to contribute to if you remove the "awaiting approval", see here.

@Goldziher
Copy link
Contributor

hopefully that's working, can you kick off tests.

BTW, the project will be much more attractive to contribute to if you remove the "awaiting approval", see here.

I changed the settings. Since you are not new user on github, you should be golden 😉

I'd love some contributions from you ofc. So with that hurdle out of the way... lol

@samuelcolvin
Copy link
Contributor Author

Okay, this is actually failing. Could it be related to pydantic/pydantic#3061?

@Goldziher
Copy link
Contributor

Okay, this is actually failing. Could it be related to pydantic/pydantic#3061?

I'll have to debug for ascertain what's the failure. Will do this shortly.

@samuelcolvin
Copy link
Contributor Author

I'm looking into it now, seems it's not pydantic/pydantic#3061.

Anything you find would be helpful.

While investigating that issue, I've found another that only applies when pydantic is not compiled, trying to isolate it now.

@Goldziher
Copy link
Contributor

So, looking at this test:

from starlette.status import HTTP_201_CREATED

from starlite import Body, RequestEncodingType, post
from starlite.testing import create_test_client
from tests.kwargs import Form


def test_request_body_url_encoded() -> None:
    body = Body(media_type=RequestEncodingType.URL_ENCODED)

    test_path = "/test"
    data = Form(name="Moishe Zuchmir", age=30, programmer=True).dict()

    @post(path=test_path)
    def test_method(data: Form = body) -> None:
        assert isinstance(data, Form)

    with create_test_client(test_method) as client:
        response = client.post(test_path, data=data)
        assert response.status_code == HTTP_201_CREATED

The model called Form is defined as:

from pydantic import BaseModel


class Form(BaseModel):
    name: str
    age: int
    programmer: bool

And we are generating a pydantic model from the route handler that has a __field__ attribute like this:

Screenshot 2022-08-24 at 18 08 10

Yes, when the generated model is called using the data kwarg, a validation exception is raised:

Screenshot 2022-08-24 at 18 05 00

This is raised here:

   @classmethod
    def parse_values_from_connection_kwargs(
        cls, connection: Union[Request, WebSocket], **kwargs: Any
    ) -> Dict[str, Any]:
        """Given a dictionary of values extracted from the connection, create
        an instance of the given SignatureModel subclass and return the parsed
        values.

        This is not equivalent to calling the '.dict'  method of the
        pydantic model, because it doesn't convert nested values into
        dictionary, just extracts the data from the signature model
        """
        try:
            signature = cls(**kwargs)
            return {key: signature.resolve_field_value(key) for key in cls.__fields__}
        except ValidationError as exc:
            raise cls.construct_exception(connection, exc) from exc

@samuelcolvin
Copy link
Contributor Author

Also, see pydantic/pydantic#4432.

That issue is probably caused by pydantic/pydantic#4253, I'll look into once I've fixed the other one.

@samuelcolvin
Copy link
Contributor Author

@Goldziher the remaining bug should be fixed in #402.

I think pydantic is now correct and starlite is incorrect, hence I'd prefer to keep the change in pydantic and fix compatibility in starlite.

Let me know what you think.

@Goldziher
Copy link
Contributor

@Goldziher the remaining bug should be fixed in #402.

I think pydantic is now correct and starlite is incorrect, hence I'd prefer to keep the change in pydantic and fix compatibility in starlite.

Let me know what you think.

great, thanks. I merged your PR.

@samuelcolvin
Copy link
Contributor Author

All passing, thanks so much for your help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants