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

Bobronium
Copy link
Contributor

@Bobronium Bobronium 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>

@Bobronium
Copy link
Contributor Author

@Bobronium Bobronium commented Jul 10, 2019

@JrooTJunior, @Senpos, check this out, please

@codecov
Copy link

@codecov codecov bot 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

@Bobronium
Copy link
Contributor Author

@Bobronium Bobronium 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
@Bobronium Bobronium 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
This will increase speed when no include or exclude given and skip_defaults is False
@enkoder

This comment has been minimized.

@Bobronium

This comment has been minimized.

@samuelcolvin

This comment has been minimized.

@Bobronium

This comment has been minimized.

@JrooTJunior

This comment has been minimized.

@samuelcolvin
Copy link
Collaborator

@samuelcolvin samuelcolvin commented Jul 15, 2019

please discuss exclude on Config on #660

Copy link
Collaborator

@samuelcolvin samuelcolvin 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
Copy link
Collaborator

@samuelcolvin samuelcolvin Jul 15, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@Bobronium Bobronium Jul 15, 2019

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@samuelcolvin samuelcolvin Jul 15, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@Bobronium Bobronium Jul 15, 2019

Choose a reason for hiding this comment

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

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.

@Bobronium
Copy link
Contributor Author

@Bobronium Bobronium 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.

@samuelcolvin
Copy link
Collaborator

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

@Bobronium
Copy link
Contributor Author

@Bobronium Bobronium commented Jul 23, 2019

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

@samuelcolvin
Copy link
Collaborator

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

@Bobronium
Copy link
Contributor Author

@Bobronium Bobronium commented Jul 23, 2019

@samuelcolvin, docs is written, check it out please

@samuelcolvin samuelcolvin merged commit 74768c1 into pydantic:master Jul 24, 2019
9 checks passed
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.

None yet

4 participants