Skip to content
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

Fix nested fields update #604

Closed
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion strawberry_django/mutations/fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@
)
from typing_extensions import Literal, Self

from .types import FullCleanOptions


def _get_validation_errors(error: Exception):
if isinstance(error, PermissionDenied):
Expand Down Expand Up @@ -176,7 +178,7 @@ class DjangoMutationCUD(DjangoMutationBase):
def __init__(
self,
input_type: type | None = None,
full_clean: bool = True,
full_clean: bool | FullCleanOptions = True,
argument_name: str | None = None,
key_attr: str | None = None,
**kwargs,
Expand Down
7 changes: 7 additions & 0 deletions strawberry_django/mutations/mutations.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
DjangoMutationBase,
DjangoUpdateMutation,
)
from .types import FullCleanOptions

_T = TypeVar("_T")

Expand Down Expand Up @@ -261,6 +262,7 @@
extensions: List[FieldExtension] = (), # type: ignore
argument_name: Optional[str] = None,
handle_django_errors: Optional[bool] = None,
full_clean: bool | FullCleanOptions = True,

Check failure on line 265 in strawberry_django/mutations/mutations.py

View workflow job for this annotation

GitHub Actions / Typing

Alternative syntax for unions requires Python 3.10 or newer (reportGeneralTypeIssues)
) -> Any:
"""Create mutation for django input fields.

Expand Down Expand Up @@ -297,6 +299,7 @@
extensions=extensions or (),
argument_name=argument_name,
handle_django_errors=handle_django_errors,
full_clean=full_clean,
)


Expand All @@ -320,6 +323,7 @@
argument_name: Optional[str] = None,
handle_django_errors: Optional[bool] = None,
key_attr: Optional[str] = None,
full_clean: bool | FullCleanOptions = True,

Check failure on line 326 in strawberry_django/mutations/mutations.py

View workflow job for this annotation

GitHub Actions / Typing

Alternative syntax for unions requires Python 3.10 or newer (reportGeneralTypeIssues)
) -> Any:
"""Update mutation for django input fields.

Expand Down Expand Up @@ -356,6 +360,7 @@
argument_name=argument_name,
handle_django_errors=handle_django_errors,
key_attr=key_attr,
full_clean=full_clean,
)


Expand All @@ -379,6 +384,7 @@
argument_name: Optional[str] = None,
handle_django_errors: Optional[bool] = None,
key_attr: Optional[str] = None,
full_clean: bool | FullCleanOptions = True,

Check failure on line 387 in strawberry_django/mutations/mutations.py

View workflow job for this annotation

GitHub Actions / Typing

Alternative syntax for unions requires Python 3.10 or newer (reportGeneralTypeIssues)
) -> Any:
return DjangoDeleteMutation(
input_type=input_type,
Expand All @@ -399,4 +405,5 @@
argument_name=argument_name,
handle_django_errors=handle_django_errors,
key_attr=key_attr,
full_clean=full_clean,
)
77 changes: 52 additions & 25 deletions strawberry_django/mutations/resolvers.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@

if key_attr in value:
obj_pk = value[key_attr]
if obj_pk is not strawberry.UNSET:
if obj_pk not in (None, strawberry.UNSET): # noqa: PLR6201
return model._default_manager.get(pk=obj_pk), value

return None, value
Expand All @@ -99,8 +99,9 @@
continue

if isinstance(v, ParsedObject):
if v.pk is None:
v = create(info, model, v.data or {}) # noqa: PLW2901
if v.pk in (None, UNSET): # noqa: PLR6201
related_model = get_model_fields(model).get(k).related_model

Check failure on line 103 in strawberry_django/mutations/resolvers.py

View workflow job for this annotation

GitHub Actions / Typing

"related_model" is not a known attribute of "None" (reportOptionalMemberAccess)
v = create(info, related_model, v.data or {}) # noqa: PLW2901

Check failure on line 104 in strawberry_django/mutations/resolvers.py

View workflow job for this annotation

GitHub Actions / Typing

No overloads for "create" match the provided arguments (reportCallIssue)

Check failure on line 104 in strawberry_django/mutations/resolvers.py

View workflow job for this annotation

GitHub Actions / Typing

Argument of type "Any | type[Model] | None" cannot be assigned to parameter "model" of type "type[_M@create]" in function "create"   Type "Any | type[Model] | None" is incompatible with type "type[_M@create]" (reportArgumentType)
Comment on lines +102 to +104
Copy link
Author

