Opened 13 years ago

Closed 12 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@… 13 years ago.
fixes deserialization bug
16208.patch (1.6 KB ) - added by Aymeric Augustin 13 years ago.

Download all attachments as: .zip

Change History (8)

by jeff.k@…, 13 years ago

Attachment: python.py.patch added

fixes deserialization bug

by Aymeric Augustin, 13 years ago

Attachment: 16208.patch added

comment:1 by Aymeric Augustin, 13 years ago

Needs tests: set
Triage Stage: UnreviewedAccepted

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 by jeff.k@…, 13 years ago

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 by gserra, 13 years ago

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 by gserra, 13 years ago

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 by kenth, 12 years ago

Owner: changed from nobody to kenth
Status: newassigned

comment:6 by Claude Paroz, 12 years ago

Resolution: invalid
Status: assignedclosed

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