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

Nested exclude support #640

Closed
Bobronium opened this issue Jul 5, 2019 · 16 comments
Closed

Nested exclude support #640

Bobronium opened this issue Jul 5, 2019 · 16 comments

Comments

@Bobronium
Copy link
Contributor

Feature Request

As @JrooTJunior pointed out, now dict, json and copy methods don't support exclude for nested models.

Using keys from original exclude arg for nested models could be confusing in some cases and also potentially will break backwards compatibility.

So I see a solution in adding new arg called nested_exclude or deep_exclude, keys in which will be excluded from all nested models and even dicts:

from pydantic import BaseModel, SecretStr

class User(BaseModel):
    id: int
    username: str
    password: SecretStr

class Transaction(BaseModel):
    id: str
    user: User
    value: int

transaction = Transaction(
    id="abcdef1234567890",
    user=User(
        id=42,
        username="someuser",
        password="hashedpassword123"
    ),
    value=9876543210
)

t_out = transaction.dict(exclude={'password'})
t_out_safe = transaction.dict(nested_exclude={'password'})
print(t_out)  # {'id': 'abcdef1234567890', 'user': {'id': 42, 'username': 'someuser', 'password': SecretStr('**********')}, 'value': 9876543210}
print(t_out_safe)  # {'id': 'abcdef1234567890', 'user': {'id': 42, 'username': 'someuser'}, 'value': 9876543210}
@samuelcolvin
Copy link
Member

personally I think it should be nested by default, we would just need to add a "break change" notice to the release.

Anyone unhappy with that?

@Bobronium
Copy link
Contributor Author

Will it affect not only nested models, but also keys with plain dict values then?

@JrooTJunior
Copy link

JrooTJunior commented Jul 5, 2019

I see in this case nested_exclude will exclude key from all of nested models but what about excluding keys from the specific model?

For example model has two nested models and this models has the same keys and need to exclude key only from one of that.

Need something like exclude={"key", "nested1.key2"}.

@senpos
Copy link

senpos commented Jul 5, 2019

it should be nested by default

Imagine we have the following code:

from pydantic import BaseModel

class Author(BaseModel):
	name: str
	surname: str
	country: str

class Book(BaseModel):
	name: str
	author: Author
	release_year: int

john_smith = Author(name='John', surname='Smith', country='England')
foo_bar_story = Book(name='The Foo Bar Story', author=john_smith, release_year=2019)

print(foo_bar_story.dict(exclude={'name'}))

Does it mean, that both Book and Author models will drop thename key?

{'author': {'surname': 'Smith', 'country': 'England'}, 'release_year': 2019}

It is not obvious, to be honest.
What will it look like if I want to preserve name in my nested model, but get rid of it in the root one?

At this point, it feels like we might need something like project in MongoDB to satisfy everybody.
But I don't know how it has to work or if it's even a pythonic thing:

foo_bar_story.dict(projection={"name": 0, "author": {"name": 0}, "author.country": 0})

@samuelcolvin
Copy link
Member

Okay, having thought about this, I think nested exclude and include should work as follows:

members of the set for exclude can either be a string in which case they work as they currently do, or a tuple in which case they indicate the nested member to exclude.

Same/opposite for include.

So in the above example to exclude the author name you'd use

foo_bar_story.dict(exclude={('author', 'name'})

If you had a list you could exclude via element id, eg.

class Author(BaseModel):
	name: str
	surname: str

class Book(BaseModel):
	name: str
	authors: List[Author]

a1 = Author(name='c', surname='d')
a2 = Author(name='e', surname='f')

b = Book(name='testing', authors=[a1, a2])

# excluded a2:
b.dict(exclude={('authors', 1)})

This will make the logic fairly complicated and somewhat slower, but it's backwards compatible and powerful.

@Bobronium
Copy link
Contributor Author

Bobronium commented Jul 7, 2019

I like this idea, but in this case personally I’d preferred using a Union[Set[str], Dict[str, Set[IntStr]] instead of Set[Union[str, Tuple[str, IntStr]], so it will be much easier to define multiple keys or indexes for a particular model:

foo_bar_story.dict(exclude={'author': {'name', 'id'}})

b.dict(exclude={'authors': {1, 2}})

What do you think?

Upd.

And for excluding entire key, for example we can use something like this:

b.dict(exclude={'authors': ...})

@samuelcolvin
Copy link
Member

dicts are mutable so can't be used in a set, let's go with Set[Union[str, Tuple[IntStr, ...]] as I suggested.

Tuple[IntStr, ...] because models can be recursive, so the references can be of any depth.

@Bobronium
Copy link
Contributor Author

Bobronium commented Jul 7, 2019

dicts are mutable so can't be used in a set

But why use them in a set? I suggested use either a set or a dict.

I still don’t get benefits of using set of tuples. How will it look with multiple fields to exclude from nested model or depth > 1?

@samuelcolvin
Copy link
Member

samuelcolvin commented Jul 7, 2019

Sorry, I though you had Set[Union[str, Dict[...]]].

Maybe dict would be easier.

Please explain what it would like when excluding the following things (all these things in the same exclude):

  • field name (str)
  • field address (sub model), exclude post_code and country (but not town)
  • field card_details (sub-model), exclude the entire sub-model
  • field hobbies (list of sub-models), exclude the last hobby entirely

?

(for reference with the tuple solution this would be exclude={'name', ('address', 'post_code'), ('address', 'country'), 'card_details', ('hobbies', -1)})

@samuelcolvin
Copy link
Member

if we wanted the tuple approach to be more succinct (but harder to develop) we could make the above example:

exclude={
    'name', 
    ('address', ('post_code', 'country')),
    'card_details', 
    ('hobbies', -1),
}

@Bobronium
Copy link
Contributor Author

Bobronium commented Jul 7, 2019

Above example should look like:

exclude={
    'name': ..., 
    'address': {'post_code', 'country'},
    'card_details': ..., 
    'hobbies': {-1},
}

I’m not sure about ellipses, maybe we should use something else to define entire field to exclude.

Tuple solution seems to be more friendly for mixing single exclude in nested model and original exclude:

{'field1', ('sub_m', 'field2')}
instead of

{'field1': ..., 'sub_m': {'field2'}}

But when it comes to excluding multiple fields from sub model, using a dict seems easier for me

@samuelcolvin
Copy link
Member

I think tuples are more correct, but dict is more readable.

Let's go with the dict approach.

@Bobronium
Copy link
Contributor Author

Another benefit from using dict: we can define whether to use old or new logic right away just with type checking the argument

If use tuple solution, we will need to check each element at any time

@Bobronium
Copy link
Contributor Author

I've almost implemented this so far. Need to deal with typing issues and write the tests

In current state his example is absolutely valid and working:

exclude={
    'name': ..., 
    'address': {
        'post_code': ..., 
        'country': {'code'}
    },
    'card_details': ..., 
    'hobbies': {
        -1: {'details'}
    },
}

@Bobronium
Copy link
Contributor Author

Bobronium commented Jul 9, 2019

Also, I need a little help with type for exclude arg.
This is my approach on this, but unfortunately it's not valid due to ellipsis and recursive typing.

DictIntStrExclude = Dict[IntStr, Union[SetIntStr, 'DictIntStrExclude', ellipsis]]
exclude: Union['SetStr', 'DictIntStrExclude']

How is the proper type should look like?

@samuelcolvin
Copy link
Member

Will have to be Dict[IntStr, Any]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants