Repository: keredson/peewee

Issue:
null FKs return non-null objects with all-null attributes
clone of https://github.com/coleifer/peewee/issues/1012:

when doing a left join across a nullable FK, peewee 2.8.1 populates an empty object where the null is, rather than putting a `None` in the attribute.  example:

```python
import logging, os, sys
import peewee as pw

logger = logging.getLogger('peewee')
logger.setLevel(logging.DEBUG)
logger.addHandler(logging.StreamHandler())

db_fn = 'example.db'
if os.path.exists(db_fn): os.remove(db_fn)
db = pw.SqliteDatabase(db_fn)

class Person(pw.Model):
  name = pw.CharField()
  class Meta:
    database = db

class Pet(pw.Model):
  name = pw.CharField()
  owner = pw.ForeignKeyField(db_column='owner_id', rel_model=Person, to_field='id', null=True)
  class Meta:
    database = db

db.connect()
db.create_tables([Person, Pet])

Pet.create(name='Chance', owner=None)

chance = Pet.select(Pet, Person).join(Person, join_type=pw.JOIN.LEFT_OUTER, on=Pet.owner).get()
print 'chance', chance
print 'chance.owner_id', chance.owner_id
print 'chance.owner', chance.owner
print 'chance.owner.id', chance.owner.id
```

output:

```
$ python test_join_null_fk.py 
('CREATE TABLE "person" ("id" INTEGER NOT NULL PRIMARY KEY, "name" VARCHAR(255) NOT NULL)', [])
('CREATE TABLE "pet" ("id" INTEGER NOT NULL PRIMARY KEY, "name" VARCHAR(255) NOT NULL, "owner_id" INTEGER, FOREIGN KEY ("owner_id") REFERENCES "person" ("id"))', [])
('CREATE INDEX "pet_owner_id" ON "pet" ("owner_id")', [])
('INSERT INTO "pet" ("name", "owner_id") VALUES (?, ?)', [u'Chance', None])
('SELECT "t1"."id", "t1"."name", "t1"."owner_id", "t2"."id", "t2"."name" FROM "pet" AS t1 LEFT OUTER JOIN "person" AS t2 ON ("t1"."owner_id" = "t2"."id") LIMIT 1 OFFSET 0', [])
chance <__main__.Pet object at 0x7f9b3c249950>
chance.owner_id None
chance.owner <__main__.Person object at 0x7f9b3c249b90>
chance.owner.id None
```

is this the intended behavior?  consensus here is this is a bug and that:

1. `chance.owner` should be `None`, not an empty `Person` object with all null attributes
2. `chance.owner.id` should throw `AttributeError: 'NoneType' object has no attribute 'id'`

we couldn't find any documentation about what this should do one way or the other, nor any old issues that address this.



Discussion:

Patch:
diff --git a/peewee.py b/peewee.py
index bdc1506..796265c 100644
--- a/peewee.py
+++ b/peewee.py
@@ -2251,6 +2251,8 @@ if _DictQueryResultWrapper is None:
 class ModelQueryResultWrapper(QueryResultWrapper):
     def initialize(self, description):
         self.column_map, model_set = self.generate_column_map()
+        self._col_set = set(col for col in self.column_meta
+                            if isinstance(col, Field))
         self.join_list = self.generate_join_list(model_set)
 
     def generate_column_map(self):
@@ -2293,7 +2295,19 @@ class ModelQueryResultWrapper(QueryResultWrapper):
             for join in joins[current]:
                 metadata = join.metadata
                 if metadata.dest in models or metadata.dest_model in models:
-                    join_list.append(metadata)
+                    if metadata.foreign_key is not None:
+                        fk_present = metadata.foreign_key in self._col_set
+                        pk_present = metadata.primary_key in self._col_set
+                        check = metadata.foreign_key.null and (fk_present or
+                                                               pk_present)
+                    else:
+                        check = fk_present = pk_present = False
+
+                    join_list.append((
+                        metadata,
+                        check,
+                        fk_present,
+                        pk_present))
                     stack.append(join.dest)
 
         return join_list
@@ -2324,13 +2338,23 @@ class ModelQueryResultWrapper(QueryResultWrapper):
 
     def follow_joins(self, collected):
         prepared = [collected[self.model]]
