> It looks like you went even further than that :D.
yeah didn't want to step on your toes but I got very excited about trying
it out 😅
> Should we still add Q.check() (which will be as you said before), then
refactor BaseConstraint.validate() to use it?
I think it would still be worth doing to avoid the Query(None), add_annotation,
except FieldError, get_compiler() boilerplate but me might need to change
the planed function signature.
What do you think of
def check(self, instance, exclude=None, using=DEFAULT_DB_ALIAS):
query = Query(None)
for field in instance._meta.local_concrete_fields:
if exclude and field.name in exclude:
continue
value = getattr(instance, field.attname)
query.add_annotation(Value(value, field), field.name, select=False)
try:
query.add_annotation(ExpressionWrapper(self, BooleanField()),
'_check')
except FieldError:
# Check is referencing an excluded field
return None
compiler = query.get_compiler(using=using)
return bool(compiler.execute_sql(SINGLE)[0])
Looks like it would deal with most of the boilerplate while still being
flexible enough for our use case. Maybe we don't want to push down the
model/exclude field notion this far though and require literals to be
passed directly instead.
def check(self, against, using=DEFAULT_DB_ALIAS):
query = Query(None)
for name, value in against.items():
if not hasattr('resolve_expression'):
value = Value(value)
query.add_annotation(value, name, select=False)
try:
query.add_annotation(ExpressionWrapper(self, BooleanField()),
'_check')
except FieldError:
# Check is referencing a missing field
return None
compiler = query.get_compiler(using=using)
return bool(compiler.execute_sql(SINGLE)[0])
I have a slight preference for the second option as it seems like it could
be used in other context than constraints[0] but I'm curious about your
thoughts? Looking at [0] in more details I also feel like matches() could
be a better name than check().
Simon
[0] https://github.com/django/django/pull/388#issuecomment-8863209
Le mercredi 16 juin 2021 à 04:44:08 UTC-4, gaga...@gmail.com a écrit :
> It looks like you went even further than that :D.
>
> Should we still add Q.check() (which will be as you said before), then
> refactor BaseConstraint.validate() to use it?
>
> Le mercredi 16 juin 2021 à 02:04:31 UTC+2, charettes a écrit :
>
>> I meant 1. in my previous email where sql.Query.model is allowed to be
>> None. The tests happen to pass on SQLite, MySQL, and Postgres.
>>
>> Le mardi 15 juin 2021 à 20:02:28 UTC-4, charettes a écrit :
>>
>>> FWIW I thought I'd give a timeboxed shot at 2. to make sure I don't send
>>> you towards a deep rabbit hole and it seems pretty straightforward!
>>>
>>>
>>> https://github.com/django/django/compare/main...charettes:query-empty-model
>>>
>>> Le lundi 14 juin 2021 à 03:09:35 UTC-4, gaga...@gmail.com a écrit :
>>>
>>>> Thanks, it clears things a lot.
>>>>
>>>> I'll try my hand at it when I'll have some more time available.
>>>>
>>>> Le jeudi 10 juin 2021 à 06:00:17 UTC+2, charettes a écrit :
>>>>
>>>>> Alright so here's for a few hints about I believe things should be
>>>>> done.
>>>>>
>>>>> First things first Lookup must be made a subclass of Expression which
>>>>> is being worked on[0].
>>>>>
>>>>> Ideally Q would also be made a subclass of Expression but that's
>>>>> likely a big can of worms so I'd focus on implementing it for Q only at
>>>>> first.
>>>>>
>>>>> Now for the compiler part. Things are bit tricky here as these
>>>>> expressions are not going to be bound to a model/table and most of the
>>>>> sql.Query and resolve_expression machinery revolves around the
>>>>> availability
>>>>> of a Query.model: models.Model property. I think there's two solutions
>>>>> here:
>>>>>
>>>>> 1. Adapt sql.Query so it can be *unbounded* meaning that it's .model
>>>>> property type would change from models.Model to Optional[models.Model].
>>>>> 2. Follow the sql.RawQuery route and define a new sql.UnboundQuery
>>>>> class that *looks* like a Query but doesn't allow any form of column
>>>>> references or JOINs or filters (WHERE).
>>>>>
>>>>> In both cases the Query like object should prevent any form of column
>>>>> references and JOINs with a comprehensible error messages (e.g. in
>>>>> resolve_ref
>>>>> and setup_join if go with 1.). I have a feeling 2. is easier to
>>>>> implement but 1. seems like it could be a much more rewarding experience
>>>>> for you and the community as you'll have to special case a couple of long
>>>>> lived assumptions in django.db.models.sql.
>>>>>
>>>>> Depending on whether you choose the 1. or 2. you'll have to implement
>>>>> a way for database backends to specify how to *wrap* the provided
>>>>> expression in a SELECT statement. Most databases won't require any
>>>>> special
>>>>> casing but I know that Oracle will require the addition of a trailing
>>>>> "DUAL" clause (SELECT ... FROM DUAL)[1] and possibly some special casing
>>>>> of
>>>>> expressions such as exists but there's already pre-existing logic for
>>>>> that[2]. If you go with 1. this can be done by returning a backend
>>>>> specific
>>>>> string in SQLCompiler.get_from_clause when self.query.alias_map is
>>>>> empty[3].
>>>>>
>>>>> In the end the call stack should be (assuming 1. is followed):
>>>>>
>>>>> Q.check(self, using):
>>>>> query = Query()
>>>>> query.add_annotations(self, '_check')
>>>>> query.set_values('_check')
>>>>> compiler = query.get_compiler(using=db)
>>>>> result = compiler.execute_sql(SINGLE)
>>>>> return bool(result[0])
>>>>>
>>>>> I hope this clears things up a bit!
>>>>>
>>>>> Cheers,
>>>>> Simon
>>>>>
>>>>> [0] https://github.com/django/django/pull/14494
>>>>> [1]
>>>>> https://docs.oracle.com/cd/B19306_01/server.102/b14200/queries009.htm
>>>>> [2]
>>>>> https://github.com/django/django/blob/ed3af3ff4b3cfb72de598f1b39a1028ba3826efb/django/db/models/expressions.py#L382-L389
>>>>> [3]
>>>>> https://github.com/django/django/blob/ed3af3ff4b3cfb72de598f1b39a1028ba3826efb/django/db/models/sql/compiler.py#L797-L810
>>>>>
>>>>> Le mercredi 2 juin 2021 à 11:36:02 UTC-4, gaga...@gmail.com a écrit :
>>>>>
>>>>>> Thanks for the thorough answer. I also realize now that it worked in
>>>>>> my app only because of another side effect when my instance was saved..
>>>>>>
>>>>>> I started to take a look at the ORM part where the check method
>>>>>> should be implemented as I'm not used to it. Shouldn't .check() be
>>>>>> implemented on Q and not on Expression? Or are you including Lookup
>>>>>> / Q in it?
>>>>>>
>>>>>> Then I'd guess it's just a matter of calling as_sql() from each part
>>>>>> and assemble them. Everythings we need seems to be done in Query and we
>>>>>> can't use it as it has to be linked to a model, so we would have to redo
>>>>>> it? Although as_sql needs a compiler which itself needs a query. I admit
>>>>>> I'm a bit lost in all those classes, everything seems to be too much
>>>>>> linked
>>>>>> to the models to do something without one.
>>>>>>
>>>>>> If you have any more hints as to where I should look, thanks again.
>>>>>> Le mercredi 2 juin 2021 à 00:33:12 UTC+2, charettes a écrit :
>>>>>>
>>>>>>> Hello there,
>>>>>>>
>>>>>>> Partial unique constraints are currently not supported during
>>>>>>> validation for reasons described in this ticket[0].
>>>>>>>
>>>>>>> For example (inspired by this Github comment[1]), if you define the
>>>>>>> following model
>>>>>>>
>>>>>>> class Article(models.Model):
>>>>>>> slug = models.CharField(max_length=100)
>>>>>>> deleted_at = models.DateTimeField(null=True)
>>>>>>>
>>>>>>> class Meta:
>>>>>>> constraints = [
>>>>>>> UniqueConstraint('slug', condition=Q(deleted_at=None),
>>>>>>> name='unique_slug'),
>>>>>>> ]
>>>>>>>
>>>>>>> Then validate_unique must perform the following query to determine
>>>>>>> if the constraint is violated
>>>>>>>
>>>>>>> SELECT NOT (%(deleted_at)s IS NULL) OR NOT EXISTS(SELECT 1 FROM
>>>>>>> article WHERE NOT id = %(id)s AND slug = %(slug)s AND deleted_at IS
>>>>>>> NULL)
>>>>>>>
>>>>>>> In other words, the validation of a partial unique constraint must
>>>>>>> check that either of these conditions are true
>>>>>>> 1. The provided instance doesn't match the condition
>>>>>>> 2. There's no existing rows matching the unique constraint
>>>>>>> (excluding the current instance if it already exists)
>>>>>>>
>>>>>>> This is not something Django supports right now.
>>>>>>>
>>>>>>> In order to add proper support for this feature I believe (personal
>>>>>>> opinion here feedback is welcome) we should follow these steps:
>>>>>>>
>>>>>>> 1. Add support for Expression.check(using: str) -> bool that would
>>>>>>> translate IsNull(deleted_at, True).check('alias') into a backend
>>>>>>> compatible 'SELECT %(deleted_at)s IS NULL' query and return whether or
>>>>>>> not
>>>>>>> it passed. That would also allow the constructions of forms like
>>>>>>>
>>>>>>> (~Q(IsNull(deleted_at, True)) |
>>>>>>> ~Exists(Article.objects.exclude(pk=pk).filter(slug=slug,
>>>>>>> deleted_at=None)).check(using)
>>>>>>>
>>>>>>> 2. Add support for Constraint.validate(instance, excluded_fields) as
>>>>>>> described in [0] that would build on top of Expression.check to
>>>>>>> implement
>>>>>>> proper UniqueConstraint, CheckConstraint, and ExclusionConstraint
>>>>>>> validation and allow for third-party app (e.g. django-rest-framework
>>>>>>> which
>>>>>>> doesn't use model level validation[2]) to take advantage of this
>>>>>>> feature.
>>>>>>> For example the unique_for_(date|month|year) feature of
>>>>>>> Date(Time)?Field could be deprecated in favour of Constraint subclasses
>>>>>>> that implement as_sql to enforce SQL level constraint if available by
>>>>>>> the
>>>>>>> current backend and implement .validate to replace the special case
>>>>>>> logic
>>>>>>> we have currently in place for these options[3].
>>>>>>>
>>>>>>> I hope this clarify the current situation.
>>>>>>>
>>>>>>> Cheers,
>>>>>>> Simon
>>>>>>>
>>>>>>> [0] https://code.djangoproject.com/ticket/30581#comment:7
>>>>>>> [1]
>>>>>>> https://github.com/django/django/pull/10796#discussion_r244216763
>>>>>>> [2] https://github.com/encode/django-rest-framework/issues/7173
>>>>>>> [3]
>>>>>>> https://github.com/django/django/blob/e703b152c6148ddda1b072a4353e9a41dca87f90/django/db/models/base.py#L1062-L1084
>>>>>>>
>>>>>>> Le mardi 1 juin 2021 à 11:18:23 UTC-4, gaga...@gmail.com a écrit :
>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> I changed several models from fields using `unique=True` to using
>>>>>>>> `UniqueConstraint` with a condition in the Meta.
>>>>>>>>
>>>>>>>> As a side-effect, the uniqueness are no longer validated during
>>>>>>>> cleaning of a Form and an integrity error is raised. This is because
>>>>>>>> partial unique indexes are excluded :
>>>>>>>>
>>>>>>>> https://github.com/django/django/blob/e703b152c6148ddda1b072a4353e9a41dca87f90/django/db/models/options.py#L865-L874
>>>>>>>>
>>>>>>>> It seems that `total_unique_constraints` is also used to check for
>>>>>>>> fields that should be unique (related fields and USERNAME_FIELD
>>>>>>>> specifically).
>>>>>>>>
>>>>>>>> I tried modifying `total_unique_constraints` and the only tests
>>>>>>>> which failed were related to the above concern and
>>>>>>>> `test_total_ordering_optimization_meta_constraints` which also uses `
>>>>>>>> total_unique_constraints`. My application works fine and the
>>>>>>>> validation error are correctly raised in my forms.
>>>>>>>>
>>>>>>>> The current behaviour of `Model.validate_unique` is also not the
>>>>>>>> one I expected as my conditional `UniqueConstraint` were not used
>>>>>>>> (which
>>>>>>>> caused the integrity error).
>>>>>>>>
>>>>>>>> Am I missing something? Or should we use all constraints (including
>>>>>>>> partial) in `Model.validate_unique`?
>>>>>>>>
>>>>>>>> If this is indeed what should be done, adding an `all_
>>>>>>>> unique_constraints` next to `total_unique_constraints` and using
>>>>>>>> it in `Model.validate_unique` instead of `total_unique_constraints`
>>>>>>>> would do the trick. I don't mind opening a ticket and doing the PR if
>>>>>>>> needed.
>>>>>>>>
>>>>>>>> Thanks.
>>>>>>>>
>>>>>>>
--
You received this message because you are subscribed to the Google Groups
"Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to django-developers+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-developers/02362f6e-6c83-4dcb-86a8-a8ff10881741n%40googlegroups.com.
