Opened 3 years ago

Closed 3 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 claudep 3 years ago.
Test showing bug
17717.diff (3.5 KB) - added by akaariai 3 years ago.
ticket-17717-proxy-model-serializers.diff (3.6 KB) - added by charettes 3 years ago.
Make sure to rely on newly introduced concrete_model model option

Download all attachments as: .zip

Change History (12)

Changed 3 years ago by claudep

Test showing bug

comment:1 Changed 3 years ago by claudep

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 3 years ago by akaariai

  • 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 3 years ago by akaariai

comment:3 Changed 3 years ago by claudep

  • Triage Stage changed from Accepted to Ready for checkin

Looks good, thanks

comment:4 Changed 3 years ago by jezdez

  • Resolution set to fixed
  • Status changed from new to closed

In [17640]:

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

comment:5 Changed 3 years ago by charettes

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 3 years ago by charettes (previous) (diff)

comment:6 Changed 3 years ago by charettes

  • Cc charette.s@… added

comment:7 Changed 3 years ago by jezdez

  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Triage Stage changed from Ready for checkin to Accepted

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

Changed 3 years ago by charettes

Make sure to rely on newly introduced concrete_model model option

comment:8 Changed 3 years ago by akaariai

  • Triage Stage changed from Accepted to Ready 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 3 years ago by akaariai (previous) (diff)

comment:9 Changed 3 years ago by jezdez

  • Resolution set to fixed
  • Status changed from reopened to closed

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