-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Json type #214
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
Json type #214
Conversation
Codecov Report
@@ Coverage Diff @@
## master #214 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 11 11
Lines 1327 1365 +38
Branches 245 249 +4
=====================================
+ Hits 1327 1365 +38 |
pydantic/validators.py
Outdated
|
||
|
||
class Json(Generic[JsonObj]): # defined not in types.py to avoid circular imports | ||
pass |
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.
Move this to types.py
and implement get_validators
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.
I also don't htink you need JsonObj = TypeVar('JsonObj', Dict[str, Any], List)
etc.
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.
I've implemented JsonObj = TypeVar('JsonObj', Dict[str, Any], List)
in order to be possible to write Json[List[int]], without it I was getting exceptions like JsonObj = TypeVar('JsonObj', Dict[str, Any], List)
or TypeError: typing.Union[typing.List, typing.Dict] is not a generic class
. So I implemented Json class a a subclass of typing.Generic. If You know other way, please let me know, because I didn't find it
pydantic/fields.py
Outdated
@@ -181,6 +184,9 @@ def _populate_sub_fields(self): | |||
elif issubclass(origin, Set): | |||
self.type_ = self.type_.__args__[0] | |||
self.shape = Shape.SET | |||
elif issubclass(origin, Json): |
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.
I don't think you need to make any changes in fields.py
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.
The reason behind changes in fields.py
is that Json[List]
had __origin__
attribute, thus in _populate_sub_fields
I was getting AssertionError after origin
was compared against List, Dict etc.
pydantic/errors.py
Outdated
|
||
|
||
class JsonError(PydanticValueError): | ||
msg_template = "{json_str} is not valid JSON and can't be decoded" |
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.
I don't think we should include the original string in the error message, it could be very long.
Ok, sounds like I need to look into it more and getting a better understanding. |
ok, I would use something like class JsonMeta(type):
def __getitem__(self, t):
return type('JsonWrapperValue', (JsonWrapper,), {'inner_type': t})
class Json(metaclass=JsonMeta):
@classmethod
def get_validators(cls):
yield str_validator
yield json_validator
class JsonWrapper:
__slots__ = 'inner_type', Naked use of class Foobar(BaseModel):
x: Json won't need anything special. Parameterised use of class Foobar(BaseModel):
x: Json[List[int]] will need some changes to self.type_ = self.type_.inner_type
self.parse_json = True Then parse json before continuing with normal validation. |
I don't really get why deploy/netlify fails. Log says it failed to clone repository:
|
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.
Looking good, just a bunch of small things to fix.
Netlify was just problems with their service, I restarted the build and it's working now.
pydantic/errors.py
Outdated
|
||
|
||
class JsonError(PydanticValueError): | ||
msg_template = "Not valid JSON provided - it can't be decoded" |
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.
Just Invalid JSON
would be more succinct and clear.
pydantic/fields.py
Outdated
loc = (loc,) | ||
loc = loc if isinstance(loc, tuple) else (loc, ) | ||
|
||
v, error = self._validate_json(v, loc) |
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.
Please can you move the if self.parse_json
to here.
so
if self.parse_json:
v, error = self._validate_json(v, loc)
if error:
...
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.
I moved checking of self.parse_json
to self._validate_json
in order to skip from flake8 warning pydantic/fields.py:244:5: C901 'Field.validate' is too complex (11)
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.
Thought that might be the case, just add # noqa: C901 (ignore complexity)
to the end of the function definition.
pydantic/fields.py
Outdated
if self.parse_json: | ||
try: | ||
return json.loads(v), None | ||
except (json.JSONDecodeError, TypeError): |
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.
you can use ValueError
instead of son.JSONDecodeError
. Do we actually need TypeError
?
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.
and below.
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.
I included TypeError
in pydantic/fields.py
to handle these cases:
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/usr/lib/python3.6/json/__init__.py", line 348, in loads
'not {!r}'.format(s.__class__.__name__))
TypeError: the JSON object must be str, bytes or bytearray, not 'int'```
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.
ok, but that should raise a descendent if TypeError
not ValueError
, best to use a separate except
pydantic/validators.py
Outdated
@@ -239,6 +251,7 @@ def arbitrary_type_validator(v) -> type_: | |||
(time, [parse_time]), | |||
(timedelta, [parse_duration]), | |||
|
|||
(JsonWrapper, [str_validator, json_validator]), |
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 doesn't make any sense, can just be removed.
pydantic/validators.py
Outdated
@@ -13,6 +14,10 @@ | |||
NoneType = type(None) | |||
|
|||
|
|||
class JsonWrapper: |
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 can be moved into types.py
pydantic/validators.py
Outdated
@@ -213,6 +218,13 @@ def path_exists_validator(v) -> Path: | |||
return v | |||
|
|||
|
|||
def json_validator(v: str): |
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.
clearer if this is moved into a validate
class method on Json
pydantic/fields.py
Outdated
@@ -154,11 +158,14 @@ def schema(self, by_alias=True): | |||
|
|||
def _populate_sub_fields(self): | |||
# typing interface is horrible, we have to do some ugly checks | |||
if inspect.isclass(self.type_) and issubclass(self.type_, JsonWrapper): |
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.
just use isinstance(self.type_, type) and issubclass(self.type_, JsonWrapper)
tests/test_types.py
Outdated
assert JsonModel(json_obj=json.dumps(obj)).dict() == {'json_obj': obj} | ||
|
||
|
||
class JsonDetailedModel(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.
can you move the class definition into each test since all the field setup is done here and can fail.
tests/test_types.py
Outdated
class JsonModel(BaseModel): | ||
json_obj: Json | ||
|
||
obj = {'a': 1, 'b': [2, 3]} |
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.
confused to dump then load. Better to use raw JSON strings in the tests
'{"a": 1, "b": [2, 3]}'
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.
and below.
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.
Can you also add a new line to HISTORY.rst and fix coverage.
pydantic/types.py
Outdated
def validate(cls, v: str): | ||
try: | ||
return json.loads(v) | ||
except json.JSONDecodeError: |
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 ValueError
pydantic/fields.py
Outdated
return v, None | ||
try: | ||
return Json.validate(v), None | ||
except (ValueError, TypeError) as exc: |
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.
split ValueError
and TypeError
as below.
pydantic/errors.py
Outdated
|
||
|
||
class JsonNotStrError(PydanticTypeError): | ||
code = 'json_not_str' |
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.
name JsonTypeError
and change code to just 'json'
which will result in the final code being type_error.json
pydantic/errors.py
Outdated
|
||
class JsonNotStrError(PydanticTypeError): | ||
code = 'json_not_str' | ||
msg_template = "JSON object must be str, bytes or bytearray" |
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.
single quotes for strings please, and above.
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.
also can you add a test for pssing in bytes
.
awesome, thank you. We also need to add docs for this. Would you be able to work on that in a separate PR? Otherwise, I'll do it when I get a chance. |
Should I add it to |
I would say best if it was a new section after Exotic Types, or perhaps a new subsection of exotic types. |
Implement #195