Opened 5 years ago

Closed 15 months ago

#18081 closed Bug (fixed)

Syncdb doesn't create database constraints for foreign keys referencing a proxy model.

Reported by: Anssi Kääriäinen Owned by: nobody
Component: Database layer (models, ORM) Version: 1.4
Severity: Normal Keywords:
Cc: Simon Charette Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Add this test to modeltests/proxy_models/tests.py:

class TransactionalProxyModelTests(TransactionTestCase):
    def test_proxy_fk(self):
        """
        Test that the DB contains proper foreign keys for proxy model references.
        """
        @transaction.commit_on_success
        def create_failing_pk():
            t = TrackerUser.objects.create(status='bar')
            Improvement.objects.create(summary='foof', version='foof',
                                       reporter_id=1, associated_bug_id=1,
                                       assignee=t)
        self.assertRaises(IntegrityError, create_failing_pk)

On MySQL this does not fail when using InnoDB. It should fail, as both reporter and associated bug with ID=1 are missing.

The reason for this is that in django/db/backends/creation.py, sql_for_pending_references() is a check:
if not model._meta.managed or model._meta.proxy: skip creation 
Now, that is incorrect: the model in question is the model we are _referring_, not the model from where the PK is from. So, under MySQL foreign keys to proxy models do not get enforced in the DB.

In addition the same bug is there for other databases, too. However, this one does not show under PostgreSQL as the foreign key is created inline. However, were the order of the models different in models.py also PostgreSQL would fail to create the foreign key.

The fix is luckily very simple: just remove the "or proxy" part. The managed part is correct: the referenced model can be a view for example, in which case referencing it would be a failure.

Attachments (1)

18081-test.diff (1.6 KB) - added by Tim Graham 16 months ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 4 years ago by Aymeric Augustin

Triage Stage: UnreviewedAccepted

comment:2 Changed 4 years ago by Simon Charette

Cc: charette.s@… added

comment:3 Changed 16 months ago by Tim Graham

I am not sure if this is still an issue with the SchemaEditor backends. I updated Anssi's test (attached) and it passes at 28cb272a7279e6dfc4d5c53838ebf7343c3e66b5, however, the SchemaEditor._model_indexes_sql() method still hasa condition to skip proxy models.

Changed 16 months ago by Tim Graham

Attachment: 18081-test.diff added

comment:4 Changed 16 months ago by Markus Holtermann

I don't see why that line in the SchemaEditor should be wrong. Proxy models don't have any database representation on their own (https://docs.djangoproject.com/en/1.8/topics/db/models/#proxy-models) and thus don't have any indices. Without digging into the issue too deep, this seems more like a bug in the assignment of related objects to me.

comment:5 Changed 16 months ago by Simon Charette

Cc: Simon Charette added; charette.s@… removed
Summary: Proxy model foreign keys not created properly by syncdbSyncdb doesn't create database constraints for foreign keys referencing a proxy model.

This is not an issue anymore since calls to _create_fk_sql are not conditional to not field.target_field.model._meta.proxy (create_model, add_field, alter_field).

I think a more appropriate regression test could live in test_operations and issue an AddField of a foreign key pointing to ProxyPony and make sure the constraint exists. Naturally the test should be skipUnlessDBFeature('supports_foreign_keys').

comment:6 Changed 16 months ago by Simon Charette

comment:7 Changed 15 months ago by Tim Graham <timograham@…>

In 8e8c0792:

Refs #18081 -- Asserted db constraints are created for fk to proxy models.

comment:8 Changed 15 months ago by Tim Graham

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top