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

Fix issue for extra fields not appearing in the errors #845

Closed
wants to merge 13 commits into from

Conversation

nofalx
Copy link
Contributor

@nofalx nofalx commented Sep 7, 2023

Solves Issue #835

Reference for impact of this change:
https://docs.pydantic.dev/latest/usage/validators/#model-validators

@nofalx
Copy link
Contributor Author

nofalx commented Sep 7, 2023

This PR actually fixes another issue as well:

when u have Schema as attribute for another schema:

class UserTokenType(Schema):
    meta_data: MetaDataSchema | None = None

and you send this json

{
    "meta_data": "xxx",
}

Django ninja reports that schema is correct, however after the change in this pr we get the correct error that we missed before

{
    "error": "validation_errors",
    "details": [
        {
            "field_name": "meta_data",
            "msg": "Input should be a valid dictionary or object to extract fields from",
            "type": "model_attributes_type"
        }
    ]

@vitalik
Copy link
Owner

vitalik commented Sep 7, 2023

well it's not that easy (as you see from failed tests)

I think there must be some trick so that pydantic thinks DjangoGetter is a dict

@vitalik
Copy link
Owner

vitalik commented Sep 8, 2023

Hi @nofalx

regarding - from_orm ? did you find this somewhere on pydantic code ? I think they deprecated this keyword in favour to "from_attributes"...

@baseplate-admin
Copy link
Contributor

Hi, Please use black on your code

@nofalx
Copy link
Contributor Author

nofalx commented Sep 8, 2023

@vitalik turns out the logic does not depend on the from_attributes but more on if the source of validate if from from model_validate or somewhere else.

I dont understand the inners of pydantic very well but I have tested the update code in a django projects and all scenarios seem to be working fine for root of the json and nested json shema we using for the API. The projects tests are all passing as well

@nofalx nofalx force-pushed the extra-fields-validations branch 2 times, most recently from c391a9e to efb7311 Compare September 11, 2023 09:13
@nofalx
Copy link
Contributor Author

nofalx commented Sep 11, 2023

Added new test cases to cover the issues this pr fixed. Code now passes lint and tests

ninja/signature/details.py Outdated Show resolved Hide resolved
@nofalx nofalx requested a review from vitalik September 11, 2023 18:48
@vitalik
Copy link
Owner

vitalik commented Sep 15, 2023

There might be also development on pydantic side for this: pydantic/pydantic#5434 will check if that covers this case as well

@vitalik
Copy link
Owner

vitalik commented Sep 25, 2023

Hi @nofalx

I simplified a bit your approach (used class.model_config to find out if default validaion wrapper is needed) - see here for more details - #863 - let me know if that works ?

@nofalx
Copy link
Contributor Author

nofalx commented Sep 25, 2023

Hi @vitalik

Looks like a better approach, hope this makes it to next beta

@nofalx nofalx closed this Sep 25, 2023
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.

3 participants