fopina / django-bulk-update-or-create

`bulk_update_or_create` for Django model managers
MIT License
148 stars 16 forks source link

KeyError when updating (ok when creating) #11

Closed kicks66 closed 3 years ago

kicks66 commented 3 years ago

Hi, love this library - exactly what I've been looking for!

However, I've just started implementing and getting the following error:

Request Method: | GET
-- | --
http://localhost:8000/perk/followers/1234
3.1.6
KeyError
**54321**
/Users/user/GitHub/talent-tutorial/venv/lib/python3.8/site-packages/bulk_update_or_create/query.py, line 113, in __bulk_update_or_create
/Users/user/GitHub/talent-tutorial/venv/bin/python
3.8.5
['/Users/user/GitHub/talent-tutorial/talenttutorial', 
'/Library/Frameworks/Python.framework/Versions/3.8/lib/python38.zip', 
'/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8', 
'/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/lib-dynload',
 '/Users/user/GitHub/talent-tutorial/venv/lib/python3.8/site-packages']

54321 is the PK of the user that should be updated in the DB, and looks to be the first update that occurs. When I run this library with a clean database I get no errors.

christianj6 commented 3 years ago

Hi. I was receiving a similar error until I inspected the contents of the obj_map dict created in line 108 of query.py.

{4862336: ... }

It seems the values I intended as varchar for the match field (which, as in your example, produced errors when entirely numeric) were cast to integers before they were assigned to the Django objects passed to the bulk_update_or_create method. I was able to resolve this by simply casting the values to strings before creating the objects.

items = [
    RandomData(uuid=str(54321), data='data for 1'),
    ...
]
RandomData.objects.bulk_update_or_create(items, ['data'], match_field='uuid')

In your case then, assuming your PK are numeric, I would try casting to int when creating the objects.

fopina commented 3 years ago

hey, sorry for the delay, could any of you write a simple test to reproduce this?

case_insensitive_match might cause unexpected behavior but it shouldn't affect int fields

fopina commented 3 years ago

I was able to reproduce this earlier today. django fields (such as IntegerField) coerce types for the DB backend in use, so str is cast to int when sent in the SQL query.

but obj_map is built with the original value in the model instance, not the coerced value, so it keeps the str value. When matching with the returned objects (with proper values out of the db), they do not match ('123' != 123)

not sure how to implement this in a generic way as that might need inspecting the model and coercing the types the same way the fields do... (maybe calling prepare_for_db...?).

Or maybe just adding a warning for now (when obj should be in obj_map but it's not) already helps and calling code should make sure they cast types properly..

fopina commented 3 years ago

Fixed it in #15 thanks for raising it.

Do let me have feedback if you re-test it.

christianj6 commented 3 years ago

Hi, thanks for addressing this so swiftly. Your solution using to_python and value_from_object seems an elegant way to avoid such type incompatibilities as we were experiencing. It works on my original example. Cheers.