Message ID | 20170831084851.9633-1-stephen@that.guru |
---|---|
State | Accepted |
Headers | show |
Series | REST: Filter projects by 'linkname', not 'name' | expand |
Hi Stephen, I was just reading though this before applying it when it occurred to me: > except ValueError: > - filters = {'name__iexact': ' '.join(value.split('-'))} > + filters = {'linkname__iexact': ' '.join(value.split('-'))} Surely a linkname shouldn't have spaces in it? It'd be of the form foo.bar.org, right? Happy to fix this up as: {'linkname__iexact': value} but thought I should probably check I haven't radically misunderstood first! Regards, Daniel > > try: > value = self.queryset.get(**filters) > diff --git a/patchwork/tests/test_rest_api.py b/patchwork/tests/test_rest_api.py > index abffd17..14e53b2 100644 > --- a/patchwork/tests/test_rest_api.py > +++ b/patchwork/tests/test_rest_api.py > @@ -97,7 +97,23 @@ class TestProjectAPI(APITestCase): > self.assertEqual(status.HTTP_200_OK, resp.status_code) > self.assertSerialized(project, resp.data) > > - def test_get_numeric_linkname(self): > + def test_get_by_id(self): > + """Validate that it's possible to filter by pk.""" > + project = create_project() > + > + resp = self.client.get(self.api_url(project.pk)) > + self.assertEqual(status.HTTP_200_OK, resp.status_code) > + self.assertSerialized(project, resp.data) > + > + def test_get_by_linkname(self): > + """Validate that it's possible to filter by linkname.""" > + project = create_project(linkname='project', name='Sample project') > + > + resp = self.client.get(self.api_url('project')) > + self.assertEqual(status.HTTP_200_OK, resp.status_code) > + self.assertSerialized(project, resp.data) > + > + def test_get_by_numeric_linkname(self): > """Validate we try to do the right thing for numeric linkname""" > project = create_project(linkname='12345') > > @@ -326,7 +342,8 @@ class TestPatchAPI(APITestCase): > self.assertEqual(0, len(resp.data)) > > state_obj = create_state(name='Under Review') > - patch_obj = create_patch(state=state_obj) > + project_obj = create_project(linkname='myproject') > + patch_obj = create_patch(state=state_obj, project=project_obj) > > # anonymous user > resp = self.client.get(self.api_url()) > @@ -338,12 +355,6 @@ class TestPatchAPI(APITestCase): > self.assertNotIn('content', patch_rsp) > self.assertNotIn('diff', patch_rsp) > > - # test filtering by state > - resp = self.client.get(self.api_url(), {'state': 'under-review'}) > - self.assertEqual([patch_obj.id], [x['id'] for x in resp.data]) > - resp = self.client.get(self.api_url(), {'state': 'missing-state'}) > - self.assertEqual(0, len(resp.data)) > - > # authenticated user > user = create_user() > self.client.force_authenticate(user=user) > @@ -353,6 +364,18 @@ class TestPatchAPI(APITestCase): > patch_rsp = resp.data[0] > self.assertSerialized(patch_obj, patch_rsp) > > + # test filtering by state > + resp = self.client.get(self.api_url(), {'state': 'under-review'}) > + self.assertEqual([patch_obj.id], [x['id'] for x in resp.data]) > + resp = self.client.get(self.api_url(), {'state': 'missing-state'}) > + self.assertEqual(0, len(resp.data)) > + > + # test filtering by project > + resp = self.client.get(self.api_url(), {'project': 'myproject'}) > + self.assertEqual([patch_obj.id], [x['id'] for x in resp.data]) > + resp = self.client.get(self.api_url(), {'project': 'invalidproject'}) > + self.assertEqual(0, len(resp.data)) > + > def test_detail(self): > """Validate we can get a specific patch.""" > patch = create_patch( > -- > 2.13.5
Hi Dnaiel, > I was just reading though this before applying it when it occurred to > me: >> except ValueError: >> - filters = {'name__iexact': ' '.join(value.split('-'))} >> + filters = {'linkname__iexact': ' '.join(value.split('-'))} > > Surely a linkname shouldn't have spaces in it? It'd be of the form > foo.bar.org, right? patchwork=# select linkname from patchwork_project limit 10; linkname ------------------- cbe-oss-dev linux-mtd linuxppc-embedded linux-ext4 netdev sparclinux linux-ide qemu-devel ubuntu-kernel yaboot So yes, we wouldn't expect spaces, and we tend to use dashes instead of dots. This is essentially used as a path component in URLs. Cheers, Jeremy
On Mon, 2017-09-04 at 16:16 +1000, Jeremy Kerr wrote: > Hi Dnaiel, > > > I was just reading though this before applying it when it occurred to > > me: > > > except ValueError: > > > - filters = {'name__iexact': ' '.join(value.split('-'))} > > > + filters = {'linkname__iexact': ' '.join(value.split('-'))} > > > > Surely a linkname shouldn't have spaces in it? It'd be of the form > > foo.bar.org, right? > > patchwork=# select linkname from patchwork_project limit 10; > linkname > ------------------- > cbe-oss-dev > linux-mtd > linuxppc-embedded > linux-ext4 > netdev > sparclinux > linux-ide > qemu-devel > ubuntu-kernel > yaboot > > So yes, we wouldn't expect spaces, and we tend to use dashes instead of > dots. This is essentially used as a path component in URLs. Yup, I wouldn't expect to see any spaces. If this isn't already forced somewhere, we should probably add a validator to do so. For now though, feel free to fix this bit when you merge. Stephen
Hi Stephen, Merged to master at 174de19d41d76f8387027e789e5975fc078f2d08 Merged to stable/2.0 at b23f206b941694a03bf3d0a94fcbdb4f3d146f85 Thanks! Regards, Daniel Stephen Finucane <stephen@that.guru> writes: > Based on a report on the OVS mailing list, it appears that projects are > being filtered on the 'Project.name' field instead of the > 'Project.linkname' field. Correct this and add regression tests to > prevent it happening again. > > Signed-off-by: Stephen Finucane <stephen@that.guru> > Fixes: 08b2115 ("REST: Allow filtering by both project ID and linkname") > Closes-bug: #117 ("Projects are filtered on the wrong field") > Cc: Daniel Axtens <dja@axtens.net> > --- > daxtens - if you've time (and this looks good), you might handle the > merge and backport part of this. Everything about the latter should be > documented in enough detail: > > https://patchwork.readthedocs.io/en/latest/development/releasing/#backporting > > Feel free to ping me here on IRC (stephenfin on #freenode) if necessary. > --- > patchwork/api/filters.py | 2 +- > patchwork/tests/test_rest_api.py | 39 +++++++++++++++++++++++++++++++-------- > 2 files changed, 32 insertions(+), 9 deletions(-) > > diff --git a/patchwork/api/filters.py b/patchwork/api/filters.py > index 9966543..314aa47 100644 > --- a/patchwork/api/filters.py > +++ b/patchwork/api/filters.py > @@ -50,7 +50,7 @@ class ProjectChoiceField(ModelChoiceField): > try: > filters = {'pk': int(value)} > except ValueError: > - filters = {'name__iexact': ' '.join(value.split('-'))} > + filters = {'linkname__iexact': ' '.join(value.split('-'))} > > try: > value = self.queryset.get(**filters) > diff --git a/patchwork/tests/test_rest_api.py b/patchwork/tests/test_rest_api.py > index abffd17..14e53b2 100644 > --- a/patchwork/tests/test_rest_api.py > +++ b/patchwork/tests/test_rest_api.py > @@ -97,7 +97,23 @@ class TestProjectAPI(APITestCase): > self.assertEqual(status.HTTP_200_OK, resp.status_code) > self.assertSerialized(project, resp.data) > > - def test_get_numeric_linkname(self): > + def test_get_by_id(self): > + """Validate that it's possible to filter by pk.""" > + project = create_project() > + > + resp = self.client.get(self.api_url(project.pk)) > + self.assertEqual(status.HTTP_200_OK, resp.status_code) > + self.assertSerialized(project, resp.data) > + > + def test_get_by_linkname(self): > + """Validate that it's possible to filter by linkname.""" > + project = create_project(linkname='project', name='Sample project') > + > + resp = self.client.get(self.api_url('project')) > + self.assertEqual(status.HTTP_200_OK, resp.status_code) > + self.assertSerialized(project, resp.data) > + > + def test_get_by_numeric_linkname(self): > """Validate we try to do the right thing for numeric linkname""" > project = create_project(linkname='12345') > > @@ -326,7 +342,8 @@ class TestPatchAPI(APITestCase): > self.assertEqual(0, len(resp.data)) > > state_obj = create_state(name='Under Review') > - patch_obj = create_patch(state=state_obj) > + project_obj = create_project(linkname='myproject') > + patch_obj = create_patch(state=state_obj, project=project_obj) > > # anonymous user > resp = self.client.get(self.api_url()) > @@ -338,12 +355,6 @@ class TestPatchAPI(APITestCase): > self.assertNotIn('content', patch_rsp) > self.assertNotIn('diff', patch_rsp) > > - # test filtering by state > - resp = self.client.get(self.api_url(), {'state': 'under-review'}) > - self.assertEqual([patch_obj.id], [x['id'] for x in resp.data]) > - resp = self.client.get(self.api_url(), {'state': 'missing-state'}) > - self.assertEqual(0, len(resp.data)) > - > # authenticated user > user = create_user() > self.client.force_authenticate(user=user) > @@ -353,6 +364,18 @@ class TestPatchAPI(APITestCase): > patch_rsp = resp.data[0] > self.assertSerialized(patch_obj, patch_rsp) > > + # test filtering by state > + resp = self.client.get(self.api_url(), {'state': 'under-review'}) > + self.assertEqual([patch_obj.id], [x['id'] for x in resp.data]) > + resp = self.client.get(self.api_url(), {'state': 'missing-state'}) > + self.assertEqual(0, len(resp.data)) > + > + # test filtering by project > + resp = self.client.get(self.api_url(), {'project': 'myproject'}) > + self.assertEqual([patch_obj.id], [x['id'] for x in resp.data]) > + resp = self.client.get(self.api_url(), {'project': 'invalidproject'}) > + self.assertEqual(0, len(resp.data)) > + > def test_detail(self): > """Validate we can get a specific patch.""" > patch = create_patch( > -- > 2.13.5
diff --git a/patchwork/api/filters.py b/patchwork/api/filters.py index 9966543..314aa47 100644 --- a/patchwork/api/filters.py +++ b/patchwork/api/filters.py @@ -50,7 +50,7 @@ class ProjectChoiceField(ModelChoiceField): try: filters = {'pk': int(value)} except ValueError: - filters = {'name__iexact': ' '.join(value.split('-'))} + filters = {'linkname__iexact': ' '.join(value.split('-'))} try: value = self.queryset.get(**filters) diff --git a/patchwork/tests/test_rest_api.py b/patchwork/tests/test_rest_api.py index abffd17..14e53b2 100644 --- a/patchwork/tests/test_rest_api.py +++ b/patchwork/tests/test_rest_api.py @@ -97,7 +97,23 @@ class TestProjectAPI(APITestCase): self.assertEqual(status.HTTP_200_OK, resp.status_code) self.assertSerialized(project, resp.data) - def test_get_numeric_linkname(self): + def test_get_by_id(self): + """Validate that it's possible to filter by pk.""" + project = create_project() + + resp = self.client.get(self.api_url(project.pk)) + self.assertEqual(status.HTTP_200_OK, resp.status_code) + self.assertSerialized(project, resp.data) + + def test_get_by_linkname(self): + """Validate that it's possible to filter by linkname.""" + project = create_project(linkname='project', name='Sample project') + + resp = self.client.get(self.api_url('project')) + self.assertEqual(status.HTTP_200_OK, resp.status_code) + self.assertSerialized(project, resp.data) + + def test_get_by_numeric_linkname(self): """Validate we try to do the right thing for numeric linkname""" project = create_project(linkname='12345') @@ -326,7 +342,8 @@ class TestPatchAPI(APITestCase): self.assertEqual(0, len(resp.data)) state_obj = create_state(name='Under Review') - patch_obj = create_patch(state=state_obj) + project_obj = create_project(linkname='myproject') + patch_obj = create_patch(state=state_obj, project=project_obj) # anonymous user resp = self.client.get(self.api_url()) @@ -338,12 +355,6 @@ class TestPatchAPI(APITestCase): self.assertNotIn('content', patch_rsp) self.assertNotIn('diff', patch_rsp) - # test filtering by state - resp = self.client.get(self.api_url(), {'state': 'under-review'}) - self.assertEqual([patch_obj.id], [x['id'] for x in resp.data]) - resp = self.client.get(self.api_url(), {'state': 'missing-state'}) - self.assertEqual(0, len(resp.data)) - # authenticated user user = create_user() self.client.force_authenticate(user=user) @@ -353,6 +364,18 @@ class TestPatchAPI(APITestCase): patch_rsp = resp.data[0] self.assertSerialized(patch_obj, patch_rsp) + # test filtering by state + resp = self.client.get(self.api_url(), {'state': 'under-review'}) + self.assertEqual([patch_obj.id], [x['id'] for x in resp.data]) + resp = self.client.get(self.api_url(), {'state': 'missing-state'}) + self.assertEqual(0, len(resp.data)) + + # test filtering by project + resp = self.client.get(self.api_url(), {'project': 'myproject'}) + self.assertEqual([patch_obj.id], [x['id'] for x in resp.data]) + resp = self.client.get(self.api_url(), {'project': 'invalidproject'}) + self.assertEqual(0, len(resp.data)) + def test_detail(self): """Validate we can get a specific patch.""" patch = create_patch(
Based on a report on the OVS mailing list, it appears that projects are being filtered on the 'Project.name' field instead of the 'Project.linkname' field. Correct this and add regression tests to prevent it happening again. Signed-off-by: Stephen Finucane <stephen@that.guru> Fixes: 08b2115 ("REST: Allow filtering by both project ID and linkname") Closes-bug: #117 ("Projects are filtered on the wrong field") Cc: Daniel Axtens <dja@axtens.net> --- daxtens - if you've time (and this looks good), you might handle the merge and backport part of this. Everything about the latter should be documented in enough detail: https://patchwork.readthedocs.io/en/latest/development/releasing/#backporting Feel free to ping me here on IRC (stephenfin on #freenode) if necessary. --- patchwork/api/filters.py | 2 +- patchwork/tests/test_rest_api.py | 39 +++++++++++++++++++++++++++++++-------- 2 files changed, 32 insertions(+), 9 deletions(-)