Skip to content

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

Merged
merged 9 commits into from
Jul 10, 2018
Merged

Json type #214

merged 9 commits into from
Jul 10, 2018

Conversation

oldPadavan
Copy link
Contributor

Implement #195

@codecov
Copy link

codecov bot commented Jun 29, 2018

Codecov Report

Merging #214 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #214   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          11     11           
  Lines        1327   1365   +38     
  Branches      245    249    +4     
=====================================
+ Hits         1327   1365   +38



class Json(Generic[JsonObj]): # defined not in types.py to avoid circular imports
pass
Copy link
Member

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

Copy link
Member

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.

Copy link
Contributor Author

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

@@ -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):
Copy link
Member

@samuelcolvin samuelcolvin Jun 29, 2018

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

Copy link
Contributor Author

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.



class JsonError(PydanticValueError):
msg_template = "{json_str} is not valid JSON and can't be decoded"
Copy link
Member

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.

@samuelcolvin
Copy link
Member

Ok, sounds like I need to look into it more and getting a better understanding.

@samuelcolvin
Copy link
Member

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 Json eg.

class Foobar(BaseModel):
    x: Json

won't need anything special.

Parameterised use of Json eg.

class Foobar(BaseModel):
    x: Json[List[int]]

will need some changes to fields.py, basically you'll just need

self.type_ = self.type_.inner_type
self.parse_json = True

Then parse json before continuing with normal validation.

@oldPadavan
Copy link
Contributor Author

I don't really get why deploy/netlify fails. Log says it failed to clone repository:

10:55:36 AM: git clone [email protected]:samuelcolvin/pydantic
10:56:10 AM: Error cloning repository: [email protected]:samuelcolvin/pydantic
10:56:10 AM: Failing build: Failed to prepare repo
10:56:10 AM: failed during stage 'preparing repo': exit status 128

Copy link
Member

@samuelcolvin samuelcolvin left a 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.



class JsonError(PydanticValueError):
msg_template = "Not valid JSON provided - it can't be decoded"
Copy link
Member

@samuelcolvin samuelcolvin Jul 10, 2018

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.

loc = (loc,)
loc = loc if isinstance(loc, tuple) else (loc, )

v, error = self._validate_json(v, loc)
Copy link
Member

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:
        ...

Copy link
Contributor Author

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)

Copy link
Member

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.

if self.parse_json:
try:
return json.loads(v), None
except (json.JSONDecodeError, TypeError):
Copy link
Member

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and below.

Copy link
Contributor Author

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'```

Copy link
Member

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

@@ -239,6 +251,7 @@ def arbitrary_type_validator(v) -> type_:
(time, [parse_time]),
(timedelta, [parse_duration]),

(JsonWrapper, [str_validator, json_validator]),
Copy link
Member

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.

@@ -13,6 +14,10 @@
NoneType = type(None)


class JsonWrapper:
Copy link
Member

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

@@ -213,6 +218,13 @@ def path_exists_validator(v) -> Path:
return v


def json_validator(v: str):
Copy link
Member

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

@@ -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):
Copy link
Member

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)

assert JsonModel(json_obj=json.dumps(obj)).dict() == {'json_obj': obj}


class JsonDetailedModel(BaseModel):
Copy link
Member

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.

class JsonModel(BaseModel):
json_obj: Json

obj = {'a': 1, 'b': [2, 3]}
Copy link
Member

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]}'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and below.

Copy link
Member

@samuelcolvin samuelcolvin left a 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.

def validate(cls, v: str):
try:
return json.loads(v)
except json.JSONDecodeError:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use ValueError

return v, None
try:
return Json.validate(v), None
except (ValueError, TypeError) as exc:
Copy link
Member

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.



class JsonNotStrError(PydanticTypeError):
code = 'json_not_str'
Copy link
Member

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


class JsonNotStrError(PydanticTypeError):
code = 'json_not_str'
msg_template = "JSON object must be str, bytes or bytearray"
Copy link
Member

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.

Copy link
Member

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.

@samuelcolvin samuelcolvin merged commit c31b8d6 into pydantic:master Jul 10, 2018
@samuelcolvin
Copy link
Member

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.

@oldPadavan
Copy link
Contributor Author

Should I add it to Exotic Types section or somewhere else?

@samuelcolvin
Copy link
Member

I would say best if it was a new section after Exotic Types, or perhaps a new subsection of exotic types.

@oldPadavan oldPadavan mentioned this pull request Jul 11, 2018
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.

2 participants