Fix properties default value

[KOliver94]

As I can see the value of properties should always be a dictionary. For this reason we don't need to handle any falsy values just set it as an empty dictionary if it does not have a valid value.

Fixes #280

[auvipy]

can you please check the follow failure

  properties = getattr(request, 'properties') or {}

E AttributeError: 'Request' object has no attribute 'properties'

django_celery_results/backends/database.py:43: AttributeError
=========================== short test summary info ============================
FAILED t/unit/backends/test_database.py::test_DatabaseBackend::test_backend__pickle_serialization__dict_result
!!!!!!!!!!!!!!!!!!!!!!!!!! stopping after 1 failures !!!!!!!!!!!!!!!!!!!!!!!!!!!
========================= 1 failed, 7 passed in 1.03s ==========================

[auvipy]

@lvelvee can you also check it please?

[KOliver94]

Sorry, I forgot that getattr throws exception if the object does not have that attribute and no default value is provided. I re-added the default value so now it should work.

[lvelvee]

@KOliver94 could you please describe when the properties will be None?

For robustness, you can add some bounds checking statements.

[lvelvee]

By the way, let's see if properties = None will introduce more regressions.

[auvipy]

what about None or {}

[KOliver94]

I changed this field to reflect the problem and create a more edge-case like scenario when the properties field are present but with None value.

[KOliver94]

@lvelvee I don't exactly know why are the properties None. I noticed this problem while running tests in my project. For example I have a test in backend/tests/no_recaptcha_pytest.py

@override_settings(CELERY_TASK_ALWAYS_EAGER=True)
@conditional_override_settings(
    EMAIL_BACKEND="tests.helpers.test_utils.CombinedEmailBackend", CONDITION=EMAIL_FILE
)
@pytest.mark.django_db
def test_contact_message_email_sent_without_captcha(api_client, disable_recaptcha):
    data = {
        "name": "Joe Bloggs",
        "email": "joe@example.com",
        "message": "Lorem ipsum dolor sit amet, consectetur adipiscing elit. Integer posuere tempus nibh et lobortis.",
    }
    response = api_client.post("/api/v1/misc/contact", data)
    assert response.status_code == status.HTTP_201_CREATED

This test calls a Django REST Framework endpoint with a POST call which validates the data and calls the email_contact_message function.

    def perform_create(self, serializer):
        name = serializer.data["name"]
        email = serializer.data["email"]
        message = serializer.data["message"]
        email_contact_message.delay(name, email, message)

@shared_task
def email_contact_message(name, email, message):
    context = {
        "name": name,
        "message": message,
    }
    msg_plain = render_to_string("email/txt/contact_message.txt", context)
    msg_html = render_to_string("email/html/contact_message.html", context)
    subject = "Kapcsolatfelvétel | Budavári Schönherz Stúdió"

    msg = (
        EmailMultiAlternatives(
            subject=subject,
            body=msg_plain,
            to=[email],
            cc=[settings.DEFAULT_REPLY_EMAIL],
            reply_to=[settings.DEFAULT_REPLY_EMAIL],
        )
        if not settings.DEBUG_EMAIL
        else debug_email(subject, msg_plain)
    )

    msg.attach_alternative(msg_html, "text/html")
    msg.send()
    return f"Contact message from {name} was sent successfully."

After this function got executed django-celery-results tries to save it but the request looks like this

<Context: {'id': 'dcb1bfb5-c030-4af7-ab8f-fd4ba03b8c7b', 'retries': 0, 'is_eager': True, 'logfile': None, 'loglevel': 0, 'hostname': 'Oliver-Notebook', 'callbacks': None, 'errbacks': None, 'headers': None, 'ignore_result': False, 'delivery_info': {'is_eager': True, 'exchange': None, 'routing_key': None, 'priority': None}, 'args': ['Joe Bloggs', 'joe@example.com', 'Lorem ipsum dolor sit amet, consectetur adipiscing elit. Integer posuere tempus nibh et lobortis.'], 'called_directly': False, 'kwargs': {}, '_children': []}>

According to the PyCharm debugger there are properties field in the object with None value and for this reason the following exception is thrown:

task_id = 'dcb1bfb5-c030-4af7-ab8f-fd4ba03b8c7b'
result = '"Contact message from Joe Bloggs was sent successfully."'
status = 'SUCCESS', traceback = None
request = <Context: {'id': 'dcb1bfb5-c030-4af7-ab8f-fd4ba03b8c7b', 'retries': 0, 'is_eager': True, 'logfile': None, 'loglevel': ... adipiscing elit. Integer posuere tempus nibh et lobortis.'], 'called_directly': False, 'kwargs': {}, '_children': []}>
using = None

    def _store_result(
            self,
            task_id,
            result,
            status,
            traceback=None,
            request=None,
            using=None
    ):
        """Store return value and status of an executed task."""
        content_type, content_encoding, result = self.encode_content(result)
        _, _, meta = self.encode_content(
            {'children': self.current_task_children(request)}
        )
    
        task_name = getattr(request, 'task', None)
        properties = getattr(request, 'properties', {})
>       periodic_task_name = properties.get('periodic_task_name', None)
E       AttributeError: 'NoneType' object has no attribute 'get'

[lvelvee]

Could you please trim the code to a Minimal Reproducible Example with version-locked for locating the reason better?

[KOliver94]

@lvelvee Here is my small demo: https://github.com/KOliver94/django-celery-results-bug-demo. Hope it helps.

There is only one test under polls/tests.py which should pass but fails with the issue I mentioned.

[lvelvee]

properties will be None if CELERY_ALWAYS_EAGER is True @KOliver94

[KOliver94]

Good to know, thanks @lvelvee. However, it is still an unhandled exception in this package, am I right?

[KOliver94]

Hi @auvipy and @xirdneh,

Should I change anything else or is this PR looking good for you?

Thanks!

[auvipy]

please add yourself to author list & have a release note entry for the change made

[KOliver94]

Hi @auvipy,

What do you mean by the release note entry? Should I add it as an unreleased version to the Changelog file?

[auvipy]

yes exactly like celery/django-celery-beat@d9ba432#diff-ead07c84baac57a9542f388a07a2a5209456ce790b04251bc9bd7d179ea85cb1R7

[KOliver94]

I checked it and I don't think a release note would be needed for this case because this issue was introduced by #261 which is an unreleased feature. If you would like I can add it anyway but in my opinion it would be strange to have an entry in the release note regarding a bug fix when the bug was not present in the last version.

[auvipy]

make sense