[Django] #23780: Easy to use natural keys from a tuple on meta
Django
--------------------------------------+--------------------
Reporter: scrummyin | Owner: nobody
Type: New feature | Status: new
Component: Core (Serialization) | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
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. Sll 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23780>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: scrummyin | Owner: nobody
Type: New feature | Status: new
(Serialization) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by scrummyin):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23780#comment:1>
Django
Reporter: scrummyin | Owner: nobody
Type: New feature | Status: new
(Serialization) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Description changed by scrummyin:
Old description:
> 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. Sll 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.
New description:
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
--
--
Ticket URL: <https://code.djangoproject.com/ticket/23780#comment:2>
Django
Reporter: scrummyin | Owner: nobody
Type: New feature | Status: new
(Serialization) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by timgraham):
* cc: freakboy3742 (added)
Comment:
Russ, you committed the initial natural keys patch in #7052. How do you
feel about this shortcut?
--
Ticket URL: <https://code.djangoproject.com/ticket/23780#comment:3>
Django
Reporter: scrummyin | Owner: nobody
Type: New feature | Status: new
(Serialization) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by freakboy3742):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23780#comment:4>
Django
Reporter: scrummyin | Owner: nobody
Type: New feature | Status: closed
Component: Core | Version: master
(Serialization) | Resolution: wontfix
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by timgraham):
* status: new => closed
* resolution: => wontfix
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23780#comment:5>