[PATCH] REST: Don't iterate through models at class level

[Stephen Finucane]

On Tue, 2017-02-07 at 07:42 +1100, Daniel Axtens wrote:
> Hi Stephen,
> 
> > This causes two issues. Firstly, on fresh installs you see the
> > following
> > error message:
> > 
> >   "Table 'patchwork.patchwork_state' doesn't exist"
> 
> Weird, I do a database reset quite often and haven't seen
> this. (Although I'm not sure I have done that since the REST stuff
> went
> in...)

I haven't seen it either, but I haven't reset in a while. I've had a
few reports of this so it's definitely an issue.

> This also seems very very similar to a bug I fixed in
> 231966452f22 ("Fix failure to start with uninitalised database"):
> should we fix both in the same way? If we could avoid the ugly hack
> in
> my patch that would be pretty great.

It's pretty much the same thing. I've have a look and submit a cleanup
patch if we can use the same approach there.

> > Secondly, any new states created when the process is running will
> > not be
> > reflected in the API until the server process is restarted.
> > 
> > Resolve this issue by moving the step into a method, thus ensuring
> > it's
> > continuously refreshed. It doesn't seem possible to add tests to
> > prevent
> > this regressing but some similarly useful tests are included to at
> > least
> > validate the behavior of that field.
> 
> What's the expected db overhead of doing this? It's one extra call
> (possibly cached) per instance of StateField?

I don't think there'll be any additional overhead, seeing as we call
'select_related' as part of the queryset for that API [1]

[1] https://github.com/getpatchwork/patchwork/blob/83a63a2a/patchwork/a
pi/patch.py#L129

> > Signed-off-by: Stephen Finucane <stephen at that.guru>
> > Fixes: a2993505 ("REST: Make 'Patch.state' editable")
> > Closes-bug: #67
> > Closes-bug: #80
> 
> I thought GitHub parsed on "Closes:", but I could be wrong...

It parses 'Closes #67', e.g. no colon. I'm happy to keep the colon,
close the bug manually, and satisfy the side of me that insists tags
have colons after them :)

> > diff --git a/patchwork/tests/test_rest_api.py
> > b/patchwork/tests/test_rest_api.py
> > index cc1fcef..ace84cf 100644
> > --- a/patchwork/tests/test_rest_api.py
> > +++ b/patchwork/tests/test_rest_api.py
> > @@ -398,6 +398,20 @@ class TestPatchAPI(APITestCase):
> >          self.assertEqual(status.HTTP_200_OK, resp.status_code)
> >          self.assertEqual(Patch.objects.get(id=patch.id).state,
> > state)
> >  
> > +    def test_update_invalid(self):
> > +        """Ensure StateField is working as expected."""
> 
> Hmm. So your name suggests this tests for invalid updates, yet the
> doc
> string is more broad...
> 
> > +        project = create_project()
> > +        patch = create_patch(project=project)
> > +        user = create_maintainer(project)
> > +
> > +        self.client.force_authenticate(user=user)
> > +        resp = self.client.get(self.api_url(patch.id))
> > +        self.assertEqual(status.HTTP_200_OK, resp.status_code)
> 
> This is a very generic API test - is it duplicating any other test?
> Does
> it belong in a more generic method?

Oops, good spot. Those lines can go, as they're remnants of an attempt
to load the STATE_MAP and provide a regression test (which doesn't
appear to be possible).

> > +
> > +        # invalid state
> > +        resp = self.client.patch(self.api_url(patch.id), {'state':
> > 'foobar'})
> > +        self.assertEqual(status.HTTP_400_BAD_REQUEST,
> > resp.status_code)
> 
> This bit I approve of :)

Excellent :)

Stephen