Skip to content

Fix raising on nullable fields part of UniqueConstraint #9531

New issue

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

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

Already on GitHub? # to your account

Merged
merged 5 commits into from
Dec 14, 2024

Conversation

browniebroke
Copy link
Member

@browniebroke browniebroke commented Sep 11, 2024

Description

Fix #9378

  • Add a few failing tests
  • Fix cause of the problem

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any progress here?

@browniebroke
Copy link
Member Author

any progress here?

Not really, I've dropped the ball a bit on this one. Will try to make some progress...

@browniebroke browniebroke marked this pull request as ready for review October 14, 2024 18:14
@browniebroke
Copy link
Member Author

Oh well, that's all passing now... Hopefully that covers the main use case reported in #9378

There is also the other semi-related bug #9358 which is NOT in scope of this PR, I believe it will be fixed by #9360

@SorianoMarmol
Copy link

Is a new version expected with this fix and others related like #9360 ? The support for UniqueConstraint is not complete and may cause issues…

Copy link
Member

@pauloxnet pauloxnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@auvipy auvipy merged commit a8595a8 into encode:master Dec 14, 2024
8 checks passed
@auvipy
Copy link
Member

auvipy commented Dec 14, 2024

thanks!

@auvipy auvipy added the Bug label Dec 14, 2024
@browniebroke browniebroke deleted the nullable-unique-constraint-fields branch December 14, 2024 10:58
@browniebroke browniebroke added this to the 3.16 milestone Jan 16, 2025
@mdellweg
Copy link

mdellweg commented Apr 4, 2025

I'm not entirely sure under which circumstances, but this change turns a SerializerMethodField() whose name appears in the uniqueness constraint of the underlying model into a HiddenField(default=None).

I'll keep investigating.

@mdellweg
Copy link

mdellweg commented Apr 7, 2025

Here is a (minimal???) reproducer:

def test_tba():
    class TestModel(models.Model):
        field_1 = models.IntegerField(null=True)
        field_2 = models.IntegerField(null=True)

        class Meta:
            unique_together = (("field_1", "field_2"),)

    class TestSerializer(serializers.ModelSerializer):
        field_1 = serializers.SerializerMethodField()

        def get_field_1(self) -> str:
            return "TEST"

        class Meta:
            model = TestModel
            fields = ["field_1", "field_2"]

    fields = TestSerializer().fields
    assert isinstance(fields["field_1"], serializers.SerializerMethodField)
    assert isinstance(fields["field_1"], serializers.SerializerMethodField)
E   AssertionError: assert False
E    +  where False = isinstance(HiddenField(default=None), <class 'rest_framework.fields.SerializerMethodField'>)
E    +    where <class 'rest_framework.fields.SerializerMethodField'> = serializers.SerializerMethodField

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants