#35648 closed Bug (fixed)
SafeString and overriding addition do not work well together
| Reported by: | Matthias Kestenholz | Owned by: | Matthias Kestenholz |
|---|---|---|---|
| Component: | Utilities | Version: | dev |
| Severity: | Normal | Keywords: | safestring |
| Cc: | Triage Stage: | Ready for checkin | |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
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.
Change History (11)
comment:1 by , 15 months ago
comment:2 by , 15 months ago
| Keywords: | safestring added |
|---|---|
| Triage Stage: | Unreviewed → Accepted |
| Version: | 5.0 → dev |
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 the docs clearly say:
A str subclass that has been specifically marked as “safe” (requires no further escaping) for HTML output purposes.
comment:3 by , 15 months ago
| Needs documentation: | set |
|---|---|
| Patch needs improvement: | set |
comment:5 by , 15 months ago
| Needs documentation: | unset |
|---|---|
| Patch needs improvement: | unset |
comment:6 by , 15 months ago
| Patch needs improvement: | set |
|---|
Branch is looking good! I requested a few small tweaks to tests.
comment:7 by , 15 months ago
| Patch needs improvement: | unset |
|---|
I have applied the requested changes (I hope!) so I'm going to unset the flag again.
comment:8 by , 15 months ago
| Triage Stage: | Accepted → Ready for checkin |
|---|
Will merge when CI is green.
PR