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.

Sign up for GitHub

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

Jump to bottom

Assume/require django_full_clean? Build our own? #128

Open
sjamaan opened this issue Apr 6, 2020 · 1 comment
Open

Assume/require django_full_clean? Build our own? #128

sjamaan opened this issue Apr 6, 2020 · 1 comment
Assignees
@sjamaan @abzainuddin @stefanmajoor @irfinnew @daanvdk

Comments

@sjamaan
Copy link
Contributor

sjamaan commented Apr 6, 2020 •
edited
Loading

In one project, I've noticed that we do quite a lot of validations, to the point where a simple PUT incurs about 40 queries. I've managed to bring it down to about two thirds of that by overriding binder_full_clean to do nothing.

As far as I know, we're using django-full-clean in all our projects, so that whenever you call save(), full_clean gets triggered anyway. Maybe it's time to make it official and require it in Binder. Then we can drop most of binder_full_clean.

I've also noticed that full_clean doesn't support selectively disabling constraint validations. I think that if we see that a model includes the LoadedValuesMixin, we can check if any relations have changed. If not, we can in principle assume that all the foreign key constraint validations will succeed and pass all unchanged relations as exclude. If none of the uniqueness fields have changed, we can also pass in validate_unique=False. Now, there can be race conditions, but I think because we're using select_for_update that isn't or can't really be the case.

If we want to leverage LoadedValuesMixin, I think we must build our own django_full_clean replacement as part of Binder. This is not a big deal, as the implementation is just a handful of lines anyway.
@sjamaan
Copy link
Contributor Author

sjamaan commented Apr 9, 2020

See also #129

sjamaan pushed a commit that referenced this issue Aug 31, 2020
Call full_clean() in save(), so we don't need django-fullclean (#128)
339db1d 
At least, for Binder models this is the case.  All models managed by
Binder views should inherit from BinderModel, and if not, the user
should code validation error checking in _store().

In all our projects we use django-fullclean because being able to
accidentally save invalid models is a huge mind fuck.  Thus, it is
pointless to call full_clean() in our views, as it means we'll be
running potentially expensive validations twice.  For example,
relation fields check that the foreign relation exists, which requires
a query per relation field.

It is technically a breaking change, but for most projects this will
be a non-issue.  The test suite of Boekestijn passed except for one
test which was specifically for its User view, which uses a non-Binder
model.  This was relatively easy to convert to use a full_clean() in
its _store() method.
sjamaan pushed a commit that referenced this issue Aug 31, 2020
Call full_clean() in save(), so we don't need django-fullclean (#128)
da93764 
At least, for Binder models this is the case.  All models managed by
Binder views should inherit from BinderModel, and if not, the user
should code validation error checking in _store().

In all our projects we use django-fullclean because being able to
accidentally save invalid models is a huge mind fuck.  Thus, it is
pointless to call full_clean() in our views, as it means we'll be
running potentially expensive validations twice.  For example,
relation fields check that the foreign relation exists, which requires
a query per relation field.

It is technically a breaking change, but for most projects this will
be a non-issue.  The test suite of Boekestijn passed except for one
test which was specifically for its User view, which uses a non-Binder
model.  This was relatively easy to convert to use a full_clean() in
its _store() method.
@sjamaan sjamaan mentioned this issue Aug 31, 2020
Merged
sjamaan pushed a commit that referenced this issue Aug 31, 2020
Call full_clean() in save(), so we don't need django-fullclean (#128)
b5ec096 
At least, for Binder models this is the case.  All models managed by
Binder views should inherit from BinderModel, and if not, the user
should code validation error checking in _store().

In all our projects we use django-fullclean because being able to
accidentally save invalid models is a huge mind fuck.  Thus, it is
pointless to call full_clean() in our views, as it means we'll be
running potentially expensive validations twice.  For example,
relation fields check that the foreign relation exists, which requires
a query per relation field.

It is technically a breaking change, but for most projects this will
be a non-issue.  The test suite of Boekestijn passed except for one
test which was specifically for its User view, which uses a non-Binder
model.  This was relatively easy to convert to use a full_clean() in
its _store() method.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sjamaan sjamaan

@abzainuddin abzainuddin

@stefanmajoor stefanmajoor

@irfinnew irfinnew

@daanvdk daanvdk

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@sjamaan @abzainuddin @stefanmajoor @irfinnew @daanvdk