Opened 4 years ago

Closed 3 years ago

#16208 closed Bug (invalid)

natural key YAML deserialization using non-list natural keys broken (with fixing patch)

Reported by: jeff.k@… Owned by: kenth
Component: Core (Serialization) Version: 1.2
Severity: Normal Keywords: yaml
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: yes UI/UX: no

Description

I'm using Django 1.2.5. I had trouble loading data from a fixture using the natural keys feature. I tracked this down to what seems to be a bug in the deserialization code for YAML data. If the field used as a natural key is a tuple, (has an iter method in the yaml reader) the get_by_natural_key() method is called, but if it's a singleton, the same code is called that would be if no get_by_natural_key() method were defined.

I made a patch that I believe fixes this bug (attached). It fixes the issue for me. I don't have a full working copy of Django set up, so I just generated this with POSIX diff; sorry if that's inconvenient.

Attachments (2)

python.py.patch (931 bytes) - added by jeff.k@… 4 years ago.
fixes deserialization bug
16208.patch (1.6 KB) - added by aaugustin 4 years ago.

Download all attachments as: .zip

Change History (8)

Changed 4 years ago by jeff.k@…

fixes deserialization bug

Changed 4 years ago by aaugustin

comment:1 Changed 4 years ago by aaugustin

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

I just attached a patch in the proper format, identical to the first one otherwise.

Jeff, could you provide an example of your problem? That would really help us reproduce it, and write a test case.

comment:2 Changed 4 years ago by jeff.k@…

Here's an example. This is modeled on the natural key use case in the docs. The important difference is that the natural key is a singleton field (name), not a list of fields (first_name, last_name)

from django.db import models

class AuthorManager(models.Manager):
    def get_by_natural_key(self, name):
        return self.get(name=name)

class Author(models.Model):
    objects = AuthorManager()    
    name = models.CharField(max_length=100, unique=True)

class Book(models.Model):
    title = models.CharField(max_length=100)
    author = models.ForeignKey(Author)

Here's the YAML data which fails to import into the resulting db

- {model: appname.author, pk: 1, fields: {name: Hemingway}}
- {model: appname.book, pk: 1, fields: {title: "For Whom the Bell Tolls", author: Hemingway}}

The incorrect behavior is to reject this data, complaning that "Hemingway" isn't an integer (which foreign keys need to be), because when the serialized data being loaded is a singleton, the get_by_natural_key() method is not employed to implement the mapping to the key value. I think the bug was just confusion about if/else indentation. It's kind of hairy in there.

comment:3 Changed 4 years ago by gserra

  • Patch needs improvement set

The current patch does not pass the django unit tests. Seems that when field_value is an integer, it executes

obj = field.rel.to._default_manager.db_manager(db).get_by_natural_key(field_value) 

But requires a 2 parameters instead of one.

comment:4 Changed 4 years ago by gserra

Dear Agustin,

Have you considered that the deserialization field "Hemingway" is not correct? If you check the django docs you can see that is necessary for the serialization of the naturals keys to return a tuple.

So, in that case, I would consider you to return (self.name,)

class AuthorManager(models.Manager):
    def get_by_natural_key(self, name):
        return (self.get(name=name),)


And see if it fails again.

comment:5 Changed 4 years ago by kenth

  • Owner changed from nobody to kenth
  • Status changed from new to assigned

comment:6 Changed 3 years ago by claudep

  • Resolution set to invalid
  • Status changed from assigned to closed

I think that the issue here is due to the natural key not being tuple/list, which is wrong, AFAIK. Reopen if you can provide a valid test case.

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