Opened 9 years ago

Closed 9 years ago

#23780 closed New feature (wontfix)

Easy to use natural keys from a tuple on meta

Reported by: Brian Faherty Owned by: nobody
Component: Core (Serialization) Version: dev
Severity: Normal Keywords:
Cc: Russell Keith-Magee Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Brian Faherty)

Having to implement two methods on two different objects in order to use natural keys is very cumbersome. I would think we could implement something on a models meta to take a tuple of field names for use in both the natural_key and get_by_natural_key methods. I implemented this in a way that would be overridable to keep compatible with the current methods but would allow for easy use in the future. Working code with this is here https://github.com/scrummyin/django/commit/7e562127c3a9610ae3ad9fab6855a005f4d4706f. Includes 6 more tests around a two new methods, with same name but on the model and manager. The old tests are still working with minimal changes. The minimal change is to use a new method to check for a natural_key instead of hasattr. The pull request branch has all

Change History (5)

comment:1 by Brian Faherty, 9 years ago

Sorry, I just realized I broke some other tests. After I broke them and took another look at them, I think I didn't break them, but there were already messed up. I have adjusted those tests for what I think is the proper use case. I believe in their original form the serializer output the models in the wrong order. I reorder the data in the test and got it to work. When I looked at the new order I realized that the new order reflected the decencies correctly. A membership had fk's to person and group and it should have come after those two objects. This is correctly reflected in the revised tests. With this git commit https://github.com/scrummyin/django/commit/ce6d437c72de478c681a934509a186cb242f84f7.

comment:2 by Brian Faherty, 9 years ago

Description: modified (diff)

comment:3 by Tim Graham, 9 years ago

Cc: Russell Keith-Magee added

Russ, you committed the initial natural keys patch in #7052. How do you feel about this shortcut?

comment:4 by Russell Keith-Magee, 9 years ago

Personally, I disagree with the original premise - I don't see defining a pair of natural key functions as "cumbersome". It's two methods, each of which are relatively straightforward to define for the simple case, but which can have some interesting edge cases for the complex case - e.g., if a foreign key is involved in a natural key, do you render the PK value? Or do you use the natural key of the foreign key? What if that natural key has a foreign key? What's the precedence of manually defined natural key function over the Meta defined one? What about if you only define *one* of the two natural key functions?

I'll note that the provided patch doesn't appear to test any of these edge cases - it only explores the simple (and obvious) case of "a single value-based field as natural key".

I see this as a case where explicit is better than implicit. If we implement this, we're going to have an implementation that is non-trivial to cover all possible use cases, which means introducing a complexity in implementation, which will need to be maintained (and will inevitably get more complex when things like composite fields, and the foreign key refactor that Anssi is sitting on hit trunk). There's also some documentation for a new feature, - instead of an explicit API entry point that implements a specific behavior, you have a configuration value, and newcomers need to know about and understand the internal behavior before they can use the feature reliably.

Or, we have a relatively simple 4 lines of explicit code on any project that needs a natural key; maybe 6 lines of code if it's a complex case (and it's the complex case that has the complex code - the simple case doesn't pay any overhead).

So - I'm a -0 on the general idea.

That said, the idea has come up a couple of times in the recent past, so there's clearly some community interest. If we *must* have an implementation of this feature, a meta attribute is about as good as it's going to get - it's certainly better than the "automagically interpret natural keys from model definition" that has been proposed by others.

comment:5 by Tim Graham, 9 years ago

Resolution: wontfix
Status: newclosed

I have similar feelings. It seems that often when we add conveniences like this to Django, we often get bogged down in accounting for corner cases and end up trying to add more magic than we originally intended.

scrummyin, please raise the issue on the DevelopersMailingList if you'd like to try to get the idea accepted.

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