added guard for null 'options' to fix crash (#1163)

[matthewhegarty]

Problem

If the forbiddenfruit library is installed, then crashes are seen at object instantiation.

The cause is that when forbiddenfruit is installed, calling dir() on a null object returns additional properties. The logic attempts to get these properties from a null options reference, which causes the crash:

'NoneType' object has no attribute 'attrs' 

#1163

Solution

This fix adds a check that options is not None.

Acceptance Criteria

• Ran tests with forbiddenfruit installed
• A test which simulates the problem is included.
• Have tested Admin console manually.

[coveralls]

Coverage remained the same at 98.063% when pulling 7a6ecb7 on matthewhegarty:tests_fail_with_forbiddenfruit_imported_issue_1163 into 89e5240 on django-import-export:main.

[matthewhegarty]

@manelclos please could you cast your eye over this change - this would be the last one to go into 2.6.0

[manelclos]

I don't understand this change, as dir(options) will not return None even when options is None.

Removed the code modifications and the tests passes with no problems.

Tried other tests to understand when it fails and why, but they all pass:

    @mock.patch("builtins.dir")
    def test_new_handles_null_options(self, mock_dir):
        # #1163 - simulates a call to dir() returning additional attributes
        mock_dir.return_value = None
        MyResource()

    @mock.patch("builtins.dir")
    def test_new_handles_null_options(self, mock_dir):
        # #1163 - simulates a call to dir() returning additional attributes
        mock_dir.return_value = []
        MyResource()

[matthewhegarty]

Hi Manel, thanks for reviewing.

The issue only occurs when the forbiddenfruit library is installed. If it is installed, a call to dir(None) returns extra attributes (eg 'attrs') which means that the option.startswith('_') check fails, then the crash occurs because there is a call to setattr(meta, option, getattr(options, option)), with getattr() being called with a null options reference.

The test has to simulate returning values from dir(None) which don't start with a leading underscore, because this is what forbiddenfruit is doing.

Removed the code modifications and the tests passes with no problems.

It will be fine unless you install forbiddenfruit, because the issue only occurs in conjunction with that. It's easy to reproduce.

[manelclos]

Hi Matthew, I see. I spent some more time checking and found that you need a base class to trigger the code we want to test, something like this does the trick:

    @mock.patch("builtins.dir")
    def test_new_handles_null_options(self, mock_dir):
        # #1163 - simulates a call to dir() returning additional attributes
        mock_dir.return_value = ['attrs']
        class A(MyResource):
            pass

        A()

Then, options is not Falsy, so the code needs to be changed to something like this:

                for option in [option for option in dir(options)
                               if not option.startswith('_') and hasattr(options, option)]:

Once I made these changes it makes sense.

Let me know what you think.

[matthewhegarty]

Looks good - thank you for those suggestions - I've added that in.
I tested as well with the forbiddenfruit lib installed.

[manelclos]

LGTM