[1/4] Enforce ordering of bundles in REST test

This is required to make the tests pass on a recent version of
postgres.

Signed-off-by: Daniel Axtens <dja@axtens.net>
---
 patchwork/tests/test_rest_api.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Mon, 2017-09-04 at 01:14 +1000, Daniel Axtens wrote:
> This is required to make the tests pass on a recent version of
> postgres.
> 
> Signed-off-by: Daniel Axtens <dja@axtens.net>
> ---
>  patchwork/tests/test_rest_api.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/patchwork/tests/test_rest_api.py
> b/patchwork/tests/test_rest_api.py
> index abffd17fddec..d4a84bd8c5ad 100644
> --- a/patchwork/tests/test_rest_api.py
> +++ b/patchwork/tests/test_rest_api.py
> @@ -738,7 +738,7 @@ class TestBundleAPI(APITestCase):
>          # authenticated user
>          # should see the public and private bundle
>          self.client.force_authenticate(user=user)
> -        resp = self.client.get(self.api_url())
> +        resp = self.client.get(self.api_url() + '?order=id')

Maybe I'm misunderstanding things here, but doesn't this suggest users would
always need to add the 'order' filter just to get things working sanely on a
PostgreSQL-backed instance? If so that doesn't sound very useful, and it would
suggest that the test is doing its job.

It doesn't seem like there's a way to apply a default filter using the
'OrderingFilter' [1]. However, could we simply add a sort to the queryset
returned by 'patchwork.api.bundle.BundleFilter.get_queryset'?

>          self.assertEqual(status.HTTP_200_OK, resp.status_code)
>          self.assertEqual(2, len(resp.data))
>          for bundle_rsp, bundle_obj in zip(

Stephen

[1] http://www.django-rest-framework.org/api-guide/filtering/#orderingfilter

Stephen Finucane <stephen@that.guru> writes:

> On Mon, 2017-09-04 at 01:14 +1000, Daniel Axtens wrote:
>> This is required to make the tests pass on a recent version of
>> postgres.
>> 
>> Signed-off-by: Daniel Axtens <dja@axtens.net>
>> ---
>>  patchwork/tests/test_rest_api.py | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/patchwork/tests/test_rest_api.py
>> b/patchwork/tests/test_rest_api.py
>> index abffd17fddec..d4a84bd8c5ad 100644
>> --- a/patchwork/tests/test_rest_api.py
>> +++ b/patchwork/tests/test_rest_api.py
>> @@ -738,7 +738,7 @@ class TestBundleAPI(APITestCase):
>>          # authenticated user
>>          # should see the public and private bundle
>>          self.client.force_authenticate(user=user)
>> -        resp = self.client.get(self.api_url())
>> +        resp = self.client.get(self.api_url() + '?order=id')
>
> Maybe I'm misunderstanding things here, but doesn't this suggest users would
> always need to add the 'order' filter just to get things working sanely on a
> PostgreSQL-backed instance? If so that doesn't sound very useful, and it would
> suggest that the test is doing its job.

I didn't think bundles were intrinsically ordered - unlike
patches/series which have a pretty obvious date-ordering, I think
bundles are a bit more arbitrary. Is there an obvious sane ordering we
should be enforcing? (IMO you could make a decent argument for either
sorting by name or sorting by (creation|modification) date.)

>
> It doesn't seem like there's a way to apply a default filter using the
> 'OrderingFilter' [1]. However, could we simply add a sort to the queryset
> returned by 'patchwork.api.bundle.BundleFilter.get_queryset'?

If we went down this route, I think we would probably want to order them
using a class Meta ordering statement in the model (see e.g. the
ordering of projects by linkname in models.py around line 101.) I assume
the REST stuff honours that ordering.

Regards,
Daniel
>
>>          self.assertEqual(status.HTTP_200_OK, resp.status_code)
>>          self.assertEqual(2, len(resp.data))
>>          for bundle_rsp, bundle_obj in zip(
>
> Stephen
>
> [1] http://www.django-rest-framework.org/api-guide/filtering/#orderingfilter

On Fri, 2017-09-08 at 07:20 +1000, Daniel Axtens wrote:
> Stephen Finucane <stephen@that.guru> writes:
> 
> > On Mon, 2017-09-04 at 01:14 +1000, Daniel Axtens wrote:
> > > This is required to make the tests pass on a recent version of
> > > postgres.
> > > 
> > > Signed-off-by: Daniel Axtens <dja@axtens.net>
> > > ---
> > >  patchwork/tests/test_rest_api.py | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/patchwork/tests/test_rest_api.py
> > > b/patchwork/tests/test_rest_api.py
> > > index abffd17fddec..d4a84bd8c5ad 100644
> > > --- a/patchwork/tests/test_rest_api.py
> > > +++ b/patchwork/tests/test_rest_api.py
> > > @@ -738,7 +738,7 @@ class TestBundleAPI(APITestCase):
> > >          # authenticated user
> > >          # should see the public and private bundle
> > >          self.client.force_authenticate(user=user)
> > > -        resp = self.client.get(self.api_url())
> > > +        resp = self.client.get(self.api_url() + '?order=id')
> > 
> > Maybe I'm misunderstanding things here, but doesn't this suggest users
> > would always need to add the 'order' filter just to get things working
> > sanely on a PostgreSQL-backed instance? If so that doesn't sound very
> > useful, and it would suggest that the test is doing its job.
> 
> I didn't think bundles were intrinsically ordered - unlike
> patches/series which have a pretty obvious date-ordering, I think
> bundles are a bit more arbitrary. Is there an obvious sane ordering we
> should be enforcing? (IMO you could make a decent argument for either
> sorting by name or sorting by (creation|modification) date.)

That's a very good point. I looked at 'patchwork.view.bundle.bundle_list', and
noted that there is no default bundling applied, however, I do think we want to
avoid the same request returning different responses simply because of the
database backend in use. Given that there's no precedent here, I guess this
means we're free to choose? Name or ID (which is implicitly data-based) would
be equal in my mind.

> > It doesn't seem like there's a way to apply a default filter using the
> > 'OrderingFilter' [1]. However, could we simply add a sort to the queryset
> > returned by 'patchwork.api.bundle.BundleFilter.get_queryset'?
> 
> If we went down this route, I think we would probably want to order them
> using a class Meta ordering statement in the model (see e.g. the
> ordering of projects by linkname in models.py around line 101.) I assume
> the REST stuff honours that ordering.

The only disadvantage to taking this route is that it will introduce a
migration, and I'm not sure if a migration is something we should be in the
habit of backporting. Perhaps we modify 'get_queryset' for 'stable/2.0' (with
tests, of course) and take the Meta class approach for 'master'? Open to other
ideas here, of course.

Stephen