I'd like to ask what the status of ticked #5833 http://code.djangoproject.com/ticket/5833 is. I see that it's listed as a "maybe" feature for 1.1. The patch is
marked "needs improvement," but it's not clear to me from the bug
discussion what improvement is being requested. I (gerdemb) have
updated the original patch submitted by fas to work with Django 1.0.2
and have included one small bug-fix. So, what is the next step?
I've never contributed to an open-source project, but I've been using
Python for several years and Django for past several months. Is there
a way that I can help here?
Also, wanted to take this opportunity to thank all the developers.
This is my first web project using Django and it's been a joy to work
with. :)
On Sat, Feb 7, 2009 at 8:27 AM, Ben Gerdemann <goo...@ibeni.net> wrote:
> Hello,
> I'd like to ask what the status of ticked #5833
> http://code.djangoproject.com/ticket/5833 > is. I see that it's listed as a "maybe" feature for 1.1. The patch is
> marked "needs improvement," but it's not clear to me from the bug
> discussion what improvement is being requested. I (gerdemb) have
> updated the original patch submitted by fas to work with Django 1.0.2
> and have included one small bug-fix. So, what is the next step?
> I've never contributed to an open-source project, but I've been using
> Python for several years and Django for past several months. Is there
> a way that I can help here?
> Also, wanted to take this opportunity to thank all the developers.
> This is my first web project using Django and it's been a joy to work
> with. :)
> Cheers,
> Ben
A couple things, first the patch still has a pair of TODO comments, so
either those comments are no longer applicable, or what they refer to should
be fixed. Secondly, it needs docs and tests. I'm not a core dev so I can't
speak to whether the approach taken is the right one(I notice it changed
radically from Honza's original patches), however those items are going to
be a baseline for whatever patch is accepted.
Alex
-- "I disapprove of what you say, but I will defend to the death your right to
say it." --Voltaire
"The people's good is the highest law."--Cicero
On Feb 7, 12:35 pm, Alex Gaynor <alex.gay...@gmail.com> wrote:
> On Sat, Feb 7, 2009 at 8:27 AM, Ben Gerdemann <goo...@ibeni.net> wrote:
> A couple things, first the patch still has a pair of TODO comments, so
> either those comments are no longer applicable, or what they refer to should
> be fixed. Secondly, it needs docs and tests. I'm not a core dev so I can't
> speak to whether the approach taken is the right one(I notice it changed
> radically from Honza's original patches), however those items are going to
> be a baseline for whatever patch is accepted.
I updated the patch to the latest HEAD and fixed the TODO comments
which were both validation items. I was going to start writing some
tests by basing them off of tests for the existing admin filterspecs,
but I couldn't find any. The closest I could find was in admin_views,
which checks some basic error conditions. Is there somewhere else I
should look?
In order to pass the existing tests, I had to disable the use of
custom GET params. In the original patch, the filterspec could consume
a custom param and then return an arbitrary queryset based on this
parameter, but to do this it silently removed the parameters that
don't match a field name from the request. The problem is that there
is a test to insure that any parameters that do not match field names
are handled by forwarding to ?e=1 (testIncorrectLookupParameters)
which failed when the previous patch silently removed them.
Additionally, the code in the previous patch didn't handle filters on
M2M relationships through an intermediate table because it assumed the
first part of the search param would be a field name.
Anyway, before we got too far with this specific implementation,
perhaps we need some developer input if this is a good way to proceed.
Being able to use custom GET parameters to filter would be a very
powerful feature and it would disappointing if we could figure out a
way to do it.
On Sun, Feb 8, 2009 at 10:28 AM, Ben Gerdemann <goo...@ibeni.net> wrote:
> On Feb 7, 12:35 pm, Alex Gaynor <alex.gay...@gmail.com> wrote: > > On Sat, Feb 7, 2009 at 8:27 AM, Ben Gerdemann <goo...@ibeni.net> wrote:
> > A couple things, first the patch still has a pair of TODO comments, so > > either those comments are no longer applicable, or what they refer to > should > > be fixed. Secondly, it needs docs and tests. I'm not a core dev so I > can't > > speak to whether the approach taken is the right one(I notice it changed > > radically from Honza's original patches), however those items are going > to > > be a baseline for whatever patch is accepted.
> I updated the patch to the latest HEAD and fixed the TODO comments > which were both validation items. I was going to start writing some > tests by basing them off of tests for the existing admin filterspecs, > but I couldn't find any. The closest I could find was in admin_views, > which checks some basic error conditions. Is there somewhere else I > should look?
> In order to pass the existing tests, I had to disable the use of > custom GET params. In the original patch, the filterspec could consume > a custom param and then return an arbitrary queryset based on this > parameter, but to do this it silently removed the parameters that > don't match a field name from the request. The problem is that there > is a test to insure that any parameters that do not match field names > are handled by forwarding to ?e=1 (testIncorrectLookupParameters) > which failed when the previous patch silently removed them. > Additionally, the code in the previous patch didn't handle filters on > M2M relationships through an intermediate table because it assumed the > first part of the search param would be a field name.
> Anyway, before we got too far with this specific implementation, > perhaps we need some developer input if this is a good way to proceed. > Being able to use custom GET parameters to filter would be a very > powerful feature and it would disappointing if we could figure out a > way to do it.
I don't have time to devote to this right now, but I'd suggest taking a look at the svn history of the tests that check for the ?e=1 redirect. I have a vague recollection that it/they may be there as a result of a problem where incorrect lookup parameters caused an exception, and when that was fixed the test was added to ensure invalid lookups didn't cause the admin to generate a Server Error. I think the main goal of the test is to ensure no exception was raised, not necessarily to lock in place the use of the ?e=1 redirect as a way of handling the situation.
If you've got something more useful to do with GET parameters that don't match field names than do a cryptic (and often huh?-inducing, since first it is easy to not notice what's happened and second it's not exactly crystal clear even if you notice the ?e=1 in the url in the address bar) redirect to ?e=1, I wouldn't necessarily think a test that checks for that ?e=1 redirect should stand in the way of that.
On Feb 8, 2:15 pm, Karen Tracey <kmtra...@gmail.com> wrote:
> I don't have time to devote to this right now, but I'd suggest taking a look
> at the svn history of the tests that check for the ?e=1 redirect. I have a
> vague recollection that it/they may be there as a result of a problem where
> incorrect lookup parameters caused an exception, and when that was fixed the
> test was added to ensure invalid lookups didn't cause the admin to generate
> a Server Error. I think the main goal of the test is to ensure no exception
> was raised, not necessarily to lock in place the use of the ?e=1 redirect as
> a way of handling the situation.
> If you've got something more useful to do with GET parameters that don't
> match field names than do a cryptic (and often huh?-inducing, since first it
> is easy to not notice what's happened and second it's not exactly crystal
> clear even if you notice the ?e=1 in the url in the address bar) redirect to
> ?e=1, I wouldn't necessarily think a test that checks for that ?e=1 redirect
> should stand in the way of that.
If we want to flag when an unknown GET parameter is passed to the
change_list, we're going to have to create a mechanism where each
filterspec "consumes" the parameters that it uses and passes the
filtered parameter list to the next filterspec until we get to the
code in get_query_set() that handles the generic cases. This won't be
difficult, except that filterspecs are designed to inherited and this
will be require every filterspec to "consume" it's own parameters. The
other option is to just ignore unknown parameters. This seems kind of
ugly, but I'll bet there are many frameworks out there that simply
ignore unknown parameters. Thoughts?
On Sun, 2009-02-08 at 10:23 -0800, Ben Gerdemann wrote: > This seems kind of > ugly, but I'll bet there are many frameworks out there that simply > ignore unknown parameters. Thoughts?
Ignoring portions of a URL sounds pretty broken. Our goal isn't to be like other frameworks. It's to behave correctly, in accordance with best practices for things like URL construction and consumption. I would be a little unhappy with 'ignoring' being something that happened there.
On Feb 10, 5:43 am, Malcolm Tredinnick <malc...@pointy-stick.com>
wrote:
> Ignoring portions of a URL sounds pretty broken. Our goal isn't to be
> like other frameworks. It's to behave correctly, in accordance with best
> practices for things like URL construction and consumption. I would be a
> little unhappy with 'ignoring' being something that happened there.
I have attached a new patch (http://code.djangoproject.com/attachment/ ticket/5833/5833-against-9836-new-proper.patch) to attempt to address
these issues. Is the approach taken acceptable? I believe the patch
still needs tests, which I will get to as soon as I can. However, it
would be nice to get some feedback on the quality of the patch before
proceeding further.
On Feb 10, 1:43 am, Malcolm Tredinnick <malc...@pointy-stick.com>
wrote:
> Ignoring portions of a URL sounds pretty broken. Our goal isn't to be
> like other frameworks. It's to behave correctly, in accordance with best
> practices for things like URL construction and consumption. I would be a
> little unhappy with 'ignoring' being something that happened there.
I agree, but I think it's a little more complicated. The basic
question is what should we do if an unknown parameter is passed to our
request. In the other parts of the admin interface that I poked around
in, the answer is nothing happens. If I put ?foo=bar on the URL it's
simply ignored. For the majority of views this is probably true--I
think that code uses the parameters it needs or expects and ignores
the rest. The exception is the change_list view, because it doesn't
have a fixed list of parameters and tries to consume them all as a
filter. So the change_list is actually an exception to the rule. In
fact, rather than silently ignoring unknown parameters, code was added
to throw away the entire request and redirect to ?e=1 when an invalid
filter was passed. This was done to prevent the admin from causing an
exception on a bad filter.
With the new filterspecs, we have the possibility to put arbitrary
parameters in our request that can be consumed by any filterspec. In
André's patch, each filterspec has to have a function to consume the
parameters that it's going to use and the remaining parameters are
checked as before. My thought was that we have a lot of extra code to
check that we are using every parameter and they are valid. If we
simply ignored parameters that didn't filter or weren't consumed by
filterspecs we would make it simpler to write an inherited filterspec
without having to consume the parameters it's using and we could
eliminate the strange forwarding to ?e=1 behavior while still having a
test that the admin doesn't cause on exception on a bad parameter.
Well, I've probably spent more time writing about this small problem
than it's really worth. :) Simply having custom filterspecs will be
useful regardless of the details of how they are implemented. Thanks
to André for updating my patch--I was going to do it, but you beat me
too it.
|
