ChristopherRabotin / bungiesearch

UNMAINTAINED CODE -- Elasticsearch-dsl-py django wrapper with mapping generator
BSD 3-Clause "New" or "Revised" License
67 stars 20 forks source link

Fix id_fields issue #77

Closed afrancis13 closed 9 years ago

afrancis13 commented 9 years ago

I was working with bungiesearch yesterday and I noticed that I was having a lot of trouble with models that had defined primary keys (models in which the pk field was automatically generated as 'id'). So I went through the code and wrote a few tests and realized that bungiesearch wouldn't pass the tests. I included the additional tests (you can move one commit backwards in Travis to see them failing) so you can see the issue. Please let me know if you have any questions. Also, if you confirm this makes sense, and that there isn't a simple workaround that I'm missing, let me know if I should include or get rid of the additional testing, at which point I'll push and fixup the commits.

ChristopherRabotin commented 9 years ago

Good stuff! I hadn't had actually noticed that problem since we've only used id as a primary key. There are a few styles changes I'll make so I'll merge it in for now, do some changes on another branch and get your input prior to merging those changes in.

afrancis13 commented 9 years ago

Great! Another thing I noticed when I was going through this is that if your primary key is a field that's a relation (i.e. models.OneToOneField, see example in Django docs below), bungiesearch won't work. Is there a reason you're filtering relation fields out in the _get_fields method in indices.py? https://docs.djangoproject.com/en/1.8/topics/db/examples/one_to_one/

ChristopherRabotin commented 9 years ago

@afrancis13 Yes, it isn't supported because I wasn't sure how to handle potential conflicts. Even if Django has that One to One relation, I worried that any table rows added manually may lead to an erroneous index.

Any thoughts on implementing this?

afrancis13 commented 9 years ago

@ChristopherRabotin Thanks for the response, I'll look into this when I get the chance.