-
Notifications
You must be signed in to change notification settings - Fork 757
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(json): revert eager check #2926
Conversation
Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## main #2926 +/- ##
==========================================
- Coverage 70.82% 70.70% -0.12%
==========================================
Files 104 104
Lines 9435 9442 +7
==========================================
- Hits 6682 6676 -6
- Misses 2753 2766 +13
|
Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
acea23b
to
741a86f
Compare
import pydantic | ||
|
||
# This is to prevent cases where custom JSON encoder is used. | ||
if isinstance(obj, pydantic.BaseModel): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use if self._pydantic_model
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not correct right? checking object instance is independent of whether self._pydantic_model
is set or not.
validate should be raised from error Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
@@ -241,14 +241,16 @@ async def from_http_request(self, request: Request) -> JSONType: | |||
pydantic_model = self._pydantic_model.parse_obj(json_obj) | |||
return pydantic_model | |||
except pydantic.ValidationError as e: | |||
raise BadInput(f"Invalid JSON input received: {e}") from None | |||
raise BadInput(f"Invalid JSON input received: {e}") from e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, do you know why this was None
before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sauyon @aarnphm https://peps.python.org/pep-0415/ that was for better exception context
This PR removes an eager check for a pydantic object.
Our JSONEncoder should already handle pydantic object lazily
https://bentoml.slack.com/archives/CKRANBHPH/p1660930201114169