Passing concrete instances to `ListInput[NodeInput]` when using model inheritance causes unintended removal of objects

[axieum]

Describe the Bug

Given the following polymorphic Django models using django-model-util's InheritanceManager -

from __future__ import annotations

from django.db.models import ManyToManyField
from django_stubs_ext.db.models import TypedModelMeta
from model_utils.managers import InheritanceManager

class Project(Model):
    """An abstract project."""

    # ...
    dependencies = ManyToManyField(
        "self",
        symmetrical=False,
        related_name="dependants",
        help_text="A list of projects required by this project.",
        blank=True,
    )
    # ...

    objects = InheritanceManager()

    class Meta(TypedModelMeta):
        verbose_name = "project"
        verbose_name_plural = "projects"


class ExternalProject(Project):
    """An external project."""

    class Meta:
        verbose_name = "external project"
        verbose_name_plural = "external projects"

And a Strawberry GraphQL type that allows mutating the dependencies and dependants many-to-many relationship -

#...

@strawberry_django.partial(models.Project, description="A partial project input.")
class ProjectInputPartial(NodeInput):
    """A partial project input."""

   # ...
    dependencies: ListInput[NodeInput] | None = strawberry_django.field(
        description="An updated list of projects required by this project."
    )
    dependants: ListInput[NodeInput] | None = strawberry_django.field(
        description="An updated list of projects that require this project."
    )
    # ...

A used in a Strawberry Django CUD mutation like so -

class ProjectUpdateMutation(DjangoUpdateMutation):
    """A mutation for updating a project."""

    def __init__(self, name: str | None = None, **kwargs: Any) -> None:
        super().__init__(graphql_name=name, **kwargs)

    @property
    def django_model(self) -> type[Model]:
        return super().django_model or models.Project


# ...
create_external_project: Any = partial(ProjectCreateMutation, input_type=ExternalProjectInput)
update_external_project: Any = partial(ProjectUpdateMutation, input_type=ExternalProjectInputPartial)
# ...
update_project: Any = partial(ProjectUpdateMutation, input_type=ProjectInputPartial)
# ...

And using it in your Strawberry schema like so -

@strawberry.type
class ProjectsMutation:
    """A collection of project-related GraphQL mutations."""

    update_external_project: ExternalProject = mutations.update_external_project(
        description="Updates an external project.",
        extensions=[HasPerm("projects.change_project", fail_silently=False), HasScope("write:project")],
    )

    # ...

    # NB: The below mutations act on the `Project` interface. Since the mutations already return a union
    #     with `OperationInfo`, and interfaces are prohibited in unions, we must expand the full `Project` type.

    update_project: ExternalProject | ... | WebProject = mutations.update_project(
        description="Updates a generic project.",
        extensions=[HasPerm("projects.change_project", fail_silently=False), HasScope("write:project")],
    )

    # ...

And executing via the following query -

mutation UpdateProject {
  updateProject(data: {
    id: "RXh0ZXJuYWxQcm9qZWN0Ojk="
    dependencies: { set: [
      { id: "V2ViUHJvamVjdDox" },
      { id: "V2ViUHJvamVjdDoy" }
    ] }
    dependants: { set: [
      { id: "RXh0ZXJuYWxQcm9qZWN0OjE=" }
    ] }
  }) {
    __typename
    ... on Project {
      id
      slug
    }
    ... on OperationInfo {
      messages {
        code
        field
        message
      }
    }
  }
}

It will reach the update_m2m function which fetches the existing objects in the relationship:

strawberry-django/strawberry_django/mutations/resolvers.py

Line 654 in 269954c

existing = set(manager.all())

This yields a set of abstract models existing: set[Project].

Since the mutation was a ListInput[NodeInput] (i.e. we pass { id: "RXh0ZXJuYWxQcm9qZWN0OjE=" }) this yields obj: ExternalProject.

Later, it will check if obj not in existing, which is always true because it is trying to compare the concrete model to the abstract model.

strawberry-django/strawberry_django/mutations/resolvers.py

Lines 697 to 700 in 269954c

	elif obj not in existing:
	to_add.append(obj)
	existing.discard(obj)

Then, it tries to existing.discard(obj) which does nothing, because obj: ExternalProject is not in set[Project].

Which means it gets to the end, re-adds the objects to the relationship, but then existing has all the original projects in it, and so it for remaining in existing removes them from the relationship immediately after.

Solution

A possible solution would be to upcast concrete types (e.g. ExternalProject) into their abstract type (e.g. Project) in values before checking if obj not in existing.

Another solution is to change the line existing = set(manager.all()) to account for inheritance querysets.

existing = set(manager.select_subclasses() if isinstance(manager, InheritanceManager) else manager.all())

Workaround

Ensure the passed id is for the abstract Project node and not the concrete type. This is not feasible and abstract Project IDs are never returned to the client as they only ever query concrete types.

Is there a way to avoid the complexities of NodeInput and just pass dependencies: { set: [ "slug-1", "slug-2" ] } and therefore avoid concrete types altogether?

System Information

• Operating system: Windows
• Python version: 3.11
• Strawberry version (if applicable): 0.65.1

Additional Context

[bellini666]

Hi @axieum ,

Hrm, interesting case. I don't use polymorphic models too much, so I'm not completely sure what is going on here.

A possible solution would be to upcast concrete types (e.g. ExternalProject) into their abstract type (e.g. Project) in values before checking if obj not in existing.
Another solution is to change the line existing = set(manager.all()) to account for inheritance querysets.

I think it is fair to have a logic to account for polymorphic in there. We have similar logics in other places

As you are already aware of the issue and the proposed solution, would you like to try to send a PR for it? I'll gladly review it and can help you if needed (ping me on discord)

[axieum]

Hey @bellini666, I've gone ahead and created a draft PR with the failing tests so far: #798

[bellini666]

@axieum hrm, I think you linked the wrong PR =P

[axieum]

Ah, I've edited my comment. As for the PR, I've only written a failing test case, hence why the PR is in draft status.

Running the test, you'll see it fails the last assertion due to the 2nd mutate removing the objects.