Invalid result in many to many list if through instance is soft deleted

[RasoulRostami]

Hi guys. I have below models

class BaseModel(SafeDeleteModel):
     _safedelete_policy = SOFT_DELETE_CASCADE
     
class Student(BaseModel):
    full_name = models.CharField(max_length=150)

class School(BaseModel):
    name = models.CharField(max_length=150)
    students = models.ManyToManyField(
        Student,
        through='SchoolStudent',
    )

class SchoolStudent(BaseModel):
    student = models.ForeignKey(Student, on_delete=models.CASCADE)
    school = models.ForeignKey(School, on_delete=models.CASCADE)

I create a relationship between a school and a student, and then I delete it. but the below event occurs

school = School.objects.create(name='new_school')
student = Student.objects.create(full_name='Tom Smith')
school.students.add(student)
# delete all relationship
SchoolStudent.objects.all().delete()

school.students.all()

<QuerySet [<Student: Tom Smith>]>

As you see. we get wrong results. we deleted the relationship but we get the queryset. Have you got any idea to solve the issue?

[Gagaro]

Hi, was it with the 1.1.0? Filtering was moved to the query itself to fix those kind of things.

[bjudson]

I'm seeing this same issue with 1.1.0. I actually was working on my own soft delete implementation and this was one of the issues that led me to start looking at 3rd party libraries. What I've found is that RelatedManager inherits the way you'd expect, but ManyRelatedManager does not. So you'll find with an example like above:

school.students.count()  # returns 1
school.schoolstudent_set.count()  # returns 0

[RasoulRostami]

Hi, was it with the 1.1.0? Filtering was moved to the query itself to fix those kind of things.

Thanks for your response. Yes. It was the 1.1.0 version. I have checked it to have the newer version.

[RasoulRostami]

I'm seeing this same issue with 1.1.0. I actually was working on my own soft delete implementation and this was one of the issues that led me to start looking at 3rd party libraries. What I've found is that RelatedManager inherits the way you'd expect, but ManyRelatedManager does not. So you'll find with an example like above:

school.students.count()  # returns 1
school.schoolstudent_set.count()  # returns 0

Thanks for your response. It worked, but as you know, it returns the SchoolStudent entities. but I need student entities.

[Gagaro]

It's probably because the through instance is deleted but not the related one. I'll try to see if I can find a quick fix,

[Gagaro]

So because create_forward_many_to_many_manager() create a new manager inheriting from our own and not the other way around, it seems complicated to fix this.

I added a test which reproduce this but it fails for now.

[bjudson]

I'm seeing this same issue with 1.1.0. I actually was working on my own soft delete implementation and this was one of the issues that led me to start looking at 3rd party libraries. What I've found is that RelatedManager inherits the way you'd expect, but ManyRelatedManager does not. So you'll find with an example like above:

school.students.count()  # returns 1
school.schoolstudent_set.count()  # returns 0

Thanks for your response. It worked, but as you know, it returns the SchoolStudent entities. but I need student entities.

Correct. As a workaround for your situation, you can just do something like:

Student.objects.filter(schoolstudent__school=school, schoolstudent__deleted=None)

But it will be difficult to remember not to use ManyRelatedManager with these objects, and likely lead to bugs at some point.

I'll keep looking at this and see if I can find any solution, and if so submit a PR. I decided to adopt this library in a project I'm working on, largely because of its strong tests and handling of tricky situations like this, so I'd love to have this one handled in an intuitive way.

Thanks for your work & responsiveness @Gagaro

[charles-hussey]

A bit late to the party, just chiming in here since I encountered the same issue. Unfortunately, I don't see a way to easily integrate a fix for this into the current code base, since the problem is due to the ManyRelatedManager bypassing the manager for through models. But a customizable solution that worked for me was just to superclass the get_queryset method on the managers for the models at both ends of the m2m field. So for your example it would look like:

class StudentManager(SafeDeleteManager):
    def get_queryset(self):
        through = getattr(self, 'through', None)
        if through and issubclass(through, SafeDeleteModel):
            self.core_filters.update({
                "schoolstudent__deleted__is_null": True
            })
        return super().get_queryset()

And the manager for the School class would look much the same. I dunno, lmk if this is poor Django form.

I'm on Django 4.1.1, in case that matters

[nathanbottomley]

I'm also seeing the same behaviour. As far as I can tell this makes the safedelete package basically incompatible with m2m relationships with through models?

At the moment we have been able to get something that works by generalising @charles-hussey's solution so that it can handle being passed to all models, and with any custom foreign key related names that might have been set on the through models.

class CustomSafeDeleteManager(SafeDeleteManager):
    def get_queryset(self):
        through = getattr(self, "through", None)
        if through and issubclass(through, SafeDeleteModel):
            for field in through._meta.get_fields():
                # Check if the field is a `ForeignKey` to the current model
                if (
                    isinstance(field, models.ForeignKey)
                    and field.related_model == self.model
                ):
                    # Filter out objects based on deleted through objects using related name or model name
                    through_lookup = (
                        field.remote_field.related_name or through._meta.model_name
                    )
                    self.core_filters.update(
                        {f"{through_lookup}__deleted__isnull": True}
                    )
        return super().get_queryset()

I'd love to hear if anyone else has solved this in a different way, or how people are still using Django's m2m fields with safedelete.

Also, if anyone knows of any issues we may have introduced generally into the ORM by using this custom manager with all models please let me know!

[marianobianchi]

I'm also seeing the same behaviour. As far as I can tell this makes the safedelete package basically incompatible with m2m relationships with through models?

At the moment we have been able to get something that works by generalising @charles-hussey's solution so that it can handle being passed to all models, and with any custom foreign key related names that might have been set on the through models.

class CustomSafeDeleteManager(SafeDeleteManager):
    def get_queryset(self):
        through = getattr(self, "through", None)
        if through and issubclass(through, SafeDeleteModel):
            for field in through._meta.get_fields():
                # Check if the field is a `ForeignKey` to the current model
                if (
                    isinstance(field, models.ForeignKey)
                    and field.related_model == self.model
                ):
                    # Filter out objects based on deleted through objects using related name or model name
                    through_lookup = (
                        field.remote_field.related_name or through._meta.model_name
                    )
                    self.core_filters.update(
                        {f"{through_lookup}__deleted__isnull": True}
                    )
        return super().get_queryset()

I'd love to hear if anyone else has solved this in a different way, or how people are still using Django's m2m fields with safedelete.

Also, if anyone knows of any issues we may have introduced generally into the ORM by using this custom manager with all models please let me know!

I tried this solution and it works only if no prefetch was made. If a prefetch was used, the core_filters updated in that piece of code are not used and you get all instances, no matter if the through model was safe deleted or not. I debug the code and this is what I found:

The query was being returned in that 1084 line (from the prefetch cache) instead of the queryset with the filters applied.

We ended up using a custom prefetch (no the basic prefetch_related("name_of_m2m")) to solve the issue: query.prefetch_related(Prefetch("name_of_m2m", queryset=Model.objects.filter(throughmodel__deleted__isnull=False)))