Raise proper exceptions instead of using "assert".

[Adam Johnson]

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'"))

[Simon Charette]

An alternative to this problem could also be to replace these assert by proper exceptions instead.

[Mariusz Felisiak]

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).

[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.

[Chris Jerdonek]

I agree with Tim. Before proceeding too far with this, it might be worth documenting in the ​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.

[Adam Johnson]

Fair enough, I agree that removing such usage is probably just as valid.

[Mariusz Felisiak]

I added ​PR for a good start.

[Carlton Gibson]

It's also common for library...

Just as a data-point, DRF makes extensive use of assert for this purpose.

[GitHub <noreply@…>]

In ba9a2b75:

Refs #32508 -- Raised TypeError instead of using "assert" on unsupported operations for sliced querysets.

[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?

[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. 😀

[Hasan Ramezani]

​PR for changing asserts in admindoc and auth middlewares

[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

[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…

[Mariusz Felisiak <felisiak.mariusz@…>]

In a2d5ea62:

Refs #32508 -- Raised ImproperlyConfigured instead of using "assert" in middlewares.

[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

[Hasan Ramezani]

I've created a ​PR to replace assert in django.utils

[Mariusz Felisiak <felisiak.mariusz@…>]

In 775b796:

Refs #32508 -- Raised ValueError instead of using "assert" in lazy().

[Mariusz Felisiak <felisiak.mariusz@…>]

In 54d91795:

Refs #32508 -- Raised ImproperlyConfigured instead of using "assert" in SessionStorage.

[Daniyal Abbasi]

I have created a ​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!

[Mariusz Felisiak]

Replying to 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 comment.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 474cc420:

Refs #32508 -- Raised Type/ValueError instead of using "assert" in django.core.

[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

[Jacob Walls]

Replying to 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 ​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. Below is what's left excluding django.db (and django.utils, already reviewed in 775b796d8d13841059850d73986d5dcc2e593077) and the test suite itself.

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.

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/views/decorators/debug.py:            assert isinstance(request, HttpRequest), (

[Bal Krishna Jha]

Replying to 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.

[Mateo Radman]

Replaced assert keywords by raising proper exception with a descriptive message included.
​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

[Mariusz Felisiak <felisiak.mariusz@…>]

In 8a7ac78b:

Refs #32508 -- Raised ImproperlyConfigured/TypeError instead of using "assert" in various code.

[Mateo Radman]

Hey guys, what is left to be done for this ticket?
Thanks.

[Mariusz Felisiak]

Replying to 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/

[Mateo Radman]

Thank you very much and apologies for my gendered greeting.
I’ll start working on replacing these two asserts.

[Mateo Radman]

Replying to Mateo Radman:

Thank you very much and apologies for my gendered greeting.
I’ll start working on replacing these two asserts.

​PR

[Mariusz Felisiak <felisiak.mariusz@…>]

In 22314299:

Refs #32508 -- Raised ImproperlyConfigured/TypeError instead of using "assert".

[Jacob Walls]

Once ​Daniyal's patch lands for django.db.models I think this will be resolved.

[Mariusz Felisiak <felisiak.mariusz@…>]

In f479df7:

Refs #32508 -- Raised Type/ValueError instead of using "assert" in django.db.models.

Co-authored-by: Mariusz Felisiak <felisiak.mariusz@…>

[Mariusz Felisiak]

IMO, the other asserts are fine.

[Chris Jerdonek]

I'd like to suggest that the asserts in hashers.py (listed above) also be considered. While reviewing ​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 (for the purposes of a data migration):
​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.

[Paolo Melchiorre]

I agree with Chris, and I think we can reopen this issue.

[Mariusz Felisiak]

OK, let's change asserts in encode() methods.

[GitHub <noreply@…>]

In 83022d2:

Refs #32508 -- Raised TypeError/ValueError instead of using "assert" in encode() methods of some password hashers.

[Mariusz Felisiak]

asserts in the following methods should be changed:

• UnsaltedSHA1PasswordHasher.encode(),
• UnsaltedMD5PasswordHasher.encode(),
• CryptPasswordHasher.encode(),

new tests are also required.

[Mateo Radman]

Replying to Mariusz Felisiak:

asserts in the following methods should be changed:

• UnsaltedSHA1PasswordHasher.encode(),
• UnsaltedMD5PasswordHasher.encode(),
• CryptPasswordHasher.encode(),

new tests are also required.

I'll do it!

[Mateo Radman]

Replying to Mariusz Felisiak:

asserts in the following methods should be changed:

• UnsaltedSHA1PasswordHasher.encode(),
• UnsaltedMD5PasswordHasher.encode(),
• CryptPasswordHasher.encode(),

new tests are also required.

​PR

[Mariusz Felisiak <felisiak.mariusz@…>]

In a7f27fca:

Refs #32508 -- Raised TypeError/ValueError instead of using "assert" in encode() methods of remaining password hashers.

[Adam Johnson]

Thanks all!

[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?

[Mariusz Felisiak]

Replying to David:

Are they going to stay there or is it ok to remove them?

We want to keep them, please check the entire discussion.