Re: Model validation incompatibility with existing Django idioms
Joseph Kocherhans wrote: > Ok. I see it. ;) :D > Sorry, I've been out of town for a while with no > internet access. 12577 is near the top of my list to look at. ok thanks :D -- ()_() | That said, I didn't actually _test_ my patch. | + (o.o) | That's what users are for! | +---+ 'm m' | (Linus Torvalds) | O | (___) | raffaele dot salmaso at gmail dot com | -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
On Tue, Jan 19, 2010 at 1:04 AM, Raffaele Salmasowrote: > Raffaele Salmaso wrote: >> Joseph Kocherhans wrote: >>> regressions? >> http://code.djangoproject.com/ticket/12577 > Hello, is anybody out there? > Sorry if I seem rude, but there is a severe regression an no one care to > say at least 'ok I see it', even when there is an *explicit* request for > regressions... > I've resolved with an horrible monkeypatch, but at least I've now a > working copy of django Ok. I see it. ;) Sorry, I've been out of town for a while with no internet access. 12577 is near the top of my list to look at. Joseph -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
Raffaele Salmaso wrote: > Joseph Kocherhans wrote: >> regressions? > http://code.djangoproject.com/ticket/12577 Hello, is anybody out there? Sorry if I seem rude, but there is a severe regression an no one care to say at least 'ok I see it', even when there is an *explicit* request for regressions... I've resolved with an horrible monkeypatch, but at least I've now a working copy of django -- ()_() | That said, I didn't actually _test_ my patch. | + (o.o) | That's what users are for! | +---+ 'm m' | (Linus Torvalds) | O | (___) | raffaele dot salmaso at gmail dot com | -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
Raffaele Salmaso wrote: > Joseph Kocherhans wrote: >> regressions? > http://code.djangoproject.com/ticket/12577 Ok, BaseGenericInlineFormSet doesn't have save_new to save the generic fk.pk Reenabled and everything go as before. -- ()_() | That said, I didn't actually _test_ my patch. | + (o.o) | That's what users are for! | +---+ 'm m' | (Linus Torvalds) | O | (___) | raffaele dot salmaso at gmail dot com | -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
Joseph Kocherhans wrote: > regressions? http://code.djangoproject.com/ticket/12577 -- ()_() | That said, I didn't actually _test_ my patch. | + (o.o) | That's what users are for! | +---+ 'm m' | (Linus Torvalds) | O | (___) | raffaele dot salmaso at gmail dot com | -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
On Wed, Jan 6, 2010 at 12:49 PM, Simon Willisonwrote: > A couple of related tickets filed today about model validation: > > http://code.djangoproject.com/ticket/12513 > http://code.djangoproject.com/ticket/12521 This has been fixed in r12206 [1]. Could people who had issues please check things out again and let me (or trac) know if you find any regressions? I think Honza and I managed to hit most of the ones that had tickets, but there were quite a few corner cases that had to be fixed in this changeset. I specifically added a test for the commit=False idiom, but I'm sure people have more complicated scenarios out there. Joseph [1] http://code.djangoproject.com/changeset/12206 -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
2010/1/10 Tai Lee> On Jan 9, 12:36 am, Honza Král wrote: > > On Fri, Jan 8, 2010 at 11:59 AM, koenb > wrote: > > > > > On 8 jan, 10:03, James Bennett wrote: > > > > >> Suppose I have a ModelForm and call save(commit=False) to get the > > >> instance so I can do some more work on it. I'm basically saying to > > >> Django "I don't think this object is ready to be saved yet, but I need > > >> the object so I can do stuff to it". If Django goes ahead and does > > >> implicit model validation there and raises an exception, it's just > > >> telling me what I already knew: the object's not ready to be saved. > > >> But now I get to do extra work to catch the exception, and that's bad > > >> and wrong. > > > > >> Calling ModelForm.save(commit=False) should simply disable model > > >> validation, period. If I want to validate the instance before saving > > >> it, I'll validate the instance before saving it, but commit=False is > > >> as clear a way as we have of saying "I'm not going to save this yet" > > >> and model validation should respect that. > > I agree with this completely. Calling ModelForm.save(commit=False) > should disable model validation, period. We're explicitly telling > Django not to save the model because it's not ready to be saved, > therefore no model validation needs to be performed. > > > > The problem is that in the idiom the is_valid() call on the modelform > > > tries to full_validate the instance, it is not in the save > > > (commit=False) call. > > > > Yes > > This is what I think should be changed. ModelForm.is_valid() should > only validate the form. Model validation errors should not be (and I > believe are not currently) returned to the user as form errors, > because they're not form errors and the user can't do anything about > them. This is only true as long as we're talking about simple validators. If there are constraints on the model that include more then one field - one which comes from the form + a generated one - then returning it as an error may be useful. Only validating the form is probably a good strategy, but I think there should be an easy way of returning model validation errors to the user. > They're errors for the developer at the point when he or she is > ready to save a model. Model validation should be moved out of > ModelForm.is_valid() and into ModelForm.save(), but only if > `commit=True`. Otherwise, model validation should be performed when > explicitly requested. > > Cheers. > Tai. > > -- > You received this message because you are subscribed to the Google Groups > "Django developers" group. > To post to this group, send email to django-develop...@googlegroups.com. > To unsubscribe from this group, send email to > django-developers+unsubscr...@googlegroups.com > . > For more options, visit this group at > http://groups.google.com/group/django-developers?hl=en. > > -- Łukasz Rekucki -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
On Jan 9, 12:36 am, Honza Králwrote: > On Fri, Jan 8, 2010 at 11:59 AM, koenb wrote: > > > On 8 jan, 10:03, James Bennett wrote: > > >> Suppose I have a ModelForm and call save(commit=False) to get the > >> instance so I can do some more work on it. I'm basically saying to > >> Django "I don't think this object is ready to be saved yet, but I need > >> the object so I can do stuff to it". If Django goes ahead and does > >> implicit model validation there and raises an exception, it's just > >> telling me what I already knew: the object's not ready to be saved. > >> But now I get to do extra work to catch the exception, and that's bad > >> and wrong. > > >> Calling ModelForm.save(commit=False) should simply disable model > >> validation, period. If I want to validate the instance before saving > >> it, I'll validate the instance before saving it, but commit=False is > >> as clear a way as we have of saying "I'm not going to save this yet" > >> and model validation should respect that. I agree with this completely. Calling ModelForm.save(commit=False) should disable model validation, period. We're explicitly telling Django not to save the model because it's not ready to be saved, therefore no model validation needs to be performed. > > The problem is that in the idiom the is_valid() call on the modelform > > tries to full_validate the instance, it is not in the save > > (commit=False) call. > > Yes This is what I think should be changed. ModelForm.is_valid() should only validate the form. Model validation errors should not be (and I believe are not currently) returned to the user as form errors, because they're not form errors and the user can't do anything about them. They're errors for the developer at the point when he or she is ready to save a model. Model validation should be moved out of ModelForm.is_valid() and into ModelForm.save(), but only if `commit=True`. Otherwise, model validation should be performed when explicitly requested. Cheers. Tai. -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
-Original Message- From: Ivan Sagalaev <man...@softwaremaniacs.org> Date: Sun, 10 Jan 2010 03:25:29 To: <django-developers@googlegroups.com> Subject: Re: Model validation incompatibility with existing Django idioms Joseph Kocherhans wrote: > # Run validation that was missed by the form. > p.validate_fields(fields=['user', 'primary_contact']) > p.validate_unique(fields=['user', 'primary_contact']) > p.validate() Can this be shortcut to p.full_validate(fields=['user', 'primary_contact']) ? If not, why not? :-) -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
On Sat, Jan 9, 2010 at 6:25 PM, Ivan Sagalaevwrote: > Joseph Kocherhans wrote: >> >> # Run validation that was missed by the form. >> p.validate_fields(fields=['user', 'primary_contact']) >> p.validate_unique(fields=['user', 'primary_contact']) >> p.validate() > > Can this be shortcut to > > p.full_validate(fields=['user', 'primary_contact']) > > ? > > If not, why not? :-) Hmm... I guess I'm -0. The caveats with that validate_unique method are such that I'd rather not abstract it behind something else. I don't think you'd want to pass the same fields to validate_fields and validate_unique in most cases. Also, it doesn't make a whole lot of sense to call validate unless you're validating everything, so we'd have to document that as well. In practice, I don't think people will do this a whole lot, so 3 method calls shouldn't be a big deal. We can always add it later if people really need it in practice. Joseph -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
Joseph Kocherhans wrote: # Run validation that was missed by the form. p.validate_fields(fields=['user', 'primary_contact']) p.validate_unique(fields=['user', 'primary_contact']) p.validate() Can this be shortcut to p.full_validate(fields=['user', 'primary_contact']) ? If not, why not? :-) -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
On Wed, Jan 6, 2010 at 12:49 PM, Simon Willisonwrote: > A couple of related tickets filed today about model validation: > > http://code.djangoproject.com/ticket/12513 > http://code.djangoproject.com/ticket/12521 > > The first one describes the issue best - the new model validation code > breaks the following common Django convention: > > form = SecretQuestionForm( {"secret_question":"foo", "answer":"bar"} ) > if form.is_valid(): >p = form.save(commit=False) >p.user = request.user >p.primary_contact = somecontact >p.save() > > The problem is that is_valid() notices that some of the required > fields in SecretQuestionForm (a ModelForm) have not been provided, > even if those fields are excluded from validation using the excludes= > or fields= properties. The exception raised is a > UnresolvableValidationError. OK. I've attached a patch for another shot at this to #12521 [1]. form.is_valid *should* act like it did before the model-validation merge. This is trickier than it sounds though, and there are probably a few more corner cases. ModelForm validation uses validation from model fields and validators, not just the form fields, so simply skipping validation for excluded fields isn't enough. model.full_validate() is *only* for validating an entire model. It calls validate_fields, validate_unique, the the validate hook in order. model.validate_fields(fields=None) Validate the fields specified, or all fields if fields is None. fields should be a list of field names. model.validate_unique(fields=None) Validate the uniqueness of the specified fields, or all fields if fields is None. fields should be a list of field names. form = SecretQuestionForm( {"secret_question":"foo", "answer":"bar"} ) if form.is_valid(): p = form.save(commit=False) p.user = request.user p.primary_contact = somecontact # You probably won't actually want to run model validation this # way, but hopefully this shows what ModelForm isn't doing. try: # Run validation that was missed by the form. p.validate_fields(fields=['user', 'primary_contact']) p.validate_unique(fields=['user', 'primary_contact']) p.validate() # Or run *all* validation again. p.full_validate() except ValidationError, e: pass # I don't know how you'd even really recover from error here. # it's too late to show any form errors. At least you # know your model is valid before saving though. p.save() Thoughts? Joseph [1] http://code.djangoproject.com/ticket/12521 -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
On Sat, Jan 9, 2010 at 7:46 AM, Ivan Sagalaevwrote: > I too would opt for an implementation that makes model validation optional, >> i.e., a call that developers must explicitly make, if they choose, before >> saving a model. >> > > I'm +1 on some way of validating a form without *fully* validating a model. > But for a bit different reason that I didn't see in this thread yet. > I don't see why model validation should be bound up with forms at all. The current release notes for model validation state that it won't be done unless explicitly requested by the developer, and it seems that validating the model inside a form (whether fully or partially) breaks that contract. Again - pardon my ignorance if there's something that I'm missing. I was just alarmed to find a thread about forms + model validation given the current state of the release notes. Tobias -- Tobias McNulty Caktus Consulting Group, LLC P.O. Box 1454 Carrboro, NC 27510 (919) 951-0052 http://www.caktusgroup.com -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
Tobias McNulty wrote: I don't see why model validation should be bound up with forms at all. I'm not the one who designed it, so it's just me view. I think this is just useful: if you have a code validating, say, a CharField at the model level why not reuse it at the form level? What's important is that *entire validity* of a form should not be bound to that of a model. This was a bug which Joseph Kocherhans is now fixing. The current release notes for model validation state that it won't be done unless explicitly requested by the developer, and it seems that validating the model inside a form (whether fully or partially) breaks that contract. Well, I think we can fix release notes for the next release :-). -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
Tobias McNulty wrote: I regret and apologize that I'm arriving to this thread rather late. To support the tradition, I'm apoligizing for this too :-). Though it's funny how everyone thinks they're "late" on a couple-of-days-old thread :-). Anyway... I too would opt for an implementation that makes model validation optional, i.e., a call that developers must explicitly make, if they choose, before saving a model. I'm +1 on some way of validating a form without *fully* validating a model. But for a bit different reason that I didn't see in this thread yet. The thing is that validating a ModelForm may not be strictly related to subsequent saving of an instance. A use-case: I have a PreviewForm that generates a preview for a comment that user is writing in a form. It uses model fields to validate currently typed text and selected markup filter and then creates a model instance to do actual formatting: class PreviewForm(ModelForm): class Meta: model = Comment fields = ['text', 'markup_filter'] def preview(self): comment = Comment( text = self.cleaned_data['text'], filter = self.cleaned_data['markup_filter'], ) return comment.html() It is then used like this: def preview_view(request): form = PreviewForm(request.POST) if form.is_valid(): # <- this now breaks return HttpResponse(PreviewForm.preview()) The thing is that the code has no intention to save an instance into a DB. It just needs it at application level. This is why I think that decisions on "full" vs. "partial" validation should not be bound to `save()`. (FYI currently the first patch (Joseph's) in #12521 does fix this problem while the second patch (Honza's) doesn't. From a quick look this is beacuse the second patch only excludes fields from validation that listed in `exclude`). -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
On Fri, Jan 8, 2010 at 4:03 AM, James Bennettwrote: > On Thu, Jan 7, 2010 at 10:40 AM, Honza Král wrote: > > ModelForm has a save() method that saves the model. It is reasonable > > to assume that if the form is valid, the save() method call should > > succeed, that's why the entire model is validated. > > Actually, I can see a strong argument against this: if I've defined a > form class that I think constructs valid instances, and it doesn't, > that's a bug in my form and Django should be making this as clear as > possible. > > Of course, the flip side is that the bug in my form may be something > subtle which breaks a constraint specified at the Python level but not > at the DB level, and so the additional automatic validation could be > the only way to catch it. > For another alternative still, a constraint may exist at the database level that Python doesn't know about (or may not be able to enforce efficiently). I regret and apologize that I'm arriving to this thread rather late. Is there a document somewhere that discusses, or could someone describe, how model validation fits between the existing form validation in Django and constraints that are specified on the database side? I too would opt for an implementation that makes model validation optional, i.e., a call that developers must explicitly make, if they choose, before saving a model. I've always been and I continue to be wary of trying to implement data constraints at the application level; that's something good relational databases have been doing and improving upon for a long time and I have little faith in my own capacity to reproduce or replace such functionality. Cheers, Tobias -- Tobias McNulty Caktus Consulting Group, LLC P.O. Box 1454 Carrboro, NC 27510 (919) 951-0052 http://www.caktusgroup.com -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
On Fri, Jan 8, 2010 at 8:36 AM, Honza Králwrote: > I just hate the name save(commit=False) when it has nothing to do with > saving or committing, it just DOESN'T call save, it's imho misleading. > I understand why that is and how it came to be, I just don't like it. > I wasn't, however, proposing we get rid of this feature, just that we > extract it to a separate method or just tell people to use > form.instance (which is all what it does + creating save_m2m which can > be put elsewhere). > There is /a lot/ of code that relies on this naming and, while it might not be the greatest name choice in the world, it's one you get used to after making use of it time and time again. I'm fairly certain the 'commit' argument is not the only instance of imperfect naming in the Django core, and fixing all of these would leave Django users with a relatively insurmountable quantity of deprecated code. Frankly I'm not sure it's worth it. Tobias -- Tobias McNulty Caktus Consulting Group, LLC P.O. Box 1454 Carrboro, NC 27510 (919) 951-0052 http://www.caktusgroup.com -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
On Fri, Jan 8, 2010 at 11:59 AM, koenbwrote: > > > On 8 jan, 10:03, James Bennett wrote: > >> Suppose I have a ModelForm and call save(commit=False) to get the >> instance so I can do some more work on it. I'm basically saying to >> Django "I don't think this object is ready to be saved yet, but I need >> the object so I can do stuff to it". If Django goes ahead and does >> implicit model validation there and raises an exception, it's just >> telling me what I already knew: the object's not ready to be saved. >> But now I get to do extra work to catch the exception, and that's bad >> and wrong. >> >> Calling ModelForm.save(commit=False) should simply disable model >> validation, period. If I want to validate the instance before saving >> it, I'll validate the instance before saving it, but commit=False is >> as clear a way as we have of saying "I'm not going to save this yet" >> and model validation should respect that. I just hate the name save(commit=False) when it has nothing to do with saving or committing, it just DOESN'T call save, it's imho misleading. I understand why that is and how it came to be, I just don't like it. I wasn't, however, proposing we get rid of this feature, just that we extract it to a separate method or just tell people to use form.instance (which is all what it does + creating save_m2m which can be put elsewhere). > > The problem is that in the idiom the is_valid() call on the modelform > tries to full_validate the instance, it is not in the save > (commit=False) call. Yes > I'm with Simon here: on an incomplete modelform, let's just ignore the > errors on excluded fields and only validate the user input at that > point. If the developer wants to be sure the model validates after he > adds the necessary extra information, it is his job to call validation > before saving. The only problem I see here is that you cannot run model.validate (so check for unique etc.) because the user might define some custom validation there that you have no control over and that can check fields you don't want it to touch. We could move validate_unique elsewhere, but that could create problem for people not wanting to have their fields validated for uniqueness (expensive hits to the DB) But overall I feel that "we only call model.full_validate when no field is excluded or we are editing an existing instance" is the desired behavior by most people. In code that would mean that self.validate in full_clean would only be called if not exclude and excluded form fields would be passed to model.full_clean when adding an instance (not when editing), that's a very simple change. We just need to document this behavior thoroughly because it can cause confusion. The question remains how to validate inlines in Admin. I think there is no question that we want to call full_validate on the inlined model eventually, the question is how to do it in a most backwards compatible and sane way. We need the FK to do the validation and we cannot get the FK without the save() of the parent model. So imho it's just a question of how much risk we are taking and at which point do we call model.save() on the parent (after the form validates, after the inline form fields validate). If we take the new proposal for ModelForm behavior it could work: MainForm.clean() for f in inline: f.validate() if all.valid: MainForm.save() for inline in inlines: for form in inline; f.instance.full_validate() inline.save() the problem is when f.instance.full_validate() fails on the inline. That's a point where the FK must exist so the parent object has to be saved already. There is no way around this if we want to have model-validation in admin uncrippled. -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
On 8 jan, 10:03, James Bennettwrote: > Suppose I have a ModelForm and call save(commit=False) to get the > instance so I can do some more work on it. I'm basically saying to > Django "I don't think this object is ready to be saved yet, but I need > the object so I can do stuff to it". If Django goes ahead and does > implicit model validation there and raises an exception, it's just > telling me what I already knew: the object's not ready to be saved. > But now I get to do extra work to catch the exception, and that's bad > and wrong. > > Calling ModelForm.save(commit=False) should simply disable model > validation, period. If I want to validate the instance before saving > it, I'll validate the instance before saving it, but commit=False is > as clear a way as we have of saying "I'm not going to save this yet" > and model validation should respect that. > The problem is that in the idiom the is_valid() call on the modelform tries to full_validate the instance, it is not in the save (commit=False) call. I'm with Simon here: on an incomplete modelform, let's just ignore the errors on excluded fields and only validate the user input at that point. If the developer wants to be sure the model validates after he adds the necessary extra information, it is his job to call validation before saving. Koen -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
On Thu, Jan 7, 2010 at 10:40 AM, Honza Králwrote: > ModelForm has a save() method that saves the model. It is reasonable > to assume that if the form is valid, the save() method call should > succeed, that's why the entire model is validated. Actually, I can see a strong argument against this: if I've defined a form class that I think constructs valid instances, and it doesn't, that's a bug in my form and Django should be making this as clear as possible. Of course, the flip side is that the bug in my form may be something subtle which breaks a constraint specified at the Python level but not at the DB level, and so the additional automatic validation could be the only way to catch it. > 2) Are you OK with ripping save(commit=False) functionality to some > other method? (current functionality can stay with a deprecation > warning for example) No. Suppose I have a ModelForm and call save(commit=False) to get the instance so I can do some more work on it. I'm basically saying to Django "I don't think this object is ready to be saved yet, but I need the object so I can do stuff to it". If Django goes ahead and does implicit model validation there and raises an exception, it's just telling me what I already knew: the object's not ready to be saved. But now I get to do extra work to catch the exception, and that's bad and wrong. Calling ModelForm.save(commit=False) should simply disable model validation, period. If I want to validate the instance before saving it, I'll validate the instance before saving it, but commit=False is as clear a way as we have of saying "I'm not going to save this yet" and model validation should respect that. -- "Bureaucrat Conrad, you are technically correct -- the best kind of correct." -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
> ModelForm has a save() method that saves the model. It is reasonable > to assume that if the form is valid, the save() method call should > succeed, that's why the entire model is validated. mf.is_valid() I have understood as validation strictly on the included fields in the form. I abuse this "feature". Once I know that something has been done validly by the user, I can bring into motion all kinds of things. As you can see all of these forms and their ilk are intentionally excluding known required fields. class MinimalContactForm(forms.ModelForm): class Meta: model = Contact fields = ('first', 'm', 'last') class PrimaryContactForm(forms.ModelForm): class Meta: model = Contact exclude = ('user','primary', 'address', 'email') class ContactForm(forms.ModelForm): class Meta: model = Contact exclude = ('user',) I know that you know what I'm saying, so let's see > We could create a PartialModelForm, that would have save() method only > when editing (and not adding) models and have other method (or > enforced commit=False) for retrieval of the partial model. This form > would only call validation on the modelfields that are part of the > form (not being excluded). This is the behavior I understand everybody > expects from ModelForm, so, for backwards compatibility, we could call > it just ModelForm. not entirely sure what you mean > only when editing (and not adding) models and have other method (or > enforced commit=False) for retrieval of the partial model . > Also please mind that it could prove difficult to do half the > validation in one place and other fields elsewhere without a form. > Models, as opposed to forms, don't have a state in sense of > validated/raw and they don't track changes to their fields' data. > That's why validation is run every time and the results are not cached > on the instance. > > Few quick questions to get some ideas: > > 1) If editing a model (passed an instance parameter to __init__), even > with a partial form, do you expect model.full_validate being called? > > 2) Are you OK with ripping save(commit=False) functionality to some > other method? (current functionality can stay with a deprecation > warning for example) I wouldn't like to see that idiom changed. We can create another attr on the form but leave is_valid()? > 3) Would you want errors raised in model validation after it being > created by a form (commit=False) to appear on the form? I suppose that I have been guilty of taking advantage of modelforms to an extreme degree. I don't picture needing model validation on my modelforms right now. I sometimes have a bunch of small forms (if the case needs) like: Applicant information {{form|as_uni_form}} {{profile_form|as_uni_form}} How may we contact you? {{contact_form|as_uni_form}} How did you hear about us? {{hear_about|as_uni_form}} Terms and Conditions {{tos_form|as_uni_form}} if form.is_valid() and profile_form.is_valid()...: ... do magic to create user ... add the required user field to the other objects with commit=False idiom Perhaps in this way I am also abusing the relational db. But, I always find occasion to add required fields (often FK to the other modelforms) after I know the form "is valid" (UnresolvableValidationError). But, I would like another attr, so I could call modelform.model_errors () or modelform.model_is_valid() or something. Just plugging my own self interest here though really. I would like to see everything that I use behave exactly as I have come to know it and then see other new features popping up around that. > > on subject of inlines: > 1) Is it acceptable to create a model and not it's inlines in the > admin if the modelform validates but the inlines don't? > > 2) How can we limit people's validation code to avoid validating FKs > in inlines since users can (and should) create their own validation > logic on Models? Especially when we try and treat admin as "just a > cool app" and not something people should really design for. > > 3) How would you feel on creating an option to enable behavior (1) ) > and document that models with side effects in their save and delete > should really go for that? > > 4) Making 3) the default and enable switching it off if people want > their admin page to save atomically. > > Please let me know what you think I don't really know enough about the internals of the admin to say much about that. -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
For us we definitely use this behavior, and I'm guessing this is about to bite us in the ass. I would think a simple fix would be to have a commit=False, and validate=True keyword arg. By default, validate is NoInput, but if commit is False it defaults to False. Wouldn't that be a simple enough backwards compatible workaround? On Jan 7, 10:40 am, Honza Králwrote: > Hi everybody, sorry for the late reply, was busy. > > Just to add few points that lead us to do it this way: > > ModelForm has a save() method that saves the model. It is reasonable > to assume that if the form is valid, the save() method call should > succeed, that's why the entire model is validated. > > We could create a PartialModelForm, that would have save() method only > when editing (and not adding) models and have other method (or > enforced commit=False) for retrieval of the partial model. This form > would only call validation on the modelfields that are part of the > form (not being excluded). This is the behavior I understand everybody > expects from ModelForm, so, for backwards compatibility, we could call > it just ModelForm. > > Also please mind that it could prove difficult to do half the > validation in one place and other fields elsewhere without a form. > Models, as opposed to forms, don't have a state in sense of > validated/raw and they don't track changes to their fields' data. > That's why validation is run every time and the results are not cached > on the instance. > > Few quick questions to get some ideas: > > 1) If editing a model (passed an instance parameter to __init__), even > with a partial form, do you expect model.full_validate being called? > > 2) Are you OK with ripping save(commit=False) functionality to some > other method? (current functionality can stay with a deprecation > warning for example) > > 3) Would you want errors raised in model validation after it being > created by a form (commit=False) to appear on the form? > > on subject of inlines: > 1) Is it acceptable to create a model and not it's inlines in the > admin if the modelform validates but the inlines don't? > > 2) How can we limit people's validation code to avoid validating FKs > in inlines since users can (and should) create their own validation > logic on Models? Especially when we try and treat admin as "just a > cool app" and not something people should really design for. > > 3) How would you feel on creating an option to enable behavior (1) ) > and document that models with side effects in their save and delete > should really go for that? > > 4) Making 3) the default and enable switching it off if people want > their admin page to save atomically. > > Please let me know what you think > > Thanks! > > Honza Král > E-Mail: honza.k...@gmail.com > Phone: +420 606 678585 -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
Hi everybody, sorry for the late reply, was busy. Just to add few points that lead us to do it this way: ModelForm has a save() method that saves the model. It is reasonable to assume that if the form is valid, the save() method call should succeed, that's why the entire model is validated. We could create a PartialModelForm, that would have save() method only when editing (and not adding) models and have other method (or enforced commit=False) for retrieval of the partial model. This form would only call validation on the modelfields that are part of the form (not being excluded). This is the behavior I understand everybody expects from ModelForm, so, for backwards compatibility, we could call it just ModelForm. Also please mind that it could prove difficult to do half the validation in one place and other fields elsewhere without a form. Models, as opposed to forms, don't have a state in sense of validated/raw and they don't track changes to their fields' data. That's why validation is run every time and the results are not cached on the instance. Few quick questions to get some ideas: 1) If editing a model (passed an instance parameter to __init__), even with a partial form, do you expect model.full_validate being called? 2) Are you OK with ripping save(commit=False) functionality to some other method? (current functionality can stay with a deprecation warning for example) 3) Would you want errors raised in model validation after it being created by a form (commit=False) to appear on the form? on subject of inlines: 1) Is it acceptable to create a model and not it's inlines in the admin if the modelform validates but the inlines don't? 2) How can we limit people's validation code to avoid validating FKs in inlines since users can (and should) create their own validation logic on Models? Especially when we try and treat admin as "just a cool app" and not something people should really design for. 3) How would you feel on creating an option to enable behavior (1) ) and document that models with side effects in their save and delete should really go for that? 4) Making 3) the default and enable switching it off if people want their admin page to save atomically. Please let me know what you think Thanks! Honza Král E-Mail: honza.k...@gmail.com Phone: +420 606 678585 -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
It makes sense to me that the developer should first check that their form is valid and second check that their model object is valid when calling ModelForm.save(commit=False). This could be done by having the developer check the result of model.full_validate() before calling model.save(), or by having model objects returned by ModelForm.save (commit=False) automatically trigger model validation when model.save () is called, even if model.save() does not normally trigger model validation? Cheers. Tai. -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
On Jan 6, 2010, at 3:59 PM, Joseph Kocherhans wrote: > On Wed, Jan 6, 2010 at 4:46 PM, Jeremy Dunckwrote: >> On Wed, Jan 6, 2010 at 3:56 PM, Joseph Kocherhans >> wrote: >> ... > On Wed, Jan 6, 2010 at 12:49 PM, Simon Willison > wrote: >> ... >> form = SecretQuestionForm( {"secret_question":"foo", "answer":"bar"} ) >> if form.is_valid(): >>p = form.save(commit=False) >>p.user = request.user >>p.primary_contact = somecontact >>p.save() >> ... >>> I had another idea that I think might work out. What if >>> ModelForm.validate() only validated fields on the form, like it worked >>> before the merge, and ModelForm.save() would automatically validate >>> the excluded fields, raising ValidationError if there was a problem? >>> Validation for each field would only happen once, it would accommodate >>> the commit=False idiom, and it would still ensure that the model >>> itself is validated in common usage. >> >> Note that p in the example above is the unsaved model instance, not >> the ModelForm. So to fix the idiom, the excluded field validation >> would need to be done on Model.save, not ModelForm.save, right? > > Ugh. Yes it would. I did mean ModelForm.save(), but as you've pointed > out, that won't work (at least for the idiom). One of the early > decisions was that Model.save() would never trigger validation for > backwards compatibility purposes. Maybe something from the idea is > salvageable, but it won't work as I presented it. I think having the > model track which validators have been run and which haven't is a > non-starter. That's something Honza actively avoided in the design. Saw this after my e-mails. This does present an issue. For sake of backwards compatibility it would seem that the second step of validation could just be done by the developer? This is mostly to prevent double validation, but also maintain compatibility with the Django idiom. Brian Rosner http://oebfare.com http://twitter.com/brosner -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
On Jan 6, 3:57 pm, Brian Rosnerwrote: > Yeah, I think that must have been a typo in Joseph's mail. The way I read it > that the model knows what fields it has already validated and the call to a > Model.save would validate the rest. Actually, I just realized that Model.save doesn't do validation now does it? Brian Rosner http://oebfare.com http://twitter.com/brosner -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
On Wed, Jan 6, 2010 at 4:46 PM, Jeremy Dunckwrote: > On Wed, Jan 6, 2010 at 3:56 PM, Joseph Kocherhans > wrote: > ... On Wed, Jan 6, 2010 at 12:49 PM, Simon Willison wrote: > ... > form = SecretQuestionForm( {"secret_question":"foo", "answer":"bar"} ) > if form.is_valid(): > p = form.save(commit=False) > p.user = request.user > p.primary_contact = somecontact > p.save() > ... >> I had another idea that I think might work out. What if >> ModelForm.validate() only validated fields on the form, like it worked >> before the merge, and ModelForm.save() would automatically validate >> the excluded fields, raising ValidationError if there was a problem? >> Validation for each field would only happen once, it would accommodate >> the commit=False idiom, and it would still ensure that the model >> itself is validated in common usage. > > Note that p in the example above is the unsaved model instance, not > the ModelForm. So to fix the idiom, the excluded field validation > would need to be done on Model.save, not ModelForm.save, right? Ugh. Yes it would. I did mean ModelForm.save(), but as you've pointed out, that won't work (at least for the idiom). One of the early decisions was that Model.save() would never trigger validation for backwards compatibility purposes. Maybe something from the idea is salvageable, but it won't work as I presented it. I think having the model track which validators have been run and which haven't is a non-starter. That's something Honza actively avoided in the design. Joseph -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
On Jan 6, 2010, at 3:46 PM, Jeremy Dunck wrote: > On Wed, Jan 6, 2010 at 3:56 PM, Joseph Kocherhans> wrote: > ... On Wed, Jan 6, 2010 at 12:49 PM, Simon Willison wrote: > ... > form = SecretQuestionForm( {"secret_question":"foo", "answer":"bar"} ) > if form.is_valid(): >p = form.save(commit=False) >p.user = request.user >p.primary_contact = somecontact >p.save() > ... >> I had another idea that I think might work out. What if >> ModelForm.validate() only validated fields on the form, like it worked >> before the merge, and ModelForm.save() would automatically validate >> the excluded fields, raising ValidationError if there was a problem? >> Validation for each field would only happen once, it would accommodate >> the commit=False idiom, and it would still ensure that the model >> itself is validated in common usage. > > Note that p in the example above is the unsaved model instance, not > the ModelForm. So to fix the idiom, the excluded field validation > would need to be done on Model.save, not ModelForm.save, right? Yeah, I think that must have been a typo in Joseph's mail. The way I read it that the model knows what fields it has already validated and the call to a Model.save would validate the rest. Brian Rosner http://oebfare.com http://twitter.com/brosner -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
On Wed, Jan 6, 2010 at 3:56 PM, Joseph Kocherhanswrote: ... >>> On Wed, Jan 6, 2010 at 12:49 PM, Simon Willison >>> wrote: ... form = SecretQuestionForm( {"secret_question":"foo", "answer":"bar"} ) if form.is_valid(): p = form.save(commit=False) p.user = request.user p.primary_contact = somecontact p.save() ... > I had another idea that I think might work out. What if > ModelForm.validate() only validated fields on the form, like it worked > before the merge, and ModelForm.save() would automatically validate > the excluded fields, raising ValidationError if there was a problem? > Validation for each field would only happen once, it would accommodate > the commit=False idiom, and it would still ensure that the model > itself is validated in common usage. Note that p in the example above is the unsaved model instance, not the ModelForm. So to fix the idiom, the excluded field validation would need to be done on Model.save, not ModelForm.save, right? -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
On Jan 6, 2010, at 2:48 PM, Łukasz Rekucki wrote: > Maybe you could do something like this: > > with form.valid_model() as model: # i'm not good at inventing names > model.user = request.user > model.primary_contact = somecontact > > The valid_model() would be a context manager that does form validation and > form.save(commit=False) on enter + model validation and save() on exit. Of > course this will only work on Python 2.5+, so it's probably no good for > django 1.2. Just wanted to share an idea. FTR, this is a pretty neat idea. The naming is a bit off, but that can be worked out. Unfortunately, we couldn't much with it now, but I'd like to look at the possibility for 1.3. Thanks for sharing. Brian Rosner http://oebfare.com http://twitter.com/brosner -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
On Wed, Jan 6, 2010 at 4:06 PM, Brian Rosnerwrote: > > On Jan 6, 2010, at 2:56 PM, Joseph Kocherhans wrote: >> I had another idea that I think might work out. What if >> ModelForm.validate() only validated fields on the form, like it worked >> before the merge, and ModelForm.save() would automatically validate >> the excluded fields, raising ValidationError if there was a problem? >> Validation for each field would only happen once, it would accommodate >> the commit=False idiom, and it would still ensure that the model >> itself is validated in common usage. > > I like this. It would then provide far superior error messages in cases where > there was real developer error. > > Brian Rosner > http://oebfare.com > http://twitter.com/brosner > > > -- > You received this message because you are subscribed to the Google Groups > "Django developers" group. > To post to this group, send email to django-develop...@googlegroups.com. > To unsubscribe from this group, send email to > django-developers+unsubscr...@googlegroups.com. > For more options, visit this group at > http://groups.google.com/group/django-developers?hl=en. > > > > I agree with Brian. I also really like the context manager based API, so for 1.3 I think it would be nice to include something like that. Alex -- "I disapprove of what you say, but I will defend to the death your right to say it." -- Voltaire "The people's good is the highest law." -- Cicero "Code can always be simpler than you think, but never as simple as you want" -- Me -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
On Jan 6, 2010, at 2:56 PM, Joseph Kocherhans wrote: > I had another idea that I think might work out. What if > ModelForm.validate() only validated fields on the form, like it worked > before the merge, and ModelForm.save() would automatically validate > the excluded fields, raising ValidationError if there was a problem? > Validation for each field would only happen once, it would accommodate > the commit=False idiom, and it would still ensure that the model > itself is validated in common usage. I like this. It would then provide far superior error messages in cases where there was real developer error. Brian Rosner http://oebfare.com http://twitter.com/brosner -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
On Wed, Jan 6, 2010 at 3:26 PM, Waylan Limbergwrote: > I've only scanned the docs the other day and haven't actually used the > new model validation stuff, so my impressions may be a little off, > but... > > On Wed, Jan 6, 2010 at 2:54 PM, Joseph Kocherhans > wrote: >> On Wed, Jan 6, 2010 at 12:49 PM, Simon Willison >> wrote: >>> A couple of related tickets filed today about model validation: >>> >>> http://code.djangoproject.com/ticket/12513 >>> http://code.djangoproject.com/ticket/12521 >>> >>> The first one describes the issue best - the new model validation code >>> breaks the following common Django convention: >>> >>> form = SecretQuestionForm( {"secret_question":"foo", "answer":"bar"} ) >>> if form.is_valid(): >>> p = form.save(commit=False) >>> p.user = request.user >>> p.primary_contact = somecontact >>> p.save() >>> > > My initial reaction to this snippet was to wonder why the model was > not being validated after the additional data was added/altered. > Shouldn't you be doing: > > form = SecretQuestionForm( {"secret_question":"foo", "answer":"bar"} ) > if form.is_valid(): > p = form.save(commit=False) > p.user = request.user > p.primary_contact = somecontact > if p.full_validate(): # < note this line > p.save() > > [snip] There are a couple of problems with p.full_validate() there. First, it would run validation a second time. Honza went to a bunch of trouble in the design to make sure that each field would only need to be validated once. Second, p.full_validate() would raise ValidationErrors, not return True or False. >> Once again, that means ModelForm won't really validate your model, >> only your form. form.save() might still throw a database error just >> like before the merge. We can slap a big warning in the ModelForm docs >> though. > > Well, that's why I expected the extra validation check on the model > itself. I understand the desire to have the ModelForm actually > validate the model and work in one step, but sometimes we need the > other way too (as you acknowledge). > >> One (probably unhelpful) way to make ModelForm validate your whole >> model would be to add a Meta flag to ModelForm: >> >> class UserForm(ModelForm): >> class Meta: >> # Defaults to False >> validate_model = True > > Well, what if one view uses that ModelForm one way and another view > uses the same ModelForm the other way? What about > ``form.is_valid(validate_model=True)``? That's a possibility, but I think it suffers from the same problems that the Meta option does. It just pushes the decision closer to runtime than coding time. That's helpful in some cases, but it doesn't solve the main part of the problem for me, which is that I don't think model validation should be an opt-in-only thing. Maybe it needs to be for a couple of releases though. I had another idea that I think might work out. What if ModelForm.validate() only validated fields on the form, like it worked before the merge, and ModelForm.save() would automatically validate the excluded fields, raising ValidationError if there was a problem? Validation for each field would only happen once, it would accommodate the commit=False idiom, and it would still ensure that the model itself is validated in common usage. I think this *might* also lead to a workable solution for ticket #12507 [1], but I need to dig into the code a little more. Joseph [1] http://code.djangoproject.com/ticket/12507 -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
First time posting here, so please excuse me if my opinions aren't very useful and my bad English. 2010/1/6 Waylan Limberg> I've only scanned the docs the other day and haven't actually used the > new model validation stuff, so my impressions may be a little off, > but... > > On Wed, Jan 6, 2010 at 2:54 PM, Joseph Kocherhans > wrote: > > On Wed, Jan 6, 2010 at 12:49 PM, Simon Willison > wrote: > >> A couple of related tickets filed today about model validation: > >> > >> http://code.djangoproject.com/ticket/12513 > >> http://code.djangoproject.com/ticket/12521 > >> > >> The first one describes the issue best - the new model validation code > >> breaks the following common Django convention: > >> > >> form = SecretQuestionForm( {"secret_question":"foo", "answer":"bar"} ) > >> if form.is_valid(): > >>p = form.save(commit=False) > >>p.user = request.user > >>p.primary_contact = somecontact > >>p.save() > >> > > My initial reaction to this snippet was to wonder why the model was > not being validated after the additional data was added/altered. > Shouldn't you be doing: > >form = SecretQuestionForm( {"secret_question":"foo", "answer":"bar"} ) >if form.is_valid(): >p = form.save(commit=False) >p.user = request.user >p.primary_contact = somecontact > if p.full_validate():# < note this line >p.save() > Maybe you could do something like this: with form.valid_model() as model: # i'm not good at inventing names model.user = request.user model.primary_contact = somecontact The valid_model() would be a context manager that does form validation and form.save(commit=False) on enter + model validation and save() on exit. Of course this will only work on Python 2.5+, so it's probably no good for django 1.2. Just wanted to share an idea. > > [snip] > > > Once again, that means ModelForm won't really validate your model, > > only your form. form.save() might still throw a database error just > > like before the merge. We can slap a big warning in the ModelForm docs > > though. > > Well, that's why I expected the extra validation check on the model > itself. I understand the desire to have the ModelForm actually > validate the model and work in one step, but sometimes we need the > other way too (as you acknowledge). > > > One (probably unhelpful) way to make ModelForm validate your whole > > model would be to add a Meta flag to ModelForm: > > > >class UserForm(ModelForm): > >class Meta: > ># Defaults to False > >validate_model = True > > Well, what if one view uses that ModelForm one way and another view > uses the same ModelForm the other way? What about > ``form.is_valid(validate_model=True)``? > This seems like a good idea. > -- > > \X/ /-\ `/ |_ /-\ |\| > Waylan Limberg > > -- > You received this message because you are subscribed to the Google Groups > "Django developers" group. > To post to this group, send email to django-develop...@googlegroups.com. > To unsubscribe from this group, send email to > django-developers+unsubscr...@googlegroups.com > . > For more options, visit this group at > http://groups.google.com/group/django-developers?hl=en. > -- Łukasz Rekucki -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
I've only scanned the docs the other day and haven't actually used the new model validation stuff, so my impressions may be a little off, but... On Wed, Jan 6, 2010 at 2:54 PM, Joseph Kocherhanswrote: > On Wed, Jan 6, 2010 at 12:49 PM, Simon Willison > wrote: >> A couple of related tickets filed today about model validation: >> >> http://code.djangoproject.com/ticket/12513 >> http://code.djangoproject.com/ticket/12521 >> >> The first one describes the issue best - the new model validation code >> breaks the following common Django convention: >> >> form = SecretQuestionForm( {"secret_question":"foo", "answer":"bar"} ) >> if form.is_valid(): >> p = form.save(commit=False) >> p.user = request.user >> p.primary_contact = somecontact >> p.save() >> My initial reaction to this snippet was to wonder why the model was not being validated after the additional data was added/altered. Shouldn't you be doing: form = SecretQuestionForm( {"secret_question":"foo", "answer":"bar"} ) if form.is_valid(): p = form.save(commit=False) p.user = request.user p.primary_contact = somecontact if p.full_validate():# < note this line p.save() [snip] > Once again, that means ModelForm won't really validate your model, > only your form. form.save() might still throw a database error just > like before the merge. We can slap a big warning in the ModelForm docs > though. Well, that's why I expected the extra validation check on the model itself. I understand the desire to have the ModelForm actually validate the model and work in one step, but sometimes we need the other way too (as you acknowledge). > One (probably unhelpful) way to make ModelForm validate your whole > model would be to add a Meta flag to ModelForm: > > class UserForm(ModelForm): > class Meta: > # Defaults to False > validate_model = True Well, what if one view uses that ModelForm one way and another view uses the same ModelForm the other way? What about ``form.is_valid(validate_model=True)``? -- \X/ /-\ `/ |_ /-\ |\| Waylan Limberg -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Model validation incompatibility with existing Django idioms
On Wed, Jan 6, 2010 at 12:49 PM, Simon Willisonwrote: > A couple of related tickets filed today about model validation: > > http://code.djangoproject.com/ticket/12513 > http://code.djangoproject.com/ticket/12521 > > The first one describes the issue best - the new model validation code > breaks the following common Django convention: > > form = SecretQuestionForm( {"secret_question":"foo", "answer":"bar"} ) > if form.is_valid(): > p = form.save(commit=False) > p.user = request.user > p.primary_contact = somecontact > p.save() > > The problem is that is_valid() notices that some of the required > fields in SecretQuestionForm (a ModelForm) have not been provided, > even if those fields are excluded from validation using the excludes= > or fields= properties. The exception raised is a > UnresolvableValidationError. I'll start off with the reasoning behind the implementation, but I agree that the current implementation is going to bite too many people to just call the old implementation a bug. Form.is_valid() now triggers model validation, and the model isn't valid. It's missing a couple of required fields. Presenting those errors to the user filling out the form is unacceptable because there is nothing the user can do to correct the error, and the developer will never get a notification about a problem that can only be solved with code. > This definitely needs to be fixed as it presumably breaks backwards > compatibility with lots of existing code (it breaks a common > ModelAdmin subclass convention as well, see #12521). Can we just > change the is_valid() logic to ignore model validation errors raised > against fields which aren't part of the ModelForm, or is it more > complicated than that? It shouldn't be much more complicated than that. Model.full_validate() takes an exclude parameter that we can use to provide a list of fields that aren't on the form. Or we can catch those errors and just throw them away. However, this means that part of the problem that model-validation was meant to solve will no longer be solved. ModelForm.is_valid() will only mean that your *form* is valid, not your *model* (which is the pre-merge semantics anyhow). Once again, that means ModelForm won't really validate your model, only your form. form.save() might still throw a database error just like before the merge. We can slap a big warning in the ModelForm docs though. One (probably unhelpful) way to make ModelForm validate your whole model would be to add a Meta flag to ModelForm: class UserForm(ModelForm): class Meta: # Defaults to False validate_model = True That would make it easy to trigger model validation, but it doesn't really help with the convention you're talking about. I don't know. Do people think triggering model validation in a ModelForm is something they need to do in a practical sense? Or is validating your form enough? Joseph -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Model validation incompatibility with existing Django idioms
A couple of related tickets filed today about model validation: http://code.djangoproject.com/ticket/12513 http://code.djangoproject.com/ticket/12521 The first one describes the issue best - the new model validation code breaks the following common Django convention: form = SecretQuestionForm( {"secret_question":"foo", "answer":"bar"} ) if form.is_valid(): p = form.save(commit=False) p.user = request.user p.primary_contact = somecontact p.save() The problem is that is_valid() notices that some of the required fields in SecretQuestionForm (a ModelForm) have not been provided, even if those fields are excluded from validation using the excludes= or fields= properties. The exception raised is a UnresolvableValidationError. This definitely needs to be fixed as it presumably breaks backwards compatibility with lots of existing code (it breaks a common ModelAdmin subclass convention as well, see #12521). Can we just change the is_valid() logic to ignore model validation errors raised against fields which aren't part of the ModelForm, or is it more complicated than that? Cheers, Simon -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.