Skip to content
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

Add advanced exclude and include support for dict, json and copy #648

Merged
merged 17 commits into from Jul 24, 2019

Conversation

@MrMrRobat
Copy link
Contributor

commented Jul 10, 2019

Change Summary

Implementation of advanced exclude support. With this changes you will be able to exclude any field on any depth of a model.

  • Changed exclude logic: now all values will be excluded in _iter (when dealing with BaseModel) and _get_values (when dealing with raw dicts and sequences) methods of model, not in dict or copy methods themselfs.
  • Yes, copy uses _iter and _get_value as well
  • _calculate_keys replaced with _update_exclude which is calculating keys to exclude (just modified _calculate_keys a bit to agree with changes)
  • added class ValueItems (used in _get_value and _iter) for more convenient calculation of excluded and included fields on values

Example:

import datetime
from typing import List

from pydantic import BaseModel

class Country(BaseModel):
    name: str
    phone_code: int

class Address(BaseModel):
    post_code: int
    country: Country

class CardDetails(BaseModel):
    number: int
    expires: datetime.date

class Hobby(BaseModel):
    name: str
    info: str

class User(BaseModel):
    name: str
    address: Address
    card_details: CardDetails
    hobbies: List[Hobby]

user = User(
    name='John',
    address=Address(
        post_code=123456,
        country=Country(
            name='USA',
            phone_code=1
        )
    ),
    card_details=CardDetails(
        number=4212934504460000,
        expires=datetime.date(2020, 5, 1)
    ),
    hobbies=[
        Hobby(name='Programming', info='Doing stuff'),
        Hobby(name='Gaming', info='Hell Yeah!!!')
    ]

)

exclude = {
    'name': ...,
    'address': {
        'post_code': ...,
        'country': {'phone_code'}
    },
    'card_details': ...,
    'hobbies': {
        -1: {'info'}
    },
}
d = user.dict(exclude=exclude)
assert d == {
    'address': {'country': {'name': 'USA'}},
    'hobbies': [
        {'name': 'Programming', 'info': 'Doing stuff'},
        {'name': 'Gaming'}
    ]
}

Related issue number

#640

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • HISTORY.rst has been updated
    • if this is the first change since a release, please add a new section
    • include the issue number or this pull request number #<number>
    • include your github username @<whomever>
@MrMrRobat

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

@JrooTJunior, @Senpos, check this out, please

@codecov

This comment has been minimized.

Copy link

commented Jul 10, 2019

Codecov Report

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

@@          Coverage Diff          @@
##           master   #648   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines        2673   2713   +40     
  Branches      530    537    +7     
=====================================
+ Hits         2673   2713   +40
Add advanced exclude support for dict, json and copy
Add new version section (v0.31)
@MrMrRobat

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

@samuelcolvin, what do you think about changes? I guess I should write tests for ValueExclude class, so I'll do it a bit later, if you don't mind.

pydantic/main.py Outdated Show resolved Hide resolved
pydantic/main.py Outdated Show resolved Hide resolved
pydantic/main.py Outdated Show resolved Hide resolved

MrMrRobat added some commits Jul 13, 2019

Add advanced include support, add more tests, improve code style
Rename ValueExclude to ValueItems and move it to utils
Use old logic to calculate keys, but still exclude it in _iter

@MrMrRobat MrMrRobat changed the title Add advanced exclude support for dict, json and copy Add advanced exclude and include support for dict, json and copy Jul 14, 2019

Removed update arg check in _calculate_keys for return None
This will increase speed when no include or exclude given and skip_defaults is False
@enkoder

This comment was marked as off-topic.

Copy link

commented Jul 15, 2019

From a user perspective I would rather add an exclude in the Config metaclass instead of passing the dict in to .json(). One of my model's fields is a Callable and I would like to call .json() on it, however I need to instead pass a custom encoder to the .json() which still puts the responsibility on the caller.

@MrMrRobat

This comment was marked as off-topic.

Copy link
Contributor Author

commented Jul 15, 2019

@enkoder thanks for your feedback. Personally, I like this idea, and I'd love to implement this.
I guess it deserves another PR, since it doesn't depend on changes of this PR. So it would be nice, if you create an issue, where we discuss an implementation of this feature.

@samuelcolvin

This comment was marked as off-topic.

Copy link
Owner

commented Jul 15, 2019

From a user perspective I would rather add an exclude in the Config metaclass instead of passing the dict in to .json(). One of my model's fields is a Callable and I would like to call .json() on it, however I need to instead pass a custom encoder to the .json() which still puts the responsibility on the caller.

Hi @enkoder please create another issue to discuss this, might be able to combine it with #624.

@MrMrRobat

This comment was marked as off-topic.

Copy link
Contributor Author

commented Jul 15, 2019

@624.

Should be #624, i guess :)

@JrooTJunior

This comment was marked as off-topic.

Copy link

commented Jul 15, 2019

create another issue to discuss this

Yeah. I'd like this feature. And i guess to implements this like exporting roles in Schematics
That mechanism is good for writing API's

You can simply describe all of public fields in model and inside controllers make dump in required set of fields by specifying only the role name.

