Opened 8 years ago

Closed 8 years ago

#26120 closed Cleanup/optimization (fixed)

Make HStoreField cast its key/values to strings

Reported by: Przemek Owned by: Greg Chapple
Component: contrib.postgres Version: 1.8
Severity: Normal Keywords:
Cc: Marc Tamlyn Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Suppose I have a simple model:

class Simple(models.Model):
    data = HStoreField(default={})

I can save hstore with mixed types, which is normal:

    simple = Simple.objects.create(data = {'a': 1, 'b': 'B'})

But the update function throws an exception:

   Simple.objects.filter(id=simple.id).update(data={'a': 1, 'b': 'B'})
Traceback (most recent call last):
  File "failing.py", line 19, in <module>
    show_error()
  File "failing.py", line 11, in show_error
    simple = Simple.objects.create(data={'a': 1, 'b': 'B'})
  File "/usr/local/lib/python2.7/dist-packages/django/db/models/manager.py", line 127, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/django/db/models/query.py", line 348, in create
    obj.save(force_insert=True, using=self.db)
  File "/usr/local/lib/python2.7/dist-packages/django/db/models/base.py", line 734, in save
    force_update=force_update, update_fields=update_fields)
  File "/usr/local/lib/python2.7/dist-packages/django/db/models/base.py", line 762, in save_base
    updated = self._save_table(raw, cls, force_insert, force_update, using, update_fields)
  File "/usr/local/lib/python2.7/dist-packages/django/db/models/base.py", line 846, in _save_table
    result = self._do_insert(cls._base_manager, using, fields, update_pk, raw)
  File "/usr/local/lib/python2.7/dist-packages/django/db/models/base.py", line 885, in _do_insert
    using=using, raw=raw)
  File "/usr/local/lib/python2.7/dist-packages/django/db/models/manager.py", line 127, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/django/db/models/query.py", line 920, in _insert
    return query.get_compiler(using=using).execute_sql(return_id)
  File "/usr/local/lib/python2.7/dist-packages/django/db/models/sql/compiler.py", line 974, in execute_sql
    cursor.execute(sql, params)
  File "/usr/local/lib/python2.7/dist-packages/django/db/backends/utils.py", line 79, in execute
    return super(CursorDebugWrapper, self).execute(sql, params)
  File "/usr/local/lib/python2.7/dist-packages/django/db/backends/utils.py", line 64, in execute
    return self.cursor.execute(sql, params)
  File "/usr/local/lib/python2.7/dist-packages/django/db/utils.py", line 98, in __exit__
    six.reraise(dj_exc_type, dj_exc_value, traceback)
  File "/usr/local/lib/python2.7/dist-packages/django/db/backends/utils.py", line 64, in execute
    return self.cursor.execute(sql, params)
DataError: invalid input syntax for integer: "B"
LINE 1: ...("data") VALUES (hstore(ARRAY['a', 'b'], ARRAY[1, 'B'])) RET...

See the toy project at https://github.com/CGenie/django_hstore_field_fail

This fails for 1.8 as well as for 1.9.

Change History (11)

comment:1 by Tim Graham, 8 years ago

Cc: Marc Tamlyn added
Severity: Release blockerNormal
Type: UncategorizedCleanup/optimization
Version: 1.91.8

The documentation says, "A field for storing mappings of strings to strings." I'm not sure if any change should be made, but doesn't qualify as a release blocker in my opinion.

comment:2 by Marc Tamlyn, 8 years ago

Yes, it's not a release blocker. Interestingly, I didn't get the same behaviour here, I got errors for any direct ORM usage with bad typing - the Form layer will tidy it up for you, but the model layer doesn't seem to.

However you do get more sensible behaviour from CharField for example here. It's a pretty straightforwards fix I think, should just need to add a get_prep_value method to the field.

comment:3 by Tim Graham, 8 years ago

What should be done in that method? Casting all keys/values in the dictionary to a string or raising an error for non-strings? The second option seems safer to me in terms of hiding possible developer error.

comment:4 by Marc Tamlyn, 8 years ago

The former actually, that's what most fields do - You can pass '1' to an IntegerField and 1 to a CharField and they'll cast it appropriately. This casting is actually pretty useful for more complex fields often - you can pass '2015-01-01' to a DateField rather than having to pass a datetime instance. The developer error point is a fair one, but consistency with the rest of the ORM is more important.

comment:5 by Tim Graham, 8 years ago

Summary: HStoreField update error when values of different typesMake HStoreField cast its key/values to strings
Triage Stage: UnreviewedAccepted

comment:6 by Przemek, 8 years ago

PostgreSQL supports arbitrary hstore values. Casting values to strings is a bad idea imho. Since the HStoreField is added to expose PostgreSQL-specific functionality, it should be supported fully, not partially with hacks around Python code. Especially that setting values one by one works fine.

in reply to:  6 comment:7 by Simon Charette, 8 years ago

Replying to CGenie:

PostgreSQL supports arbitrary hstore values. Casting values to strings is a bad idea imho. Since the HStoreField is added to expose PostgreSQL-specific functionality, it should be supported fully, not partially with hacks around Python code. Especially that setting values one by one works fine.

PostgreSQL will cast values to text anyway:

This module implements the hstore data type for storing sets of key/value pairs within a single PostgreSQL value. This can be useful in various scenarios, such as rows with many attributes that are rarely examined, or semi-structured data. Keys and values are simply text strings.

comment:8 by Przemek, 8 years ago

Ah, OK, sorry, my bad, didn't read thoroughly enough :P

comment:9 by Greg Chapple, 8 years ago

Owner: set to Greg Chapple
Status: newassigned

comment:10 by Greg Chapple, 8 years ago

Has patch: set

Added a pull request for this issue. Let me know your comments.

comment:11 by Tim Graham <timograham@…>, 8 years ago

Resolution: fixed
Status: assignedclosed

In 8dea9f0:

Fixed #26120 -- Made HStoreField cast keys and values to strings.

HStoreField now converts all keys and values to string before they're
saved to the database.

Note: See TracTickets for help on using tickets.
Back to Top