Repository: pydantic/pydantic
Pull request: Fix __fields_set__ not using alias field names (#517)
fix #517

<!-- Thank you for your contribution! -->
<!-- See https://pydantic-docs.helpmanual.io/#contributing-to-pydantic for help on Contributing -->
<!-- Don't worry about making lots of commits on a pull request, they'll be squashed on merge anyway -->

## Change Summary

Basically, instead of just setting `__fields_set__` from `input_data.keys()` (which could contain aliases instead of the field names), it checks all the fields and adds the field's name if it or the alias is in `input_data`. Not sure if this is the best way to do this.

## Related issue number

#517 

## Checklist

* [x] Unit tests for the changes exist
* [ ] Tests pass on CI and coverage remains at 100%
* [ ] Documentation reflects the changes where applicable
* [x] `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>`


Problem statement:
.dict(skip_defaults=True) skips fields populated by alias
<!-- Questions, Feature Requests, and Bug Reports are all welcome -->
<!-- delete as applicable: -->
# Bug

For bugs/questions:
* OS: Linux
* Python version `import sys; print(sys.version)`: `3.7.3 (default, Mar 26 2019, 21:43:19) [GCC 8.2.1 20181127]`

* Pydantic version `import pydantic; print(pydantic.VERSION)`: 0.25

When using `.dict(skip_defaults=True)` with an aliased field, if that field was populated by the alias (i.e. `Foo(alias_name=...)` instead of `Foo(field_name=...)`) then it is always skipped. Happens with `by_alias` set to `True` and `False` as well.

```py
from pydantic import BaseModel, Schema


class Foo(BaseModel):
    class Config:
        allow_population_by_alias = True
    
    a: int = Schema(1, alias="b")


# Correct
print(Foo())  # Foo a=1
print(Foo().dict(by_alias=False, skip_defaults=False))  # {'a': 1}
print(Foo().dict(by_alias=True, skip_defaults=False))  # {'b': 1}
print(Foo().dict(by_alias=False, skip_defaults=True))  # {}
print(Foo().dict(by_alias=True, skip_defaults=True))  # {}

# Correct
print(Foo(a=2))  # Foo a=2
print(Foo(a=2).dict(by_alias=False, skip_defaults=False))  # {'a': 2}
print(Foo(a=2).dict(by_alias=True, skip_defaults=False))  # {'b': 2}
print(Foo(a=2).dict(by_alias=False, skip_defaults=True))  # {'a': 2}
print(Foo(a=2).dict(by_alias=True, skip_defaults=True))  # {'b': 2}

# Correct
print(Foo(b=2))  # Foo a=2
print(Foo(b=2).dict(by_alias=False, skip_defaults=False))  # {'a': 2}
print(Foo(b=2).dict(by_alias=True, skip_defaults=False))  # {'b': 2}
# Incorrect
print(Foo(b=2).dict(by_alias=False, skip_defaults=True))  # {}
print(Foo(b=2).dict(by_alias=True, skip_defaults=True))  # {}
```

Hints from issue discussion:
After looking into it a bit, it looks like `.dict()` get's the values of the set fields from `__fields_set__`. `__fields_set__` doesn't contain the name of the field if it was set by the alias. E.g:

```py
print(Foo().__values__)  # {'a': 1}
print(Foo().__fields_set__)  # set()
print(Foo(a=2).__values__)  # {'a': 2}
print(Foo(a=2).__fields_set__)  # {'a'}
print(Foo(b=2).__values__)  # {'a': 2}
print(Foo(b=2).__fields_set__)  # set()
```

Is the behaviour for `__fields_set__` wrong or should `.dict()` be using something else to get the fields?

Commit message: Update HISTORY.rst

Patch to review:
diff --git a/HISTORY.rst b/HISTORY.rst
index 18cdfa118b1..8552e812681 100644
--- a/HISTORY.rst
+++ b/HISTORY.rst
@@ -7,6 +7,7 @@ v0.26 (unreleased)
 ..................
 * fix to schema generation for ``IPvAnyAddress``, ``IPvAnyInterface``, ``IPvAnyNetwork`` #498 by @pilosus
 * fix variable length tuples support, #495 by @pilosus
+* fix ``.dict(skip_keys=True)`` skipping values set via alias, #517 by @sommd
 
 v0.25 (2019-05-05)
 ..................
diff --git a/pydantic/main.py b/pydantic/main.py
index 6873842aa2d..2e0bedccd50 100644
--- a/pydantic/main.py
+++ b/pydantic/main.py
@@ -233,11 +233,7 @@ def __init__(self, **data: Any) -> None:
             self.__values__: Dict[str, Any] = {}
             self.__fields_set__: 'SetStr' = set()
         object.__setattr__(self, '__values__', self._process_values(data))
-        if self.__config__.extra is Extra.allow:
-            fields_set = set(data.keys())
-        else:
-            fields_set = data.keys() & self.__values__.keys()  # type: ignore
-        object.__setattr__(self, '__fields_set__', fields_set)
+        object.__setattr__(self, '__fields_set__', self._process_fields(data))
 
     @no_type_check
     def __getattr__(self, name):
@@ -437,6 +433,20 @@ def _process_values(self, input_data: Any) -> 'DictStrAny':
         # (casting here is slow so use ignore)
         return validate_model(self, input_data)  # type: ignore
 
+    def _process_fields(self, input_data: Any) -> 'SetStr':
+        config = self.__config__
+        populate_by_alias = config.allow_population_by_alias
+
+        fields = set()
+        for name, field in self.__fields__.items():
+            if name in input_data or populate_by_alias and field.alias in input_data:
+                fields.add(name)
+
+        if config.extra is Extra.allow:
+            fields |= input_data.keys()
+
+        return fields
+
     @classmethod
     def _get_value(cls, v: Any, by_alias: bool, skip_defaults: bool) -> Any:
         if isinstance(v, BaseModel):
diff --git a/tests/test_main.py b/tests/test_main.py
index 2142dcf5633..5392eaad8e9 100644
--- a/tests/test_main.py
+++ b/tests/test_main.py
@@ -598,6 +598,35 @@ class ModelB(BaseModel):
     assert dict(m) == {'c': 5, 'd': 2, 'e': {'a': 0, 'b': 1}}
 
 
+def test_dict_skip_defaults_populated_by_alias():
+    class MyModel(BaseModel):
+        class Config:
+            allow_population_by_alias = True
+
+        a: str = Schema("default", alias="alias_a")
+        b: str = Schema("default", alias="alias_b")
+
+    m = MyModel(alias_a="a")
+
+    assert m.dict(skip_defaults=True) == {"a": "a"}
+    assert m.dict(skip_defaults=True, by_alias=True) == {"alias_a": "a"}
+
+
+def test_dict_skip_defaults_populated_by_alias_with_extra():
+    class MyModel(BaseModel):
+        class Config:
+            allow_population_by_alias = True
+            extra = "allow"
+
+        a: str = Schema("default", alias="alias_a")
+        b: str = Schema("default", alias="alias_b")
+
+    m = MyModel(alias_a="a", c="c")
+
+    assert m.dict(skip_defaults=True) == {"a": "a", "c": "c"}
+    assert m.dict(skip_defaults=True, by_alias=True) == {"alias_a": "a", "c": "c"}
+
+
 def test_dir_fields():
     class MyModel(BaseModel):
         attribute_a: int


Code review comments:
diff_hunk: @@ -233,11 +233,7 @@ def __init__(self, **data: Any) -> None:
             self.__values__: Dict[str, Any] = {}
             self.__fields_set__: 'SetStr' = set()
         object.__setattr__(self, '__values__', self._process_values(data))
-        if self.__config__.extra is Extra.allow:
-            fields_set = set(data.keys())
-        else:
-            fields_set = data.keys() & self.__values__.keys()  # type: ignore
-        object.__setattr__(self, '__fields_set__', fields_set)
+        object.__setattr__(self, '__fields_set__', self._process_fields(data))
path: pydantic/main.py
text: @user1:
humm, I'm not sure this is the easiest or most efficient way of doing things. It might be easier to build `__fields_set__` in `validate_model`.

I think you can change like ~610 to

```py
else:
    names_used.add(field.name if using_name else field.alias)
```
Then add `names_used` along with the other results.

@author:
`validate_model` currently doesn't mutate the model object at all, so if someone is using it then it may break unexpectedly. There's also no way to return the names_used. If you think it's OK to call `object.__setattr__(model, '__fields_set__', names_used)` from `validate_model` then I can change it to do that.

@user1:
I would change the signature of `validate_model` to return a tuple including `fields_set`.

No one should be using `validate_model` directly so this should be fine.

@user1:
It was a mistake to have to different return types for `validate_model`, best to change it to always return `Tuple['DictStrAny', Set[str], Optional[ValidationError]]`

@user1:
If that's too much work for this PR, I could implement a PR to make the return type always `Tuple['DictStrAny', Optional[ValidationError]]`, then you could rebase off that?

@author:
Managed to do it. Might need a bit more testing but it should work.

@author:
Also if no one should be using `validate_model` model it should probably be `_validate_model` or at least not exported in `__init__`.

diff_hunk: @@ -598,6 +598,35 @@ class ModelB(BaseModel):
     assert dict(m) == {'c': 5, 'd': 2, 'e': {'a': 0, 'b': 1}}
 
 
+def test_dict_skip_defaults_populated_by_alias():
+    class MyModel(BaseModel):
+        class Config:
+            allow_population_by_alias = True
+
+        a: str = Schema("default", alias="alias_a")
path: tests/test_main.py
text: @user1:
please always use single quotes unless the string contains a single quote.

diff_hunk: @@ -437,6 +433,20 @@ def _process_values(self, input_data: Any) -> 'DictStrAny':
         # (casting here is slow so use ignore)
         return validate_model(self, input_data)  # type: ignore
 
+    def _process_fields(self, input_data: Any) -> 'SetStr':
+        config = self.__config__
+        populate_by_alias = config.allow_population_by_alias
+
+        fields = set()
+        for name, field in self.__fields__.items():
+            if name in input_data or populate_by_alias and field.alias in input_data:
path: pydantic/main.py
text: @user1:
I don't think you need `populate_by_alias` here.