#13252 closed New feature (fixed)
Use the natural key instead of the primary key when serializing
Reported by: | Chris Beaven | Owned by: | Malcolm Tredinnick |
---|---|---|---|
Component: | Core (Serialization) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Tai Lee, chris@…, Chris Spencer, dev@…, charette.s@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
This would allow existing objects to be updated and missing objects to be added across databases (e.g. when the autonumber primary keys may not match).
Attachments (7)
Change History (36)
by , 15 years ago
Attachment: | 13252.diff added |
---|
comment:1 by , 15 years ago
Has patch: | set |
---|---|
Needs documentation: | set |
Needs tests: | set |
comment:2 by , 15 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Unreviewed → Accepted |
I accept the use case - that you should be able to say "update the existing object that matches the natural key, or create a new object if one doesn't exist". However I'm not convinced that providing a natural key for the PK value is the right way to do this.
Essentially, this approach means specifying the natural key data twice - once in the fields of the object, and once in the primary key value. This strikes me as especially un-DRY.
I haven't given this a lot of thought, but there might be a way of using the natural key without to specify it in the pk attribute. A natural key needs to be unique, so if a model defines a natural key, you shouldn't need to have the PK value specifically (or the natural key value as the primary key) in order to reconstruct the natural key for an object. This does leave the question of when the 'update or create' behavior should be invoked, but I think this will be less prone to error than reproducing the natural key.
by , 15 years ago
Attachment: | 13252.2.diff added |
---|
comment:3 by , 15 years ago
Patch needs improvement: | unset |
---|
comment:4 by , 14 years ago
Needs documentation: | unset |
---|---|
Needs tests: | unset |
comment:5 by , 14 years ago
Owner: | changed from | to
---|
comment:7 by , 14 years ago
Cc: | added |
---|
by , 14 years ago
Attachment: | 13252-natural-key-serializing-r14992.diff added |
---|
comment:8 by , 14 years ago
Updated patch to work on r14992. Appears to work well locally. serializers_regress tests pass (on sqlite at least).
by , 14 years ago
Attachment: | 13252-natural-key-serializing-r14995.diff added |
---|
comment:11 by , 14 years ago
I've just updated the patch after feedback from freakboy3742 on IRC.
The new patch adds --natural-foreign
and --natural-primary
options to the dumpdata
management command, maps the -n
and --natural
options to --natural-foreign
and adds a note in the documentation that the old options are deprecated. This should retain backwards compatibility with anyone who was depending on data dumped with natural keys still containing a pk
field for models that support natural keys. It also adds the corresponding use_natural_foreign_keys
and use_natural_primary_keys
arguments to serializers.serialize()
and maps use_natural_keys
to use_natural_foreign_keys
.
I've left a comment in the source code where I think we should probably raise a deprecation warning (but not actually raised it) if that ends up being the desired behaviour.
One quirk is that the code beginning at line 30 in django/core/serializers/python.py appears to affect the order of fields in serialized output depending on how the data dict is constructed, even though the resulting data dict contains the same data. If I first create the data dict with model
and fields
items and then conditionally add a pk
item (as the old patch did), then the serialized output is different to when I create the data dict with model
, pk
and fields
items. Rather than change a tonne of hard coded tests, I construct the data dict the same way that it would have been before the patch which results in a couple of duplicated lines of code.
comment:12 by , 14 years ago
Type: | → New feature |
---|
comment:13 by , 14 years ago
Severity: | → Normal |
---|
comment:14 by , 14 years ago
Easy pickings: | unset |
---|---|
Patch needs improvement: | set |
13252-natural-key-serializing-r14995.diff fails to apply cleanly on to trunk
by , 13 years ago
Attachment: | 13252-natural-key-serializing-r16406.diff added |
---|
Updated to apply cleanly on current trunk
comment:15 by , 13 years ago
Patch needs improvement: | unset |
---|---|
UI/UX: | unset |
comment:16 by , 13 years ago
Patch needs improvement: | set |
---|
The new behaviour needs to be optional to be backwards compatible.
comment:17 by , 13 years ago
Cc: | added |
---|
comment:18 by , 13 years ago
Cc: | added |
---|
by , 13 years ago
Attachment: | 13252-natural-key-serializing-r17262.diff added |
---|
Updated to current trunk
comment:19 by , 13 years ago
Cc: | added |
---|
comment:20 by , 12 years ago
Cc: | added |
---|
comment:21 by , 12 years ago
I've updated this patch again on a branch at GitHub.
https://github.com/thirstydigital/django/tree/tickets/13252-natural-key-serializing
SmileyChris, can you elaborate on making the new behaviour optional? Unless I'm missing something, it is? The --natural
option maps to --natural-foreign
(which behaves the same, with a pending deprecation warning) and we now have a new --natural-primary
option as well.
I believe I've incorporated all the changes russellm requested and this should be RFC now.
comment:22 by , 12 years ago
FWIW all tests pass on python 2.7.3rc1 sqlite3. I guess the backward compatibility or must opt-in issue SmileyChris was referring to is no more with two new flags?
comment:23 by , 12 years ago
I would be very pleased to see this feature enhancement reach the master branch asap 'cause I actually pretty much need it and have write myself some kind of the same..
comment:25 by , 12 years ago
Status: | reopened → new |
---|
comment:26 by , 11 years ago
Patch needs improvement: | unset |
---|
Rebased onto master and changes for feedback integrated.
comment:28 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
This also fixes #11486 by proxy.