Skip to content

Conversation

amykyta
Copy link

@amykyta amykyta commented May 19, 2017

Allow the defaults argument to update_or_create()/get_or_create() to contain property attributes. The added tests further demonstrate the issue.

For context, in 1.11 the defaults argument passed to update_or_create() and get_or_create() is validated and non-fields are reported as FieldErrors (introduced by #7255). There is an exception made for pk (added in #7338) but I believe it should apply to all _meta._property_names for the creation behavior to be consistent with Model.create and better backwards compatibility.

One thing to note @francoisfreitag, if self.model._meta._property_names always has pk then the param test can be made simpler by not checking for pk explicitly.

https://code.djangoproject.com/ticket/28222

Copy link
Contributor

@francoisfreitag francoisfreitag May 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the comment from _property_names should change.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should it change to?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the comment to mention the new use.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I remove the setter from the code, I get the following TypeError:

======================================================================
ERROR: test_get_or_create_with_model_property_defaults (get_or_create.tests.GetOrCreateTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib64/python3.6/unittest/case.py", line 59, in testPartExecutor
    yield
  File "/usr/lib64/python3.6/unittest/case.py", line 601, in run
    testMethod()
  File "/home/freitafr/dev/django/tests/get_or_create/tests.py", line 80, in test_get_or_create_with_model_property_defaults
    t, _ = Thing.objects.get_or_create(defaults={'capitalized_name_property': "annie"}, pk=1)
  File "/home/freitafr/dev/django/django/db/models/manager.py", line 82, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/home/freitafr/dev/django/django/db/models/query.py", line 452, in get_or_create
    return self._create_object_from_params(lookup, params)
  File "/home/freitafr/dev/django/django/db/models/query.py", line 484, in _create_object_from_params
    obj = self.create(**params)
  File "/home/freitafr/dev/django/django/db/models/query.py", line 378, in create
    obj = self.model(**kwargs)
  File "/home/freitafr/dev/django/django/db/models/base.py", line 484, in __init__
    raise TypeError("'%s' is an invalid keyword argument for this function" % list(kwargs)[0])
TypeError: 'capitalized_name_property' is an invalid keyword argument for this function

I think Django should verify that the field can be set, just like the model initialization does (see ticket #25910).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point but the model init code is actually setting the values, not just checking it there.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@francoisfreitag , I've looked into this and do not understand your suggestion. That TypeError is the result of the work in ticket #25910. Can you please clarify what you meant?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the suggestion is to maintain the "Invalid field name(s) for model ..." error message a property that doesn't have a setter.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That helps. Thanks, Tim.

@timgraham timgraham merged commit 37ab3c3 into django:master May 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants