Message ID | 20170903151444.25660-2-dja@axtens.net |
---|---|
State | Superseded |
Delegated to: | Stephen Finucane |
Headers | show |
Series | PostgreSQL fixes and test support | expand |
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
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') self.assertEqual(status.HTTP_200_OK, resp.status_code) self.assertEqual(2, len(resp.data)) for bundle_rsp, bundle_obj in zip(
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(-)