strawberry-graphql / strawberry-django

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

FileField Model validation doesn't work with default resolvers #564

Closed aprams closed 5 months ago

aprams commented 5 months ago

There are many ways to to validations, one of them is through a custom clean method override on the django model or on the model fields themselves. We have a use case, for a FileAttachment model, where we want to ensure that the file has certain properties. (Also we allow either a file to be attached or an external url to be set)

This used to work well, but with some of the "recent" changes (we're upgrading from ~0.22.0), this does not work anymore.

Example:

    def clean(self) -> None:
        if not bool(self.file) ^ bool(self.external_url):
            # Either both or none of file/external_url is set => error
            raise ValidationError(
                "Exactly one of file and external_url have to be set"
            )

This causes an issue with create mutations because the resolver code looks like this:

    # manager create overrides. This also ensures support for proxy-models.
    instance = model._default_manager.create(**create_kwargs)

    # Now that the instance has been created, go and assign
    # files and many2many fields.
    if files:
        for file_field, value in files:
            file_field.save_form_data(instance, value)
        instance.save()

Here, the model is saved first, and then the files are added. I understand the reasoning for doing this for M2M, but file fields could be set before the save() call, right?

System Information

Additional Context

Upvote & Fund

Fund with Polar

bellini666 commented 5 months ago

I think you are correct. This is probably a regression from https://github.com/strawberry-graphql/strawberry-django/pull/394

I just tested setting it directly and the tests I added in https://github.com/strawberry-graphql/strawberry-django/pull/464 are passing correctly, and it seems that it will not break proxy models as well. Will open a PR right now to fix that