Issue: A little typo makes using select with several models impossible in seldom cases
Author: navolotsky
State: closed
Created: 2020-05-20 20:34:26

It looks like `instance` must be `src` because `set_keys` is a `dict` that contains models.
https://github.com/coleifer/peewee/blob/d6318dce0e5149a61511521a7dcb96600bec9b25/peewee.py#L7521


If we do `select` with several models (`B` and `A`) and `B` is unhashable, we will get an exception.
The following code is just an example. It will not throw an exception.

```python
import traceback

from peewee import CharField, ForeignKeyField, Model, PostgresqlDatabase

db_conn_params = {
        "dbname": "peewee_fail_test",
        "user": "postgres",
        "password": "***",
        "host": "localhost",
        "port": "5433"
}

database = PostgresqlDatabase(
    db_conn_params.pop('dbname'), **db_conn_params)


class BaseModel(Model):
    class Meta:
        database = database


class A(BaseModel):
    some_str = CharField()


class B(BaseModel):
    another_str = CharField()
    a = ForeignKeyField(model=A)


if __name__ == "__main__":
    database.create_tables(models=[A, B])
    for i in range(10):
        a = A.create(some_str='s{i}')
        b = B.create(another_str=f'as1_{i}', another_str2=f'as2_{i}', a=a)
    try:
        for b in B.select(B, A).join(A):
            pass
    except:
        traceback.print_exc()

```

```
Traceback (most recent call last):
  File "c:\Projects\Python\project\peewee_models.py", line 1314, in <module>
        for b in B.select(B, A).join(A):
  File "c:\Projects\Python\project\.venv_3.8.2-64\lib\site-packages\peewee.py", line 4329, in next
    self.cursor_wrapper.iterate()
  File "c:\Projects\Python\project\.venv_3.8.2-64\lib\site-packages\peewee.py", line 4248, in iterate
    result = self.process_row(row)
  File "c:\Projects\Python\project\.venv_3.8.2-64\lib\site-packages\peewee.py", line 7516, in process_row
    if instance not in set_keys and dest not in set_keys \
TypeError: unhashable type: 'B'
```
Unfortunately, I could not reproduce this on arbitry models because I couldn't find conditions that make model instance unhashable.

>The following code is just an example. It will not throw an exception.

If you can produce code that reliably causes problems, I will be happy to investigate... Did you hand-write that traceback, though? I'm confused because you have said that the code does *not* produce an error, but then you have pasted what looks like an error from such code.

I am sorry for confusing you. This is the real traceback from my project, I just replaced class names. I wanted to give a clear example because I cannot copy here all of my models.

Anyway, I found that makes model unhashable. I defined custom `__eq__` without `__hash__`.
Now this code will produce an error.
```python
import traceback

from peewee import (CharField, ForeignKeyField, Model,
                    SqliteDatabase)

database = SqliteDatabase(":memory:")

class BaseModel(Model):
    class Meta:
        database = database
    def __eq__(self, other):
        return super().__eq__(other)
    # def __hash__(self):
    #     """If it isn't defined, when __eq__ is defined, object is unhashable
    #     https://stackoverflow.com/questions/1608842/types-that-define-eq-are-unhashable"""
    #     return super().__hash__()

class A(BaseModel):
    some_str = CharField()

class B(BaseModel):
    another_str = CharField()
    a = ForeignKeyField(model=A)

if __name__ == "__main__":
    database.create_tables(models=[A, B])
    a = A.create(some_str='s1')
    b = B.create(another_str=f's2', a=a)
    try:
        for b in B.select(B, A).join(A):
            print(b)
    except:
        traceback.print_exc()
```
<details>
<summary>Screenshot</summary>
![изображение](https://user-images.githubusercontent.com/43314216/82521319-ff2a8900-9b2e-11ea-9205-cf4d7ab302b8.png)
</details>

Why are you implementing a custom `__eq__()` on a peewee model class? The peewee model class already implements a correct equals (and hash). I suggest you refer to the peewee source code and review the implementations there. This is a Python thing -- you'll need to also implement a hash method if you are overriding equals -- but I'd strongly suggest you just use the default implementations in peewee.

> Why are you implementing a custom `__eq__()` on a peewee model class?

My code logic with kludges needs this. I check if an instance updated in a DB excluding some fields from a `list` defined in model classes.

At the same time, I try to write efficient queries, because I know a little SQL. These I like `peewee` that it is flexible and similar to SQL.

> This is a Python thing -- you'll need to also implement a hash method if you are overriding equals

I already understood this as I wrote above.

> I'd strongly suggest you just use the default implementations 

I will follow your advice and will rename custom `__eq__()` to something like `equal_to()` then it will not affect on the `peewee` default implementation.

> I suggest you refer to the peewee source code and review the implementations there.

I read and tried to understand `ModelCursorWrapper.process_row()`. Please take five minutes to look into it. 
https://github.com/coleifer/peewee/blob/d6318dce0e5149a61511521a7dcb96600bec9b25/peewee.py#L7519-L7523
This condition in the `if` will never be `True`. `instance` cannot contains in `set_keys` due to the `set_keys` stores model classes and `instance` is a model instance. It looks like `src` must be instead of `instance` there.

If you will have time, I am also interesting in cases covered by this `if` because I could not imagine them when I was trying to find cases that would be affected by this typo. Thanks a lot!


The condition is to avoid populating non-selected intermediary models.

> This condition in the if will never be True. 

Sorry, I made a typo. I mean that the "`instance not in set_keys`" part of the `if` condition will never be `False`. The full condition can be both `True` or `False` of course.


I suggest you try removing the clause and running the tests, if you're curious.