New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #22728 - Prohibited lookups usage for get_or_create #3793

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@coldmind
Contributor

coldmind commented Dec 26, 2014

Patch proposes how this problem can be solved.
I decided to make this backwards incompatible, because the way django does it now should not be encouraged.

One more thing - ticket is about get_or_create, but I think update_or_create should have the same behavior.

@@ -494,6 +494,10 @@ def _extract_model_params(self, defaults, **kwargs):
if f.attname in lookup:
lookup[f.name] = lookup.pop(f.attname)
params = {k: v for k, v in kwargs.items() if LOOKUP_SEP not in k}
lookup_parameters = set(kwargs.keys()) - set(params.keys())

This comment has been minimized.

@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

No need to compute this if lookup_parameters_allowed is True. I don't know the ORM code enough, so you can ignore my comment if it sounds like a premature optimization:

if not lookup_parameters_allowed:
    lookup_parameters = set(kwargs.keys()) - set(params.keys())
    if lookup_parameters:
        # ...
@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

No need to compute this if lookup_parameters_allowed is True. I don't know the ORM code enough, so you can ignore my comment if it sounds like a premature optimization:

if not lookup_parameters_allowed:
    lookup_parameters = set(kwargs.keys()) - set(params.keys())
    if lookup_parameters:
        # ...
@@ -494,6 +494,10 @@ def _extract_model_params(self, defaults, **kwargs):
if f.attname in lookup:
lookup[f.name] = lookup.pop(f.attname)
params = {k: v for k, v in kwargs.items() if LOOKUP_SEP not in k}
lookup_parameters = set(kwargs.keys()) - set(params.keys())
if not lookup_parameters_allowed and lookup_parameters:
raise TypeError("Lookups are not allowed for get_or_create. "

This comment has been minimized.

@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

get_or_create -> get_or_create(). The exception message can be much simpler in my opinion. Also, if len(lookup_parameters) is 1, then the "Wrong parameters:" part is should be "Wrong parameter:" .

@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

get_or_create -> get_or_create(). The exception message can be much simpler in my opinion. Also, if len(lookup_parameters) is 1, then the "Wrong parameters:" part is should be "Wrong parameter:" .

@@ -1680,6 +1680,14 @@ whenever a request to a page has a side effect on your data. For more, see
chapter because it isn't related to that book, but it can't create it either
because ``title`` field should be unique.
.. warning::

This comment has been minimized.

@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

I think we don't need a warning directive for this kind of information. You can add a separate versionchanged directive.

@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

I think we don't need a warning directive for this kind of information. You can add a separate versionchanged directive.

If you will try to use it, ``TypeError`` will be raised::
>>> Person.objects.get_or_create(name='Bob', birthday__gte=date(1800, 12, 12))
*** TypeError: Lookups are not allowed for get_or_create. Wrong parameters: ['birthday__gte']

This comment has been minimized.

@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

The exception output is not from the stock Python REPL, right? I'd stick with the original traceback or remove it completely.

@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

The exception output is not from the stock Python REPL, right? I'd stick with the original traceback or remove it completely.

@@ -125,6 +125,14 @@ def test_get_or_create_on_related_manager(self):
# The publisher should have three books.
self.assertEqual(p.books.count(), 3)
def test_get_or_create_with_lookups_raises_exception(self):
Person.objects.create(first_name="Morihei", last_name="Ueshiba", birthday=date(1883, 12, 14))
self.assertRaises(

This comment has been minimized.

@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

Please use assertRaisesMessage.

@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

Please use assertRaisesMessage.

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Apr 23, 2015

Member

Yes, we can also apply this to update_or_create()

Member

timgraham commented Apr 23, 2015

Yes, we can also apply this to update_or_create()

.. warning::
Lookups usage is not allowed for ``get_or_create()``.
If you will try to use it, ``TypeError`` will be raised::

This comment has been minimized.

@kezabelle

kezabelle May 2, 2015

Contributor

Stilted sentence up to the comma, IMHO. Perhaps lead with Attempting to ...?

@kezabelle

kezabelle May 2, 2015

Contributor

Stilted sentence up to the comma, IMHO. Perhaps lead with Attempting to ...?

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Jun 2, 2015

Member

@coldmind, do you have time/interest in following up on this? Was thinking we could offer completing it as a task for the DjangoconEU sprints if not.

Member

timgraham commented Jun 2, 2015

@coldmind, do you have time/interest in following up on this? Was thinking we could offer completing it as a task for the DjangoconEU sprints if not.

@coldmind

This comment has been minimized.

Show comment
Hide comment
@coldmind

coldmind Jun 2, 2015

Contributor

@timgraham, I'm actually on DjangoConEU now, and also will participate in sprints, but we can propose this task for someone else who had not commited to django before.

Contributor

coldmind commented Jun 2, 2015

@timgraham, I'm actually on DjangoConEU now, and also will participate in sprints, but we can propose this task for someone else who had not commited to django before.

@coldmind

This comment has been minimized.

Show comment
Hide comment
@coldmind

coldmind Jun 4, 2015

Contributor

New PR: #4779

Contributor

coldmind commented Jun 4, 2015

New PR: #4779

@coldmind coldmind closed this Jun 4, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment