Opinions requested about validating edit_inline fields
Malcolm Tredinnick
I was doing a slightly chaotic wander through the open tickets this
evening and came across http://code.djangoproject.com/ticket/1690 --
discussing validators and fields with edit_inline set.
Initially I thought that the reporter had a good point and now I'm not
so sure that the "right" answer is obvious, so I'd like some feedback on
how we might like this to behave. For illustrative purposes, suppose we
have
class Client(models.Model):
address = models.TextField(validator_list =
[some_validator])
...
class Project(models.Model):
client = models.ForeignKey(Client, edit_inline = True)
...
The problem, in case it's not apparent from the ticket, is that when a
ForeignKey field is marked as edit_inline, the field names passed to the
validator for the related object are no longer something like "address",
but rather "client.0.address", where "client" is the model for the field
and "0" is the position of this related object amongst all the possible
related objects for the model (so that if there are two ForeignKeys,
each pointing to a Client model, you get client.0.address and
client.1.address and are able to differentiate).
Now, the ticket is saying that when setting a validator on a field in
the related object (the Client model, in this case) it makes sense to
talk about the 'address' field when creating things like
core.validators.RequiredIfOtherFieldsNotGiven(...). But when validation
occurs, we don't get "address", we get "client.0.address".
So, at first blush, this seems counter-intuitive. But then I thought it
may not be so clear: since we editing Client instances inline with the
Project instances, we possibly want to be able to validate clients
against the related Project. Currently this is possible; the change
request in #1690 would make it impossible.
A solution that fits all needs would be to create an extra parameter for
these validators (the ones that take field names) that indicates that
only fields for this class should be considered (e.g. only things that
start with model.__name__, in effect) and the dotted bits
("model.position.") should be stripped before comparison. But that
removes the ability to differentiate between client.0... and
client.1...; not sure if that is a real problem or not at the moment.
Fixing this latter case is not completely trivial, because validating
form input does not know about models or edit_inlines or anything:
everything is just a bunch of strings at that point.
What do people think? Am I reading too much into the problem? Is my
solution in the last paragraph good enough for the 90% case and people
wanting more can write their own validators?
Regards,
Malcolm
Jason Davies
> A solution that fits all needs would be to create an extra parameter for
> these validators (the ones that take field names) that indicates that
> only fields for this class should be considered (e.g. only things that
> start with model.__name__, in effect) and the dotted bits
> ("model.position.") should be stripped before comparison. But that
> removes the ability to differentiate between client.0... and
> client.1...; not sure if that is a real problem or not at the moment.
> Fixing this latter case is not completely trivial, because validating
> form input does not know about models or edit_inlines or anything:
> everything is just a bunch of strings at that point.
How about passing an optional name_prefix parameter to these
validators, which would allow them to look up things like
all_data[name_prefix + other_field_name]? name_prefix would be
something like 'foo.0.' or 'foo.1.' when edit_inline is used.
Regards,
Jason
Malcolm Tredinnick
I do not think it makes any difference whether you do it in the thing
calling the validator or in the validator itself. The thing calling the
validator still doesn't know anything about models or any semantics, it
just works with strings. So all you can do is look at the string and say
"hey, this is a string, followed by a dot followed by a digit followed
by a dot. I should strip it". Might as well put that inside the
validator if it is wanted.
What you can't do is determine "now I am processing the model with
edit_inline" set, because it is only working with strings.
So I think your solution is exactly the same as the one I suggest above,
just triggered from a different place. It seems harder to set up the
calling section (your method) to pass in the optional name parameter,
too, rather than setting the option on the validator. But maybe I'm
missing something.
Also, whatever is the "right" solution here has to be reasonably
straightforward for people implementing their own validators. I think
"straightforward" is going to have to mean "really well documented", but
that is kind of why I'm asking for suggestions: maybe there is a cute,
clever solution and I'm not bright enough to see it.
Thanks for the input anyway; it's something more to think about.
Regards,
Malcolm
Jason Davies
I'm not sure your solution will work properly, since validators still
need some way of knowing what "position" is in "model.position.field"
when they're called. Currently they are only passed (field_data,
all_data) as parameters. How about passing a field_name parameter too,
then the validator can mess around with stripping dots and so on if
needed?
Cheers,
Jason
Malcolm Tredinnick
>
> Malcolm Tredinnick wrote:
> > On Thu, 2006-04-27 at 09:19 -0700, Jason Davies wrote:
> > > How about passing an optional name_prefix parameter to these
> > > validators, which would allow them to look up things like
> > > all_data[name_prefix + other_field_name]? name_prefix would be
> > > something like 'foo.0.' or 'foo.1.' when edit_inline is used.
> > So I think your solution is exactly the same as the one I suggest above,
> > just triggered from a different place. It seems harder to set up the
> > calling section (your method) to pass in the optional name parameter,
> > too, rather than setting the option on the validator. But maybe I'm
> > missing something.
>
> I'm not sure your solution will work properly, since validators still
> need some way of knowing what "position" is in "model.position.field"
> when they're called. Currently they are only passed (field_data,
> all_data) as parameters. How about passing a field_name parameter too,
> then the validator can mess around with stripping dots and so on if
> needed?
Yes, that's kind of implicit in my description. Sorry, should have
mentioned it. The problem is not just the field we are checking
currently, though. It is also the other fields that may be used for
auxiliary checks (such as "required if other field empty"). I am trying
to get a feel for what people want to do in general, without trying to
delve too deep into the implementation yet.
In any case, looks like this might have to wait until after
magic-removal is merged. Except for you and I, there have been no other
responses. People are busy; it can wait.
Thanks for your thinking, anyway. It's helpful.
Malcolm