Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

handle none #92

Merged
merged 2 commits into from
Apr 10, 2015
Merged

handle none #92

merged 2 commits into from
Apr 10, 2015

Conversation

amitu
Copy link
Contributor

@amitu amitu commented Jan 11, 2015

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
Copy link

Coverage Status

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

@nemesifier
Copy link
Member

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
Copy link
Contributor Author

amitu commented Jan 14, 2015

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
Copy link

Coverage Status

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

@amitu
Copy link
Contributor Author

amitu commented Jan 17, 2015

@nemesisdesign any thoughts?

@nemesifier
Copy link
Member

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

@nemesifier
Copy link
Member

on which version of django bytheway?

@amitu
Copy link
Contributor Author

amitu commented Jan 21, 2015

I am using django 1.7.

I will try to create a patch containing tests.

@landscape-bot
Copy link

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

@aidanlister
Copy link
Contributor

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
Copy link
Member

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
Copy link

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 nemesifier added this to the 1.3.6 milestone Mar 31, 2015
@nemesifier nemesifier added the bug label Mar 31, 2015
@nemesifier nemesifier self-assigned this Apr 10, 2015
@nemesifier nemesifier merged commit bf7a81a into djangonauts:master Apr 10, 2015
nemesifier added a commit that referenced this pull request Apr 10, 2015
@nemesifier
Copy link
Member

test case here: 2c7d49d
thanks to all you guys 👍

nemesifier added a commit that referenced this pull request Apr 10, 2015
@amitu
Copy link
Contributor Author

amitu commented Apr 10, 2015

\o/ :-)

@amitu amitu deleted the handle_none branch April 10, 2015 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants