[PATCHv2 08/10] REST API: make patch.state a string rather than int

[Finucane, Stephen]

On 10 May 17:39, Andy Doan wrote:
> The int value isn't very useful and the goal is to move this to an enum
> over time.
> 
> Signed-off-by: Andy Doan <andy.doan at linaro.org>

Some questions over performance below.

Stephen

> ---
>  patchwork/rest_serializers.py    | 21 ++++++++++++++++++++-
>  patchwork/tests/test_rest_api.py |  8 +++++---
>  2 files changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/patchwork/rest_serializers.py b/patchwork/rest_serializers.py
> index 6760948..f189e89 100644
> --- a/patchwork/rest_serializers.py
> +++ b/patchwork/rest_serializers.py
> @@ -17,7 +17,7 @@
>  # along with Patchwork; if not, write to the Free Software
>  # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>  
> -from patchwork.models import Check, Patch, Person, Project
> +from patchwork.models import Check, Patch, Person, Project, State
>  
>  from rest_framework.serializers import (
>      CurrentUserDefault, ModelSerializer, HiddenField, PrimaryKeyRelatedField,
> @@ -41,6 +41,25 @@ class PatchSerializer(ModelSerializer):
>                              'content', 'hash', 'msgid')
>      mbox_url = SerializerMethodField()
>  
> +    def run_validation(self, data):
> +        for state in State.objects.all():
> +            if state.name == data['state']:
> +                data['state'] = state.id
> +                break
> +        return super(PatchSerializer, self).run_validation(data)
> +
> +    def to_representation(self, instance):
> +        data = super(PatchSerializer, self).to_representation(instance)
> +        # an instance of this object lives across a single http request, so
> +        # we can cache patch states once and not do repeated lookups
> +        if not getattr(self, 'patch_states_cache', None):
> +            self.patch_states_cache = [
> +                (x.id, x.name) for x in State.objects.all()]

I haven't benchmarked this yet, but won't this result in a query on the
State table every time? How about adding 'state__name' to the queried
fields and using 'prefetch_related' instead? Alternatively, store the
cache as part of the class instead (a singleton pattern?).

> +        for state, name in self.patch_states_cache:
> +            if state == data['state']:
> +                data['state'] = name
> +        return data
> +
>      def get_mbox_url(self, patch):
>          return patch.get_mbox_url()