Re: Validate a form's excluded fields if a value is present
I've been brainstorming (ie, drinking more coffee than I should), and what I came up with to be the best (IMHO) solution for A) Less confusion, and B) Less risk of API breakage is: ModelForm.is_valid(include_all_fields=True) or ModelForm.is_all_fields_valid() and neither are going to be an issue to add in the future if the API is locked, so I'm tabling the idea and I'll bring it back up in the 1.3 discussion. Thanks for spending time on this with me this week. Michael -- 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: Validate a form's excluded fields if a value is present
On Sat, Apr 3, 2010 at 3:03 PM, orokusaki wrote: > Russ, > > I think you're 100% right, and the "wrong place" part hit the nail on > the head. This morning I got really frustrated because I couldn't > quite see the big picure yet pertaining to the ORM and it's > relationship with ModelForm, partly because there is so much going on > with state changes and you know how it is trying to understand > somebody else's code. To add to the problem is the possibility of a > missunderstanding of the API developers who could think extra_data > will take care of them. I suposedly I didn't really think of how this > might relate (or not relate) with fields other than a foreign key > (user, etc), and what a user would think of an error message > like"'Tracking Instance ID' has 16 digits but should have only 12" > while filling out a contact form or something like that, and I suppose > you can't anticipate every possible runtime error either. With that, > do you think it's something worth pursuing. I admit I do like the idea > of using the instance passed into the form much more. I just have such > a large couple of projects coming up that DRY really comes to mind > when using try: instance.full_clean(). As I have already said, I think the goal of making full model validation easier to use is certainly desirable. However, I don't have any particularly inspired ideas on how it could be improved. I'm certainly open to any suggestions. That said, I'm not especially interested in getting into a massive design discussion *right now*. The core team is trying to get Django 1.2 out the door, and now is *not* the time to be discussing new features. The only reason to be seriously discussing new features *right now* is if freezing the new model validation API will make adding a new feature impossible in the future. Unless you can make the case for an API change that won't be possible once the API is frozen for 1.2, I'd rather table this discussion until the 1.3 feature discussion period. Yours, Russ Magee %-) -- 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: Validate a form's excluded fields if a value is present
On Sat, Apr 3, 2010 at 8:14 AM, Boris Schaeling wrote: > On Thu, 01 Apr 2010 11:11:47 +0300, Russell Keith-Magee > wrote: > >> [...]I don't deny that it would be *really* nice to be able to >> automatically call full model validation on a model on form save - the >> problem is that we can't do that while retaining backwards >> compatibility. > > How about a setting like BACKWARDS_COMPATIBLE which could be set to a > version number? It could be set to 1.0 by default. Those of us who don't > care about being backwards compatible to 1.0 (because we never used this > version or upgraded our own code already) could change the setting > accordingly. Maybe such a setting could be a compromise between backwards > compatibility and progress? By introducing a setting that controls fundamental behavior of a feature, you double the number of testing configurations that you need to evaluate, and you double the number of potential interactions that could go wrong, and so on. It also complicates documentation, and makes a nightmare of reusable applications (since if I write an application assuming a v1.0 API, and you deploy it in a project that assumes the v1.1 API, my application won't work as advertised in your project). This way lead madness :-) Yours, Russ Magee %-) -- 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: Validate a form's excluded fields if a value is present
Russ, I think you're 100% right, and the "wrong place" part hit the nail on the head. This morning I got really frustrated because I couldn't quite see the big picure yet pertaining to the ORM and it's relationship with ModelForm, partly because there is so much going on with state changes and you know how it is trying to understand somebody else's code. To add to the problem is the possibility of a missunderstanding of the API developers who could think extra_data will take care of them. I suposedly I didn't really think of how this might relate (or not relate) with fields other than a foreign key (user, etc), and what a user would think of an error message like"'Tracking Instance ID' has 16 digits but should have only 12" while filling out a contact form or something like that, and I suppose you can't anticipate every possible runtime error either. With that, do you think it's something worth pursuing. I admit I do like the idea of using the instance passed into the form much more. I just have such a large couple of projects coming up that DRY really comes to mind when using try: instance.full_clean(). P.S. sorry for the noobish formatting on this one. IPhone apparently doesn't have backtick ;( -- 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: Validate a form's excluded fields if a value is present
On Thu, 01 Apr 2010 11:11:47 +0300, Russell Keith-Magee wrote: [...]I don't deny that it would be *really* nice to be able to automatically call full model validation on a model on form save - the problem is that we can't do that while retaining backwards compatibility. How about a setting like BACKWARDS_COMPATIBLE which could be set to a version number? It could be set to 1.0 by default. Those of us who don't care about being backwards compatible to 1.0 (because we never used this version or upgraded our own code already) could change the setting accordingly. Maybe such a setting could be a compromise between backwards compatibility and progress? Boris [...] -- 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: Validate a form's excluded fields if a value is present
On Sat, Apr 3, 2010 at 2:51 AM, orokusaki wrote:
> On Apr 2, 2:00 am, Russell Keith-Magee wrote:
>
>> The broad goal is certainly reasonable and desirable. It's really a
>> matter of finding a way to make it work that doesn't involve
>> completely breaking (or disfiguring) the API that we already have.
>>
>> Yours,
>> Russ Magee %-)
>
> I've been working on this since last night, and I came up with a
> decent idea, but there is one serious flaw:
>
> My version would be used like this in a view:
>
>
> form = SomeModelFormSubClass(request.POST, extra_data={"some_field":
> "Some Value"}) # This would work OK, but see the next example
My issue with this approach is that it's attacking the problem at the
wrong place.
As I've said before, the Django idiom for 'extra data that didn't come
from a form' is to create an unsaved instance, add the extra data to
that instance, and then pass that instance to the form. If the problem
is that the form isn't validating the right set of fields, then the
solution is to get the form to validate the right set of fields, not
to try and fake the data going into the form.
The other issue is how to handle errors. One of the issues associated
with this problem that isn't strictly a backwards incompatibility
issue, but is concerning, is how to deal with validation errors that
aren't on the form fields. If an 'extra' field isn't represented on
the form, how and where do you display validation errors that
originate from that field? It's not hard to think of a circumstance
where you could end up raising an error that the end-user is not in a
position to fix, because the fields that are problematic are all
'extra' fields.
If you haven't already read up on the back history of model
validation, I strongly recommend that you read this thread:
http://groups.google.com/group/django-developers/browse_thread/thread/cee43f109e0cf6b
It's very long (and sometimes meandering) but it does walk through the
relevant issues quite well.
Yours,
Russ Magee %-)
--
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: Validate a form's excluded fields if a value is present
On Apr 2, 2:00 am, Russell Keith-Magee wrote:
> The broad goal is certainly reasonable and desirable. It's really a
> matter of finding a way to make it work that doesn't involve
> completely breaking (or disfiguring) the API that we already have.
>
> Yours,
> Russ Magee %-)
I've been working on this since last night, and I came up with a
decent idea, but there is one serious flaw:
My version would be used like this in a view:
form = SomeModelFormSubClass(request.POST, extra_data={"some_field":
"Some Value"}) # This would work OK, but see the next example
The patch would be to use ``kwargs.pop('extra_data', None)`` to add
subsequent fields back to the form, and un-exclude them from
validation. The problem I ran into before even beginning work on
implementation was this:
form = SomeModelFormSubClass(request.POST, extra_data={"created_by":
request.user}) # Doesn't work because it's expecting a raw value (in
this case, an integer for the ``auth.User.pk``)
The problem is that it in the case of a foreign key (which is where
this would be used most) you have to pass the ``request.user.pk``, but
that doesn't seem natural to do at all. What's your thought on this.
Should it be handled magically behind the scenes like (in
``__init__()``):
extra_data = kwargs.pop("extra_data", None)
if extra_data:
from django.db.models.fields import Field
for key, value in extra_data.items():
if isinstance(value, Field):
value = value.get_form_value()
# Now we can do processing to add the
# field back into the form validation, and
# something to ensure it gets put into
# self.cleaned_data, or set a flag for later
# (haven't checked the order of all that yet)
What's your thoughts? With a little bit of guidance, I can work up a
patch and test it.
--
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: Validate a form's excluded fields if a value is present
On Fri, Apr 2, 2010 at 6:25 AM, orokusaki wrote: > Hey Russ, > > I'm not on the model.full_clean stuff anymore, and I apologize for > burning so many cycles on that point when you're in the middle of 1.2 > dev. I'm just wondering if what I proposed above sounds reasonable. > The form validation turned off completely for fields that aren't > included makes it hard to do custom things. It makes sense by default > but it would be nice if there was a way to override it or if it was > intelligent to whether there was a value or not. The broad goal is certainly reasonable and desirable. It's really a matter of finding a way to make it work that doesn't involve completely breaking (or disfiguring) the API that we already have. Yours, Russ Magee %-) -- 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: Validate a form's excluded fields if a value is present
Thanks Jacob, I'll give that a try. On Apr 1, 7:44 am, Jacob Kaplan-Moss wrote: > On Thu, Apr 1, 2010 at 3:11 AM, Russell Keith-Magee > > wrote: > > Melodrama aside, as we've told you before, the docs clearly say that > > full_clean() isn't called by form.save(). The docs also give you the > > reason - backwards compatibility. > > > I don't deny that it would be *really* nice to be able to > > automatically call full model validation on a model on form save - the > > problem is that we can't do that while retaining backwards > > compatibility. > > Well, *we* can't... but *users* can: just override your form's > ``clean`` method, and call ``full_clean`` from there. > > In general, if you find yourself doing the same thing over and over in > different views, it's a sign that you're operating at the wrong level > of abstraction. In this case, solve the problem once by overriding > what you need on the form, and then let your views be as "pretty" as > you'd like. > > Jacob -- 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: Validate a form's excluded fields if a value is present
Hey Russ, I'm not on the model.full_clean stuff anymore, and I apologize for burning so many cycles on that point when you're in the middle of 1.2 dev. I'm just wondering if what I proposed above sounds reasonable. The form validation turned off completely for fields that aren't included makes it hard to do custom things. It makes sense by default but it would be nice if there was a way to override it or if it was intelligent to whether there was a value or not. ,Michael -- 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: Validate a form's excluded fields if a value is present
On Thu, Apr 1, 2010 at 3:11 AM, Russell Keith-Magee wrote: > Melodrama aside, as we've told you before, the docs clearly say that > full_clean() isn't called by form.save(). The docs also give you the > reason - backwards compatibility. > > I don't deny that it would be *really* nice to be able to > automatically call full model validation on a model on form save - the > problem is that we can't do that while retaining backwards > compatibility. Well, *we* can't... but *users* can: just override your form's ``clean`` method, and call ``full_clean`` from there. In general, if you find yourself doing the same thing over and over in different views, it's a sign that you're operating at the wrong level of abstraction. In this case, solve the problem once by overriding what you need on the form, and then let your views be as "pretty" as you'd like. Jacob -- 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: Validate a form's excluded fields if a value is present
On Thu, Apr 1, 2010 at 9:11 AM, orokusaki wrote: > I'm working on an SAAS project, and there is an ``account`` attribute > (foreign key) on every model in the project (similar to those who have > a ``user`` or ``created_by`` attribute on every model). ``account`` is > added to the request object using a MiddleWare class. > > When I'm writing views, I have to do this in each view: > > > if request.method == 'POST': > form = SomeSpamForm(request.POST) > form.instance.account = request.account > if form.is_valid(): > instance = form.save(commit=False) > try: > instance.full_clean() > except ValidationError, e: > form._update_errors(e.message_dict) > else: > instance.save() > > > I admit that I tend to be a bit OCD about coding style, but I think > most programmers would agree that doing that in every single view is a > very bad idea, especially if you ever intend on upgrading Django in > the future. Imagine having 178 views in 19 apps in a project with that > same code, and updating every single one. Of course I could write some > sort of monkey patch and pray, but instead I thought of what might be > a good solution: Melodrama aside, as we've told you before, the docs clearly say that full_clean() isn't called by form.save(). The docs also give you the reason - backwards compatibility. I don't deny that it would be *really* nice to be able to automatically call full model validation on a model on form save - the problem is that we can't do that while retaining backwards compatibility. This suggestion falls into the same category; if you follow the history of the lines in question (tickets #12521 and #12901 in particular), you should be able to see why validating non-form fields is problematic. > In forms.models.BaseModelForm._get_validation_exclusions() ( Defined > on line # 266 in SVN) you'll find: > > 283 # Exclude fields that aren't on the form. The > developer may be > 284 # adding these values to the model after form > validation. > 285 if field not in self.fields: > 286 exclude.append(f.name) > > Couldn't this be changed to: > > 283 # Exclude fields that aren't on the form. The > developer may be > 284 # adding these values to the model after form > validation. > 285 if field not in self.fields and not > self.cleaned_data.get(field, None): > 286 exclude.append(f.name) I'm not sure what you expect this to do, but it didn't work in my tests. if "field" isn't in "self.fields", it isn't going to be in "self.cleaned_data" either, so the half of the if clause you've added is completely redundant as far as I can make out. It certainly doesn't affect the fields that end up in the results from _get_validation_errors(). On a related note - thanks for brining the discussion to django-dev. For future reference, if you're discussing an idea that comes out of a ticket, it's customary to reference the ticket (in this case, #13249). At the very least, this ticket gives a full set of models that demonstrates the problem you are trying to solve. Yours, Russ Magee %-) -- 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: Validate a form's excluded fields if a value is present
On Thu, Apr 1, 2010 at 2:17 PM, subs...@gmail.com wrote: > Seems like a security hole, whereby people may supply additional > fields if they can guess their counterparts on the model. Its > 'exclude', not 'exclude_maybe'. Please be *very* careful about using the words "security hole" - those words make people (understandably) jumpy. Even if this idea were implemented as described, I can't see any way that it could represent a security hole. It doesn't expose anything in a way that a malicious *user* could manipulate to cause security effects. It might be confusing or cause unusual side effects from a *developer* perspective, but that doesn't constitute a security hole - its just for a developer to misuse an API. Yours, Russ Magee %-) -- 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: Validate a form's excluded fields if a value is present
Seems like a security hole, whereby people may supply additional fields if they can guess their counterparts on the model. Its 'exclude', not 'exclude_maybe'. ...Unless I'm missing something fundamental. -S On Mar 31, 9:11 pm, orokusaki wrote: > I'm working on an SAAS project, and there is an ``account`` attribute > (foreign key) on every model in the project (similar to those who have > a ``user`` or ``created_by`` attribute on every model). ``account`` is > added to the request object using a MiddleWare class. > > When I'm writing views, I have to do this in each view: > > if request.method == 'POST': > form = SomeSpamForm(request.POST) > form.instance.account = request.account > if form.is_valid(): > instance = form.save(commit=False) > try: > instance.full_clean() > except ValidationError, e: > form._update_errors(e.message_dict) > else: > instance.save() > > I admit that I tend to be a bit OCD about coding style, but I think > most programmers would agree that doing that in every single view is a > very bad idea, especially if you ever intend on upgrading Django in > the future. Imagine having 178 views in 19 apps in a project with that > same code, and updating every single one. Of course I could write some > sort of monkey patch and pray, but instead I thought of what might be > a good solution: > > In forms.models.BaseModelForm._get_validation_exclusions() ( Defined > on line # 266 in SVN) you'll find: > > 283 # Exclude fields that aren't on the form. The > developer may be > 284 # adding these values to the model after form > validation. > 285 if field not in self.fields: > 286 exclude.append(f.name) > > Couldn't this be changed to: > > 283 # Exclude fields that aren't on the form. The > developer may be > 284 # adding these values to the model after form > validation. > 285 if field not in self.fields and not > self.cleaned_data.get(field, None): > 286 exclude.append(f.name) > > This would allow me to do this instead of my above example: > > if request.method == 'POST': > form = SomeSpamForm(request.POST) > form.instance.account = request.account > if form.is_valid(): > form.save() > > That seems way better than manually running validation outside of the > excellent ``ModelForm`` built-in error handling. I'm not trying to > brown nose here (OK, I am a little), but ``ModelForm`` is so great to > use that it's a shame to have the best part (validation) leak into my > views. -- 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: Validate a form's excluded fields if a value is present
Let me just say that my non-patch above is just an abstract idea, and I don't know if it will work like that without other changes, but I think it gets the idea across. -- 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.
Validate a form's excluded fields if a value is present
I'm working on an SAAS project, and there is an ``account`` attribute (foreign key) on every model in the project (similar to those who have a ``user`` or ``created_by`` attribute on every model). ``account`` is added to the request object using a MiddleWare class. When I'm writing views, I have to do this in each view: if request.method == 'POST': form = SomeSpamForm(request.POST) form.instance.account = request.account if form.is_valid(): instance = form.save(commit=False) try: instance.full_clean() except ValidationError, e: form._update_errors(e.message_dict) else: instance.save() I admit that I tend to be a bit OCD about coding style, but I think most programmers would agree that doing that in every single view is a very bad idea, especially if you ever intend on upgrading Django in the future. Imagine having 178 views in 19 apps in a project with that same code, and updating every single one. Of course I could write some sort of monkey patch and pray, but instead I thought of what might be a good solution: In forms.models.BaseModelForm._get_validation_exclusions() ( Defined on line # 266 in SVN) you'll find: 283 # Exclude fields that aren't on the form. The developer may be 284 # adding these values to the model after form validation. 285 if field not in self.fields: 286 exclude.append(f.name) Couldn't this be changed to: 283 # Exclude fields that aren't on the form. The developer may be 284 # adding these values to the model after form validation. 285 if field not in self.fields and not self.cleaned_data.get(field, None): 286 exclude.append(f.name) This would allow me to do this instead of my above example: if request.method == 'POST': form = SomeSpamForm(request.POST) form.instance.account = request.account if form.is_valid(): form.save() That seems way better than manually running validation outside of the excellent ``ModelForm`` built-in error handling. I'm not trying to brown nose here (OK, I am a little), but ``ModelForm`` is so great to use that it's a shame to have the best part (validation) leak into my views. -- 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.
