Re: "unique_together" only validated in modelform.is_valid() if ALL of the referenced fields (Ticket #13091)
Carl, Thanks for this professional reply. After rereading my post (immediately after submitting it), I realized that I was much more critical than I would normally think is fair, which is why I removed it. It's sometimes necessary, I think, to remind ourselves that most of us are volunteers here, including the core developers. I also have to say that you have made yourself available in a very generous manner of late; for that I am quite appreciative. Regards, Ed Gutierrez On Apr 28, 8:34 pm, Carl Meyer wrote: > Hi Eduardo, > > On 04/28/2011 06:36 PM, legutierr wrote: > > > This is extraordinarily discouraging. > > I can understand why. > > I've also spent a number of hours thinking about this, reviewing the > patch, considering alternatives, coming up with cases that might break, > etc. I'd like to set aside those sunk costs (which I don't think were > wasted in either case) and keep the focus on the best way to solve the > issue in Django moving forward - that's what I owe to the rest of the > core development team and to the community. > > > This is the second time that I > > have devoted tremendous energy to a patch, trying to coordinate with > > core developers, not doing any work until I get the green light from > > core developers regarding an implementation plan (trying to avoid this > > very same eventuality), only to be told, after working code + tests + > > docs have been attached to the ticket, after several iterations of > > feedback: nope, this is not the way that we want to do this policy- > > wise, there's this other approach we want to take, so never mind. > > I'm not certain what the other situation is that you're referring to, so > I can't speak to that. My observation has been that this isn't the > common experience (unfortunately, getting no attention to a bug/patch in > the first place has at times been a more common one, though that too is > getting better -- unreviewed bug count is currently zero!), but I'm > sorry you've experienced it, and I regret having contributed to it in > this case. I will certainly be more careful in the future about > expressing optimism that an approach might be workable, especially if > (as in this case) I have reservations about it from the start. > > > I can understand going through the bureaucratic rigmarole that comes > > with contributing to Django--in fact, I support it--but to go through > > all of the discussion, justification, and *time* required to get a > > simple bug fix checked in (no, this really *is* a bug--look, there are > > five other tickets filed. sure, let's analyze the problem from every > > angle. sure, I'll rewrite it so it matches exactly your > > specifications.), only to be told that someone who wasn't even > > involved in this ticket and discussion *at all* until now thinks it > > isn't worth it, makes a guy like me want to tear his hair out. You > > say that this is "in the best interests of Django", but you must know > > that Django will suffer if people like me stop wanting to contribute > > because of things like this. > > Indeed, and I hope that you don't lose interest in contributing. > > I don't think that the time spent discussing and analyzing this, even > writing and reviewing a patch, is wasted. From my perspective, it has > clearly revealed that the current approach of trying to do > partial-model-validation is broken in concept and not reliably fixable. > That's useful information, and moves us (has already moved us) towards a > better solution. > > I can't agree that this is a simple bug fix. The current behavior is > wrong in some cases. The behavior after this "fix" would still be wrong > in some cases, although fewer. A simple bug fix is one where the fix is > clear, obviously correct, and definitively solves the reported problem. > I don't think that describes this case. Model and form validation is a > complex area, and it's easy for seemingly small changes to have > unintended effects that cause more maintenance burdens down the road. > > > How often has it been the case that some really cool new feature gets > > delayed because the core developer who was working on it was unable to > > dedicate the time they thought they would be able to? Can I help move > > it along, can you work with me to write it? Why can't we check this > > one in, close two tickets (as well as satisfying three or four > > duplicates) and then move on to the more definitive fix? > > I'm committed to having these tickets closed one way or another before > Django 1.4 is released (and neither fix here would be a candidate for > backporting to a 1.3.X release anyway), so let's focus on making the > best fix we can. If the ideas we have in mind for that turn out to be > unworkable for some reason, I still think that the current patch would > be preferable to no change. > > Carl -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django
Re: "unique_together" only validated in modelform.is_valid() if ALL of the referenced fields (Ticket #13091)
Hi Lior, (moved from another thread) On Apr 29, 12:16 am, Lior Sion wrote: > I looked at the sample you wrote on the other thread (unique together > on username and date, and having a null username with a given date) of > when the old behavior is the right one and it didn't quite convince me > - I do believe that the right implementation would fail a case of > NULLed username and repeating dates, when a unique together exists. That would clearly violate the currently-established consistent semantic for modelform validation, which is that the modelform only validates the fields that are actually included in the form. Any fields that are not included in the form, it must be assumed that they might change before saving, and their current value is unreliable (it might just be the default value for a new object, or something else entirely, it's not necessarily a value that's actually intended to be validated or saved). In essence, what you are proposing is validation of "possibly-junk" data. Because of this, any fix has to be absolutely 100% sure that it will never generate a false positive on the unique_together check because of the non-included field's value, or else we're violating the documented expectation that non-included fields' values are not part of validation. The exceptions for default and blank in the current patch was my best attempt to achieve that, but I don't even think that's sufficient - really, modelform validation can't assume anything about the data on self.instance for fields that aren't in the form, regardless of whether the field is blank or has a default. This is why I'm saying that the current expected semantics, that a modelform can provide meaningful validation while ignoring some fields of the model, is inherently broken. No matter what you do with constraints that include some included and some excluded fields (whether they are unique_together or custom clean methods, as in Mikhail's example), you're doing the wrong thing - you can't validate the constraint because you can't use some of the data that's needed to validate it, but if you drop the constraint entirely (as we do now) you require the developer to re-validate everything themselves later (and probably stuff those later model-validation errors back into the form, too, and then break out of the is_valid clause because its no longer valid), which is really ugly and painful in custom code and currently not done at all in the admin. And that's why I think the only long-term solution, that isn't just going to cause more problems, is to begin moving towards a new expectation for validating modelforms, where its expected that full model validation is run, and any tweaks to the model must be made before validation rather than after. Carl -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@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: "unique_together" only validated in modelform.is_valid() if ALL of the referenced fields (Ticket #13091)
Hi Mikhail, On Apr 24, 7:46 am, Mikhail Korobov wrote: > The issue is not only with unique_together indeed. Please correct me if I'm > wrong, but it seems there is no way currently to use model validation with > fields dependent on each other when one of these fields is not POSTed > directly by user. Example: > > # models.py > class Ticket(models.Model): > created_by = models.ForeignKey(User, related_name='created_tickets') > responsible = models.ForeignKey(User, related_name='todo') > > def clean(): > # we don't want to allow tickets where created_by == responsible for > some reason > from django.core.exceptions import ValidationError > if self.created_by == self.responsible: > raise ValidationError('Responsible is incorrect.') > > # views.py > class TicketForm(forms.ModelForm): > class Meta: > model = Ticket > exclude = ['created_by'] > > @login_required > def add_ticket(request): > form = TicketForm(request.POST or None) > if form.is_valid(): > ticket = form.save(commit=False) > ticket.created_by = request.user > > # todo: handle ticket.full_clean() > > ticket.save() > return redirect('ticket_list') > return TemplateResponse(request, 'tickets/add.html', {'form': form}) > > Model.clean method is always called on form validation (unlike field > validators that are excluded if field is excluded from form). And we can't > write validator for 'responsible' field or for 'created_by' field (so that > it will be excluded from validation on form.is_valid) because this validator > won't have an access to other model fields. So the only option here seems to > move validation from model level to form level. > > Please consider this use case while rethinking model validation. The > proposed context manager seems to be a very good idea and I hope it will fix > this issue as well. Sorry, I missed this somehow earlier. I agree with you that this is another example of why attempting to do partial model validation is simply broken. I think the context manager proposal (which I've posted in a new thread) does fix this case as well, and I'd be happy for your comments on it. Carl -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@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: "unique_together" only validated in modelform.is_valid() if ALL of the referenced fields (Ticket #13091)
On 04/28/2011 07:12 PM, legutierr wrote: > I'm up for working on the new idiom now. I've put this much time into > it, I don't want to waste the momentum. What's the approach you are > thinking of, and how can I get started in the implementation? Much appreciated. I have a half-written post about exactly that to begin a new thread - should be up shortly! Carl -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@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: "unique_together" only validated in modelform.is_valid() if ALL of the referenced fields (Ticket #13091)
Hi Eduardo, On 04/28/2011 06:36 PM, legutierr wrote: > This is extraordinarily discouraging. I can understand why. I've also spent a number of hours thinking about this, reviewing the patch, considering alternatives, coming up with cases that might break, etc. I'd like to set aside those sunk costs (which I don't think were wasted in either case) and keep the focus on the best way to solve the issue in Django moving forward - that's what I owe to the rest of the core development team and to the community. > This is the second time that I > have devoted tremendous energy to a patch, trying to coordinate with > core developers, not doing any work until I get the green light from > core developers regarding an implementation plan (trying to avoid this > very same eventuality), only to be told, after working code + tests + > docs have been attached to the ticket, after several iterations of > feedback: nope, this is not the way that we want to do this policy- > wise, there's this other approach we want to take, so never mind. I'm not certain what the other situation is that you're referring to, so I can't speak to that. My observation has been that this isn't the common experience (unfortunately, getting no attention to a bug/patch in the first place has at times been a more common one, though that too is getting better -- unreviewed bug count is currently zero!), but I'm sorry you've experienced it, and I regret having contributed to it in this case. I will certainly be more careful in the future about expressing optimism that an approach might be workable, especially if (as in this case) I have reservations about it from the start. > I can understand going through the bureaucratic rigmarole that comes > with contributing to Django--in fact, I support it--but to go through > all of the discussion, justification, and *time* required to get a > simple bug fix checked in (no, this really *is* a bug--look, there are > five other tickets filed. sure, let's analyze the problem from every > angle. sure, I'll rewrite it so it matches exactly your > specifications.), only to be told that someone who wasn't even > involved in this ticket and discussion *at all* until now thinks it > isn't worth it, makes a guy like me want to tear his hair out. You > say that this is "in the best interests of Django", but you must know > that Django will suffer if people like me stop wanting to contribute > because of things like this. Indeed, and I hope that you don't lose interest in contributing. I don't think that the time spent discussing and analyzing this, even writing and reviewing a patch, is wasted. From my perspective, it has clearly revealed that the current approach of trying to do partial-model-validation is broken in concept and not reliably fixable. That's useful information, and moves us (has already moved us) towards a better solution. I can't agree that this is a simple bug fix. The current behavior is wrong in some cases. The behavior after this "fix" would still be wrong in some cases, although fewer. A simple bug fix is one where the fix is clear, obviously correct, and definitively solves the reported problem. I don't think that describes this case. Model and form validation is a complex area, and it's easy for seemingly small changes to have unintended effects that cause more maintenance burdens down the road. > How often has it been the case that some really cool new feature gets > delayed because the core developer who was working on it was unable to > dedicate the time they thought they would be able to? Can I help move > it along, can you work with me to write it? Why can't we check this > one in, close two tickets (as well as satisfying three or four > duplicates) and then move on to the more definitive fix? I'm committed to having these tickets closed one way or another before Django 1.4 is released (and neither fix here would be a candidate for backporting to a 1.3.X release anyway), so let's focus on making the best fix we can. If the ideas we have in mind for that turn out to be unworkable for some reason, I still think that the current patch would be preferable to no change. Carl -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@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: "unique_together" only validated in modelform.is_valid() if ALL of the referenced fields (Ticket #13091)
I'm up for working on the new idiom now. I've put this much time into it, I don't want to waste the momentum. What's the approach you are thinking of, and how can I get started in the implementation? On Apr 28, 4:28 pm, Carl Meyer wrote: > Hi Eduardo, > > On 04/28/2011 12:35 PM, legutierr wrote: > > > I just added a new patch in response to Carl's comments on the ticket. > > >http://code.djangoproject.com/ticket/13091 > > So, in the process of reviewing and tweaking this patch for commit, I > checked in the #django-dev IRC channel for any other core dev opinionsm > since I didn't feel 100% confident that we were doing the right thing > here. I talked through the issue with Alex Gaynor, and he successfully > convinced me that we aren't. Specifically: > > 1. We have an idea in mind, as I mentioned above, for a new > modelform-validation idiom that would solve this problem fully, by > requiring tweaks to the model to happen pre-validation and then > validating the full model. > > 2. If we implement the new idiom, and convert the admin to use it, then > anyone who runs into the problems with the current partial-validation > scheme in their own code can simply switch to the new recommended idiom. > Nobody will be stuck. > > 3. The current proposed patch on #13091 only improves the current > situation very marginally; there are a lot of cases it still wouldn't > catch (anytime a field involved in a unique-together is modified > post-validation and pre-save, and the odd exclusions for default/blank > fields). It's very much an incomplete fix, and yet it introduces new > complexity both to the code and the documentation, when we already have > a better alternative fix in mind. > > So I apologize for leading you to spend time on that patch and then > switching gears. In terms of what's best for Django, I think Alex is > right on this one, so I plan to just work on the better fix rather than > committing the current patch on #13091. > > Carl -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@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: "unique_together" only validated in modelform.is_valid() if ALL of the referenced fields (Ticket #13091)
This is extraordinarily discouraging. This is the second time that I have devoted tremendous energy to a patch, trying to coordinate with core developers, not doing any work until I get the green light from core developers regarding an implementation plan (trying to avoid this very same eventuality), only to be told, after working code + tests + docs have been attached to the ticket, after several iterations of feedback: nope, this is not the way that we want to do this policy- wise, there's this other approach we want to take, so never mind. I can understand going through the bureaucratic rigmarole that comes with contributing to Django--in fact, I support it--but to go through all of the discussion, justification, and *time* required to get a simple bug fix checked in (no, this really *is* a bug--look, there are five other tickets filed. sure, let's analyze the problem from every angle. sure, I'll rewrite it so it matches exactly your specifications.), only to be told that someone who wasn't even involved in this ticket and discussion *at all* until now thinks it isn't worth it, makes a guy like me want to tear his hair out. You say that this is "in the best interests of Django", but you must know that Django will suffer if people like me stop wanting to contribute because of things like this. I accept your apology, truly, but can I ask something else of you? Don't treat your contributors' time as something so easy to throw away. You and I (and others) have been talking about this for days. I have spent at least a day coding and analyzing. When the whole thing gets thrown away because of an IRC discussion, I don't know what to think. Carl, I asked you specifically in this very thread whether your new idiom would prevent this fix from getting checked in, before I started coding, and you answered that you "didn't think so". Should I have read that differently, more in the negative than in the affirmative? Perhaps, and I certainly will ask for clarification from you in the future. But you also gave me the green light to go ahead an write the patch in a manner that we discussed. Even if sometimes in the final event your original decision to pursue a certain path might prove not to be optimal, can I ask you to stick with it, so that people like me don't have to worry about the rug being pulled out from underneath us at the last moment? To place this in perspective: this is a *small* bug fix that adds six (6) lines of code to the file and prevents exceptions being raised to the user in the admin (and elsewhere). The "problematic" special case here is a rare edge case that, conceivably, could even be left out from the implementation. And yet, as someone who is still trying to figure out what kind of effort to put into contribute to Django, for me it is seriously discouraging for the work to be discarded. > In terms of what's best for Django, I think Alex is right on this one, so I > plan to just work on the better fix How often has it been the case that some really cool new feature gets delayed because the core developer who was working on it was unable to dedicate the time they thought they would be able to? Can I help move it along, can you work with me to write it? Why can't we check this one in, close two tickets (as well as satisfying three or four duplicates) and then move on to the more definitive fix? Regards, Eduardo PS I wrote much of this in anger, so it is rather more critical than anything I would normally post, but I think that it needs to be said. On Apr 28, 4:28 pm, Carl Meyer wrote: > Hi Eduardo, > > On 04/28/2011 12:35 PM, legutierr wrote: > > > I just added a new patch in response to Carl's comments on the ticket. > > >http://code.djangoproject.com/ticket/13091 > > So, in the process of reviewing and tweaking this patch for commit, I > checked in the #django-dev IRC channel for any other core dev opinionsm > since I didn't feel 100% confident that we were doing the right thing > here. I talked through the issue with Alex Gaynor, and he successfully > convinced me that we aren't. Specifically: > > 1. We have an idea in mind, as I mentioned above, for a new > modelform-validation idiom that would solve this problem fully, by > requiring tweaks to the model to happen pre-validation and then > validating the full model. > > 2. If we implement the new idiom, and convert the admin to use it, then > anyone who runs into the problems with the current partial-validation > scheme in their own code can simply switch to the new recommended idiom. > Nobody will be stuck. > > 3. The current proposed patch on #13091 only improves the current > situation very marginally; there are a lot of cases it still wouldn't > catch (anytime a field involved in a unique-together is modified > post-validation and pre-save, and the odd exclusions for default/blank > fields). It's very much an incomplete fix, and yet it introduces new > complexity both to the code and the documentation, when we alr
Re: "unique_together" only validated in modelform.is_valid() if ALL of the referenced fields (Ticket #13091)
Hi Eduardo, On 04/28/2011 12:35 PM, legutierr wrote: > I just added a new patch in response to Carl's comments on the ticket. > > http://code.djangoproject.com/ticket/13091 So, in the process of reviewing and tweaking this patch for commit, I checked in the #django-dev IRC channel for any other core dev opinionsm since I didn't feel 100% confident that we were doing the right thing here. I talked through the issue with Alex Gaynor, and he successfully convinced me that we aren't. Specifically: 1. We have an idea in mind, as I mentioned above, for a new modelform-validation idiom that would solve this problem fully, by requiring tweaks to the model to happen pre-validation and then validating the full model. 2. If we implement the new idiom, and convert the admin to use it, then anyone who runs into the problems with the current partial-validation scheme in their own code can simply switch to the new recommended idiom. Nobody will be stuck. 3. The current proposed patch on #13091 only improves the current situation very marginally; there are a lot of cases it still wouldn't catch (anytime a field involved in a unique-together is modified post-validation and pre-save, and the odd exclusions for default/blank fields). It's very much an incomplete fix, and yet it introduces new complexity both to the code and the documentation, when we already have a better alternative fix in mind. So I apologize for leading you to spend time on that patch and then switching gears. In terms of what's best for Django, I think Alex is right on this one, so I plan to just work on the better fix rather than committing the current patch on #13091. Carl -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@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: "unique_together" only validated in modelform.is_valid() if ALL of the referenced fields (Ticket #13091)
I just added a new patch in response to Carl's comments on the ticket. http://code.djangoproject.com/ticket/13091 Regards, Eduardo -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@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: "unique_together" only validated in modelform.is_valid() if ALL of the referenced fields (Ticket #13091)
I didn't link to where I uploded the patch. It is here: http://code.djangoproject.com/ticket/13091 On Apr 28, 1:43 am, legutierr wrote: > OK, I just uploaded a patch against trunk that should be consistent > with this discussion. As I note in the ticket, I kept the tests from > the prior patch, which tests were specifically relevant to the admin > as reported by the original ticket, but I also added additional, more > focused tests. I also had to modify one of the model_formsets tests, > as it was assuming the old behavior. > > I ran my changes against the whole of the Django test suite, and no > relevant errors seem to have been generated. > > I hope this is OK to check in :) > > Regards, > Eduardo > > On Apr 27, 3:13 pm, Carl Meyer wrote: > > > > > > > > > On 04/27/2011 02:02 PM, legutierr wrote: > > > > Ok, I'll create a patch soon (with tests + documentation) that > > > hopefully works for you. I don't think it will be very complicated > > > implementation-wise, just a few additional lines, I think. With > > > regards to the documentation, I'll add a note here: > > > >http://docs.djangoproject.com/en/1.3/ref/models/options/#unique-together > > > > and here: > > > >http://docs.djangoproject.com/en/1.3/ref/models/instances/#django.db > > > > Including a note saying that the behavior has changed > > > Great, thanks. I think this behavior change only needs to be described > > in one place (the validate_unique docs), but the text at the former link > > is actually inaccurate ever since model validation - it should be > > updated to mention that unique_together is also checked by model > > validation, with a link to the validate_unique docs. > > > Carl -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@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: "unique_together" only validated in modelform.is_valid() if ALL of the referenced fields (Ticket #13091)
OK, I just uploaded a patch against trunk that should be consistent with this discussion. As I note in the ticket, I kept the tests from the prior patch, which tests were specifically relevant to the admin as reported by the original ticket, but I also added additional, more focused tests. I also had to modify one of the model_formsets tests, as it was assuming the old behavior. I ran my changes against the whole of the Django test suite, and no relevant errors seem to have been generated. I hope this is OK to check in :) Regards, Eduardo On Apr 27, 3:13 pm, Carl Meyer wrote: > On 04/27/2011 02:02 PM, legutierr wrote: > > > Ok, I'll create a patch soon (with tests + documentation) that > > hopefully works for you. I don't think it will be very complicated > > implementation-wise, just a few additional lines, I think. With > > regards to the documentation, I'll add a note here: > > >http://docs.djangoproject.com/en/1.3/ref/models/options/#unique-together > > > and here: > > >http://docs.djangoproject.com/en/1.3/ref/models/instances/#django.db > > > Including a note saying that the behavior has changed > > Great, thanks. I think this behavior change only needs to be described > in one place (the validate_unique docs), but the text at the former link > is actually inaccurate ever since model validation - it should be > updated to mention that unique_together is also checked by model > validation, with a link to the validate_unique docs. > > Carl -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@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: "unique_together" only validated in modelform.is_valid() if ALL of the referenced fields (Ticket #13091)
On 04/27/2011 02:02 PM, legutierr wrote: > Ok, I'll create a patch soon (with tests + documentation) that > hopefully works for you. I don't think it will be very complicated > implementation-wise, just a few additional lines, I think. With > regards to the documentation, I'll add a note here: > > http://docs.djangoproject.com/en/1.3/ref/models/options/#unique-together > > and here: > > http://docs.djangoproject.com/en/1.3/ref/models/instances/#django.db.models.Model.validate_unique > > Including a note saying that the behavior has changed Great, thanks. I think this behavior change only needs to be described in one place (the validate_unique docs), but the text at the former link is actually inaccurate ever since model validation - it should be updated to mention that unique_together is also checked by model validation, with a link to the validate_unique docs. Carl -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@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: "unique_together" only validated in modelform.is_valid() if ALL of the referenced fields (Ticket #13091)
Hi Carl- > Hmm, that's interesting. I'm not super-enthused about the complexity > there (Zen of Python: "if the implementation is hard to explain, it's a > bad idea"), but I think you're right that it's feasible. Note that > nullable fields would be ok to go ahead with (because NULL is not equal > to NULL in SQL, it won't cause false positives on the uniqueness check); > it's just fields with non-null defaults that could cause the false > positive if they are excluded from the form but included in a > unique-together check. > > If the implementation (and documentation) for that patch doesn't look > too terrible, I'd consider it - I do think it gets the behavior closer > to right than what we do now, and I'm not sure it's really possible to > get it fully right in all cases as long as we're trying to do validation > on a partial model. I'd be interested in others' thoughts, of course. Ok, I'll create a patch soon (with tests + documentation) that hopefully works for you. I don't think it will be very complicated implementation-wise, just a few additional lines, I think. With regards to the documentation, I'll add a note here: http://docs.djangoproject.com/en/1.3/ref/models/options/#unique-together and here: http://docs.djangoproject.com/en/1.3/ref/models/instances/#django.db.models.Model.validate_unique Including a note saying that the behavior has changed Regards, Eduardo -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@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: "unique_together" only validated in modelform.is_valid() if ALL of the referenced fields (Ticket #13091)
Hi Eduardo, On 04/26/2011 02:47 PM, legutierr wrote: >> With your proposed change, if I happen to have a FavoriteBirthdayPresent >> in the database for a given year with an empty "username" field, it >> would incorrectly prevent any user from creating their own >> FavoriteBirthdayPresent for that year. >> >> I'll readily admit that's a corner case that requires several >> perhaps-unusual conditions: >> - the excluded field that's part of a unique_together has a default >> value which is also a valid value in the database, and is not None/NULL >> (because NULL != NULL in SQL), and >> - there actually might be a record in the database with that default value. >> >> And I think there are probably many more cases where your proposed >> behavior would be correct. I'd just be happier marking #13091 Accepted >> if I could see a solution that seemed more clearly correct in all cases. > > Regarding this, I have two somewhat contradictory responses: > > 1) It would be feasible to treat the case where a default value is > defined on a field (or where the field is allowed to be null) as being > distinct from the case where the default value is not defined and the > field is not permitted to be null. In other words, in the case that > you cite the current behavior could be maintained (unique_together > tuples containing any field with a default value would be ignored in > model validation), while my proposed behavior would only be > implemented when model validation is certain not to create the > circumstances you describe. I would be happy to write a patch for > this + tests if you are OK with the approach. Hmm, that's interesting. I'm not super-enthused about the complexity there (Zen of Python: "if the implementation is hard to explain, it's a bad idea"), but I think you're right that it's feasible. Note that nullable fields would be ok to go ahead with (because NULL is not equal to NULL in SQL, it won't cause false positives on the uniqueness check); it's just fields with non-null defaults that could cause the false positive if they are excluded from the form but included in a unique-together check. If the implementation (and documentation) for that patch doesn't look too terrible, I'd consider it - I do think it gets the behavior closer to right than what we do now, and I'm not sure it's really possible to get it fully right in all cases as long as we're trying to do validation on a partial model. I'd be interested in others' thoughts, of course. > 2) I am not sure, however, that the case you site is really a > problem. So what if the user is told that the "year" data they have > supplied in such a case is not "sufficiently unique"? It would be > true (would it not?) that the default "username" would already have > their favorite birthday present assigned for that year (even if the > default is null), and it seems to me that such a fact is intelligible > to the user (The error message could read: "The data you supplied for > field 'year' is not sufficiently unique for username 'default'," or > perhaps simply "The 'year' you specified is not sufficiently > unique."). That error message is not intelligible to the user, because they aren't trying to save data for the user "default" at all, they are trying to save data for their own user (and they can't change the user, because its excluded from the form and assigned in the view code after validation). The error is wrong because its checking uniqueness for user "default" when it ought to be checking for user "janedoe", and it prevents Jane from saving data for herself for any year that "default" has data for. It would also be fully within the power of the user to > modify the form in order to get it to pass model validation and be > saved. Only if they give up on saving any data for themselves for that year. >> This is really giving me the itch to build a new context-manager-based >> idiom for ModelForm validation in 1.4 that would allow modification of >> the to-be-saved object within the context manager and always perform >> full validation of the model on the way out. This idea was raised >> briefly in discussions right around the time model-validation landed, >> but was tabled due to the need (at that point) to support Python 2.4. >> Seems like that could neatly resolve all these knotted issues around >> validation of partial ModelForms. > > I am sure that whatever idiom you create will be an improvement over > the current approach, but unfortunately, I think that what you are > describing is a little over my head. Regardless, I hope that the > prospect of introducing this new idiom will not prevent #12082 and > #13091 from being resolved without the acceptance of a new approach. I don't think it needs to. > While we are on the subject of new idioms, I am curious as to what > might be wrong with this slight amendment to the current idiom: > > form = ModelForm(data) > form.instance.excluded_field = programatic_data > if form.is_valid(): > fo
Re: "unique_together" only validated in modelform.is_valid() if ALL of the referenced fields (Ticket #13091)
Hi Carl- > With your proposed change, if I happen to have a FavoriteBirthdayPresent > in the database for a given year with an empty "username" field, it > would incorrectly prevent any user from creating their own > FavoriteBirthdayPresent for that year. > > I'll readily admit that's a corner case that requires several > perhaps-unusual conditions: > - the excluded field that's part of a unique_together has a default > value which is also a valid value in the database, and is not None/NULL > (because NULL != NULL in SQL), and > - there actually might be a record in the database with that default value. > > And I think there are probably many more cases where your proposed > behavior would be correct. I'd just be happier marking #13091 Accepted > if I could see a solution that seemed more clearly correct in all cases. Regarding this, I have two somewhat contradictory responses: 1) It would be feasible to treat the case where a default value is defined on a field (or where the field is allowed to be null) as being distinct from the case where the default value is not defined and the field is not permitted to be null. In other words, in the case that you cite the current behavior could be maintained (unique_together tuples containing any field with a default value would be ignored in model validation), while my proposed behavior would only be implemented when model validation is certain not to create the circumstances you describe. I would be happy to write a patch for this + tests if you are OK with the approach. 2) I am not sure, however, that the case you site is really a problem. So what if the user is told that the "year" data they have supplied in such a case is not "sufficiently unique"? It would be true (would it not?) that the default "username" would already have their favorite birthday present assigned for that year (even if the default is null), and it seems to me that such a fact is intelligible to the user (The error message could read: "The data you supplied for field 'year' is not sufficiently unique for username 'default'," or perhaps simply "The 'year' you specified is not sufficiently unique."). It would also be fully within the power of the user to modify the form in order to get it to pass model validation and be saved. Maybe this is a fine distinction, but I think that saying that a field or set of fields is not "sufficiently unique" is perfectly sufficient from a usability point of view. It is a lot better than raising an exception after saving, and if the error message isn't good enough in any circumstance, it would be as easy as it is now to override it programmatically. > This is really giving me the itch to build a new context-manager-based > idiom for ModelForm validation in 1.4 that would allow modification of > the to-be-saved object within the context manager and always perform > full validation of the model on the way out. This idea was raised > briefly in discussions right around the time model-validation landed, > but was tabled due to the need (at that point) to support Python 2.4. > Seems like that could neatly resolve all these knotted issues around > validation of partial ModelForms. I am sure that whatever idiom you create will be an improvement over the current approach, but unfortunately, I think that what you are describing is a little over my head. Regardless, I hope that the prospect of introducing this new idiom will not prevent #12082 and #13091 from being resolved without the acceptance of a new approach. While we are on the subject of new idioms, I am curious as to what might be wrong with this slight amendment to the current idiom: form = ModelForm(data) form.instance.excluded_field = programatic_data if form.is_valid(): form.save() Is there some reason why programatic data needs to be assigned to the form instance *after* validation takes place? Is there something about the instance that is returned by form.save(commit=False) that distinguishes it from form.instance in such a way that it would break form.is_valid() or form.save()? Regards, Eduardo -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@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: "unique_together" only validated in modelform.is_valid() if ALL of the referenced fields (Ticket #13091)
Hi Carl, The issue is not only with unique_together indeed. Please correct me if I'm wrong, but it seems there is no way currently to use model validation with fields dependent on each other when one of these fields is not POSTed directly by user. Example: # models.py class Ticket(models.Model): created_by = models.ForeignKey(User, related_name='created_tickets') responsible = models.ForeignKey(User, related_name='todo') def clean(): # we don't want to allow tickets where created_by == responsible for some reason from django.core.exceptions import ValidationError if self.created_by == self.responsible: raise ValidationError('Responsible is incorrect.') # views.py class TicketForm(forms.ModelForm): class Meta: model = Ticket exclude = ['created_by'] @login_required def add_ticket(request): form = TicketForm(request.POST or None) if form.is_valid(): ticket = form.save(commit=False) ticket.created_by = request.user # todo: handle ticket.full_clean() ticket.save() return redirect('ticket_list') return TemplateResponse(request, 'tickets/add.html', {'form': form}) Model.clean method is always called on form validation (unlike field validators that are excluded if field is excluded from form). And we can't write validator for 'responsible' field or for 'created_by' field (so that it will be excluded from validation on form.is_valid) because this validator won't have an access to other model fields. So the only option here seems to move validation from model level to form level. Please consider this use case while rethinking model validation. The proposed context manager seems to be a very good idea and I hope it will fix this issue as well. -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@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: "unique_together" only validated in modelform.is_valid() if ALL of the referenced fields (Ticket #13091)
On Apr 23, 7:12 pm, Florian Apolloner wrote: > > In any case, if we have this, I could see switching the admin to use it, > > and perhaps adding an overridable method that's called from within the > > context manager, to allow you to complete/tweak the model instance > > before full validation is run. Seems like this would solve your problem? > > Hell yeah, that would solve all of my problems :þ At least if I can defer validation of some fields on the form to the end (in my case the user) -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@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: "unique_together" only validated in modelform.is_valid() if ALL of the referenced fields (Ticket #13091)
Hey Carl, On Apr 23, 5:55 pm, Carl Meyer wrote: > http://groups.google.com/group/django-developers/msg/3014f29c5125653ais > where it was briefly mentioned by Lukasz, I haven't seen any discussion > since. Thx > In any case, if we have this, I could see switching the admin to use it, > and perhaps adding an overridable method that's called from within the > context manager, to allow you to complete/tweak the model instance > before full validation is run. Seems like this would solve your problem? Hell yeah, that would solve all of my problems :þ Regards, Florian -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@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: "unique_together" only validated in modelform.is_valid() if ALL of the referenced fields (Ticket #13091)
Hi Florian, On 04/23/2011 08:18 AM, Florian Apolloner wrote: > Nice, something like that would be great. One of my bigger problems > regarding modelforms lately was that I wanted something like: "If you > don't fill out the user it's set to the current user", in the admin, > with as little modifications as possible. So save_model sounded like > the right spot but modelvalidation will make the form invalid (I want > the user field on the form…). Still no idea on how to solve that > properly and nicely; if the context managers can take care of that too > (though I doubt it since I can't change the is_valid call in the > admin) I am all in for it. Btw any threads around that subject I could > read? http://groups.google.com/group/django-developers/msg/3014f29c5125653a is where it was briefly mentioned by Lukasz, I haven't seen any discussion since. As Joseph says later in that thread, two design constraints of model validation were to avoid double-validation and avoid storing metadata on the model about which validations have already been run. I think a context manager might be able to avoid that problem by storing data about which validations have been run temporarily on the context manager itself, rather than on the model. This might allow form-validation on the way in, full model validation on the way out. If there are problems with that, it might work to just do no validation on the way in and full model validation on the way out, though it'd be nicer if the code inside the context manager could rely on at least validation of the fields included in the form. In any case, if we have this, I could see switching the admin to use it, and perhaps adding an overridable method that's called from within the context manager, to allow you to complete/tweak the model instance before full validation is run. Seems like this would solve your problem? Carl -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@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: "unique_together" only validated in modelform.is_valid() if ALL of the referenced fields (Ticket #13091)
On Apr 23, 7:23 am, Carl Meyer wrote: > This is really giving me the itch to build a new context-manager-based > idiom for ModelForm validation in 1.4 that would allow modification of > the to-be-saved object within the context manager and always perform > full validation of the model on the way out. This idea was raised > briefly in discussions right around the time model-validation landed, > but was tabled due to the need (at that point) to support Python 2.4. > Seems like that could neatly resolve all these knotted issues around > validation of partial ModelForms. Nice, something like that would be great. One of my bigger problems regarding modelforms lately was that I wanted something like: "If you don't fill out the user it's set to the current user", in the admin, with as little modifications as possible. So save_model sounded like the right spot but modelvalidation will make the form invalid (I want the user field on the form…). Still no idea on how to solve that properly and nicely; if the context managers can take care of that too (though I doubt it since I can't change the is_valid call in the admin) I am all in for it. Btw any threads around that subject I could read? Regards, Florian -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@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: "unique_together" only validated in modelform.is_valid() if ALL of the referenced fields (Ticket #13091)
On 04/22/2011 02:42 AM, legutierr wrote: > However, in the case of a tuple of fields that are "unique together", > the proper behavior should be that if *any* of those fields are > editable in the form, the constraint should be validated by > is_valid(). In the current implementation, *all* of the unique- > together fields have to be editable for the constraint to be > validated; THAT is the real bug here. It is always fully within the > power of a user to resolve a "unique together" constraint violation > simply by modifying any one of the fields subject to that constraint; > the only thing required is that the user be told which editable > field(s) are causing the constraint violation. Furthermore, such > violations are not uncommon, and should not require the intervention > of the developer. > > Note that this is not just a problem for me, but is also a problem at > the root of several tickets that have already been accepted, several > over a year old. These two open tickets pertain to admin and content > types, and have the same root cause as I am describing above: > > http://code.djangoproject.com/ticket/12028 > http://code.djangoproject.com/ticket/13091 > > also: > > http://code.djangoproject.com/ticket/13249 > http://code.djangoproject.com/ticket/15326 Having reviewed all these related tickets, I do think that in _many_ cases it would be useful and expected to have ModelForm validate a unique_together constraint if any of the unique_together fields are included in the ModelForm, as you're proposing. I don't think it's cut-and-dried, though -- I can imagine cases where the new behavior would be wrong. For instance, this model: class FavoriteBirthdayPresent(models.Model): username = models.CharField(max_length=30) year = models.IntegerField() description = models.CharField(max_length=200) class Meta: unique_together = [["username", "year"]] and this ModelForm: class FavoriteBirthdayPresentForm(forms.ModelForm): class Meta: model = FavoriteBirthdayPresent exclude = ["username"] and this view code: if form.is_valid(): fave = form.save(commit=False) fave.username = request.user.username fave.save() With your proposed change, if I happen to have a FavoriteBirthdayPresent in the database for a given year with an empty "username" field, it would incorrectly prevent any user from creating their own FavoriteBirthdayPresent for that year. I'll readily admit that's a corner case that requires several perhaps-unusual conditions: - the excluded field that's part of a unique_together has a default value which is also a valid value in the database, and is not None/NULL (because NULL != NULL in SQL), and - there actually might be a record in the database with that default value. And I think there are probably many more cases where your proposed behavior would be correct. I'd just be happier marking #13091 Accepted if I could see a solution that seemed more clearly correct in all cases. This is really giving me the itch to build a new context-manager-based idiom for ModelForm validation in 1.4 that would allow modification of the to-be-saved object within the context manager and always perform full validation of the model on the way out. This idea was raised briefly in discussions right around the time model-validation landed, but was tabled due to the need (at that point) to support Python 2.4. Seems like that could neatly resolve all these knotted issues around validation of partial ModelForms. > Something to take note of regarding #13091, which seems to be the > canonical ticket: the current patch attached to this ticket would > eliminate *all* exclusions from the call to the validate_unique() > model validations. This I now disagree with (as I am sure you do, > too), although I originally proposed doing just that. I also think > that in the case of a "unique together" tuple where *none* of the > fields are editable, model validation should be skipped inside > is_valid(). However, I do think that a modification is warranted to > resolve the underlying error of the above-listed tickets, as well as > my own. Yes, I agree that the current patch on #13091 is too aggressive. Carl -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@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.
"unique_together" only validated in modelform.is_valid() if ALL of the referenced fields (Ticket #13091)
Thanks for your response, Florian. > > modelform.is_valid() fails to anticipate database integrity errors > > when those errors involve any fields that are not part of that form > > itself. > > That is wanted behaviour, eg consider my workflow: > > class SomeForm(ModelForm): > class Meta: > exclude = ['user'] > model = … > > Now in the view I do this: > if form.is_valid(): > instance = form.save(commit=False) > instance.user = request.user After reading the thread that you referenced, I agree with you that it would be *incorrect* to individually validate fields that are excluded from the form in the `is_valid()` model validation, especially in light of this common idiom (which I do use myself). I think that I described my problem too generically above; nonetheless, I do believe that there is *still* something wrong with the current implementation. I have modified the subject of this thread to reflect real source of my complaint, which is described in the last paragraph of my original ticket: > For me, this is a problem in the case of "unique_together" fields, > where one field is editable on the form, and the other is set at > record creation time or in some other programmatic way. It is > possible, even likely, that a uniqueness constraint will be violated > by a user changing the editable field, causing in the current > implementation an IntegrityError to rise to the top of the stack, > directly impacting the user. Instead, the user should be told that the > data they entered is not sufficiently unique. If I understood the gist of the model-validation thread you referenced, it is (1) that users of a form should not be presented with a validation error that they are unable to fix by modifying submission data, and (2) that developers should get directly involved if there is any error being generated at form submission time that is out of the control of the user. In other words, it is a GOOD thing for IntegrityError to be raised if instance data that is excluded from the form violates a constraint. I think that the argument is convincing, and I agree with all of these sentiments. However, in the case of a tuple of fields that are "unique together", the proper behavior should be that if *any* of those fields are editable in the form, the constraint should be validated by is_valid(). In the current implementation, *all* of the unique- together fields have to be editable for the constraint to be validated; THAT is the real bug here. It is always fully within the power of a user to resolve a "unique together" constraint violation simply by modifying any one of the fields subject to that constraint; the only thing required is that the user be told which editable field(s) are causing the constraint violation. Furthermore, such violations are not uncommon, and should not require the intervention of the developer. Note that this is not just a problem for me, but is also a problem at the root of several tickets that have already been accepted, several over a year old. These two open tickets pertain to admin and content types, and have the same root cause as I am describing above: http://code.djangoproject.com/ticket/12028 http://code.djangoproject.com/ticket/13091 also: http://code.djangoproject.com/ticket/13249 http://code.djangoproject.com/ticket/15326 Something to take note of regarding #13091, which seems to be the canonical ticket: the current patch attached to this ticket would eliminate *all* exclusions from the call to the validate_unique() model validations. This I now disagree with (as I am sure you do, too), although I originally proposed doing just that. I also think that in the case of a "unique together" tuple where *none* of the fields are editable, model validation should be skipped inside is_valid(). However, I do think that a modification is warranted to resolve the underlying error of the above-listed tickets, as well as my own. > Btw, just my humble opinion: If you exclude fields from the ModelForm > it's your duty to check for valid data. I think this is partly true. However, I believe that with respect to `unique_together`, you should relax this standard. In the case of content-types and generic fields, it is very common to exclude the content-type and object-id fields form the forms, but to also group the content-type and object-id fields with some other editable field in defining a "unique_together" constraint. In such a case, requiring a developer to write boilerplate code to validate uniqueness seems inconvenient, counterintuitive and kind of difficult, actually (IMHO, of course). Furthermore, without adding a whole bunch of complex special-case code to the admin, the current conflict between "list_editable" and "unique_together" is only solvable with the kind of change I am proposing be made to model validation. -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send