Re: Adding generated common table expressions
Hello everyone,
I'm often using django-cte and I'd be thrilled to have CTE in the core.
If I'm not mistaken, the only DB currently supported by Django and not
having CTE support is MySQL 5.7 (with an end of life in October 2023). I
don't know if Django 4.2 will support it, but it should be dropped for
Django 5.0 as it will be released in 2023. So we should have all supported
DB supporting CTE when this feature would be over.
The ticket (https://code.djangoproject.com/ticket/28919) has been stalled
for a few years now, this thread as well. I am willing to work on this but
I would like more information first.
If I try to list all the requirements, we should have:
* A way to add one or more CTE.
* A way to reference the columns from the CTE.
* A way to join them in the main query.
* Setting a CTE as recursive?
* Choosing if a CTE is materialized or not (Not all DB support that, and
I'm not sure if they all handle it the same way)?
* Insert / delete CTE with returning data?
Do we have a better idea now of what the API should look like?
Thanks.
Le jeudi 17 octobre 2019 à 23:43:49 UTC+2, buzzi@gmail.com a écrit :
> What do you think of this syntax instead?
>
> q1 = Book.objects.values('author_id').annotate(avg_price=Avg('price'))
>
> q2 = Author.objects.attach('book_prices', q1, id=F(
> 'book_prices__author_id'))
>
>
> def attach(name, queryset, **params):
># Would look something like this.
>...
>
>
> Same sql output.
>
> On Thursday, April 6, 2017 at 9:14:01 AM UTC-4, Anssi Kääriäinen wrote:
>>
>> On Thursday, April 6, 2017 at 11:53:32 AM UTC+3, Marc Tamlyn wrote:
>>>
>>> Regarding Anssi's comments about SubQuery, we do now have that in core
>>> as of 1.11 [0]. It does look like an .attach() approach might actually have
>>> been a nicer version of this, but on the other hand it's currently
>>> implementable solely with the Expressions API. It seems like the OuterRef
>>> is very similar to your queryset.ref(). An even nicer approach using attach
>>> could be to say qs.attach(q1=some_qs).filter(a=F('q1__b'))?
>>>
>>
>> Hmmh, we have one form of SubQuery, but that's actually for SELECT
>> clause, not for FROM clause. I believe the same class won't work for the
>> CTE or subquery in FROM clause case.
>>
>> As for the attach(), seems like a really nice syntax. We do need
>> something for generating the join clause for the JOIN. If you look at an
>> example:
>> q1 = Book.objects.values('author_id').annotate(avg_price=Avg('price'))
>> q2 = Author.objects.attach(q1=q1)
>> it needs to create something like:
>> WITH q1 AS (
>> SELECT author_id, avg(price) FROM book GROUP BY author_id
>> )
>> SELECT author.id, author.name
>>FROM author
>>LEFT JOIN q1 ON author.id = q1.author_id;
>>
>> Or, equivalently without the CTE:
>>
>> SELECT author.id, author.name
>>FROM author
>>LEFT JOIN ( SELECT author_id, avg(price) FROM book GROUP BY author_id)
>> ON author.id = q1.author_id;
>>
>> Now, the main points are:
>>1. There is no need to design this to be about CTEs. That just limits
>> the feature from backends that don't have CTEs without any real benefit.
>> From Django's perspective the two above queries are the same.
>>2. We do need something for the JOIN ON condition. In some cases
>> Django could guess this, but there needs to be an explicit way to express
>> the join condition.
>>
>> If we allow usage of expressions from the attached queryset, but don't
>> try to go for cases where model instance are created from the attached
>> queryset, this will be both possible to implement without having to write a
>> change-everything patch, and this will also be a really nice feature.
>>
>> For recursive CTEs, I'd leave that strictly as a later step. The only
>> thing we need to check right now is that we don't do something that
>> prevents a good recursive CTEs implementation later on.
>>
>> - Anssi
>>
>>>
>>> Looking forwards to seeing a DEP!
>>>
>>> [0]
>>> https://docs.djangoproject.com/en/1.11/ref/models/expressions/#subquery-expressions
>>>
>>> On 22 March 2017 at 01:32, Ashley Waite wrote:
>>>
Here's the code changes I've made, noting that some of them were to
shove in a generalised VALUES clause that mocks being a queryset, so that
it plays with the same interface.
https://github.com/django/django/compare/master...ashleywaite:cte-dev#files_bucket
I've had a glance at cte-trees/cte-forest and once general CTEs are
worked out expanding that to include recursive CTEs wouldn't be too
difficult, and that would greatly simplify the implementation of
cte-forest
to the extent that it might be viable as a django data/reference type.
- Ashley
On Saturday, March 18, 2017 at 8:28:53 PM UTC+11, Josh Smeaton wrote:
>
> Thanks for bringing this up Ashley, and for all of the detail you
> provided. I'd certainly like to see CTEs make their way into Django,
>
Re: Adding generated common table expressions
As far as I know, CTE and subquery are equivalent when used only in a
single place in the query.
CTE should be better when a single query is used several times.
If we want to reuse the Subquery API, we should find a way to be able to
reuse a subquery (hash its content and use that as a key to detect the
duplication?).
Le mardi 10 mai 2022 à 16:08:06 UTC+2, matthew.pava a écrit :
> I will always suggest that we use the Subquery API to make CTEs. To make
> them recursive, just add a keyword argument (recursive=True) and/or use a
> union.
>
>
>
> It’s been a while since I looked at CTEs, so I might be missing something.
>
>
>
> I would hate to see us create an entirely separate API for CTEs.
>
>
>
>
>
> *From:* django-d...@googlegroups.com *On
> Behalf Of *Gaga Ro
> *Sent:* Tuesday, May 10, 2022 9:01 AM
> *To:* Django developers (Contributions to Django itself) <
> django-d...@googlegroups.com>
> *Subject:* Re: Adding generated common table expressions
>
>
>
> Hello everyone,
>
>
>
> I'm often using django-cte and I'd be thrilled to have CTE in the core.
>
>
>
> If I'm not mistaken, the only DB currently supported by Django and not
> having CTE support is MySQL 5.7 (with an end of life in October 2023). I
> don't know if Django 4.2 will support it, but it should be dropped for
> Django 5.0 as it will be released in 2023. So we should have all supported
> DB supporting CTE when this feature would be over.
>
>
>
> The ticket (https://code.djangoproject.com/ticket/28919) has been stalled
> for a few years now, this thread as well. I am willing to work on this but
> I would like more information first.
>
>
>
> If I try to list all the requirements, we should have:
>
>
>
> * A way to add one or more CTE.
>
> * A way to reference the columns from the CTE.
>
> * A way to join them in the main query.
>
> * Setting a CTE as recursive?
>
> * Choosing if a CTE is materialized or not (Not all DB support that, and
> I'm not sure if they all handle it the same way)?
>
> * Insert / delete CTE with returning data?
>
>
>
> Do we have a better idea now of what the API should look like?
>
>
>
> Thanks.
>
> Le jeudi 17 octobre 2019 à 23:43:49 UTC+2, buzzi@gmail.com a écrit :
>
> What do you think of this syntax instead?
>
> q1 = Book.objects.values('author_id').annotate(avg_price=Avg('price'))
>
>
> q2 = Author.objects.attach('book_prices', q1, id=F(
> 'book_prices__author_id'))
>
>
> def attach(name, queryset, **params):
># Would look something like this.
>...
>
>
>
>
>
> Same sql output.
>
>
> On Thursday, April 6, 2017 at 9:14:01 AM UTC-4, Anssi Kääriäinen wrote:
>
> On Thursday, April 6, 2017 at 11:53:32 AM UTC+3, Marc Tamlyn wrote:
>
> Regarding Anssi's comments about SubQuery, we do now have that in core as
> of 1.11 [0]. It does look like an .attach() approach might actually have
> been a nicer version of this, but on the other hand it's currently
> implementable solely with the Expressions API. It seems like the OuterRef
> is very similar to your queryset.ref(). An even nicer approach using attach
> could be to say qs.attach(q1=some_qs).filter(a=F('q1__b'))?
>
>
>
> Hmmh, we have one form of SubQuery, but that's actually for SELECT clause,
> not for FROM clause. I believe the same class won't work for the CTE or
> subquery in FROM clause case.
>
>
>
> As for the attach(), seems like a really nice syntax. We do need something
> for generating the join clause for the JOIN. If you look at an example:
>
> q1 = Book.objects.values('author_id').annotate(avg_price=Avg('price'))
>
> q2 = Author.objects.attach(q1=q1)
>
> it needs to create something like:
>
> WITH q1 AS (
>
> SELECT author_id, avg(price) FROM book GROUP BY author_id
>
> )
>
> SELECT author.id
> <https://us-east-2.protection.sophos.com?d=author.id&u=aHR0cDovL2F1dGhvci5pZA==&i=NWVjN2YxNzUxNGEyNzMxNmMyMGRkZGU1&t=Z0JhMUVTd2l1QmxSMDlQeU9kaHNOQW03N21LM3UwTFlJTDB6aitGcE1URT0=&h=5e5d822fc76540f8bea7d6204744abc8>,
>
> author.name
> <https://us-east-2.protection.sophos.com?d=author.name&u=aHR0cDovL2F1dGhvci5uYW1l&i=NWVjN2YxNzUxNGEyNzMxNmMyMGRkZGU1&t=WXM2NWFjb2s0ejAvN0ZWS3V2aXJDbVIrZnUzNmlhL0hnQTQyeWx2K1lEMD0=&h=5e5d822fc76540f8bea7d6204744abc8>
>
>FROM author
>
>LEFT JOIN q1 ON author.id
> <https://us-east-2.protection.sophos.com?d=author.id&u=aHR0cDovL2F1dGhvci5
`Model.validate_unique` excluding partial unique constraint
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/09c08268-e094-4152-94e2-265d93481891n%40googlegroups.com.
Re: `Model.validate_unique` excluding partial unique constraint
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, addi
Re: `Model.validate_unique` excluding partial unique constraint
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 ins
Re: `Model.validate_unique` excluding partial unique constraint
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
Re: `Model.validate_unique` excluding partial unique constraint
> yeah didn't want to step on your toes but I got very excited about trying it out 😅 Don't worry about that, it's a good thing this motivated you enough to advance on this topic. > 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? Yes I agree that this should be independent from models. There is no reason to tie it to them, and if we want to do it anyway, it would be trivial to do using the new method. Or maybe we could have both we different named parameters and change the behaviour deping on those. > Looking at [0] in more details I also feel like matches() could be a better name than check(). It should be obvious that the method return a boolean, matches sounds like that it could returns a list of the matches to me. -- 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/5160faab-5c8f-42a5-b7d6-6594770cee65n%40googlegroups.com.
Re: `Model.validate_unique` excluding partial unique constraint
I tried my hand at implementing Q.check() (https://github.com/Gagaro/django/tree/ticket-30581). A few things: 1/ Is the exclude parameter there because of Model.validate_unique signature? Conditional UniqueConstraint might not work in those cases if a field use in a the condition is not in the form for example. 2/ Shouldn't we let the FieldError raising in Q.check() instead of returning None? Or raise another (new one?) exception? 3/ I'm not so sure anymore about the check name being better than matches. We might need more inputs on that one :). 4/ Should we raise NotImplementedError in BaseConstraint.validate? Thanks for your inputs. Le mercredi 16 juin 2021 à 17:08:24 UTC+2, Gaga Ro a écrit : > > yeah didn't want to step on your toes but I got very excited about > trying it out 😅 > > Don't worry about that, it's a good thing this motivated you enough to > advance on this topic. > > > 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? > > Yes I agree that this should be independent from models. There is no > reason to tie it to them, and if we want to do it anyway, it would be > trivial to do using the new method. Or maybe we could have both we > different named parameters and change the behaviour deping on those. > > > Looking at [0] in more details I also feel like matches() could be a > better name than check(). > > It should be obvious that the method return a boolean, matches sounds like > that it could returns a list of the matches to me. > -- 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/c74725c3-8b6d-4f55-b06c-8d202409d958n%40googlegroups.com.
Re: `Model.validate_unique` excluding partial unique constraint
So am I right that the example model with deleted_at will not be validated by ModelForm as deleted_at will never be included in it? I tried implementing ExclusionConstraint.validate (https://github.com/Gagaro/django/commit/558f33f574838b21cc9bf58a825ef337e7b1d0b2) but I had to use RawSQL as I didn't find another way to use the operator. It works great when running the query alone but: I don't like using raw SQL when there is a better way to do it (is there?). And it doesn't work when used in the Exists as the table is aliased and the raw SQL is not. Do you have any idea how to fix/improve that? Thanks again! Le lundi 21 juin 2021 à 18:00:03 UTC+2, charettes a écrit : > That's looking great :) > > 1. Yes and that's expected. If a form/serializer doesn't provide some > fields included in the constraint the database client side of the > validation can't do much about it. It might result in an integrity error > but that's a misuse of the API. I guess a check/runtime warning could be > emitted when creating model forms/serializers that don't fully cover > constraints define on attached models but that's already an issue with the > existing validate_unique logic. > 2. I guess it could be surfaced in Q.check and expected to be caught > Constraint.validate. Whichever layer performs the field exclusion should be > responsible for handling the FieldError. > 3. Yep definitely something we can bring to this list once we're satisfied > with the API. > 4. I guess we could yes! That makes me think we'll also want to implement > it for ExclusionConstraint if you're up for it! > > Simon > > Le lundi 21 juin 2021 à 08:54:59 UTC-4, gaga...@gmail.com a écrit : > >> I tried my hand at implementing Q.check() ( >> https://github.com/Gagaro/django/tree/ticket-30581). >> >> A few things: >> >> 1/ Is the exclude parameter there because of Model.validate_unique >> signature? Conditional UniqueConstraint might not work in those cases if a >> field use in a the condition is not in the form for example. >> 2/ Shouldn't we let the FieldError raising in Q.check() instead of >> returning None? Or raise another (new one?) exception? >> 3/ I'm not so sure anymore about the check name being better than >> matches. We might need more inputs on that one :). >> 4/ Should we raise NotImplementedError in BaseConstraint.validate? >> >> Thanks for your inputs. >> Le mercredi 16 juin 2021 à 17:08:24 UTC+2, Gaga Ro a écrit : >> >>> > yeah didn't want to step on your toes but I got very excited about >>> trying it out 😅 >>> >>> Don't worry about that, it's a good thing this motivated you enough to >>> advance on this topic. >>> >>> > 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? >>> >>> Yes I agree that this should be independent from models. There is no >>> reason to tie it to them, and if we want to do it anyway, it would be >>> trivial to do using the new method. Or maybe we could have both we >>> different named parameters and change the behaviour deping on those. >>> >>> > Looking at [0] in more details I also feel like matches() could be a >>> better name than check(). >>> >>> It should be obvious that the method return a boolean, matches sounds >>> like that it could returns a list of the matches to me. >>> >> -- 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/4bd947bb-d726-429b-8e02-4cf89809292bn%40googlegroups.com.
Re: `Model.validate_unique` excluding partial unique constraint
I took a bit of time to try with the new lookups and it looks much better! Also it actually works now :). Is the code ready for a PR? Or should I add the documentation / more tests before? https://github.com/Gagaro/django/commit/cfd10066359c175179ae937a287eb9f0240354f1 Le mardi 22 juin 2021 à 16:43:36 UTC+2, charettes a écrit : > > I don't like using raw SQL when there is a better way to do it (is > there?). > > And it doesn't work when used in the Exists as the table is aliased and > the raw SQL is not. > > I think the issue is that you're resolving before annotation/aliasing. If > #27021 > landed you could actually create a Lookup instance to contain both the rhs > and lhs and just use that but in the mean time you're kind of stuck to > writing your own Expression subclass. Maybe we should just rebase the > branch on top of the current work of #27021[0] since it should land before > this work anyway? > > Simon > > [0] https://github.com/django/django/pull/14494 > Le lundi 21 juin 2021 à 17:07:39 UTC-4, gaga...@gmail.com a écrit : > >> So am I right that the example model with deleted_at will not be >> validated by ModelForm as deleted_at will never be included in it? >> >> I tried implementing ExclusionConstraint.validate ( >> https://github.com/Gagaro/django/commit/558f33f574838b21cc9bf58a825ef337e7b1d0b2) >> >> but I had to use RawSQL as I didn't find another way to use the operator. >> It works great when running the query alone but: >> >> I don't like using raw SQL when there is a better way to do it (is >> there?). >> And it doesn't work when used in the Exists as the table is aliased and >> the raw SQL is not. >> >> Do you have any idea how to fix/improve that? >> >> Thanks again! >> >> Le lundi 21 juin 2021 à 18:00:03 UTC+2, charettes a écrit : >> >>> That's looking great :) >>> >>> 1. Yes and that's expected. If a form/serializer doesn't provide some >>> fields included in the constraint the database client side of the >>> validation can't do much about it. It might result in an integrity error >>> but that's a misuse of the API. I guess a check/runtime warning could be >>> emitted when creating model forms/serializers that don't fully cover >>> constraints define on attached models but that's already an issue with the >>> existing validate_unique logic. >>> 2. I guess it could be surfaced in Q.check and expected to be caught >>> Constraint.validate. Whichever layer performs the field exclusion should be >>> responsible for handling the FieldError. >>> 3. Yep definitely something we can bring to this list once we're >>> satisfied with the API. >>> 4. I guess we could yes! That makes me think we'll also want to >>> implement it for ExclusionConstraint if you're up for it! >>> >>> Simon >>> >>> Le lundi 21 juin 2021 à 08:54:59 UTC-4, gaga...@gmail.com a écrit : >>> >>>> I tried my hand at implementing Q.check() ( >>>> https://github.com/Gagaro/django/tree/ticket-30581). >>>> >>>> A few things: >>>> >>>> 1/ Is the exclude parameter there because of Model.validate_unique >>>> signature? Conditional UniqueConstraint might not work in those cases if a >>>> field use in a the condition is not in the form for example. >>>> 2/ Shouldn't we let the FieldError raising in Q.check() instead of >>>> returning None? Or raise another (new one?) exception? >>>> 3/ I'm not so sure anymore about the check name being better than >>>> matches. We might need more inputs on that one :). >>>> 4/ Should we raise NotImplementedError in BaseConstraint.validate? >>>> >>>> Thanks for your inputs. >>>> Le mercredi 16 juin 2021 à 17:08:24 UTC+2, Gaga Ro a écrit : >>>> >>>>> > yeah didn't want to step on your toes but I got very excited about >>>>> trying it out 😅 >>>>> >>>>> Don't worry about that, it's a good thing this motivated you enough to >>>>> advance on this topic. >>>>> >>>>> > 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? >>>>> >>>>> Yes I agree that this should be independent from models. There is no >&
Re: `Model.validate_unique` excluding partial unique constraint
No problem. The lookup PR has been merged, I'll work on our PR monday. I'm all up for the invalid message, it will be shown to the end user after all. Le mercredi 7 juillet 2021 à 20:53:23 UTC+2, charettes a écrit : > Just a small note that I didn't forget about this thread but I was waiting > for lookup annotation support to land before focusing on it[0]. > > I guess you could go ahead and create a PR once it lands. > > I assume we'll want to have Model.full_clean take advantage of this new > Constraint.validate method and remove the special handling in > _validate_unique as well. > > I wonder if we'll want to add a Constraint(invalid_message) argument to > allow for localized error messages to be raised on violation like we do > with validators? > > Simon > > [0] https://github.com/django/django/pull/14494/ > > Le jeudi 24 juin 2021 à 08:25:21 UTC-4, gaga...@gmail.com a écrit : > >> I took a bit of time to try with the new lookups and it looks much >> better! Also it actually works now :). >> >> Is the code ready for a PR? Or should I add the documentation / more >> tests before? >> >> >> https://github.com/Gagaro/django/commit/cfd10066359c175179ae937a287eb9f0240354f1 >> >> Le mardi 22 juin 2021 à 16:43:36 UTC+2, charettes a écrit : >> >>> > I don't like using raw SQL when there is a better way to do it (is >>> there?). >>> > And it doesn't work when used in the Exists as the table is aliased >>> and the raw SQL is not. >>> >>> I think the issue is that you're resolving before annotation/aliasing. >>> If #27021 landed you could actually create a Lookup instance to contain >>> both the rhs and lhs and just use that but in the mean time you're kind of >>> stuck to writing your own Expression subclass. Maybe we should just rebase >>> the branch on top of the current work of #27021[0] since it should land >>> before this work anyway? >>> >>> Simon >>> >>> [0] https://github.com/django/django/pull/14494 >>> Le lundi 21 juin 2021 à 17:07:39 UTC-4, gaga...@gmail.com a écrit : >>> >>>> So am I right that the example model with deleted_at will not be >>>> validated by ModelForm as deleted_at will never be included in it? >>>> >>>> I tried implementing ExclusionConstraint.validate ( >>>> https://github.com/Gagaro/django/commit/558f33f574838b21cc9bf58a825ef337e7b1d0b2) >>>> >>>> but I had to use RawSQL as I didn't find another way to use the operator. >>>> It works great when running the query alone but: >>>> >>>> I don't like using raw SQL when there is a better way to do it (is >>>> there?). >>>> And it doesn't work when used in the Exists as the table is aliased and >>>> the raw SQL is not. >>>> >>>> Do you have any idea how to fix/improve that? >>>> >>>> Thanks again! >>>> >>>> Le lundi 21 juin 2021 à 18:00:03 UTC+2, charettes a écrit : >>>> >>>>> That's looking great :) >>>>> >>>>> 1. Yes and that's expected. If a form/serializer doesn't provide some >>>>> fields included in the constraint the database client side of the >>>>> validation can't do much about it. It might result in an integrity error >>>>> but that's a misuse of the API. I guess a check/runtime warning could be >>>>> emitted when creating model forms/serializers that don't fully cover >>>>> constraints define on attached models but that's already an issue with >>>>> the >>>>> existing validate_unique logic. >>>>> 2. I guess it could be surfaced in Q.check and expected to be caught >>>>> Constraint.validate. Whichever layer performs the field exclusion should >>>>> be >>>>> responsible for handling the FieldError. >>>>> 3. Yep definitely something we can bring to this list once we're >>>>> satisfied with the API. >>>>> 4. I guess we could yes! That makes me think we'll also want to >>>>> implement it for ExclusionConstraint if you're up for it! >>>>> >>>>> Simon >>>>> >>>>> Le lundi 21 juin 2021 à 08:54:59 UTC-4, gaga...@gmail.com a écrit : >>>>> >>>>>> I tried my hand at implementing Q.check() ( >>>>>> https://gith
Re: `Model.validate_unique` excluding partial unique constraint
ully cover >>>>>>> constraints define on attached models but that's already an issue with >>>>>>> the >>>>>>> existing validate_unique logic. >>>>>>> 2. I guess it could be surfaced in Q.check and expected to be caught >>>>>>> Constraint.validate. Whichever layer performs the field exclusion >>>>>>> should be >>>>>>> responsible for handling the FieldError. >>>>>>> 3. Yep definitely something we can bring to this list once we're >>>>>>> satisfied with the API. >>>>>>> 4. I guess we could yes! That makes me think we'll also want to >>>>>>> implement it for ExclusionConstraint if you're up for it! >>>>>>> >>>>>>> Simon >>>>>>> >>>>>>> Le lundi 21 juin 2021 à 08:54:59 UTC-4, gaga...@gmail.com a écrit : >>>>>>> >>>>>>>> I tried my hand at implementing Q.check() ( >>>>>>>> https://github.com/Gagaro/django/tree/ticket-30581). >>>>>>>> >>>>>>>> A few things: >>>>>>>> >>>>>>>> 1/ Is the exclude parameter there because of Model.validate_unique >>>>>>>> signature? Conditional UniqueConstraint might not work in those cases >>>>>>>> if a >>>>>>>> field use in a the condition is not in the form for example. >>>>>>>> 2/ Shouldn't we let the FieldError raising in Q.check() instead of >>>>>>>> returning None? Or raise another (new one?) exception? >>>>>>>> 3/ I'm not so sure anymore about the check name being better than >>>>>>>> matches. We might need more inputs on that one :). >>>>>>>> 4/ Should we raise NotImplementedError in BaseConstraint.validate? >>>>>>>> >>>>>>>> Thanks for your inputs. >>>>>>>> Le mercredi 16 juin 2021 à 17:08:24 UTC+2, Gaga Ro a écrit : >>>>>>>> >>>>>>>>> > yeah didn't want to step on your toes but I got very excited >>>>>>>>> about trying it out 😅 >>>>>>>>> >>>>>>>>> Don't worry about that, it's a good thing this motivated you >>>>>>>>> enough to advance on this topic. >>>>>>>>> >>>>>>>>> > 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? >>>>>>>>> >>>>>>>>> Yes I agree that this should be independent from models. There is >>>>>>>>> no reason to tie it to them, and if we want to do it anyway, it would >>>>>>>>> be >>>>>>>>> trivial to do using the new method. Or maybe we could have both we >>>>>>>>> different named parameters and change the behaviour deping on those. >>>>>>>>> >>>>>>>>> > Looking at [0] in more details I also feel like matches() could >>>>>>>>> be a better name than check(). >>>>>>>>> >>>>>>>>> It should be obvious that the method return a boolean, matches >>>>>>>>> sounds like that it could returns a list of the matches to me. >>>>>>>>> >>>>>>>> -- 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/e5f610e9-90b5-473e-80bd-1ad77ba39554n%40googlegroups.com.
