algolia / algoliasearch-django

Seamless integration of Algolia into your Django project.
https://www.algolia.com
MIT License
173 stars 65 forks source link

Support auto-indexing for proxy models #270

Closed browniebroke closed 3 years ago

browniebroke commented 6 years ago

Description

We started using algoliasearch-django recently and we had to compute some fields to be indexed. In our case, the current solution to pre-process fields wasn't suitable as there is a conflict in field names. The model to index already had method/property used in our Django app, and we wanted the same field in Algolia, in a different format.

To implement this, we used a proxy model that we register to our index.

This works pretty well, except for the auto-indexing. Algolia connects the signal receivers to listen for changes on our proxy model, but they never get fired, they are fired through on the base model.

This means we have to do this registration ourselves, which isn't a big deal, but I think this library could benefit from this feature.

Steps To Reproduce

Create a proxy model. This is inspired from the test suite, assuming UnregisteredWebsite is a model similar to Website:

class WebsiteProxy(UnregisteredWebsite):
    class Meta:
        proxy = True

Register it with Algolia:

algolia_engine.register(WebsiteProxy)

Calling save() or delete() on instances of UnregisteredWebsite do not trigger the auto-indexing behaviour.

Possible solutions I've thought of

  1. Add a new argument to the register method, for example: auto_index_model=None
  2. Add a new option to the proxy model Meta class, algolia_auto_index_model
  3. Infer it automatically using WebsiteProxy._meta.proxy and WebsiteProxy._meta.concrete_model

If one of the above is acceptable, I can try to put together a pull request.

Versions

julienbourdeau commented 6 years ago

Thank you @browniebroke for taking the time to write up this proposal.

I'm not a Django expert but from what I understand, proxy class should be reindexed automatically if the underlying model is modified. So I'm not really in favor of passing a new arg or option, your solution 3 seems to be the best but I'm not sure I understand the details.

@clemfromspace What do you think?

clemfromspace commented 6 years ago

Hi @browniebroke and sorry for the late answer,

The third option (Infer it automatically using WebsiteProxy._meta.proxy and WebsiteProxy._meta.concrete_model) looks like the best to me.

Happy to review your PR / help on this :)

tusharsrivastava commented 5 years ago

Hi @browniebroke Thank you very much for this code submission. I've implemented your logic in one of my projects and I'm afraid to say that it is not working. I updated the model and it did not sync as expected. I'm not sure what exactly is the problem here. Can you guide me through?

browniebroke commented 5 years ago

I'm afraid I stalled a bit on the pull request, unfortunately. I hit some blocks, and it might be more complicated than I initially thought. The third option doesn't seem doable after all.

It would be a huge help if I could get the CI to run @clemfromspace @julienbourdeau, I cannot get the tests to run locally and not having any sort of feedback is like coding blindfolded.

lggwettmann commented 5 years ago

Same problem here, but I'm not sure what to call from the original model's save and delete signals?

AlgoliaEngine.__post_save_receiver(instance) ?

lggwettmann commented 5 years ago

Is there anyone who can help me fix this or at least give a hint? Any help is appreciated.

tkrugg commented 3 years ago

closing stalled issue. Feel free to reopen if you want to further discuss the matter.