Issue: `ignore_unknown` unexpectedly doesn't ignore `id` field.
Author: blakewest
State: closed
Created: 2019-06-19 04:06:30

Hi, I just started working with PeeWee, and came across some issues while working with the `ignore_unknown` option. Specifically, when trying to use `dict_to_model` with the `ignore_unknown` option, it doesn't seem to actually ignore all unknown traits. It ignores everything except for `id`. Here's a short example below that demonstrates the problem...


```python
from database.base_model import BaseModel
from peewee import *
from playhouse.shortcuts import dict_to_model, update_model_from_dict

Website.drop_table()

class BaseModel(Model):
    class Meta:
        db_file = './database/demo/test.db.sqlite'
        database = SqliteDatabase(db_file)


class Website(BaseModel):
    url = CharField(primary_key=True)
    name = CharField()

Website.create_table()

attrs = {"url": "https://some_website.com/some_resource/123245", "name": "Cool Website", "id": 456, "some_random_attr": True}
website = dict_to_model(Website, attrs, ignore_unknown=True)
website.save(force_insert=True) # => Throws OperationalError: table website has no column named id
```

I would have expected the save call to work. I know the table has no column `id`. I explicitly didn't give it such a column. I used `url` as the primary key instead. To give some context... I'm using PeeWee with a little scraper I'm building. It so happens that I'm combining my scraped data with some other data that includes a field called `id`. For my purposes I want to ignore this field (and all others that I didn't explicitly lay out in the model), hence me setting the `ignore_unknown` option. 

I saw a similar error when updating an existing model through a similar means.  For example... the following code fails in the same way:

```python
existing = Website.get_or_none(Website.url == 'https://some_website.com/some_resource/123245')
updated_attrs = {"url": "https://some_website.com/some_resource/123245", "name": "Very Cool Website", "id": 456, "some_other_random_attr": True}
updated = update_model_from_dict(existing, updated_attrs, ignore_unknown=True)
updated.save() # Fail! OperationError
```

In my actual project, I was seeing a more cryptic error, and actually was getting `ValueError: invalid literal for int() with base 10: 'http://www.some_website.com/index.php?a=odds&ma_id=823911369`. I couldn't get that error to neatly re-produce. But it stemmed from the same issue. In my code I had to put a `if 'id' in attrs: attrs.pop('id')`, which solved it. But this seems gross, and not what is expected with `ignore_unknown`.

Thoughts?

This makes sense -- so the issue is that your `BaseModel` class has an `id` primary key field (since no primary key is declared). When you subclass it, just because of how Python works, you get an `id` attribute on your subclass (`Website`).

To work around this, just declare your base model with the primary key explicitly disabled:

```python
class BaseModel(Model):
    class Meta:
        database = ...
        primary_key = False
```

@coleifer thanks for the quick response and for pointing out the issue. I would still view this as a bug though, for a few reasons:
1.) It's simply not expected that if I explicitly declare a primary key, and don't have `id` anywhere on the model, that somehow `id` is in fact actually on the model.

2.) To drive this point home, PeeWee explicitly tells me `id` is not a field. See this screenshot: 
![image](https://user-images.githubusercontent.com/2524396/59786865-160c8f80-927d-11e9-8119-4ac32c2302c8.png)

3.) Having to put `primary_key = False` on my BaseModel kind of defeats the point of my BaseModel. I may only want to have this non-traditional primary key on one model, not all of them. And I can't even directly add `Meta: primary_key = False` onto my `Website` class, and still have it inherit from BaseModel.

My expectation would be that if I don't put `id` on the model, it's not on the model. And especially if I ask PeeWee directly what fields are on my model, and it gives me a list... then those would in fact be the only fields on the model. Is there a higher level reason why the current behavior is intended?

> It's simply not expected that if I explicitly declare a primary key, and don't have id anywhere on the model, that somehow id is in fact actually on the model.

I understand and agree with you. There's a convention that if no primary key is defined, an `id` auto-incrementing pk will be added automatically. This gets inherited. It is what it is. This problem is really just confined to this particular case (implicit "id" being inherited, then a different primary key being declared).

The other problem is that you are explicitly specifying "ignore_unknown", which patches all attributes directly onto the model instance. If you specify `ignore_unknown=False` then Peewee will check that the key corresponds to a field declared directly on the model.

I'd suggest you do not use `ignore_unknown=True`.

The proper way to fix this, I think, would be to modify the `FieldAccessor` so that setting the value of the field attribute (via the descriptor) would validate that the field belongs to the instance you're setting the value on. I don't want to introduce that complexity, though, as this seems like a pretty rare situation to add the overhead of doing a check every single time you set an instance attribute.

@coleifer ah, yeah, the `ignore_unknown` part. I actually was gonna bring this up earlier. This is also very unexpected behavior to me. I saw in the source [here](https://github.com/coleifer/peewee/blob/master/playhouse/shortcuts.py#L139) that you call `setattr(instance, key, val)` whenever an unknown attribute is found. What is the reasoning for that behavior? My expectation would have been to actually ignore it. As in, do nothing (ie. just the `continue` part in this case). 
If `ignore_unknown` actually ignored the unknown attributes, then the bug also would not have happened. 
As for why I don't just use `ignore_unknown=False`, it's because I'm cobbling together this model from various data sources. There are loads of columns on it that are irrelevant for my needs, so `ignore_unknown` seemed like exactly what I need. I basically wanted my explicitly declared fields to act as a whitelist. And sure I could whitelist it all on my own, but I thought that's why ignore unknown would do for me. 

So all in all, I think my fix for this would be to change the behavior of ignore_unknown to actually ignore_unknown attributes. Or introduce a new option that does (maybe `only_set_known_fields`?). I get this would be a backwards-incompatible change, but I would argue the current behavior is incorrect. And if you're relying on `ignore_unknown` to set the unknown ones on the model, then it seems like you're doing something wrong.

I'd be happy to raise a PR if you agreed with either of those paths.

Here's the thing -- `dict_to_model` is just a simple shortcut. Models already accept `**kwargs` in their constructor. `dict_to_model` is helpful especially for taking a nested dictionary and turning it into a graph of models and their dependencies.

I think you're really wasting way too much time. In the time you've spent writing these comments you could have very easily written a filter expression that does exactly what you need:

```python
field_names = MyModel._meta.sorted_field_names
f = lambda i: i[0] in field_names
def clean_data(data):
    return dict(filter(f, data.items()))

# NB: use a transaction or a bulk insert instead!
for d in list_of_dicts:
    MyModel.create(**clean_data(d))
```