[Django] #29086: Bytestrings on CharFields getting saved as "b'data'"
Django
-------------------------------------+-------------------------------------
Reporter: Collin | Owner: nobody
Anderson |
Type: Bug | Status: new
Component: Database | Version: 2.0
layer (models, ORM) |
Severity: Release | Keywords:
blocker |
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Django 1.11 behavior (python 3):
{{{
o = MyModel()
o.myfield = b'123'
o.save()
o.refresh_from_db()
o.myfield == "123"
}}}
Django 2.0 behavior:
{{{
o = MyModel()
o.myfield = b'123'
o.save()
o.refresh_from_db()
o.myfield == "b'123'"
}}}
Could we restore the old behavior, maybe with a deprecation warning?
--
Ticket URL: <https://code.djangoproject.com/ticket/29086>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
> Django 1.11 behavior (python 3):
> {{{
> o = MyModel()
> o.myfield = b'123'
> o.save()
> o.refresh_from_db()
> o.myfield == "123"
> }}}
>
> Django 2.0 behavior:
> {{{
> o = MyModel()
> o.myfield = b'123'
> o.save()
> o.refresh_from_db()
> o.myfield == "b'123'"
> }}}
>
> Could we restore the old behavior, maybe with a deprecation warning?
New description:
Django 1.11 behavior (python 3):
{{{
o = MyModel()
o.myfield = b'123'
o.save()
o.refresh_from_db()
o.myfield == "123"
}}}
Django 2.0 behavior:
{{{
o = MyModel()
o.myfield = b'123'
o.save()
o.refresh_from_db()
o.myfield == "b'123'"
}}}
Could we restore the old behavior, maybe with a deprecation warning?
Either that or raise an error?
--
--
Ticket URL: <https://code.djangoproject.com/ticket/29086#comment:1>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Collin Anderson (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/29086#comment:2>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
How does that mistake (assigning bytes) happen? I doubt that it's very
common but am open to being persuaded otherwise.
--
Ticket URL: <https://code.djangoproject.com/ticket/29086#comment:3>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
As I've mentioned in
[https://github.com/django/django/pull/9617#issuecomment-360546339
#28748's PR] I feel like this should be the the correct behavior in a
Python 3 only code base and this was probably caused by an overlooked case
during the porting of the affected code bases from Python 2 to Python 3.
In other words, if you are passing binary data to an interface expecting
text you should expect bad things to happen -- Python 3 is stricter about
this concept and it's a good thing IMHO.
Now, this change should at least have been mentioned in the 2.0 release
notes. The fact it wasn't makes me think there might other instances
changed by #27795 related commits that might have introduced similar
''regressions''.
--
Ticket URL: <https://code.djangoproject.com/ticket/29086#comment:4>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jon Dufresne):
> Now, this change should at least have been mentioned in the 2.0 release
notes.
The [https://docs.djangoproject.com/en/dev/releases/2.0/#removed-support-
for-bytestrings-in-some-places release notes say]:
> To support native Python 2 strings, older Django versions had to accept
both bytestrings and unicode strings. Now that Python 2 support is
dropped, bytestrings should only be encountered around input/output
boundaries (handling of binary fields or HTTP streams, for example). You
might have to update your code to limit bytestring usage to a minimum, as
Django no longer accepts bytestrings in certain code paths.
>
> For example, `reverse()` now uses `str()` instead of `force_text()` to
coerce the `args` and `kwargs` it receives, prior to their placement in
the URL. For bytestrings, this creates a string with an undesired `b`
prefix as well as additional quotes (`str(b'foo')` is `"b'foo'"`). To
adapt, call `decode()` on the bytestring before passing it to `reverse()`.
Does that cover what you're looking for?
--
Ticket URL: <https://code.djangoproject.com/ticket/29086#comment:5>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Collin Anderson):
"Django no longer accepts bytestrings in certain code paths"
"Python 3 is stricter about this concept and it's a good thing IMHO." -
Right, it raises an error when you pass bytes, it seems to me Django
should do the same thing.
It seems to me the current behavior is really bad for projects that are
upgrading. Maybe the force_text() calls (#27795) need to be
bytes_not_allowed() or something. (Too late to raise an error for 2.0, but
maybe we could start raising a warning?)
--
Ticket URL: <https://code.djangoproject.com/ticket/29086#comment:6>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jon Dufresne):
> It seems to me the current behavior is really bad for projects that are
upgrading, because there's no warning or error, just silent bad data.
Maybe the force_text() calls (#27795) need to be bytes_not_allowed() or
something. (Too late to raise an error for 2.0, but maybe we could start
raising a warning?)
This idea of warn/error when mixing bytes with text is actually built into
Python with the [https://docs.python.org/3/using/cmdline.html#cmdoption-b
`-b` and `-bb` options]. I've used it to great success in my project to
discover mishandling of bytes and text. I don't think Django needs to
duplicate this functionality, but maybe the release notes could be
modified to recommend using `-b` to help discover missed cases when
upgrading.
--
Ticket URL: <https://code.djangoproject.com/ticket/29086#comment:7>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
Thanks for release note mention Jon, I completely missed it when I skimmed
through the docs.
+1 on mentioning the `-b` option!
--
Ticket URL: <https://code.djangoproject.com/ticket/29086#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Collin Anderson | Owner: nobody
Component: Database layer | Version: 2.0
(models, ORM) |
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => wontfix
Comment:
[https://github.com/django/django/pull/9637 PR] to mention `-b` in the
release notes.
--
Ticket URL: <https://code.djangoproject.com/ticket/29086#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Collin Anderson | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Release blocker | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by GitHub <noreply@…>):
In [changeset:"c10cb9716f8fd7398a8206cd8b33ed2f03065f85" c10cb97]:
{{{
#!CommitTicketReference repository=""
revision="c10cb9716f8fd7398a8206cd8b33ed2f03065f85"
Refs #29086 -- Doc'd how to detect bytestring mistakes.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29086#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Collin Anderson | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Release blocker | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"ddc49820f7716a9e521e8bafda97294065d47b93" ddc4982]:
{{{
#!CommitTicketReference repository=""
revision="ddc49820f7716a9e521e8bafda97294065d47b93"
[2.0.x] Refs #29086 -- Doc'd how to detect bytestring mistakes.
Backport of c10cb9716f8fd7398a8206cd8b33ed2f03065f85 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29086#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Collin Anderson | Owner: nobody
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Release blocker | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: wontfix =>
Comment:
I'd like to re-open this for discussion, if possible.
Ideally, saving bytes to a CharField wouldn't give a warning. It would
just trust that the developer knows what they're doing. Namely, that they
took care of the decode step, and to just pass the bytes right onto the
database.
I have a situation here where I have a CharField, I have some byte data, I
know it's encoded in the same manner as the destination varchar field in
the database. All these things are known to me. And yet, Django attempts
to coerce the byte value into a string (adding a "b" in front and wrapping
the thing in quotes as it goes), just before it's converted ''back'' into
bytes when its sent to the database. That doesn't really make sense.
The best course of action here is to just trust the developer. If the
CharField value is bytes, leave it be.
--
Ticket URL: <https://code.djangoproject.com/ticket/29086#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Collin Anderson | Owner: nobody
Component: Database layer | Version: 2.0
(models, ORM) |
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => wontfix
Comment:
Trac is not a place for a discussion.
[https://docs.djangoproject.com/en/stable/internals/contributing/triaging-
tickets/#closing-tickets Follow triaging guidelines with regards to
wontfix tickets.]
--
Ticket URL: <https://code.djangoproject.com/ticket/29086#comment:13>