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

ManyToMany field is not updated correctly #202

Open
PLNech opened this issue Apr 24, 2017 · 6 comments
Open

ManyToMany field is not updated correctly #202

PLNech opened this issue Apr 24, 2017 · 6 comments
Labels
Milestone

Comments

@PLNech
Copy link
Member

PLNech commented Apr 24, 2017

From a customer's e-mail:

Context

My project has a kind of scenario where my Django app has a model called Package and another called Author. My Package model on Django is structured so that it has a ManyToMany relation field with the Author model, so that one Package can have many Authors.

As I want to store the list of the Authors names on every Package record, I've made a method authors_names of the Package model class, that returns the list of Authors related to a Package ( simply like return list(self.authors.values_list('full_name', flat=True))), and so then I've put it on my fields list attribute of the class for the Algolia's Package Index.

In the Index fields list I've got other values related to my Package model, both 'direct' fields and others callable methods.
So, using the 'algolia_reindex' as shown here brings every Package objects I've got on my Django database to Algolia.

Problem

The problem I've encountered, comes to the update of objects in Django:

  1. Go to a page of an existing Package object on the Django admin, make some edits to fields and for example add an author on the many to many Author field of the Package, then save
  2. The post_save signal of Algolia is called to update the data on Algolia, but the record is not correctly updated: every edited 'direct' field of the Package is correctly updated, but the 'callable' fields referrer of a method of the Package class are updated before the Package's save itself, so their update is done using the old data (in the example I don't see on the updated Algolia record the newly added Author name on the list for the Package record just edited).

So the fields listed with the 'direct' field name string are always handled correctly, but briefly the problem is that the fields indicated on the Index with a string that refer to the name of a model method, are not updated correctly on Algolia when the objects of the related model are updated on the Django database.

Potential solution

I think that the reason is that the Algolia client for Django hooks the objects update on the post_save signal of Django, but this doesn't work with the cases I've described last time, so to work, it should be hooked after the transaction is completed (after the commit), so instead of the post_save signal the implementation should be a pre_save signal that calls transaction.on_commit and then update the object on Algolia.

@PLNech PLNech changed the title ManyToMany fields is not updated correctly ManyToMany field is not updated correctly Apr 24, 2017
@PLNech PLNech self-assigned this Apr 24, 2017
@PLNech PLNech added the bug label Apr 24, 2017
@alexhayes
Copy link

Using Django 1.11.7 an approach, albeit not great one, for doing this is as follows;

import algoliasearch_django

class PackageAdmin(admin.ModelAdmin):

    class Meta:
        model = Package

    def response_add(self, request, obj: Package, post_url_continue=None):
        # Ensure we update Algolia's index once all many-to-many fields have been updated.
        algoliasearch_django.save_record(obj)

        return super().response_add(request=request, obj=obj, post_url_continue=post_url_continue)

    def response_change(self, request, obj: Package):
        # Ensure we update Algolia's index once all many-to-many fields have been updated.
        algoliasearch_django.save_record(obj)

        return super().response_change(request=request, obj=obj)

Note that you should also set AUTO_INDEXING to False and also set auto_indexing=False when registering your Package model.

You probably also want to pay special attention to Django's change log or have some good tests that ensure when you upgrade Django this continues to work.

@MaximeThoonsen
Copy link

MaximeThoonsen commented Jan 24, 2018

Thanks @alexhayes for your message ! I'll add our use case if it can help someone else.

We have a One-To-Many relation and I want to have everything in a single object. We have this class Benefits linked to another class named Vendors.

class Benefits(models.Model):
    id_vendor = models.ForeignKey(
        'Vendors', models.DO_NOTHING, related_name='benefits', db_column='id_vendor')

When we update a Benefit object in its admin, we can update the associated Vendors object with this:

def response_add(self, request, obj: Benefits, post_url_continue=None):
        # Ensure we update Algolia's index once all many-to-many fields have been updated.
        algoliasearch_django.save_record(obj.id_vendor)

        return super().response_add(request=request, obj=obj, post_url_continue=post_url_continue)

def response_change(self, request, obj: Benefits):
        # Ensure we update Algolia's index once all many-to-many fields have been updated.
        algoliasearch_django.save_record(obj.id_vendor)

        return super().response_change(request=request, obj=obj)

@qcaron
Copy link
Contributor

qcaron commented Jan 29, 2018

It worked like a charm for me. Only thing I changed is the obj: Package to obj (using Python 2.7, Django 1.8.18 so you know).

The AUTO_INDEXING = False should go in your project settings and auto_indexing=False should go in your apps.XXXAppConfig.ready method.

Maybe this update should be planned in a milestone or next release?

@julienbourdeau julienbourdeau added Bug and removed bug labels Mar 5, 2018
@PLNech PLNech added this to the minor milestone Mar 8, 2018
@PLNech
Copy link
Member Author

PLNech commented Mar 8, 2018

Hi there, sincere apologies for the slow reply.

That's a nice solution to this issue, with the same concern as @alexhayes that as it relies on behavior from a contrib class, we'll need to take extra care in making sure this is thoroughly tested.

I added this to the minor milestone, we'll address it in the next release. @julienbourdeau do you want to own this one to get started in the codebase? Unless one of the participants here want to send a PR and be credited as contributors 🙂

@soulshake
Copy link

Note that you should also set AUTO_INDEXING to False and also set auto_indexing=False when registering your Package model.

@alexhayes Can you elaborate on why this is necessary? Wouldn't that prevent the index being updated for normal object saves?

@alexhayes
Copy link

@soulshake sorry, I can't remember why I suggested setting those values like that and I'm no longer working at that company.

I've said when "registering the model" which makes me wonder if it should then be changed 🤔

@PLNech PLNech removed their assignment May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants