strawberry-graphql / strawberry-django

Strawberry GraphQL Django extension
https://strawberry.rocks/docs/django
MIT License
421 stars 122 forks source link

Setting value to False on instances of models.FileField causes "False" to be saved to the database instead of None when creating new records. #599

Closed m4riok closed 4 months ago

m4riok commented 4 months ago

Hi @bellini666,

I came across a bit of a bug with creating new records of a model with a FileField. It seems to instert False into the FileField db record instead of NULL when creating a new record.

I tracked down the bug to the following code ...

In mutations/resolvers.py on line 250 you have the following code:

if field is None or value is UNSET:
    # Dont use these, fallback to model defaults.
     direct_field_value = False
elif isinstance(field, models.FileField):
     if value is None:
         # We want to reset the file field value when None was passed in the
         # input, but `FileField.save_form_data` ignores None values. In that
         # case we manually pass False which clears the file.
          value = False  # noqa: PLW2901

Django in db/models/fields/files.py in the definition of the FileField class has the following:

def save_form_data(self, instance, data):
    # Important: None means "no change", other false value means "clear"
    # This subtle distinction (rather than a more explicit marker) is
    # needed because we need to consume values that are also sane for a
    # regular (non Model-) Form to find in its cleaned_data dictionary.
    if data is not None:
        # This value will be converted to str and stored in the
        # database, so leaving False as-is is not acceptable.
        setattr(instance, self.name, data or "")

This works as intended when updating an instance, but when creating, it creates records like so:

MariaDB [beast]> select * from core_company;
+----+---------------+-------+---------+------+----------+------------------------------------------------------------------+-------+
| id | name          | email | address | city | postcode | logo                                                             | phone |
+----+---------------+-------+---------+------+----------+------------------------------------------------------------------+-------+
|  2 | company 1     | NULL  | NULL    | NULL | NULL     | uploaded/company_logos/cc625d08-864d-42ea-9edd-0fe6afb473ae.webp | NULL  |
|  3 | Company 2     | NULL  | NULL    | NULL | NULL     | uploaded/company_logos/3aa6b13c-a335-4052-8c4a-98e8d662b73c.webp | NULL  |
|  4 | Karamitsos AE | NULL  | NULL    | NULL | NULL     |                                                                  | NULL  |
| 48 | skata         | NULL  | NULL    | NULL | NULL     | False                                                            | NULL  |
| 49 | Skata1        | NULL  | NULL    | NULL | NULL     | False                                                            | NULL  |
| 68 | fefe          | NULL  | NULL    | NULL | NULL     | uploaded/company_logos/66f6c3ba-de8c-49f7-ba80-520c7868731b.webp | NULL  |
+----+---------------+-------+---------+------+----------+------------------------------------------------------------------+-------+

Saving the False value assigned to the logo field to the newly inserted record.

Upvote & Fund

Fund with Polar

bellini666 commented 4 months ago

Thank you for the detailed description. I think I found the issue :)

Let me know if the next release solves if for you

m4riok commented 4 months ago

Thanks for the quick fix. It works fine now.