djangonauts / django-hstore

PostgreSQL HStore support for Django.
http://django-hstore.readthedocs.io/
Other
517 stars 142 forks source link

handle none #92

Closed amitu closed 9 years ago

amitu commented 9 years ago

My migrations contains:

        migrations.AddField(
            model_name='author',
            name='data',
            field=django_hstore.fields.DictionaryField(null=True, editable=False),
            preserve_default=True,
        ),

Not sure if this is the proper fix, or if I should create the field non null with default value of empty dictionaty.

I think I followed the instructions.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.03%) when pulling 9ac3f0349ffbbb32e98036a9fdf98d5a8f0186cd on amitu:handle_none into c2714b7b66f2650ce0e5af17229e40a1d96f3edb on djangonauts:master.

nemesifier commented 9 years ago

What bug are you trying to solve? I'm not aware of a bug that has to do with None values, so if there is any, how can I reproduce it?

amitu commented 9 years ago

I am getting "'NoneType' object does not support item assignment" in a few places. When trying to save for example on a model with hstore field.

I have allowed null on it.

    data        = hstore.DictionaryField(
        schema=[
            {
                "name": "wikipedia",
                "class": "URLField",
                "kwargs": {"blank": True}
            },
            {
                "name": "homepage",
                "class": "URLField",
                "kwargs": {"blank": True}
            },
        ]
    )

The migration I posted in description shows that null is allowed.

When I edit in admin, set in django-hstore/django_hstore/virtual.py is getting called.

 def __set__(self, instance, value):
        """
        set value on hstore dictionary
        """
        hstore_dictionary = getattr(instance, self.hstore_field_name)
        hstore_dictionary[self.name] = value 

As you can see there is no None handling. I handled None in one place, am going to handle it here too.

Let me know if I am doing something wrong to have all these nulls in DB to begin with. Else we should handle nulls like I am doing.

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-0.19%) when pulling bf7a81a67491b98793504a7638103bdf74182e39 on amitu:handle_none into c2714b7b66f2650ce0e5af17229e40a1d96f3edb on djangonauts:master.

amitu commented 9 years ago

@nemesisdesign any thoughts?

nemesifier commented 9 years ago

I think this patch needs a test to replicate the bug in the automated test environment and then apply the patch.

nemesifier commented 9 years ago

on which version of django bytheway?

amitu commented 9 years ago

I am using django 1.7.

I will try to create a patch containing tests.

landscape-bot commented 9 years ago

Code Health Repository health increased by 6% when pulling bf7a81a on amitu:handle_none into c2714b7 on djangonauts:master.

aidanlister commented 9 years ago

I'm seeing the same issue -- I swapped from blank=True to null=True,blank=True to solve another error, and then this error started appearing on fields without data in them.

nemesifier commented 9 years ago

I tried altering SchemaDataBag in tests.django_hstore_tests.tests the following way:

class SchemaDataBag(HStoreModel):
        name = models.CharField(max_length=32)
        data = hstore.DictionaryField(null=True, blank=True, schema=[
            {
                'name': 'number',
                'class': 'IntegerField',
                'kwargs': {
                    'default': 0
                }
            }
            # ... [cut] ...
      ])

Then ran the tests, all passed. Then tried some interactive shell, I tried to set the value of number, tried full_clean and then save, no issue found.

How can I reproduce the issue you are experiencing? I can't merge code that I do not understand in the first place. If there is a bug, I should be able to reproduce it and then apply your fix.

Even if you don't have time to write a test case, just write here the general steps.

Let me know.

Federico

thibault commented 9 years ago

I ran into this issue, and found a way to reproduce it.

I just created a new blank django project to test this issue. Here is my requirements:

Django==1.7.7
django-hstore==1.3.5
psycopg2==2.6

Then I created a stupid model:

class Document(models.Model):
    title = models.CharField(max_length=255)
    data = hstore.DictionaryField(null=True, blank=True, schema=[
        {
            'name': 'number',
            'class': 'IntegerField',
            'kwargs': {
                'default': 0
            }
        },
        {
            'name': 'float',
            'class': 'FloatField',
            'kwargs': {
                'default': 1.0
            }
        }
    ])

In a shell, I do this:

>>> from documents.models import Document
>>> doc = Document()
>>> doc.title = 'test'
>>> doc.data = None
>>> doc.save()

Now, if I try to visualize this document in the admin module, I've got this exception:

'NoneType' object has no attribute 'get'

I met the issue in my real project because the "data" field was added through a migration, and the values were defaulted to "None".

Applying the patch in this pull request fixes the issue.

Hope that helps.

nemesifier commented 9 years ago

test case here: 2c7d49d thanks to all you guys :+1:

amitu commented 9 years ago

\o/ :-)