Re: [Django] #15130: Model.validate_unique method doesn't take in account multi-db
Django
-------------------------------------+-------------------------------------
Reporter: t2y | Owner:
Type: Bug | Status: reopened
Milestone: 1.3 | Component: Database layer
Version: 1.2 | (models, ORM)
Resolution: | Severity: Normal
Triage Stage: Accepted | Keywords: multi-db
Needs documentation: 0 | Has patch: 1
Patch needs improvement: 1 | Needs tests: 0
| Easy pickings: 0
-------------------------------------+-------------------------------------
Changes (by ramiro):
* component: ORM aggregation => Database layer (models, ORM)
* easy: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/15130#comment:15>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: t2y | Owner:
Component: Database layer | Version: 1.2
(models, ORM) |
Keywords: multi-db | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timgraham):
#20301 is a duplicate.
--
Ticket URL: <https://code.djangoproject.com/ticket/15130#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: t2y | Owner:
Type: Bug | Status: new
Component: Database layer | Version: 1.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: multi-db | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timgraham):
#25514 is another duplicate.
--
Ticket URL: <https://code.djangoproject.com/ticket/15130#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: t2y | Owner:
Type: Bug | Status: new
Component: Database layer | Version: 1.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: multi-db | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by akaariai):
Hmmh, I think the core problem is that .validate() doesn't take a using
argument, while some of the checks are dependent on the used database. I
don't think we can fix this without adding the using argument.
If we do that, then ModelForms should likely also have
.is_valid(using=...) keyword argument. The next question is should the
using argument be used for ModelChoiceField queryset checks, too? I say
yes, with the exception that if the queryset has an explicit using
argument, then honor that.
We can start with adding using argument to .validate() only, and then
continue to further changes if wanted.
Any opinions if we should go forward with this?
--
Ticket URL: <https://code.djangoproject.com/ticket/15130#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: t2y | Owner:
Type: Bug | Status: new
Component: Database layer | Version: 1.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: multi-db | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timgraham):
Did you see the solution proposed in attachment:15130.2.diff? That seems
on the right track to me. I don't see why the programmer should have to
specify the database when we already have database routers to figure out
where the model will be saved?
--
Ticket URL: <https://code.djangoproject.com/ticket/15130#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: t2y | Owner:
Type: Bug | Status: new
Component: Database layer | Version: 1.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: multi-db | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by akaariai):
Yes, that would be a step in the right direction. But using the write
router is just a guess. There is an explicit way to tell which database to
use when saving, so there should also be an explicit way to tell Django
which database to use when validating.
The solution I see as the right one is:
{{{
1) Use explicitly given database for validation
2) Use write router's database for validation
3) Use the default database for validation
}}}
And currently, if I'm not mistake, we have this:
{{{
1) Use read router's database for validation
2) Use the default database for validation
(here 2 is actually implemented by the router for reading)
}}}
So, just changing Django to use write router instead of read router as
done in the attached patch would be an improvement, but not a complete
solution.
--
Ticket URL: <https://code.djangoproject.com/ticket/15130#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: t2y | Owner:
Type: Bug | Status: new
Component: Database layer | Version: 1.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: multi-db | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by akaariai):
As an addition - maybe we don't even want the complete solution. Because
if we add using to model.validate(), we kind of have to add it to
`ModelForm.is_clean()`, too. And then we have to decide what we do with
`ModelChoiceField(Author.objects.all())`, shouldn't it use the specified
database for validation, too?
But it seems usages where you need to validate the same model against
multiple databases really aren't that common. So, how much effort should
we spend on fixing this, instead of for example trying to add composite
foreign key support?
--
Ticket URL: <https://code.djangoproject.com/ticket/15130#comment:18>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 1.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: multi-db | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Michel Perez):
Is this already fixed?, I'm still having this problem using Django==1.10.3
and Python 3.5
--
Ticket URL: <https://code.djangoproject.com/ticket/15130#comment:19>
Django
-------------------------------------+-------------------------------------
Reporter: Tetsuya Morimoto | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 1.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: multi-db | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
Replying to [comment:19 Michel Perez]:
> Is this already fixed?, I'm still having this problem using
Django==1.10.3 and Python 3.5
This has not been fixed yet.
--
Ticket URL: <https://code.djangoproject.com/ticket/15130#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: Tetsuya Morimoto | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 1.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: multi-db | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Carlos Palol (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/15130#comment:21>
Django
-------------------------------------+-------------------------------------
Reporter: Tetsuya Morimoto | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 1.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: multi-db | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Menno Manheim (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/15130#comment:22>
Django
-------------------------------------+-------------------------------------
Reporter: Tetsuya Morimoto | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 1.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: multi-db | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by François Granade):
I got badly bitten by this bug in #33912, and I think that there are two
issues, related but not the same, here -
1) the current behavior (validating against the default database for
object that live in another database) does not make sense
2) it should be be possible to define against what DB the validation
happens.
To me, the first issue is a very serious bug, and the second a useful
feature.
What about fixing the first one first ? For that, the patch 15130.2.diff
above would work.
What would be needed for this patch to be accepted ? I can clean it up
--
Ticket URL: <https://code.djangoproject.com/ticket/15130#comment:23>
Django
-------------------------------------+-------------------------------------
Reporter: Tetsuya Morimoto | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 1.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: multi-db | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by François Granade):
Proposing a PR
--
Ticket URL: <https://code.djangoproject.com/ticket/15130#comment:24>
Django
-------------------------------------+-------------------------------------
| Granade
Type: Bug | Status: assigned
Component: Database layer | Version: 1.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: multi-db | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: (none) => François Granade
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/15130#comment:25>