[Django] #19126: runserver versus model validation
Django
--------------------------------------------+--------------------
Reporter: direvus | Owner: nobody
Type: Uncategorized | Status: new
Component: Core (Management commands) | Version: 1.3
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------------+--------------------
I recently encountered a frustrating little issue in one of my apps. I
wasn't able to run the development server because of a non-critical model
validation failure.
The error was '"ordering" refers to "split_code", a field that doesn't
exist.'
In several of my models, "ordering" refers to a field which isn't part of
the main model definition, but *is* added to the query set by a custom
default Manager. So in practice (when I run it under mod_wsgi) the
ordering works exactly as intended. This is a case where the model fails
to validate, but actually works properly in a live-fire situation.
Perhaps the model validation is too strict in this case, but I can
understand why. It would probably be a lot of fuss to try to make it
clever enough to detect additional fields that are pulled in by the
queryset on the Manager. But there's a broader issue in that *any* model
validation failure, no matter how unimportant, is enough to stop the
development server from working with an "Unhandled exception" in 1.3.1.
I think it's fine that I get a message about the "ordering" definition,
but I don't think it should be a showstopper.
There are few different ways to get past this. My suggestion would be
that when executing "runserver", we should catch CommandErrors coming out
of 'validate', report them, and then keep on trying to run the server
anyway.
I've included a patch along these lines.
Other options include
* teach 'validate' to recognise fields that are included by the default
Manager's queryset,
* teach 'validate' to differentiate between errors that are critical in
nature, and those that really only need to be warnings,
* allow SQL expressions in 'ordering', not just field names, so that the
custom Manager field isn't necessary in the first place.
... but they all seem like a lot of effort.
Thanks for your time.
--
Ticket URL: <https://code.djangoproject.com/ticket/19126>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: direvus | Owner: nobody
Type: Uncategorized | Status: new
commands) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by lrekucki):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
The problem is, there are parts of Django that do not use a default
manager, but a base manager, which is always a Manager instance and will
not return QuerySet with your custom field. I suspect (but didn't check)
that {{{YourModel._base_manager.filter()}}} will fail then, but you just
didn't hit that functionality.
--
Ticket URL: <https://code.djangoproject.com/ticket/19126#comment:1>
Django
Reporter: direvus | Owner: nobody
Type: Uncategorized | Status: new
commands) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by direvus):
Replying to [comment:1 lrekucki]:
> The problem is, there are parts of Django that do not use a default
manager, but a base manager, which is always a Manager instance and will
not return QuerySet with your custom field. I suspect (but didn't check)
that {{{YourModel._base_manager.filter()}}} will fail then, but you just
didn't hit that functionality.
I tried {{{._base_manager.filter()}}}, and it didn't seem to have a
problem with my 'ordering' trick. In fact, ._base_manager ''is'' an
instance of my custom manager, not of Manager.
I may be using the terminology incorrectly, but when I said "default
manager", I meant that I had defined my own Manager subclass with
{{{use_for_related_fields = True}}}, and a custom {{{get_query_set}}}
method, and then assigned an instance of it to the 'objects' property of
the model class.
It might help if I explain my use-case. I have several tables which use a
dotted integer format as their primary identifier, e.g., "3.1.1.12". In
the database these are of text type, which is fine except for sorting,
where I want them to sort numerically on each integer in turn, not
lexicographically (3.1.1.9 should sort before 3.1.1.12).
If I were doing everything in SQL, this would be no big deal, I'd just
write {{{ORDER BY string_to_array(code, '.')::int[]}}}.
In Django, as far as I know you can't order by arbitrary SQL expressions,
only fields. So I needed to get the code in its integer-array form in
there as a "field" of sorts. So that's why I cooked up the custom Manager
with {{{.extra(select={'split_code': "string_to_array(account.code,
'.')::int[]"})}}} added to get_query_set, and then set {{{ordering =
['split_code']}}} in the model.
If there's a more Djangoistic way to approach this, I'm all ears. I
''could'' define a custom data type with comparison functions for sorting
in the database, but that seems a bit like overkill.
Setting all that aside, though, I think my original point about model
validation and runserver is still sane. What's the upside of pre-
emptively killing runserver? If the model validation problems are severe
enough to crash the app, then they'll crash the app and you'll get a shiny
debug page. If they're not severe enough to crash the app then they are
warnings.
--
Ticket URL: <https://code.djangoproject.com/ticket/19126#comment:2>
Django
Reporter: direvus | Owner: nobody
Type: Bug | Status: new
Component: Core (Management | Version: 1.3
commands) | Resolution:
Keywords: | decision needed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by aaugustin):
* type: Uncategorized => Bug
* stage: Unreviewed => Design decision needed
Comment:
One might try to work around this by inheriting `runserver.Command` and
disabling validation, but that requires copying `inner_run`, a 50-lines
method, just to remove the call to `validate`.
--
Ticket URL: <https://code.djangoproject.com/ticket/19126#comment:3>
Django
Reporter: direvus | Owner: nobody
Type: Bug | Status: new
Component: Core (Management | Version: 1.3
commands) | Resolution:
Severity: Normal | Triage Stage: Design
Keywords: | decision needed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by anonymous):
This bug effectively kills using Celery's django integration
http://pypi.python.org/pypi/django-celery since it uses `manage.py` to
start the Celery workers.
--
Ticket URL: <https://code.djangoproject.com/ticket/19126#comment:4>
Django
Reporter: direvus | Owner: nobody
Type: Bug | Status: new
Component: Core (Management | Version: 1.3
commands) | Resolution:
Severity: Normal | Triage Stage: Design
Keywords: | decision needed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by kmtracey):
It is djcelery itself that is requesting model validation on its command,
e.g. for celeryd: https://github.com/celery/django-
celery/blob/master/djcelery/management/commands/celeryd.py#L19
--
Ticket URL: <https://code.djangoproject.com/ticket/19126#comment:5>
Django
Reporter: direvus | Owner: nobody
Type: Bug | Status: new
Component: Core (Management | Version: 1.3
commands) | Resolution:
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by aaugustin):
* stage: Design decision needed => Accepted
Comment:
runserver currently contains this code:
{{{
# Validatioisn called explicitly each time the server is reloaded.
requires_model_validation = False
}}}
I propose to change this to `True`, and take it into account in
`inner_run`.
Untested patch:
{{{
--- a/django/core/management/commands/runserver.py
+++ b/django/core/management/commands/runserver.py
@@ -30,9 +30,6 @@ class Command(BaseCommand):
help = "Starts a lightweight Web server for development."
args = '[optional port number, or ipaddr:port]'
- # Validation is called explicitly each time the server is reloaded.
- requires_model_validation = False
-
def get_handler(self, *args, **options):
"""
Returns the default WSGI handler for the runner.
@@ -89,7 +86,8 @@ class Command(BaseCommand):
quit_command = (sys.platform == 'win32') and 'CTRL-BREAK' or
'CONTROL-C'
self.stdout.write("Validating models...\n\n")
- self.validate(display_num_errors=True)
+ if self.requires_model_validation:
+ self.validate(display_num_errors=True)
self.stdout.write((
"%(started_at)s\n"
"Django version %(version)s, using settings %(settings)r\n"
}}}
That would make it trivial to disable model validation when needed.
--
Ticket URL: <https://code.djangoproject.com/ticket/19126#comment:6>
Django
Reporter: direvus | Owner: nobody
Type: Bug | Status: new
Component: Core (Management | Version: 1.3
commands) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by direvus@…):
Replying to [comment:6 aaugustin]:
> That would make it trivial to disable model validation when needed.
That seems like a good idea, but it wouldn't really solve the problem I
originally posted about. I can already disable model validation, but I'd
rather not. What I want is for model validation to run, and report its
findings, but not to prevent runserver from starting up.
Model validation is great! I want it to do its thing. The issue is that
runserver's response to any model validation problem at all, is to commit
immediate suicide. If all model validation problems were of a fatal
nature, that would make sense. But they're not, so it doesn't.
--
Ticket URL: <https://code.djangoproject.com/ticket/19126#comment:7>
Django
Reporter: direvus | Owner: nobody
Type: Bug | Status: new
Component: Core (Management | Version: 1.3
commands) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by aaugustin):
Can you bring this up on django-developers? If other users are affected by
this issue, their feedback will be interesting.
--
Ticket URL: <https://code.djangoproject.com/ticket/19126#comment:8>
Django
Reporter: direvus | Owner: nobody
Type: Bug | Status: new
Component: Core (Management | Version: 1.3
commands) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by direvus):
Replying to [comment:8 aaugustin]:
> Can you bring this up on django-developers? If other users are affected
by this issue, their feedback will be interesting.
No problem -- I have posted my message but it hasn't shown up on the list
yet, I suppose it's waiting for moderation.
--
Ticket URL: <https://code.djangoproject.com/ticket/19126#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: direvus | Owner: nobody
Type: New feature | Status: new
Component: Core (Management | Version: master
commands) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* version: 1.3 => master
* type: Bug => New feature
Comment:
django-developers thread: https://groups.google.com/d/topic/django-
developers/kdeQEG1qFy8/discussion
--
Ticket URL: <https://code.djangoproject.com/ticket/19126#comment:10>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: closed
Component: Core (Management | Version: master
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by timo):
* status: new => closed
* resolution: => fixed
Comment:
The system check framework's `SILENCED_SYSTEM_CHECKS` setting allows you
to ignore model validation errors and allow `runserver` to proceed.
--
Ticket URL: <https://code.djangoproject.com/ticket/19126#comment:11>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: closed
Component: Core (Management | Version: master
commands) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by boxed):
This ticket was never fixed. The SILENCED_SYSTEM_CHECKS feature doesn't do
what you would think it does based on this discussion, and even if it did
work as the documentation says (it doesn't!) it would still be no good:
1. This ticket asked for the runserver to not die when there are
validation errors. This request was totally ignored, yet the ticket was
set to "fixed".
2. SILENCED_SYSTEM_CHECKS is a list of strings for what to disable. But
what strings are valid? If you put something invalid here it's ignored.
You have to step through the code I guess.
3. Model validation checks aren't even in the SILENCED_SYSTEM_CHECKS list
from what I can tell.
4. SILENCED_SYSTEM_CHECKS only silences the OUTPUT of the checks. If they
fail hard it still fails hard.
--
Ticket URL: <https://code.djangoproject.com/ticket/19126#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Brendan Jurd | Owner: nobody
Type: New feature | Status: closed
Component: Core (Management | Version: master
commands) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by Brendan Jurd):
Replying to [comment:12 boxed]:
> This ticket was never fixed. The SILENCED_SYSTEM_CHECKS feature doesn't
do what you would think it does based on this discussion, and even if it
did work as the documentation says (it doesn't!) it would still be no
good:
Wow, OP here. thanks for the reply, boxed. I certainly was surprised to
see this old thing pop up in my inbox.
I agree with your assessment that the original request was never
satisfied, and that this ticket was closed improperly. I didn't get too
worked up about that at the time, because I'd already found a way to work
around the particular issue I was having.
I think my original point here is still valid, I still believe that
runserver should proceed to run even if model validation fails, or at
least, there should be an option to enable that behaviour.
Having said that, in the eight years (yikes) since I wrote the original
ticket, I've long since stopped trying to coerce the Django ORM into doing
anything beyond the basics. These days I just bomb straight out to raw
SQL for anything even slightly exotic. There's a lot to like about
Django, but its ORM (really, ORMs in general) is just a big old energy
drain for me. It took me a while to realise this. SQL is fine. It was
fine all along.
So TL;DR: you're right, but I am no longer invested.
--
Ticket URL: <https://code.djangoproject.com/ticket/19126#comment:13>