Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#13030 closed (fixed)

Natural Key deserialization fails on foreign key to field that is itself a foreign key

Reported by: yishaibeeri Owned by: nobody
Component: Core (Serialization) Version: master
Severity: Keywords: natural keys
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Attached diff has a test showing this:

In (nonabstract) model inheritance, the child model's B primary key is not the usual integer, but rather a FK to the parent A.

Suppose model B has defined natural_key() and get_by_natural_key().

If a third model C has a field that is a foreign key to B, loading it using deserialization with natural keys will fail. During creation of the C instance (let's call it objC), this is what happens:

  • First, the natural key will be used to successfully construct the instance of B (let's call it objB).
  • Then, an assignment is made to set the relevant field of objC to point to objB. However, instead of the naive objC.thefield = objB, the actual assignment uses the relevant field.rel.field_name to do something along the lines of:
       field_name = field.rel.field_name
       objC.thefield_id = getattr(objB, field_name)
    
  • In the usual case where field_name is a simple primary key (such as 'id'), this works nicely. However,
  • when this field is in itself a foreignkey, the result from getattr is an object - the relevant objA(!). This of course throws a nasty exception.

I have not tried, but it appears this would fail in a similar fashion in any case where the target of the first FK (from objC) is itself an FK and not a simple field (even without involving inheritance).

Perhaps the naive approach (objC.thefield = objB) would work better for all cases? This is a bit too deep for me, I'm afraid.

Attachments (1)

nk_inherit_test.diff (2.3 KB) - added by yishaibeeri 5 years ago.
diff with test showing bug

Download all attachments as: .zip

Change History (4)

Changed 5 years ago by yishaibeeri

diff with test showing bug

comment:1 Changed 5 years ago by russellm

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

Thanks for the report - but most of all, thanks for the awesome ready-to-use test case.

comment:2 Changed 5 years ago by russellm

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

(In [12804]) Fixed #13030 -- Corrected natural key deserialization to subclasses. Thanks to yishaibeeri for the report and test case.

comment:3 Changed 4 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

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