Re: [Django] #33608: select_related() to nullable FK with db_constraint=False do not promote OUTER joins.

[Django]

#33608: select_related() to nullable FK with db_constraint=False do not promote
OUTER joins.
-------------------------------------+-------------------------------------
Reporter: Mathieu Hinderyckx | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

The question here is should the ORM account for possible integrity
violations in the first place, lets not forget that `db_constraint` was
basically introduced as
[https://github.com/django/django/commit/b55cde054ee7dd22f93c3522a8ddb1d04193bcac
some form of shadow option] without in-dept discussion about its desired
behaviour with regards to violations.

How the ORM should account for possible integrity violation was already
discussed in the past plenty when usage of the MySQL's MyISAM backend (and
SQLite in older versions) which doesn't support foreign keys was more
common and caused users to run into this exact issue.

Given we've opted not to special case JOIN'ing logic for
`supports_foreign_keys` in the past I don't see why we should take a
different stance for `db_constraint=False`.

If we allowed
`Object.objects.filter(remote_object_id=1234).select_related('remote_object')`
to return `Object` instances what would their `.remote_object` be? A mock
`RemoteObject(id=1234)` object? Simply `None`? Aren't both approaches
breaking symmetry with
`Object.objects.filter(remote_object_id=1234).first().remote_object`
raising a `RemoteObject.DoesNotExist` exception?

--
Ticket URL: <https://code.djangoproject.com/ticket/33608#comment:3>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#33608: select_related() to nullable FK with db_constraint=False do not promote
OUTER joins.
-------------------------------------+-------------------------------------
Reporter: Mathieu Hinderyckx | Owner: nobody

Type: Bug | Status: closed

Component: Database layer | Version: 3.2
(models, ORM) |

Severity: Normal | Resolution: invalid

Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* status: new => closed
* resolution: => invalid

Comment:

Simon, Thanks for the comment. Agreed, `db_constraint = False` in general
should not be used, except for a few really specific cases. Closing as
"invalid" as this case is not supported.

--
Ticket URL: <https://code.djangoproject.com/ticket/33608#comment:4>

[Django]

#33608: select_related() to nullable FK with db_constraint=False do not promote
OUTER joins.
-------------------------------------+-------------------------------------
Reporter: Mathieu Hinderyckx | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Mathieu Hinderyckx):

Replying to [comment:3 Simon Charette]:

> The question here is should the ORM account for possible integrity
violations in the first place, lets not forget that `db_constraint` was
basically introduced as
[https://github.com/django/django/commit/b55cde054ee7dd22f93c3522a8ddb1d04193bcac
some form of shadow option] without in-dept discussion about its desired
behaviour with regards to violations.
>
> How the ORM should account for possible integrity violation was already
discussed in the past plenty when usage of the MySQL's MyISAM backend (and
SQLite in older versions) which doesn't support foreign keys was more
common and caused users to run into this exact issue.
>
> Given we've opted not to special case JOIN'ing logic for
`supports_foreign_keys` in the past I don't see why we should take a
different stance for `db_constraint=False`.
>
> If we allowed
`Object.objects.filter(remote_object_id=1234).select_related('remote_object')`
to return `Object` instances what would their `.remote_object` be? A mock
`RemoteObject(id=1234)` object? Simply `None`? Aren't both approaches
breaking symmetry with
`Object.objects.filter(remote_object_id=1234).first().remote_object`
raising a `RemoteObject.DoesNotExist` exception?

`Object.objects.filter(remote_object_id=1234).first().remote_object`
doesn't raise a DoesNotExist exception. It returns null, as it should be.
First it fetches the Object with `remote_object_id=1234` which exists.
Secondly, It then accesses the FK field on that Object pointing to a
RemoteObject, which is null as it does not exist in the remote table
(hence why the db_constraint=False). That works as expected, just like an
'ordinary' FK. What doesn't work as expected is that adding a
select_related() breaks the filter statement. Select_related is an
optimization, the expectation is that it does the same two steps as before
but with a single DB query. That isn't the case currently; it silently
breaks the filtering logic, and doesn't event return the Object at all.

--
Ticket URL: <https://code.djangoproject.com/ticket/33608#comment:5>

[Django]

#33608: select_related() to nullable FK with db_constraint=False do not promote
OUTER joins.
-------------------------------------+-------------------------------------
Reporter: Mathieu Hinderyckx | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

> `Object.objects.filter(remote_object_id=1234).first().remote_object`
doesn't raise a `DoesNotExist` exception. It returns null, as it should
be.

I think you might be wrong here. Did you test this statement? I did with
Django 3.2.12 and 4.0.3.

{{{#!diff
diff --git a/poc/poc_models/tests.py b/poc/poc_models/tests.py
index 27b8249..f7edb32 100644
--- a/poc/poc_models/tests.py
+++ b/poc/poc_models/tests.py
@@ -10,6 +10,10 @@ def setUpTestData(cls):

# All of the test methods below try to fetch the same object which
was created in setUpTestData

+ def test_access_remote_object_broken_referential_integrity(self):
+ with self.assertRaises(RemoteObject.DoesNotExist):
+
Object.objects.filter(remote_object_id=1234).first().remote_object
+
def test_remote_object_does_exist_object_should_be_found(self):
RemoteObject.objects.create(remote_id=1234)
}}}

A `DoesNotExist` **is** raised when accessing `.remote_object` because
your data breaks integrity expectations that the ORM is built around. If
you break these integrity expectations you should expect undefined
behaviour.

--
Ticket URL: <https://code.djangoproject.com/ticket/33608#comment:6>