-        for metadata in self.join_list:
+        for (metadata, check_null, fk_present, pk_present) in self.join_list:
             inst = collected[metadata.src]
             try:
                 joined_inst = collected[metadata.dest]
             except KeyError:
                 joined_inst = collected[metadata.dest_model]
 
+            has_fk = True
+            if check_null:
+                if fk_present:
+                    has_fk = inst._data.get(metadata.foreign_key.name)
+                elif pk_present:
+                    has_fk = joined_inst._data.get(metadata.primary_key.name)
+
+            if not has_fk:
+                continue
+
             # Can we populate a value on the joined instance using the current?
             mpk = metadata.primary_key is not None
             can_populate_joined_pk = (
@@ -2384,7 +2408,7 @@ class AggregateQueryResultWrapper(ModelQueryResultWrapper):
         self.source_to_dest = {}
         self.dest_to_source = {}
 
-        for metadata in self.join_list:
+        for (metadata, _, _, _) in self.join_list:
             if metadata.is_backref:
                 att_name = metadata.foreign_key.related_name
             else:


Test patch:
diff --git a/playhouse/tests/test_models.py b/playhouse/tests/test_models.py
index 90ec728..e23e99d 100644
--- a/playhouse/tests/test_models.py
+++ b/playhouse/tests/test_models.py
@@ -2058,3 +2058,76 @@ class TestDeleteNullableForeignKeys(ModelTestCase):
         self.assertEqual(nf2.delete_instance(), 1)
         self.assertEqual(nf3.delete_instance(), 1)
         self.assertEqual(nf4.delete_instance(), 1)
+
+
+class TestJoinNullableForeignKey(ModelTestCase):
+    requires = [Parent, Orphan, Child]
+
+    def setUp(self):
+        super(TestJoinNullableForeignKey, self).setUp()
+
+        p1 = Parent.create(data='p1')
+        p2 = Parent.create(data='p2')
+        for i in range(1, 3):
+            Child.create(parent=p1, data='child%s-p1' % i)
+            Child.create(parent=p2, data='child%s-p2' % i)
+            Orphan.create(parent=p1, data='orphan%s-p1' % i)
+
+        Orphan.create(data='orphan1-noparent')
+        Orphan.create(data='orphan2-noparent')
+
+    def test_no_empty_instances(self):
+        with self.assertQueryCount(1):
+            query = (Orphan
+                     .select(Orphan, Parent)
+                     .join(Parent, JOIN.LEFT_OUTER)
+                     .order_by(Orphan.id))
+            res = [(orphan.data, orphan.parent is None) for orphan in query]
+
+        self.assertEqual(res, [
+            ('orphan1-p1', False),
+            ('orphan2-p1', False),
+            ('orphan1-noparent', True),
+            ('orphan2-noparent', True),
+        ])
+
+    def test_unselected_fk_pk(self):
+        with self.assertQueryCount(1):
+            query = (Orphan
+                     .select(Orphan.data, Parent.data)
+                     .join(Parent, JOIN.LEFT_OUTER)
+                     .order_by(Orphan.id))
+            res = [(orphan.data, orphan.parent is None) for orphan in query]
+
+        self.assertEqual(res, [
+            ('orphan1-p1', False),
+            ('orphan2-p1', False),
+            ('orphan1-noparent', False),
+            ('orphan2-noparent', False),
+        ])
+
+    def test_non_null_fk_unselected_fk(self):
+        with self.assertQueryCount(1):
+            query = (Child
+                     .select(Child.data, Parent.data)
+                     .join(Parent, JOIN.LEFT_OUTER)
+                     .order_by(Child.id))
+            res = [(child.data, child.parent is None) for child in query]
+
+        self.assertEqual(res, [
+            ('child1-p1', False),
+            ('child1-p2', False),
+            ('child2-p1', False),
+            ('child2-p2', False),
+        ])
+
+        res = [child.parent.data for child in query]
+        self.assertEqual(res, ['p1', 'p2', 'p1', 'p2'])
+
+        res = [(child._data['parent'], child.parent.id) for child in query]
+        self.assertEqual(res, [
+            (None, None),
+            (None, None),
+            (None, None),
+            (None, None),
+        ])