this change fixes an issue we encountered with pydantic validator… #899
Conversation
…corators
- create_model() is now called using a __base__ parameter vs. a __config__ parameter
- isinstance comparisons now work as expected
- unit test created for fastapi#889 still passes as before
- apply black formatting
Codecov Report
@@ Coverage Diff @@
## master #899 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 293 293
Lines 7692 7701 +9
=====================================
+ Hits 7692 7701 +9
Continue to review full report at Codecov.
|
| use_type = create_model( | ||
| original_type.__name__, __config__=original_type.__config__ | ||
| ) | ||
| use_type = create_model(original_type.__name__, __base__=original_type) |
There was a problem hiding this comment.
This change looks correct to me, based on the current v1.4 definition, and old v0.32 definition.
We encountered the same issue, so great to see a fix!
fastapi/utils.py
Outdated
| use_type = create_model( | ||
| original_type.__name__, __config__=original_type.__config__ | ||
| ) | ||
| use_type = create_model(original_type.__name__, __base__=original_type) | ||
| for f in original_type.__fields__.values(): | ||
| use_type.__fields__[f.name] = create_cloned_field(f) | ||
| use_type.__validators__ = original_type.__validators__ |
There was a problem hiding this comment.
It does seem like there may be a simpler way to refactor this bit. However I found that if I remove the create_model() call on line 96 it fails 2 of the unit tests so I don't know of a better solution for now:
======================================================== FAILURES =========================================================
__________________________________________________ test_filter_sub_model __________________________________________________
def test_filter_sub_model():
response = client.get("/model")
assert response.status_code == 200
> assert response.json() == {
"name": "model-a-name",
"description": "model-a-desc",
"model_b": {"username": "test-user"},
}
E AssertionError: assert {'description...model-a-name'} == {'description...model-a-name'}
E Omitting 2 identical items, use -vv to show
E Differing items:
E {'model_b': {'password': 'test-password', 'username': 'test-user'}} != {'model_b': {'username': 'test-user'}}
E Use -v to get the full diff
tests/test_filter_pydantic_sub_model.py:92: AssertionError
_______________________________________________________ test_token ________________________________________________________
def test_token():
access_token = get_access_token(scope="me")
response = client.get(
"/users/me", headers={"Authorization": f"Bearer {access_token}"}
)
assert response.status_code == 200
> assert response.json() == {
"username": "johndoe",
"full_name": "John Doe",
"email": "johndoe@example.com",
"disabled": False,
}
E AssertionError: assert {'disabled': ...Gga31lW', ...} == {'disabled': ...e': 'johndoe'}
E Omitting 4 identical items, use -vv to show
E Left contains 1 more item:
E {'hashed_password': '$2b$12$EixZaYVK1fsbw1ZfbX3OXePaWxn96p36WQoeG6Lruj3vjPGga31lW'}
E Use -v to get the full diff
tests/test_tutorial/test_security/test_tutorial005.py:231: AssertionError
============================================== 2 failed, 744 passed in 7.04s ==============================================
There was a problem hiding this comment.
I found removing line 99 still had the tests pass:
--- a/fastapi/utils.py
+++ b/fastapi/utils.py
@@ -96,7 +96,6 @@ def create_cloned_field(field: ModelField) -> ModelField:
use_type = create_model(original_type.__name__, __base__=original_type)
for f in original_type.__fields__.values():
use_type.__fields__[f.name] = create_cloned_field(f)
- use_type.__validators__ = original_type.__validators__
if PYDANTIC_1:
new_field = ModelField(
name=field.name,There was a problem hiding this comment.
Nice, I found same. Updated the branch and unit tests.
- update related unit test
| @@ -8,10 +8,20 @@ | |||
| class ModelB(BaseModel): | |||
| username: str | |||
|
|
|||
| @validator("username") | |||
| def check_username(cls, username, values): | |||
There was a problem hiding this comment.
does adding this test new scenario? If the tests here don't trigger a failure case, I don't think adding these two validators adds anything, whereas testing for failure would make sure that the validators are inherited (worth checking in other places to make sure that isn't covered elsewhere, as this module seems to have a specific scenario in mind)
|
Thanks for your contribution @deuce2367 ! 🎉 🍰 And thanks for the review and analysis @Code0x58 ! 🤓 🔍 I updated it a bit to test that the validator is preserved and merged it 🎉 🚀 |
… decorators