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

Add m2m_changed signal #26

Closed
KengoWada opened this issue Oct 22, 2023 · 6 comments · Fixed by #30, #44 or #48
Closed

Add m2m_changed signal #26

KengoWada opened this issue Oct 22, 2023 · 6 comments · Fixed by #30, #44 or #48
Assignees
Labels
bug Something isn't working

Comments

@KengoWada
Copy link
Collaborator

KengoWada commented Oct 22, 2023

Problem Statement

Take the following models:

# models.py
class Artist(models.Model):
    name = models.CharField(max_length=100)
    ...

class Song(TypesenseModelMixin)
    ...
    artists = models.ManyToManyField(Artist)
    collection_class = SongCollection

    def artists_names(self):
        return list(self.artists.all().values_list('name', flat=True))

When a new song is created and then an artist is added, it does not save the artist's name to TypeSense and when querying it returns an empty list.

artist = Artist.objects.create(name='J Cole')
song = Song.objects.create(name='Fire Squad')
song.artists.add(artist)

In this case, only the post_save_typesense_models is hit after the creation of the song which has an empty list of artists and after the artist is added(or removed) to the song it doesn't reflect in TypeSense.

Solution

  • Add a m2m_changed signal
  • Consider the post_add, post_remove and post_clear actions
@EricOuma EricOuma added the bug Something isn't working label Oct 25, 2023
@KengoWada
Copy link
Collaborator Author

Hey @EricOuma please assign this to me. Thanks

KengoWada added a commit to KengoWada/django-typesense that referenced this issue Nov 2, 2023
KengoWada added a commit to KengoWada/django-typesense that referenced this issue Nov 2, 2023
KengoWada added a commit to KengoWada/django-typesense that referenced this issue Nov 2, 2023
KengoWada added a commit to KengoWada/django-typesense that referenced this issue Nov 2, 2023
EricOuma pushed a commit that referenced this issue Nov 7, 2023
* #26 Added m2m_changed signal for django typesense models.

* #26 Added tests for django_typesense signals.

* #26 Added get_document helper.

* #26 Updated to use utils get_document.
@EricOuma
Copy link
Contributor

@KengoWada There is some problem with the implementation of this. Here is how to reproduce:

class Library(models.Model)
    songs = models.ManyToManyField(Song)

This will eventually try calling Library.get_collection(instance).update()

Your fix might look like this:

@receiver(m2m_changed)
def m2m_changed_typesense_models(instance, model, action, reverse, **kwargs):
    if action in ["post_add", "post_remove", "post_clear"]:
        if reverse and issubclass(model, TypesenseModelMixin):
            pk_set = list(kwargs.get("pk_set"))
            obj = model.objects.filter(pk__in=pk_set)
            model.get_collection(obj=obj, many=True).update()
        else:
            if isinstance(instance, TypesenseModelMixin):
                instance_class = instance.__class__
                instance_class.get_collection(instance).update()

@EricOuma EricOuma reopened this Nov 17, 2023
@EricOuma
Copy link
Contributor

@KengoWada It's a bit urgent so let me know if you are able to work on it today

@KengoWada
Copy link
Collaborator Author

@EricOuma let me work on it now.

KengoWada added a commit to KengoWada/django-typesense that referenced this issue Nov 17, 2023
EricOuma pushed a commit that referenced this issue Nov 18, 2023
@EricOuma EricOuma reopened this Nov 20, 2023
@EricOuma
Copy link
Contributor

EricOuma commented Nov 20, 2023

@KengoWada Did you try testing with the sample model I gave?

library.songs.add(...) does not work as expected

class Song(TypesenseModelMixin):
     ...
    @property
    def library_ids(self):
        return list(self.libraries.values_list('id', flat=True))
class Library(models.Model)
    songs = models.ManyToManyField(Song, related_name='libraries')
class SongCollection(TypesenseCollection):
    ...
    library_ids = fields.TypesenseArrayField(base_field=fields.TypesenseSmallIntegerField(), value='library_ids')

@KengoWada
Copy link
Collaborator Author

Hey @EricOuma I only run the existing tests. I have seen the issue. Let me work on a fix.

KengoWada added a commit to KengoWada/django-typesense that referenced this issue Nov 20, 2023
KengoWada added a commit to KengoWada/django-typesense that referenced this issue Nov 20, 2023
KengoWada added a commit to KengoWada/django-typesense that referenced this issue Nov 20, 2023
KengoWada added a commit to KengoWada/django-typesense that referenced this issue Nov 20, 2023
KengoWada added a commit to KengoWada/django-typesense that referenced this issue Nov 20, 2023
KengoWada added a commit to KengoWada/django-typesense that referenced this issue Nov 20, 2023
KengoWada added a commit to KengoWada/django-typesense that referenced this issue Nov 20, 2023
KengoWada added a commit to KengoWada/django-typesense that referenced this issue Nov 20, 2023
EricOuma pushed a commit that referenced this issue Nov 20, 2023
* #26 Removed check for reversed.

* #26 Added library_ids to SongCollection

* #26 Added library model and property for library_ids

* #26 Updated m2m_changed tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants