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: | 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)
Change History (8)
by , 13 years ago
Attachment: | python.py.patch added |
---|
by , 13 years ago
Attachment: | 16208.patch added |
---|
comment:1 by , 13 years ago
Needs tests: | set |
---|---|
Triage Stage: | Unreviewed → 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 by , 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 , 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 , 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 , 13 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:6 by , 12 years ago
Resolution: | → invalid |
---|---|
Status: | assigned → 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.
fixes deserialization bug