Opened 14 years ago

Closed 12 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 12 years ago.
checking self.stream before return
test_serializers_http.patch​ (1.9 KB ) - added by Hiroki Kiyohara 12 years ago.
Added a test using HttpResponse to streamTest
15197-3.diff​ (3.1 KB ) - added by Claude Paroz 12 years ago.
Alternative patch
15197-4.diff​ (2.8 KB ) - added by Claude Paroz 12 years ago.
Patch without HttpResponse.getvalue()

Download all attachments as: .zip

Change History (19)

comment:1 by Russell Keith-Magee, 14 years ago

Triage Stage: Unreviewed → Accepted

comment:2 by Łukasz Rekucki, 14 years ago

Severity: → Normal
Type: → Bug

comment:3 by Aymeric Augustin, 13 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:4 by Aymeric Augustin, 13 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

by Hiroki Kiyohara, 12 years ago

Attachment: check_stream.patch​ added

checking self.stream before return

comment:5 by Hiroki Kiyohara, 12 years ago

Cc: hirokiky@… added
Has patch: set

comment:6 by Claude Paroz, 12 years ago

Needs tests: set

comment:7 by Hiroki Kiyohara, 12 years ago

Owner: changed from nobody to Hiroki Kiyohara

by Hiroki Kiyohara, 12 years ago

Attachment: test_serializers_http.patch​ added

Added a test using HttpResponse to streamTest

comment:8 by anonymous, 12 years ago

Needs tests: unset

by Claude Paroz, 12 years ago

Attachment: 15197-3.diff​ added

Alternative patch

comment:9 by Claude Paroz, 12 years ago

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 by Hiroki Kiyohara, 12 years ago

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 by Claude Paroz, 12 years ago

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

by Claude Paroz, 12 years ago

Attachment: 15197-4.diff​ added

Patch without HttpResponse.getvalue()

comment:12 by Hiroki Kiyohara, 12 years ago

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

comment:13 by Claude Paroz, 12 years ago

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 by anonymous, 12 years ago

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

comment:15 by Claude Paroz <claude@…>, 12 years ago

Resolution: → fixed
Status: new → closed

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