[Django] #35648: SafeString and overriding addition do not work well together

39 views
Skip to first unread message

Django

unread,
Jul 31, 2024, 11:49:43 AM7/31/24
to django-...@googlegroups.com
#35648: SafeString and overriding addition do not work well together
-------------------------------------+-------------------------------------
Reporter: Matthias | Owner: Matthias Kestenholz
Kestenholz |
Type: Bug | Status: assigned
Component: Utilities | Version: 5.0
Severity: Normal | Keywords:
Triage Stage: | Has patch: 1
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
When you have an object which is added to a string you can override
`__radd__`.

However, the current implementation of `SafeString.__add__`
unconditionally calls `super().__add__`, which means that the RHS doesn't
get a chance at handling the addition. I propose making
`SafeString.__add__` a bit more defensive and only call `super().__add__`
if it actually knows that it can handle the type.

The full diff of my proposed change is here:
https://github.com/django/django/compare/main...matthiask:django:safe-
string-add-safety

The first commit is
https://github.com/django/django/commit/2a118c2bec3e2952b7ab344e12e95cf42554cd5b

It shows that overriding `__radd__` works with `str` objects on the LHS
but doesn't work with `SafeString` objects.

The second commit is
https://github.com/django/django/commit/61767c66c00323b7b862d812679879a4fdc47a43

It allows the test to pass. I'm not 100% sure the proposed `__add__`
implementation handles everything it should.
--
Ticket URL: <https://code.djangoproject.com/ticket/35648>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

Django

unread,
Jul 31, 2024, 11:51:31 AM7/31/24
to django-...@googlegroups.com
#35648: SafeString and overriding addition do not work well together
-------------------------------------+-------------------------------------
Reporter: Matthias Kestenholz | Owner: Matthias
| Kestenholz
Type: Bug | Status: assigned
Component: Utilities | Version: 5.0
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Matthias Kestenholz):

[https://github.com/django/django/pull/18434 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/35648#comment:1>

Django

unread,
Jul 31, 2024, 3:39:16 PM7/31/24
to django-...@googlegroups.com
#35648: SafeString and overriding addition do not work well together
-------------------------------------+-------------------------------------
Reporter: Matthias Kestenholz | Owner: Matthias
| Kestenholz
Type: Bug | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: safestring | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Natalia Bidart):

* keywords: => safestring
* stage: Unreviewed => Accepted
* version: 5.0 => dev

Comment:

Hello Matthias! Thank you for taking the time to create this ticket and
the corresponding PR.

I'm accepting on the basis that I agree with your reasoning that it is
expected that a `SafeString` instance should work and operate like any
`str` since
[https://docs.djangoproject.com/en/5.0/ref/utils/#django.utils.safestring.SafeString
the docs] clearly say:

> A str subclass that has been specifically marked as “safe” (requires no
further escaping) for HTML output purposes.
--
Ticket URL: <https://code.djangoproject.com/ticket/35648#comment:2>

Django

unread,
Jul 31, 2024, 3:55:30 PM7/31/24
to django-...@googlegroups.com
#35648: SafeString and overriding addition do not work well together
-------------------------------------+-------------------------------------
Reporter: Matthias Kestenholz | Owner: Matthias
| Kestenholz
Type: Bug | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: safestring | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Natalia Bidart):

* needs_better_patch: 0 => 1
* needs_docs: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/35648#comment:3>

Django

unread,
Jul 31, 2024, 4:07:32 PM7/31/24
to django-...@googlegroups.com
#35648: SafeString and overriding addition do not work well together
-------------------------------------+-------------------------------------
Reporter: Matthias Kestenholz | Owner: Matthias
| Kestenholz
Type: Bug | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: safestring | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Natalia Bidart):

Similar-ish to #26287.
--
Ticket URL: <https://code.djangoproject.com/ticket/35648#comment:4>

Django

unread,
Aug 1, 2024, 2:15:13 AM8/1/24
to django-...@googlegroups.com
#35648: SafeString and overriding addition do not work well together
-------------------------------------+-------------------------------------
Reporter: Matthias Kestenholz | Owner: Matthias
| Kestenholz
Type: Bug | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: safestring | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Matthias Kestenholz):

