Opened 5 years ago

Closed 5 years ago

#17717 closed Bug (fixed)

django.core.serializers.serialize doesn't deal with proxy models

Reported by: lakin Owned by: nobody
Component: Core (Serialization) Version: 1.1
Severity: Normal Keywords:
Cc: anssi.kaariainen@…, charette.s@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

If you try to serialize a proxy=True model - you get no fields serialized.

I wish I had more time to make a test case, but I don't at the moment. The easy test case is to make a model, make another model that proxies to the original model. Insert a row of data, and then serialize an instance of both models to see that the proxy one is empty.

Marking it as 1.1 as that is what I'm using and I haven't had time to double check the other versions.

Attachments (3)

17717-test.diff (2.1 KB) - added by Claude Paroz 5 years ago.
Test showing bug
17717.diff (3.5 KB) - added by Anssi Kääriäinen 5 years ago.
ticket-17717-proxy-model-serializers.diff (3.6 KB) - added by Simon Charette 5 years ago.
Make sure to rely on newly introduced concrete_model model option

Download all attachments as: .zip

Change History (12)

Changed 5 years ago by Claude Paroz

Attachment: 17717-test.diff added

Test showing bug

comment:1 Changed 5 years ago by Claude Paroz

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted

comment:2 Changed 5 years ago by Anssi Kääriäinen

Cc: anssi.kaariainen@… added
Has patch: set

In the attached patch the concrete_model._meta.local_fields are used to find out the fields to dump instead of obj._meta.local_fields.

Changed 5 years ago by Anssi Kääriäinen

Attachment: 17717.diff added

comment:3 Changed 5 years ago by Claude Paroz

Triage Stage: AcceptedReady for checkin

Looks good, thanks

comment:4 Changed 5 years ago by Jannis Leidel

Resolution: fixed
Status: newclosed

In [17640]:

Fixed #17717 -- Fixed serialization of proxy models. Thanks, Anssi Kääriäinen.

comment:5 Changed 5 years ago by Simon Charette

Now that there's a concrete_model property and that proxy_for_model is actually a proxy chain we should probably just get it from there?

Correct me if I'm wrong but the patch checked in r17640 will fail for proxy of a proxy model.

Should the patch be checked out and corrected with additional tests?

edit: I meant concrete_model and not concrete_class

Last edited 5 years ago by Simon Charette (previous) (diff)

comment:6 Changed 5 years ago by Simon Charette

Cc: charette.s@… added

comment:7 Changed 5 years ago by Jannis Leidel

Resolution: fixed
Status: closedreopened
Triage Stage: Ready for checkinAccepted

Oh, yeah, I didn't see the changes done in r17573, this new "concrete_model" property might indeed fix this easier. Re-opening..

Changed 5 years ago by Simon Charette

Make sure to rely on newly introduced concrete_model model option

comment:8 Changed 5 years ago by Anssi Kääriäinen

Triage Stage: AcceptedReady for checkin

Yes, that should be fixed. Now proxy_for_model is the actual proxied model, so as written the committed patch isn't correct any more. That is my failing, as I should have written it using the while opts._proxy_for_model: opts = opts._proxy_for_model._meta idiom instead of assuming the buggy behavior in the patch.

I tested the patch and without the fix the added test fails, with the fix in the patch it succeeds. I quickly skimmed through it and it seems ready for checkin to me.

Last edited 5 years ago by Anssi Kääriäinen (previous) (diff)

comment:9 Changed 5 years ago by Jannis Leidel

Resolution: fixed
Status: reopenedclosed

In [17643]:

Fixed #17717 (again) -- Used the new API for concrete models added in r17573. Many thanks to Simon Charette and Anssi Kääriäinen.

Note: See TracTickets for help on using tickets.
Back to Top