Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Fix unique together validator doesn't respect condition's fields #9360

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kalekseev
Copy link
Contributor

fixes #9358

else:
queryset = model._default_manager.filter(constraint.condition)
condition_fields = [
f[0].split("__")[0] for f in constraint.condition.children
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure how reliable this method of fields extraction

Copy link
Contributor

@sevdog sevdog Apr 23, 2024

Choose a reason for hiding this comment

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

I duobt that this will work when the condition uses a cross-reference between two fields like: Q(first_field__gte=F('second_field')), however I dunno if an unique constraint like that makes sense.

I would like to see also a more complex condition like Q(fielda__isnull=True) | Q(fieldb=False) to be addressed by this validator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These case should be handled now

@tomchristie
Copy link
Member

@kalekseev - Could you talk me through this?

My understanding is that it's resolving an issue in #7438, is that correct?

@kalekseev
Copy link
Contributor Author

kalekseev commented May 16, 2024

@kalekseev - Could you talk me through this?

My understanding is that it's resolving an issue in #7438, is that correct?

UPDATE: Yes, this pr is supposed to fix the initial implementation of UniqueConstraint support but at the moment it doesn't work. The problem is that DRF doesn't check if fields in data match the unique constraint condition or not, so for example constraint is:

            models.UniqueConstraint(
                name="unique_constraint_model_together_uniq2",
                fields=('race_name', 'position'),
                condition=models.Q(fancy_conditions__gte=10),
            )

we have record in db that should be unique

        UniqueConstraintModel.objects.create(
            race_name='condition',
            position=1,
            global_id=10,
            fancy_conditions=10
        )

The data below should be valid because it has fancy_conditions=9 that doesn't match constraint condition models.Q(fancy_conditions__gte=10):

       serializer = UniqueConstraintSerializer(data={
            'race_name': 'condition',
            'position': 1,
            'global_id': 11,
            'fancy_conditions': 9,
        })

but this should fail

      serializer = UniqueConstraintSerializer(data={
            'race_name': 'condition',
            'position': 1,
            'global_id': 11,
            'fancy_conditions': 11,
        })

@kalekseev kalekseev force-pushed the fix-unique-together-condition branch from fcc7c8d to 472a323 Compare May 17, 2024 00:08
@detetiveselvagem
Copy link

Is there any movement on this? This is a pretty serious bug

@kalekseev kalekseev force-pushed the fix-unique-together-condition branch 3 times, most recently from 0a64365 to 9b7db30 Compare June 23, 2024 14:56
@kalekseev
Copy link
Contributor Author

@kalekseev - Could you talk me through this?

My understanding is that it's resolving an issue in #7438, is that correct?

@tomchristie I think this PR is ready for review, I described issue in previous comment, the fix is here

https://github.com/encode/django-rest-framework/pull/9360/files#diff-9240f7dfb9eb2cace23eef00e4c922d6e2b6e85dc58b3c9d804e78c6c6227614R27-R34 the rest is just support code to perform that check (make sure that conditional fields values are present in serializer to perform check against condition).

The condition check is actually copy of code from https://github.com/django/django/blob/7ba2a0db20c37a5b1500434ca4ed48022311c171/django/db/models/constraints.py#L672

@auvipy
Copy link
Member

auvipy commented Aug 1, 2024

we have this separate PR which seems to fix a regression, can you guys please check? #9482

@MaxNstk
Copy link

MaxNstk commented Sep 30, 2024

I do not know how the situation is right now but I noticed that when upgrading from the 3.14.0 version to 3.15.2 my condition started to broke. Here is the model:

`
class DailyDataRecord(SynchronizationRecord):

date = models.DateField(verbose_name='Data')
is_active = models.BooleanField(default=False,verbose_name='Ativo')

class Meta:
    ordering= ['-date']
    verbose_name='Registro diário'
    verbose_name_plural='Registros diários'
    abstract=True
    constraints = [
        models.UniqueConstraint(
            fields=['date'],
            condition=models.Q(is_active=True),
            name='%(app_label)s_%(class)s_unique_active_dailyrecord_for_date'
        )
    ]

`

In the current state it stops to check the condition is_active=True and raises and exception if I try to create a new record with is_active=False for the same date.
Ps. The serializer is just a normal ModelSerializer with any model that extends of this class

@SorianoMarmol
Copy link

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

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

Successfully merging this pull request may close these issues.

3.15 regression: Serializer validation failed for unique together constraint
7 participants