lazybird / django-carton

A simple and lightweight application for shopping carts and wish lists.
Other
274 stars 101 forks source link

KeyError when adding item to cart #13

Closed sect2k closed 10 years ago

sect2k commented 10 years ago

After an item is added to cart, KeyError is raised on subsequent pages. This is because keys in cart_representation dictionary are strings and if product.pk is a number, the key is not found in the dict.

The fix is simple, change

 item = cart_representation[product.pk]

to

 item = cart_representation[str(product.pk)]

Pushed a fix.

lazybird commented 10 years ago

I'm not sure why we need to cast product.pk to a string. It seems to me that the key in cart_representation is already a numeric.

In fact I actually get a key error when I use item = cart_representation[str(product.pk)].

For now, I will revert the change - since it break tests and it's committed on master.

sect2k, I'm curious to understand how you ran into this issue and how come you had to cast the key into a string.

Thanks.

sect2k commented 10 years ago

Not sure what exactly causes this issue, but the behavior is strange, as sometimes the key in cart_representation is a string and other times it's a number.

In my case, I started getting the error I described in OP, then it worked fine for a while after I cast the key into string and then a few days a go I started getting KeyError again, this time because the key was again a number.

My solution was to cast also explicitly cast the key to string when creating cart_representation, this way the key is always a string, and since string is more versatile then number (some products might actually have a string for a primary key) I believe this is the best fix for the issue.

Do to a busy schedule this week, I sort of forgot to push the fix to github.

lazybird commented 10 years ago

I'm wondering if the issue on your side could come from old cached data. I would suggest to keep the key to product.pk without casting it, both when storing the data in the session and also when retrieving it.

lazybird commented 10 years ago

Looking again at the way you've described the inconsistencies, it's possible that the issue on your end is at product model's level... May be something to with pk field.

sect2k commented 10 years ago

I doubt it's cached data and it's not my product pk, it's a standard django auto id, more likely it has to do with JSON decoding/decoding, since session data is JSON serialized.

Might be some inconsistencies in how handles numbers as keys, since in JSON keys are always converted to strings and from a quick experiment in python shell:

d = {123: 'Product'}
j = json.dumps(d)
d2 = json.loads(j)
d2
{u'123':  u'Product'}

Numeric key from d gets converted to unicode string in d2.

I still think the safest option is to cast the key to string explicitly as is seems that JSON serialization does not produce 100% consistent results. Best to be on the safe side.

lazybird commented 10 years ago

I have made a change to use string instead of numeric for the dict key before going thought JSON serialization.

Thanks flagging that issue and being patient and precise on the discussion.