Choose a reason for hiding this comment

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

Seems like pk may be either UNSET or None, in both cases it's necessary to create nested object. And to further properly perform full_clean and save we should identify related model (as in test type hierarchy most models use name clashing field names, cleaning could pass somehow, but finally object of different type could be created).

elif isinstance(v.pk, models.Model) and v.data:
v = update(info, v.pk, v.data, key_attr=key_attr) # noqa: PLW2901
else:
Expand Down Expand Up @@ -178,6 +179,9 @@
)

if isinstance(data, (OneToOneInput, OneToManyInput)):
if not data.set:
return None

return ParsedObject(
pk=parse_input(info, data.set, key_attr=key_attr),
)
Expand Down Expand Up @@ -240,6 +244,12 @@
if dataclasses.is_dataclass(data):
data = vars(data)

# `pk` may be explicitly passed as `None` to force crete object, `pk` is not a field and won't be handled below,
# so manually add one into `direct_field_values`
pk = data.pop("pk", UNSET)
if pk is not UNSET:
direct_field_values["pk"] = pk

for name, value in data.items():
field = fields.get(name)
direct_field_value = True
Expand Down Expand Up @@ -366,16 +376,24 @@
key_attr=key_attr,
)

# Creating the instance directly via create() without full-clean will
# raise ugly error messages. To generate user-friendly ones, we want
# full-clean() to trigger form-validation style error messages.
full_clean_options = full_clean if isinstance(full_clean, dict) else {}
if full_clean:
try:
# Instead of using `get_or_create` shortcut first use `get()`
# (which will also raise `DoesNotExist` in case pk `None` value explicitly
# passed), if no records found - invoke `full_clean()` and `create`
# right after. The idea is to bypass cleaning first to allow getting
# an object by e.g. unique fields. In case `full_clean()` invoked before,
# it would raise unique constraint validation error
instance = model._default_manager.get(**create_kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

Is this safe? I mean, how can we ensure that create_kwargs will identify an object by unique fields?

Suppose that we have a model that has a name, and it is not unique nor something that should identify it. Then we get create_kwargs = {"name": "Foobar"}. If we already have that object, wouldn't this get that object by mistake instead of creating a new one (which is what we want)?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, you are completely right, that is my main concern. Unfortunately an object with Foobar name will be returned and no new object created. Django built-inget_or_create shortcut will do exactly the same, I tried to deep-dive into Django code to find any implementation of identifying all unique fields (FK, unique prop/other unique constraints), but it seems that's tricky.

This idea comes from #360, as I tried to unify nested object creation, something breaks here, either #360 fix or new object creation. I shared my thoughts in the current PR description, could you please also have a look?

The idea to identify all unique fields seems promising, but I'm not sure that we can easily cover all possible scenarios, probably there are some DB-specific tricks also. I could not find any built-in implementation for that, Django total_unique_constraints property does not solve that problem.

I see two options here to keep both #360 fix and duplicated objects creation:

  • Introduce convention to explicitly add id=None in payload
  • Identify all unique fields and execute get() only if unique fields are in the query

Please share your thoughts. I'd personally prefer option 2, but some more research needed, have some doubts here. Option 1 looks a bit weird, but it's bullet-proof.

Probably option 3 is to remove get() and always execute full_clean followed by create, but it would break #360.

Copy link
Author

@EvSpirit EvSpirit Jul 30, 2024

Choose a reason for hiding this comment

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

I've checked Django code for unique fields validations, there are a few helpful fragments:

  1. This one covers unique_together and total_unique_constraints
    https://github.com/django/django/blob/9cf9c796be8dd53bc3b11355ff39d65c81d7be6d/django/contrib/admin/views/main.py#L473-L484
constraint_field_names = (
    *self.lookup_opts.unique_together,
    *(
        constraint.fields
        for constraint in self.lookup_opts.total_unique_constraints
    ),
)
  1. unique field + null check
    https://github.com/django/django/blob/9cf9c796be8dd53bc3b11355ff39d65c81d7be6d/django/contrib/admin/views/main.py#L441-L445
total_ordering_fields = {"pk"} | {
    field.attname
    for field in self.lookup_opts.fields
    if field.unique and not field.null
}

So what we can try to do is to extract all unique fields using these three "sources" (unique, unique_together and UniqueConstraint), what do you think? I suppose in case only one of three types is used it should be relatively simple, but in case multiple one are in, that may be tricky.

But I still have doubts, should such a logic be a part of the library? Please share your thoughts.

Copy link
Member

Choose a reason for hiding this comment

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

Although I do like the "identify the unique sources" idea, I'm worried if that could end up reusing some object where we were supposed to create a new one (and if it was unique, the user would maybe expect the code to break)

I'm thinking about a fourth option: What about expanding key_attr (or creating another field and deprecating it) to tell the list of fields/field combinations that are to be considered "keys"? Just like unique_together = [("foo", "bar"), ("bin",)] for the django model

In that example, someone could only pass "bin" to act like "get_or_create", but ("foo", "bar") would still try to only create, etc.

except model.DoesNotExist:
# Creating the instance directly via create() without full-clean will
# raise ugly error messages. To generate user-friendly ones, we want
# full-clean() to trigger form-validation style error messages.
full_clean_options = full_clean if isinstance(full_clean, dict) else {}
dummy_instance.full_clean(**full_clean_options) # type: ignore
Comment on lines +379 to 392
Copy link
Author

Choose a reason for hiding this comment

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

Instead of using get_or_create shortcut I finally came up with a decision to use get and create separately, but invoke full_clean only after get() is invoked as full_clean may raise unique constraint violation exception (e.g. in use case described in #360 )


# Create the instance using the manager create method to respect
# manager create overrides. This also ensures support for proxy-models.
instance = model._default_manager.create(**create_kwargs)
# Create the instance using the manager create method to respect
# manager create overrides. This also ensures support for proxy-models.
instance = model._default_manager.create(**create_kwargs)

for field, value in m2m:
update_m2m(info, instance, field, value, key_attr)
Expand Down Expand Up @@ -552,16 +570,33 @@

use_remove = True
if isinstance(field, ManyToManyField):
manager = cast("RelatedManager", getattr(instance, field.attname))
remote_field_name = field.attname
reverse_field_name = field.remote_field.related_name

Check failure on line 574 in strawberry_django/mutations/resolvers.py

View workflow job for this annotation

GitHub Actions / Typing

Cannot access attribute "related_name" for class "Field[Sequence[Unknown], ManyToManyRelatedManager[Unknown, Unknown]]"   Attribute "related_name" is unknown (reportAttributeAccessIssue)
Copy link
Author

Choose a reason for hiding this comment

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

Have some doubts here, probably there is any other way to get reverse relationship name.

else:
assert isinstance(field, (ManyToManyRel, ManyToOneRel))
accessor_name = field.get_accessor_name()
assert accessor_name
manager = cast("RelatedManager", getattr(instance, accessor_name))
remote_field_name = field.get_accessor_name()
reverse_field_name = field.field.name
if field.one_to_many:
# remove if field is nullable, otherwise delete
use_remove = field.remote_field.null is True

assert remote_field_name
assert reverse_field_name
manager = cast("RelatedManager", getattr(instance, remote_field_name))

def create_nested(d):
ref_instance = (
manager.instance

Check failure on line 589 in strawberry_django/mutations/resolvers.py

View workflow job for this annotation

GitHub Actions / Typing

Cannot access attribute "instance" for class "RelatedManager[Unknown]"   Attribute "instance" is unknown (reportAttributeAccessIssue)

Check failure on line 589 in strawberry_django/mutations/resolvers.py

View workflow job for this annotation

GitHub Actions / Typing

Cannot access attribute "instance" for class "ManyToManyRelatedManager[Unknown, Unknown]"   Attribute "instance" is unknown (reportAttributeAccessIssue)
if field.one_to_many or field.one_to_one
else [manager.instance]

Check failure on line 591 in strawberry_django/mutations/resolvers.py

View workflow job for this annotation

GitHub Actions / Typing

Cannot access attribute "instance" for class "RelatedManager[Unknown]"   Attribute "instance" is unknown (reportAttributeAccessIssue)
)
return create(
info,
manager.model,
d | {reverse_field_name: ref_instance},
full_clean=full_clean,
)

to_add = []
to_remove = []
to_delete = []
Expand Down Expand Up @@ -620,11 +655,7 @@

existing.discard(obj)
else:
if key_attr not in data: # we have a Input Type
obj, _ = manager.get_or_create(**data)
else:
data.pop(key_attr)
obj = manager.create(**data)
obj = create_nested(data)

if full_clean:
obj.full_clean(**full_clean_options)
Expand Down Expand Up @@ -655,11 +686,7 @@
data.pop(key_attr, None)
to_add.append(obj)
elif data:
if key_attr not in data:
manager.get_or_create(**data)
else:
data.pop(key_attr)
manager.create(**data)
create_nested(data)
else:
raise AssertionError

Expand Down
6 changes: 5 additions & 1 deletion strawberry_django/mutations/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,12 @@ class ParsedObject:
data: dict[str, Any] | None = None

def parse(self, model: type[_M]) -> tuple[_M | None, dict[str, Any] | None]:
if self.pk is None or self.pk is UNSET:
if self.pk is UNSET:
return None, self.data
if self.pk is None:
# if `pk` was explicitly passed with `None` value, keep one in data as it indicates
# that object must be forced created when Django `get_or_create()` invoked
return None, (self.data or {}) | {"pk": None}

if isinstance(self.pk, models.Model):
assert isinstance(self.pk, model)
Expand Down
5 changes: 4 additions & 1 deletion tests/mutations/test_mutations.py
Original file line number Diff line number Diff line change
Expand Up @@ -205,8 +205,11 @@ def test_update_m2m_with_new_different_objects(mutation, fruit):


def test_update_m2m_with_duplicates(mutation, fruit):
# Pass id `null` value in the second argument to force create one.
# If id is not specified, `apple` type won't be duplicated as
# "get-or-create" approach is used under the hood
result = mutation(
'{ fruits: updateFruits(data: { types: [{name: "apple"}, {name: "apple"}]}) { id types { id name }}}'
'{ fruits: updateFruits(data: { types: [{ name: "apple"}, {id: null, name: "apple"}]}) { id types { id name }}}'
Copy link
Member

Choose a reason for hiding this comment

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

This is what I was afraid in a previous comment. Should this case be forced to pass id: null? name is not an fk and neither a key that is used to reference the model, so it seems to me that we should not need this change to make this test work

Copy link
Author

Choose a reason for hiding this comment

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

Yes, unfortunately without explicit id: null duplicated object won't be created as Django ORM get(...) method would return an object found by name.

Provided more details in the previous reply: https://github.com/strawberry-graphql/strawberry-django/pull/604/files#r1696994011, let's continue discussion there if you don't mind

)
assert not result.errors
assert result.data["fruits"][0]["types"] == [
Expand Down
31 changes: 29 additions & 2 deletions tests/projects/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,11 @@ class Meta: # type: ignore
objects = FavoriteQuerySet.as_manager()


class BugReproduction(models.Model):
description = models.TextField()


class Issue(NamedModel):
comments: "RelatedManager[Issue]"
issue_assignees: "RelatedManager[Assignee]"

class Kind(models.TextChoices):
Expand Down Expand Up @@ -160,6 +163,13 @@ class Kind(models.TextChoices):
through="Assignee",
related_name="+",
)
bug_reproduction = models.OneToOneField["BugReproduction"](
"BugReproduction",
null=True,
blank=True,
related_name="issue",
on_delete=models.CASCADE,
)

@property
def name_with_kind(self) -> str:
Expand All @@ -171,6 +181,21 @@ def name_with_priority(self) -> str:
return f"{self.kind}: {self.priority}"


class Version(NamedModel):
Copy link
Author

Choose a reason for hiding this comment

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

I tried not to further extend types hierarchy, but this one is required to cover #362 changes

issue_id: int
issue = models.ForeignKey[Issue](
Issue,
on_delete=models.CASCADE,
related_name="versions",
related_query_name="version",
)

class Meta:
unique_together = [ # noqa: RUF012
("name", "issue")
]


class Assignee(models.Model):
class Meta: # type: ignore
unique_together = [ # noqa: RUF012
Expand Down Expand Up @@ -202,14 +227,16 @@ class Meta: # type: ignore
)


class Tag(NamedModel):
class Tag(models.Model):
issues: "RelatedManager[Issue]"

id = models.BigAutoField(
verbose_name="ID",
primary_key=True,
)

name = models.CharField(max_length=255, unique=True)
Copy link
Author

Choose a reason for hiding this comment

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

Also related to #362 . Not only unique_together may be the case but also a single unique field.



class Quiz(models.Model):
title = models.CharField(
Expand Down
Loading
Loading