* needs_better_patch: 1 => 0
* needs_docs: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/35648#comment:5>

Django

unread,
Aug 9, 2024, 10:57:02 AM8/9/24
to django-...@googlegroups.com
#35648: SafeString and overriding addition do not work well together
-------------------------------------+-------------------------------------
Reporter: Matthias Kestenholz | Owner: Matthias
| Kestenholz
Type: Bug | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: safestring | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Natalia Bidart):

* needs_better_patch: 0 => 1

Comment:

Branch is looking good! I requested a few small tweaks to tests.
--
Ticket URL: <https://code.djangoproject.com/ticket/35648#comment:6>

Django

unread,
Aug 9, 2024, 11:46:19 AM8/9/24
to django-...@googlegroups.com
#35648: SafeString and overriding addition do not work well together
-------------------------------------+-------------------------------------
Reporter: Matthias Kestenholz | Owner: Matthias
| Kestenholz
Type: Bug | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: safestring | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Matthias Kestenholz):

* needs_better_patch: 1 => 0

Comment:

I have applied the requested changes (I hope!) so I'm going to unset the
flag again.
--
Ticket URL: <https://code.djangoproject.com/ticket/35648#comment:7>

Django

unread,
Aug 12, 2024, 12:24:27 PM8/12/24
to django-...@googlegroups.com
#35648: SafeString and overriding addition do not work well together
-------------------------------------+-------------------------------------
Reporter: Matthias Kestenholz | Owner: Matthias
| Kestenholz
Type: Bug | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: safestring | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Natalia Bidart):

* stage: Accepted => Ready for checkin

Comment:

Will merge when CI is green.
--
Ticket URL: <https://code.djangoproject.com/ticket/35648#comment:8>

Django

unread,
Aug 12, 2024, 1:25:15 PM8/12/24
to django-...@googlegroups.com
#35648: SafeString and overriding addition do not work well together
-------------------------------------+-------------------------------------
Reporter: Matthias Kestenholz | Owner: Matthias
| Kestenholz
Type: Bug | Status: closed
Component: Utilities | Version: dev
Severity: Normal | Resolution: fixed
Keywords: safestring | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by nessita <124304+nessita@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"d84200e4eb4d20116080130c612ba157a4718977" d84200e4]:
{{{#!CommitTicketReference repository=""
revision="d84200e4eb4d20116080130c612ba157a4718977"
Fixed #35648 -- Raised NotImplementedError in SafeString.__add__ for non-
string RHS.

This change ensures SafeString addition operations handle non-string RHS
properly, allowing them to implement __radd__ for better compatibility.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35648#comment:10>

Django

unread,
Aug 12, 2024, 1:25:15 PM8/12/24
to django-...@googlegroups.com
#35648: SafeString and overriding addition do not work well together
-------------------------------------+-------------------------------------
Reporter: Matthias Kestenholz | Owner: Matthias
| Kestenholz
Type: Bug | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: safestring | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by nessita <124304+nessita@…>):

In [changeset:"b5c048f5ecbf1c718102f19c3018b1fb62d66b3d" b5c048f5]:
{{{#!CommitTicketReference repository=""
revision="b5c048f5ecbf1c718102f19c3018b1fb62d66b3d"
Refs #35648 -- Added test for addition between SafeString and str in
utils_tests.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35648#comment:9>

Django

unread,
Aug 12, 2024, 2:56:09 PM8/12/24
to django-...@googlegroups.com
#35648: SafeString and overriding addition do not work well together
-------------------------------------+-------------------------------------
Reporter: Matthias Kestenholz | Owner: Matthias
| Kestenholz
Type: Bug | Status: closed
Component: Utilities | Version: dev
Severity: Normal | Resolution: fixed
Keywords: safestring | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by GitHub <noreply@…>):

In [changeset:"6f0a4c1f3f015f917feb8b34ae24232d14ced04a" 6f0a4c1]:
{{{#!CommitTicketReference repository=""
revision="6f0a4c1f3f015f917feb8b34ae24232d14ced04a"
Refs #35648 -- Corrected release notes for SafeString.__add__() changes.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35648#comment:11>
Reply all
Reply to author
Forward
0 new messages