[Django] #32508: Add a system check that 'assert' is not disabled
Django
-----------------------------------------+------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
Python's -O flag strips 'assert' statements:
https://docs.python.org/3/using/cmdline.html#cmdoption-o . A naive user
might enable this flag to try "make their code go faster", whilst any gain
would be very marginal.
Django currently has 89 assert statements guarding against various kinds
of bad data. It's also common for library or project code to use 'assert'
without realizing it could be turned off.
I propose adding a system check to warn or error if the assert statement
doesn't work. This can be checked with something like:
{{{
try:
assert False
except AssertionError:
pass
else:
errors.append(checks.Error("Django relies on the 'assert' statement,
do not disable it with 'python -O'"))
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32508>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Adam Johnson | Owner: nobody
Type: Uncategorized | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
An alternative to this problem could also be to replace these `assert` by
proper exceptions instead.
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:1>
Django
--------------------------------------+------------------------------------
Reporter: Adam Johnson | Owner: nobody
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* type: Uncategorized => Cleanup/optimization
* component: Uncategorized => Core (Other)
* stage: Unreviewed => Accepted
Comment:
I agree with Simon. We don't add `assert` to Django anymore, so I would
prefer to replace these by proper exceptions (mostly `TypeError` and
`ValueError`).
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:2>
Django
--------------------------------------+------------------------------------
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
I agree anything user-facing should be an exception, but it might not be
necessary to replace all usages. For example, there might be `assert`
conditions in the ORM that aren't expected to be triggered unless there's
some bug in Django. In this case, the `assert` merely helps when
refactoring, etc.
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:3>
Django
--------------------------------------+------------------------------------
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Chris Jerdonek):
I agree with Tim. Before proceeding too far with this, it might be worth
documenting in the
[https://docs.djangoproject.com/en/dev/internals/contributing/writing-code
/coding-style/ coding style guidelines] when `assert` is appropriate so
you have something to judge cases by. It's useful e.g. when you know an
invariant should be preserved by Django, but you don't want to regularly
incur the performance hit when invoked with `-O`.
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:4>
Django
--------------------------------------+------------------------------------
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Adam Johnson):
Fair enough, I agree that removing such usage is probably just as valid.
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:5>
Django
--------------------------------------+------------------------------------
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak):
I added [https://github.com/django/django/pull/14099 PR] for a good start.
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:6>
Django
--------------------------------------+------------------------------------
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
> It's also common for library...
Just as a data-point, DRF makes extensive use of assert for this purpose.
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:7>
Django
--------------------------------------+------------------------------------
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by GitHub <noreply@…>):
In [changeset:"ba9a2b754452b542d3f472f0acce6f940911aced" ba9a2b75]:
{{{
#!CommitTicketReference repository=""
revision="ba9a2b754452b542d3f472f0acce6f940911aced"
Refs #32508 -- Raised TypeError instead of using "assert" on unsupported
operations for sliced querysets.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:8>
Django
--------------------------------------+------------------------------------
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Adam Johnson):
> Just as a data-point, DRF makes extensive use of assert for this
purpose.
Would you make a ticket for DRF? Or is it just too much work?
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:9>
Django
--------------------------------------+------------------------------------
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
**Maybe** a system check for DRF is a good idea…
I can't quite decide. I've never run `python -O`. It sort of misses the
point of Python IMO... — but if someone were to do it, yes. I guess a PR
with it fully in place would be better than a ticket. 😀
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:10>
Django
--------------------------------------+------------------------------------
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Hasan Ramezani):
[https://github.com/django/django/pull/14107 PR] for changing asserts in
admindoc and auth middlewares
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:11>
Django
--------------------------------------+------------------------------------
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Adam Johnson):
> It sort of misses the point of Python IMO...
Sure, but that doesn't stop someone seeing their code is slow, trying it,
and leaving it in place.
> Maybe a system check for DRF is a good idea…
If a system check is a good idea on DRF I don't see why it wouldn't be a
good idea in Django itself. Putting it in DRF it would effectively force
79.5%* of the ecosystem to not use `-O`, so we'd be most of the way. Even
if django itself stops using `assert` there's an unknown amount of code in
third party libraries etc. using it.
*2020 survey stat
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:12>
Django
--------------------------------------+------------------------------------
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
> If a system check is a good idea on DRF...
Yeah... **if** — note I said **maybe** (with emphasis). I'm kind of -0 on
this (for here or there) — I don't think we can sustainably add system
checks for every which way you might possibly shoot yourself in the foot.
If using `-O` is a bad move, that's something that Python should warn
about no?
I don't know that the issue tracker is the best place to knock this back
and forth. The first four responses here seem against adding a system
check, without at least doing various other things first, so I think we're
a way from a consensus **to add** one…
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:13>
Django
--------------------------------------+------------------------------------
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"a2d5ea626ec6aa4eab6b018c4bce58cb27e20676" a2d5ea62]:
{{{
#!CommitTicketReference repository=""
revision="a2d5ea626ec6aa4eab6b018c4bce58cb27e20676"
Refs #32508 -- Raised ImproperlyConfigured instead of using "assert" in
middlewares.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:14>
Django
--------------------------------------+------------------------------------
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Adam Johnson):
> I don't think we can sustainably add system checks for every which way
you might possibly shoot yourself in the foot.
True - I just get tired of repairing feet 😅
> I don't know that the issue tracker is the best place to knock this back
and forth.
Agree. Let's just stick with the current proposal to remove use of assert
in prod code. I made an issue for DRF: https://github.com/encode/django-
rest-framework/issues/7831
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:15>
Django
--------------------------------------+------------------------------------
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Hasan Ramezani):
I've created a [https://github.com/django/django/pull/14114 PR] to replace
`assert` in `django.utils`
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:16>
Django
--------------------------------------+------------------------------------
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"775b796d8d13841059850d73986d5dcc2e593077" 775b796]:
{{{
#!CommitTicketReference repository=""
revision="775b796d8d13841059850d73986d5dcc2e593077"
Refs #32508 -- Raised ValueError instead of using "assert" in lazy().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:17>
Django
--------------------------------------+------------------------------------
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"54d91795408c878ad335226a87a44961d7f646a9" 54d91795]:
{{{
#!CommitTicketReference repository=""
revision="54d91795408c878ad335226a87a44961d7f646a9"
Refs #32508 -- Raised ImproperlyConfigured instead of using "assert" in
SessionStorage.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:18>
Django
--------------------------------------+------------------------------------
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Daniyal Abbasi):
I have created a [https://github.com/django/django/pull/14135 PR] to
remove usage of assert in django.core.
Next up, I would like to work on removing the usage of assert in django.db
module. If anyone else is already working on it, do let me know. Thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:19>
Django
--------------------------------------+------------------------------------
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak):
Replying to [comment:19 Daniyal Abbasi]:
> Next up, I would like to work on removing the usage of assert in
django.db module.
Please take into account Tim's
[https://code.djangoproject.com/ticket/32508#comment:3 comment].
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:20>
Django
--------------------------------------+------------------------------------
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"474cc420bf6bc1067e2aaa4b40cf6a08d62096f7" 474cc420]:
{{{
#!CommitTicketReference repository=""
revision="474cc420bf6bc1067e2aaa4b40cf6a08d62096f7"
Refs #32508 -- Raised Type/ValueError instead of using "assert" in
django.core.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:21>
Django
--------------------------------------+------------------------------------
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Nishant Sagar):
May I take this ticket and if yes can anyone suggests me the ways to
proceed as I'm fairly new to this
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:22>
Django
--------------------------------------+------------------------------------
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Jacob Walls):
Replying to [comment:22 Nishant Sagar]:
> May I take this ticket and if yes can anyone suggests me the ways to
proceed as I'm fairly new to this
Thanks for your interest. Daniyal handled the bulk of what was left here
in [https://github.com/django/django/pull/14173 PR 14173], so if you want
to tackle the rest, be sure to skip the django.db module, which was the
focus of that PR.
Toward getting started, this is a great time to learn about grep, e.g.
`grep 'assert\s' django/* -r`. Here's what's left excluding django.db and
the test suite itself:
{{{
django/contrib/auth/hashers.py: assert password is not None
django/contrib/auth/hashers.py: assert salt and '$' not in salt
django/contrib/auth/hashers.py: assert algorithm == self.algorithm
django/contrib/auth/hashers.py: assert algorithm == self.algorithm
django/contrib/auth/hashers.py: assert algorithm == self.algorithm
django/contrib/auth/hashers.py: assert algorithm == self.algorithm
django/contrib/auth/hashers.py: assert algorithm == self.algorithm
django/contrib/auth/hashers.py: assert password is not None
django/contrib/auth/hashers.py: assert salt and '$' not in salt
django/contrib/auth/hashers.py: assert algorithm == self.algorithm
django/contrib/auth/hashers.py: assert password is not None
django/contrib/auth/hashers.py: assert salt and '$' not in salt
django/contrib/auth/hashers.py: assert algorithm == self.algorithm
django/contrib/auth/hashers.py: assert salt == ''
django/contrib/auth/hashers.py: assert encoded.startswith('sha1$$')
django/contrib/auth/hashers.py: assert salt == ''
django/contrib/auth/hashers.py: assert len(salt) == 2
django/contrib/auth/hashers.py: assert hash is not None # A
platform like OpenBSD with a dummy crypt module.
django/contrib/auth/hashers.py: assert algorithm == self.algorithm
django/contrib/auth/views.py: assert 'uidb64' in kwargs and 'token'
in kwargs
django/contrib/staticfiles/storage.py: assert
url_path.startswith(settings.STATIC_URL)
django/dispatch/dispatcher.py: assert callable(receiver),
"Signal receivers must be callable."
django/http/multipartparser.py: assert remaining > 0,
'remaining bytes to read should never go negative'
django/test/client.py: assert self.__len >= num_bytes, "Cannot read
more than the available bytes from the HTTP incoming data."
django/test/utils.py: assert not kwargs
django/test/utils.py: assert not args
django/test/testcases.py: assert not self.reset_sequences,
'reset_sequences cannot be used on TestCase instances'
django/utils/version.py: assert len(version) == 5
django/utils/version.py: assert version[3] in ('alpha', 'beta',
'rc', 'final')
django/utils/regex_helper.py: assert not flags, (
django/views/decorators/debug.py: assert isinstance(request,
HttpRequest), (
}}}
The work to finish this off is to decide which ones remaining are worth
changing given comments above from Tim and Chris. I suspect not many. The
last one in the list could be; it's potentially user-facing and already
has a nice exception message, so we can just change to `TypeError`. The
ones in contrib.auth.hashers look like sanity checks we don't need to
touch.
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:23>
Django
--------------------------------------+------------------------------------
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Bal Krishna Jha):
Replying to [comment:22 Nishant Sagar]:
> May I take this ticket and if yes can anyone suggests me the ways to
proceed as I'm fairly new to this
If you're still working on it, please let me know the progress and
component you've chosen. So that I can choose other component or leave
this ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:24>
Django
--------------------------------------+------------------------------------
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mateo Radman):
Replaced assert keywords by raising proper exception with a descriptive
message included.
[https://github.com/django/django/pull/14545 PR]
Files changed:
- views/decorators/debug.py
- dispatch/dispatcher.py
- http/multipartparser.py
- utils/regex_helper.py
- contrib/staticfiles/storage.py
- contrib/auth/views.py
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:25>
Django
--------------------------------------+------------------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"8a7ac78b706797a03d26b88eddb9d1067ed35b66" 8a7ac78b]:
{{{
#!CommitTicketReference repository=""
revision="8a7ac78b706797a03d26b88eddb9d1067ed35b66"
Refs #32508 -- Raised ImproperlyConfigured/TypeError instead of using
"assert" in various code.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:26>
Django
--------------------------------------+------------------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by Mateo Radman):
Hey guys, what is left to be done for this ticket?
Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:27>
Django
--------------------------------------+------------------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by Mariusz Felisiak):
Replying to [comment:27 Mateo Radman]:
> Hey guys, what is left to be done for this ticket?
> Thanks.
IMO, we have only two cases that could be updated (outside of
`django.db.models`):
- `django/db/backends/postgresql/creation.py` - `assert
test_settings['COLLATION'] is None`
- `django/test/testcases.py` - `assert not self.reset_sequences,
'reset_sequences cannot be used on TestCase instances'`
P.S. Note that we're not all "guys" so please use gender neutral
greetings, https://heyguys.cc/
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:28>
Django
--------------------------------------+------------------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by Mateo Radman):
Thank you very much and apologies for my gendered greeting.
I’ll start working on replacing these two asserts.
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:29>
Django
--------------------------------------+------------------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by Mateo Radman):
Replying to [comment:29 Mateo Radman]:
> Thank you very much and apologies for my gendered greeting.
> I’ll start working on replacing these two asserts.
[https://github.com/django/django/pull/14586 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:30>
Django
--------------------------------------+------------------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"2231429991ee17987e3897238f9e5551c5c7ab2e" 22314299]:
{{{
#!CommitTicketReference repository=""
revision="2231429991ee17987e3897238f9e5551c5c7ab2e"
Refs #32508 -- Raised ImproperlyConfigured/TypeError instead of using
"assert".
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:31>
Django
Reporter: Adam Johnson | Owner: Daniyal
Type: | Abbasi
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* owner: nobody => Daniyal Abbasi
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
* status: new => assigned
Comment:
Once [https://github.com/django/django/pull/14173 Daniyal's patch] lands
for `django.db.models` I think this will be resolved.
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:32>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Daniyal
Type: | Abbasi
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"f479df7f8d03ab767bb5e5d655243191087d6432" f479df7]:
{{{
#!CommitTicketReference repository=""
revision="f479df7f8d03ab767bb5e5d655243191087d6432"
Refs #32508 -- Raised Type/ValueError instead of using "assert" in
django.db.models.
Co-authored-by: Mariusz Felisiak <felisiak...@gmail.com>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:33>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Daniyal
Type: | Abbasi
Component: Core (Other) | Version: dev
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* needs_better_patch: 1 => 0
* resolution: => fixed
Comment:
IMO, the other `assert`s are fine.
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:34>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Daniyal
Type: | Abbasi
Cleanup/optimization | Status: closed
Component: Core (Other) | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Chris Jerdonek):
I'd like to suggest that the asserts in `hashers.py` (listed above) also
be considered. While reviewing
[https://github.com/django/django/pull/13799 PR #13799], I noticed there
are cases where user code can encounter them.
For example, here are a couple of the asserts, in the case of
`PBKDF2PasswordHasher.encode()`:
https://github.com/django/django/blob/012f38f9594b35743e9ab231757b7b62db638323/django/contrib/auth/hashers.py#L271-L273
And here is an example in the Django docs instructing users to call this
method with their own code:
https://docs.djangoproject.com/en/3.2/topics/auth/passwords/#password-
upgrading-without-requiring-a-login
That section also tells the user they "can modify the pattern to work with
any algorithm or with a custom user model." Since Django has examples
telling users to call this method with their own code, and also since this
is a security-related code path, I think it would be worth switching from
`assert` statements to more explicit argument checking.
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:35>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Daniyal
Type: | Abbasi
Cleanup/optimization | Status: closed
Component: Core (Other) | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Paolo Melchiorre (added)
Comment:
I agree with Chris, and I think we can reopen this issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:36>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Daniyal
Type: | Abbasi
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: fixed =>
Comment:
OK, let's change asserts in `encode()` methods.
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:37>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Daniyal
Type: | Abbasi
Cleanup/optimization | Status: new
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
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:"83022d279c585be2c4173b36a92d4399e738150e" 83022d2]:
{{{
#!CommitTicketReference repository=""
revision="83022d279c585be2c4173b36a92d4399e738150e"
Refs #32508 -- Raised TypeError/ValueError instead of using "assert" in
encode() methods of some password hashers.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:38>
Django
Reporter: Adam Johnson | Owner: (none)
Type: Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* owner: Daniyal Abbasi => (none)
* status: new => assigned
* has_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:39>
Django
--------------------------------------+------------------------------------
Reporter: Adam Johnson | Owner: (none)
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Changes (by Mariusz Felisiak):
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:40>
Django
Reporter: Adam Johnson | Owner: Mateo
Type: | Radman
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mateo Radman):
* owner: (none) => Mateo Radman
* status: new => assigned
Comment:
Replying to [comment:40 Mariusz Felisiak]:
> `assert`s in the following methods should be changed:
> - `UnsaltedSHA1PasswordHasher.encode()`,
> - `UnsaltedMD5PasswordHasher.encode()`,
> - `CryptPasswordHasher.encode()`,
> new tests are also required.
I'll do it!
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:41>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Mateo
Type: | Radman
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mateo Radman):
Replying to [comment:40 Mariusz Felisiak]:
> `assert`s in the following methods should be changed:
> - `UnsaltedSHA1PasswordHasher.encode()`,
> - `UnsaltedMD5PasswordHasher.encode()`,
> - `CryptPasswordHasher.encode()`,
> new tests are also required.
[https://github.com/django/django/pull/14836 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:42>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Mateo
Type: | Radman
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:43>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Mateo
Type: | Radman
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"a7f27fca5239ff3840735dc6dc731b71a99a1d57" a7f27fca]:
{{{
#!CommitTicketReference repository=""
revision="a7f27fca5239ff3840735dc6dc731b71a99a1d57"
Refs #32508 -- Raised TypeError/ValueError instead of using "assert" in
encode() methods of remaining password hashers.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:44>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Mateo
Type: | Radman
Component: Core (Other) | Version: dev
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* has_patch: 1 => 0
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:45>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Mateo
Type: | Radman
Cleanup/optimization | Status: closed
Component: Core (Other) | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Adam Johnson):
Thanks all!
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:46>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Mateo
Type: | Radman
Cleanup/optimization | Status: closed
Component: Core (Other) | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by David):
There are still many `assert` statements in the codebase
https://github.com/search?q=repo%3Adjango%2Fdjango+language%3APython+path%3A%2F%5Edjango%5C%2F%2F++content%3A%22+assert+%22&type=code
Are they going to stay there or is it ok to remove them?
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:47>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Mateo
Type: | Radman
Cleanup/optimization | Status: closed
Component: Core (Other) | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
Replying to [comment:47 David]:
> Are they going to stay there or is it ok to remove them?
We want to keep them, please check the entire discussion.
--
Ticket URL: <https://code.djangoproject.com/ticket/32508#comment:48>