api: Show all headers with the same key

Message ID 20180406112449.17184-1-vkabatov@redhat.com
State Accepted
Headers show
Series
  • api: Show all headers with the same key
Related show

Commit Message

Veronika Kabatova April 6, 2018, 11:24 a.m.
From: Veronika Kabatova <vkabatov@redhat.com>

While the code on our side returns all (key, value) pairs for email
headers, Django's REST framework probably uses dictionaries behind the
scenes. This means that having multiple headers with same key (eg
'Received', which is totally valid and common situation), only one of
these headers is visible in the REST API.

Let's hack around this by returning a list of values in case the key is
present multiple times.

Signed-off-by: Veronika Kabatova <vkabatov@redhat.com>
---
 patchwork/api/cover.py | 12 +++++++++++-
 patchwork/api/patch.py | 12 +++++++++++-
 2 files changed, 22 insertions(+), 2 deletions(-)

Comments

Stephen Finucane April 24, 2018, 10:23 p.m. | #1
On Fri, 2018-04-06 at 13:24 +0200, vkabatov@redhat.com wrote:
> From: Veronika Kabatova <vkabatov@redhat.com>
> 
> While the code on our side returns all (key, value) pairs for email
> headers, Django's REST framework probably uses dictionaries behind the
> scenes. This means that having multiple headers with same key (eg
> 'Received', which is totally valid and common situation), only one of
> these headers is visible in the REST API.
> 
> Let's hack around this by returning a list of values in case the key is
> present multiple times.
> 
> Signed-off-by: Veronika Kabatova <vkabatov@redhat.com>

Sorry for the delay in getting to this. This looks good but I feel it
needs a regression test. If you could provide one, I'd be happy to
squash it in here and apply this.

Cheers,
Stephen

> ---
>  patchwork/api/cover.py | 12 +++++++++++-
>  patchwork/api/patch.py | 12 +++++++++++-
>  2 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/patchwork/api/cover.py b/patchwork/api/cover.py
> index 1064504..dd1db97 100644
> --- a/patchwork/api/cover.py
> +++ b/patchwork/api/cover.py
> @@ -57,8 +57,18 @@ class CoverLetterDetailSerializer(CoverLetterListSerializer):
>      headers = SerializerMethodField()
>  
>      def get_headers(self, instance):
> +        headers = {}
> +
>          if instance.headers:
> -            return email.parser.Parser().parsestr(instance.headers, True)
> +            parsed = email.parser.Parser().parsestr(instance.headers, True)
> +            for key in parsed.keys():
> +                headers[key] = parsed.get_all(key)
> +                # Let's return a single string instead of a list if only one
> +                # header with this key is present
> +                if len(headers[key]) == 1:
> +                    headers[key] = headers[key][0]
> +
> +        return headers
>  
>      class Meta:
>          model = CoverLetter
> diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py
> index 115feff..645b0e9 100644
> --- a/patchwork/api/patch.py
> +++ b/patchwork/api/patch.py
> @@ -123,8 +123,18 @@ class PatchDetailSerializer(PatchListSerializer):
>      prefixes = SerializerMethodField()
>  
>      def get_headers(self, patch):
> +        headers = {}
> +
>          if patch.headers:
> -            return email.parser.Parser().parsestr(patch.headers, True)
> +            parsed = email.parser.Parser().parsestr(patch.headers, True)
> +            for key in parsed.keys():
> +                headers[key] = parsed.get_all(key)
> +                # Let's return a single string instead of a list if only one
> +                # header with this key is present
> +                if len(headers[key]) == 1:
> +                    headers[key] = headers[key][0]
> +
> +        return headers
>  
>      def get_prefixes(self, instance):
>          return clean_subject(instance.name)[1]
Stephen Finucane April 25, 2018, 8:09 p.m. | #2
On Tue, 2018-04-24 at 23:23 +0100, Stephen Finucane wrote:
> On Fri, 2018-04-06 at 13:24 +0200, vkabatov@redhat.com wrote:
> > From: Veronika Kabatova <vkabatov@redhat.com>
> > 
> > While the code on our side returns all (key, value) pairs for email
> > headers, Django's REST framework probably uses dictionaries behind the
> > scenes. This means that having multiple headers with same key (eg
> > 'Received', which is totally valid and common situation), only one of
> > these headers is visible in the REST API.
> > 
> > Let's hack around this by returning a list of values in case the key is
> > present multiple times.
> > 
> > Signed-off-by: Veronika Kabatova <vkabatov@redhat.com>
> 
> Sorry for the delay in getting to this. This looks good but I feel it
> needs a regression test. If you could provide one, I'd be happy to
> squash it in here and apply this.

With the follow up patch added to this, I'm happy to apply it.

Reviewed-by: Stephen Finucane <stephen@that.guru>

Patch

diff --git a/patchwork/api/cover.py b/patchwork/api/cover.py
index 1064504..dd1db97 100644
--- a/patchwork/api/cover.py
+++ b/patchwork/api/cover.py
@@ -57,8 +57,18 @@  class CoverLetterDetailSerializer(CoverLetterListSerializer):
     headers = SerializerMethodField()
 
     def get_headers(self, instance):
+        headers = {}
+
         if instance.headers:
-            return email.parser.Parser().parsestr(instance.headers, True)
+            parsed = email.parser.Parser().parsestr(instance.headers, True)
+            for key in parsed.keys():
+                headers[key] = parsed.get_all(key)
+                # Let's return a single string instead of a list if only one
+                # header with this key is present
+                if len(headers[key]) == 1:
+                    headers[key] = headers[key][0]
+
+        return headers
 
     class Meta:
         model = CoverLetter
diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py
index 115feff..645b0e9 100644
--- a/patchwork/api/patch.py
+++ b/patchwork/api/patch.py
@@ -123,8 +123,18 @@  class PatchDetailSerializer(PatchListSerializer):
     prefixes = SerializerMethodField()
 
     def get_headers(self, patch):
+        headers = {}
+
         if patch.headers:
-            return email.parser.Parser().parsestr(patch.headers, True)
+            parsed = email.parser.Parser().parsestr(patch.headers, True)
+            for key in parsed.keys():
+                headers[key] = parsed.get_all(key)
+                # Let's return a single string instead of a list if only one
+                # header with this key is present
+                if len(headers[key]) == 1:
+                    headers[key] = headers[key][0]
+
+        return headers
 
     def get_prefixes(self, instance):
         return clean_subject(instance.name)[1]