Opened 6 years ago

Closed 4 years ago

#15197 closed Bug (fixed)

yaml serialization to HttpResponse fails

Reported by: fourga38 Owned by: Hiroki Kiyohara
Component: Core (Serialization) Version: 1.2
Severity: Normal Keywords: yaml
Cc: hirokiky@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In django.core.serializers.pyyaml, the getvalue() method should check if self.stream has also a getvalue() method, which is not the case of HttpResponse objects.

Attachments (4)

check_stream.patch (564 bytes) - added by Hiroki Kiyohara 4 years ago.
checking self.stream before return
test_serializers_http.patch (1.9 KB) - added by Hiroki Kiyohara 4 years ago.
Added a test using HttpResponse to streamTest
15197-3.diff (3.1 KB) - added by Claude Paroz 4 years ago.
Alternative patch
15197-4.diff (2.8 KB) - added by Claude Paroz 4 years ago.
Patch without HttpResponse.getvalue()

Download all attachments as: .zip

Change History (19)

comment:1 Changed 6 years ago by Russell Keith-Magee

Triage Stage: UnreviewedAccepted

comment:2 Changed 6 years ago by Łukasz Rekucki

Severity: Normal
Type: Bug

comment:3 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:4 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

Changed 4 years ago by Hiroki Kiyohara

Attachment: check_stream.patch added

checking self.stream before return

comment:5 Changed 4 years ago by Hiroki Kiyohara

Cc: hirokiky@… added
Has patch: set

comment:6 Changed 4 years ago by Claude Paroz

Needs tests: set

comment:7 Changed 4 years ago by Hiroki Kiyohara

Owner: changed from nobody to Hiroki Kiyohara

Changed 4 years ago by Hiroki Kiyohara

Attachment: test_serializers_http.patch added

Added a test using HttpResponse to streamTest

comment:8 Changed 4 years ago by anonymous

Needs tests: unset

Changed 4 years ago by Claude Paroz

Attachment: 15197-3.diff added

Alternative patch

comment:9 Changed 4 years ago by Claude Paroz

Thanks for your work. Here is an alternative approach. However, adding a new method to HttpResponse is not to be taken lightly, therefore I'd like to have another core dev approval for this.

comment:10 Changed 4 years ago by Hiroki Kiyohara

Thank you. It's nice to be able to get the value with same method from both StringIO and HttpResponse. We may use them with same name 'stream'. However, I fell, adding a new method to HttpResponse is far from this ticket. This problem should be solved in new ticket (e.g. Adding getvalue to HttpResponse).

comment:11 Changed 4 years ago by Claude Paroz

You're totally right. I'm attaching a revised patch. Ticket #18523 created.

Changed 4 years ago by Claude Paroz

Attachment: 15197-4.diff added

Patch without HttpResponse.getvalue()

comment:12 Changed 4 years ago by Hiroki Kiyohara

Nice. I have been convinced. I wait another core dev approval for this too.

comment:13 Changed 4 years ago by Claude Paroz

It doesn't need a second approval if we don't change HttpResponse API. Just mark it as Ready for checkin and I will commit it soon.

comment:14 Changed 4 years ago by anonymous

Okey, thank you. I'm glad to contribute for Django.

comment:15 Changed 4 years ago by Claude Paroz <claude@…>

Resolution: fixed
Status: newclosed

In [26cb227cfe42d2500eac62dc0d90fb5227b275b3]:

Fixed #15197 -- Fixed yaml serialization into HttpResponse

Thanks fourga38 for the report and hirokiky at gmail.com for the
initial patch.

Note: See TracTickets for help on using tickets.
Back to Top