@samuelcolvin

This comment has been minimized.

Copy link
Owner

commented Jul 15, 2019

please discuss exclude on Config on #660

@samuelcolvin
Copy link
Owner

left a comment

This looks great! but it's a big change and I need to play with it more and convince myself:

  • it doesn't slow things down
  • it won't break people's current (mis)use of pydantic
  • it doesn't stop us fixing #508 when we're ready for a breaking change
  • the whole _iter vs. _get_values thing looks a bit ugly, but maybe it has to be

@MrMrRobat please could you rebase and update HISTORY, otherwise I'll go through it more later in the week.

@@ -487,17 +511,56 @@ def _decompose_class(cls: Type['Model'], obj: Any) -> GetterDict:
return GetterDict(obj)

@classmethod
def _get_value(cls, v: Any, by_alias: bool, skip_defaults: bool) -> Any:
@no_type_check

This comment has been minimized.

Copy link
@samuelcolvin

samuelcolvin Jul 15, 2019

Owner

why do we need this? maybe better to have a typing ignore comment on a few lines?

This comment has been minimized.

Copy link
@MrMrRobat

MrMrRobat Jul 15, 2019

Author Contributor

It will be on every line we call this method, so I thought having decorator would be better. But yeah, I can replace it with # type: ignore if you wish

This comment has been minimized.

Copy link
@samuelcolvin

samuelcolvin Jul 15, 2019

Owner

no that's. Sounds like I need to look at this more.

This comment has been minimized.

Copy link
@MrMrRobat

MrMrRobat Jul 15, 2019

Author Contributor

MyPy throws error here because we use and operator on exclude=value_items and value_items.for_value(k)

pydantic/main.py:597: error: Argument "exclude" to "_get_value" of "BaseModel" has incompatible type "Union[ValueItems, None, Any]"; expected "Union[Set[Union[int, str]], Dict[Union[int, str], Any], None]"

So it thinks that potentially we can pass an instance of ValueItems itseld, but since bool(ValueItems()) always will be equivalent for True, it will never happen, so I guess we absolutely ok here.

@MrMrRobat

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

  • it doesn't slow things down

I guess it definitely will slow something down. Unfortunately I didn't run any comparison benchmarks. So please let me know what you find out

  • the whole _iter vs. _get_values thing looks a bit ugly ...

Agree on that and will appreciate any suggestions how we can make this look better

please could you rebase and update HISTORY ...

@samuelcolvin yep, done.

MrMrRobat and others added some commits Jul 16, 2019

@samuelcolvin

This comment has been minimized.

Copy link
Owner

commented Jul 23, 2019

Okay, I've made a few tweaks, but in general I think this is good.

I've tested the performance with

import json
import timeit
from datetime import datetime
from pathlib import Path
from typing import List

from pydantic import BaseModel, PositiveInt, ValidationError, constr

THIS_DIR = Path(__file__).parent


def main():
    class Model(BaseModel):
        id: int
        client_name: constr(max_length=255)
        sort_index: float
        # client_email: EmailStr = None
        client_phone: constr(max_length=255) = None

        class Location(BaseModel):
            latitude: float = None
            longitude: float = None

        location: Location = None

        contractor: PositiveInt = None
        upstream_http_referrer: constr(max_length=1023) = None
        grecaptcha_response: constr(min_length=20, max_length=1000)
        last_updated: datetime = None

        class Skill(BaseModel):
            subject: str
            subject_id: int
            category: str
            qual_level: str
            qual_level_id: int
            qual_level_ranking: float = 0

        skills: List[Skill] = []

    json_path = THIS_DIR / 'benchmarks' / 'cases.json'
    with json_path.open() as f:
        cases = json.load(f)

    results = []
    for case in cases:
        try:
            m = Model(**case)
        except ValidationError:
            pass
        else:
            # return m
            r = m.dict(exclude={'location', 'grecaptcha_response'})
            results.append(r)


if __name__ == '__main__':
    repeats = 20
    v = timeit.timeit(main, number=repeats)
    print(f'time taken: {v / repeats * 1000:0.3f}ms')

(cases.json can be downloaded from here)

Basically this work doesn't seem to make a significant difference.

@MrMrRobat please confirm you're happy with this, if so I'll merge.

@MrMrRobat

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

Great, I’m absolutely happy with this. Thanks for your tweaks :)

@samuelcolvin

This comment has been minimized.

Copy link
Owner

commented Jul 23, 2019

@MrMrRobat sorry, I forgot. I guess we need some docs to explain how this works. If you could do that I'll merge.

@MrMrRobat

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

@samuelcolvin, docs is written, check it out please

@samuelcolvin samuelcolvin merged commit 74768c1 into samuelcolvin:master Jul 24, 2019

6 of 9 checks passed

Header rules No header rules processed
Details
Pages changed 3 new files uploaded
Details
Redirect rules No redirect rules processed
Details
Mixed content No mixed content detected
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
samuelcolvin.pydantic Build #20190724.2 succeeded
Details
samuelcolvin.pydantic (Job Python36) Job Python36 succeeded
Details
samuelcolvin.pydantic (Job Python37) Job Python37 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.