fix: PUT API response does not send ValidationError when expected #235
Conversation
There was a problem hiding this comment.
Hey @Abbas-Askari, I added some comments. @CharlesNepote @alexgarel please feel free to add anything else.
folksonomy/api.py
Outdated
| if product_tag.version != latest_version + 1: | ||
| raise HTTPException( | ||
| status_code=422, | ||
| detail=[ | ||
| { | ||
| "type": "value_error", | ||
| "loc": ["body", "version"], | ||
| "msg": f"Value error, version must be exactly {latest_version + 1}", | ||
| "input": product_tag.version, | ||
| } | ||
| ], | ||
| ) |
There was a problem hiding this comment.
I think you'll need to use Pydantic models for Validation errors. They should give you a json automatically.
There was a problem hiding this comment.
@aadarsh-ram Ofc, validation errors should be dealt with in Pydantic models, But I didn't want to doa DB fetch in the model, I think DB fetches should be performed at the endpoint function.
Current validation of version in the pydantic model:
@field_validator('version')
def version_check(cls, version):
if version < 1:
raise ValueError('version must be greater or equal to 1')
return versionI can, however, move it to there if you say so.
There was a problem hiding this comment.
Ah, I meant it'll be better if the JSON sent was constructed from an Pydantic error, rather than you constructing it. The current validation model doesn't need to be changed.
There was a problem hiding this comment.
Oh, Okay. I'll change.
There was a problem hiding this comment.
@Abbas-Askari I would not do a specific model for that, but you could isolate the dict creation in a function to improve readability.
| if not latest_version_row: | ||
| raise HTTPException(status_code=404, detail="Key was not found") |
There was a problem hiding this comment.
I think this is being checked already. If you're adding this, please remove the other instance.
There was a problem hiding this comment.
@aadarsh-ram great to see you around 🙂 !
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #235 +/- ##
==========================================
- Coverage 95.06% 94.53% -0.54%
==========================================
Files 5 5
Lines 324 384 +60
==========================================
+ Hits 308 363 +55
- Misses 16 21 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Great @Abbas-Askari this looks good to me :-)
Only thing maybe, make create_version_error a private function, that is just rename it _create_version_error.
I approve the PR but you can do the change before merging (provided tests pass).
Ok, I'll change |
|
@alexgarel Done! |
|
@Abbas-Askari can you fix the tests ? |
|
Great @Abbas-Askari ! |
What
Fix the issue described in #117. The API previously just sent
{"detail":"next version must be equal to 2, was 1"}instead of a ValidationError. Now the endpoint first fetches the current version from db, and send a validation error back if the version is not properly incremented.Screenshot
{ "detail": [ { "type": "value_error", "loc": [ "body", "version" ], "msg": "Value error, version must be exactly 2", "input": 4 } ] }Fixes bug(s)