Code

Opened 3 years ago

Closed 2 years ago

#15197 closed Bug (fixed)

yaml serialization to HttpResponse fails

Reported by: fourga38 Owned by: hirokiky
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 hirokiky 2 years ago.
checking self.stream before return
test_serializers_http.patch (1.9 KB) - added by hirokiky 2 years ago.
Added a test using HttpResponse to streamTest
15197-3.diff (3.1 KB) - added by claudep 2 years ago.
Alternative patch
15197-4.diff (2.8 KB) - added by claudep 2 years ago.
Patch without HttpResponse.getvalue()

Download all attachments as: .zip

Change History (19)

comment:1 Changed 3 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 3 years ago by lrekucki

  • Severity set to Normal
  • Type set to Bug

comment:3 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:4 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

Changed 2 years ago by hirokiky

checking self.stream before return

comment:5 Changed 2 years ago by hirokiky

  • Cc hirokiky@… added
  • Has patch set

comment:6 Changed 2 years ago by claudep

  • Needs tests set

comment:7 Changed 2 years ago by hirokiky

  • Owner changed from nobody to hirokiky

Changed 2 years ago by hirokiky

Added a test using HttpResponse to streamTest

comment:8 Changed 2 years ago by anonymous

  • Needs tests unset

Changed 2 years ago by claudep

Alternative patch

comment:9 Changed 2 years ago by claudep

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 2 years ago by hirokiky

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 2 years ago by claudep

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

Changed 2 years ago by claudep

Patch without HttpResponse.getvalue()

comment:12 Changed 2 years ago by hirokiky

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

comment:13 Changed 2 years ago by claudep

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

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

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

  • Resolution set to fixed
  • Status changed from new to closed

In [26cb227cfe42d2500eac62dc0d90fb5227b275b3]:

Fixed #15197 -- Fixed yaml serialization into HttpResponse

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.