Recently, I had my first data loss due to a Django update since 2012. When Django 4.2 came out, it wanted to migrate the DEFAULT_FILE_STORAGE and STATICFILES_STORAGE settings to the new variable but got some weird issues with sorl_thumbnail.
I left it then and forgot about it. When we upgraded to Django 5.1, django-storagesbroke silently by not being able to create a connection to AWS S3. Still, all uploads seemed to have worked but nothing ended up in the buckets. Since it’s very difficult to build, I don’t have any integration tests for my storage.
After fixing the settings variables, the packages worked again but for two days, all uploads were lost.
Why I’m creating this ticket: I’d love to have some kind of hard warning or trigger that I have to do something.
django-upgrade didn’t fix it for me
No system check telling me that I still have outdated / invalid variables lying around in my settings
To be clear: It’s 100% my fault and I don’t blame anyone. But if it can happen to me, it might be happing again. And since Django has a focus on smooth upgrades, I think we should do something about it.
So, the new STORAGES has been around since Django 4.2. The old settings were deprecated then, and have been issuing warnings for the whole time since. The advice on How to upgrade Django shows running with those warnings enabled, but they’ve been noisy by default for a full major version, and you can use the -Werror flag to make that stronger still.
The issue here isn’t so much that the deprecation period expired by that there was no error raised in Django-storages it seems.
We had similar issue in django-compressor (that I have a related thread on) but in that case it simply broke on 5.1 prior to our changes. We had a tracking ticket for the entire time between the 4.2 alpha and the 5.1 alpha, when we finally addressed it.
When updating, I will add an assert to my projects along the lines of assert not DJ51, "Must update to new STORAGES setting before upgrade". I find that quite manageable.
I’m wondering what else Django itself might reasonably do here?
[…] and have been issuing warnings for the whole time since
Did they? I always update very close to release and I’ve never seen a system check warning or a Python warning (which are active, I’m able to see others).
If you are right, this would be a very stupid thing, no matter the reason. Nontheless, if it was a Python warning, many people don’t know that you can activate them and they are somehow implicit and hidden.
Solutionwise, I’m thinking about maybe a list of deprecated settings which we could check for in a system check. This would help people coming from even older version, too.
Yes, the warning works. I just double-checked with Django 4.2.15:
$ python -Xdev -m IPython
Python 3.12.2 (main, Feb 25 2024, 03:55:42) [Clang 17.0.6 ]
Type 'copyright', 'credits' or 'license' for more information
IPython 8.26.0 -- An enhanced Interactive Python. Type '?' for help.
In [1]: from django.conf import settings
In [2]: settings.configure(DEFAULT_FILE_STORAGE="something")
/.../django/conf/__init__.py:343: RemovedInDjango51Warning: The DEFAULT_FILE_STORAGE setting is deprecated. Use STORAGES instead.
warnings.warn(DEFAULT_FILE_STORAGE_DEPRECATED_MSG, RemovedInDjango51Warning)
I would be +1 on this. It could just be a general warning, mapping the setting name to the removed version, like “Setting DEFAULT_FILE_STORAGE was removed in Django 5.0.”.
This would help folks who need to jump many Django versions when reviving old projects. On the other hand, for those following the recommended one-version-at-a-time approach, if they don’t react to the deprecation warnings, it’s not clear that an extra warning would do any good.
Yes. There’s nothing Django can do for storage backends that silently “blackhole” files.
@adamchainz As I said, it’s very likely that I was 100% my fault.
But since @adamchainz and @carltongibson agree on a system check, what would be the next step? I talked about this with @sarahboyce before, so what would you suggest? Create a ticket? Create a PR?
Since I believe that this would be a huge improvement for all those people maintaining projects, I’d volunteer for the task
Given the current conversation and agreements (which I do not object), I think we should re-open and re-purpose the ticket to consider “outdated settings”. I’ll do that next Monday to allow for further feedback in this thread.
Yes, nice @nessita Your triage there is exactly as said here.
I lean towards us doing that little bit more because there are these case where things drop off the end of the deprecation period without folks having acted. We’d be well within right to sort of wash our hands, and sometimes we have to. But if we can do something easy/low-cost, then that seems worth the extra affordance.
This ticket might also be related: #35397 (override_settings for STORAGES loses OPTIONS) – Django
Though it’s too late to make updates to the way this was deprecated.
If this is the same issue, I’m confused why @GitRon encountered an issue on 5.1 rather than during the deprecation (4.2 or 5.0)
Awesome @nessita! Thanks, I hope I’ll manage to provide something solid in the next days.
One question: How do the deployment checks work internally? Do they look for system checks or are these a different kind of warnings? Would it make sense to bring this togehter, meaning if you start your Django server, you’ll get a warning and if you do manage.py check --deploy, you’ll see it, too?
So the proposal is to add a system check that warns if using any past setting that’s been removed from Django (more than 50 settings). That isn’t ideal for a few reasons:
Disallowing something so generic as LOGOUT_URL will cause unneeded annoyance. Third-party code sometimes reimplements functionality like this. Now we would suddenly break things for them.
Although the proposed check could be disabled, usage of one disallowed setting requires disabling the check completely in order to silence the warning.
This may have been more visible than a deprecation warning in the case of DEFAULT_FILE_STORAGE. We won’t have to keep these checks around indefinitely and they can be specifically silenced as needed.
I looked at the PR. Yes, I agree it’s not ideal as implemented
A few thoughts (just to sound it out) :
How did we get here? Unseen deprecation warnings. So checks are more visible than those. Perhaps in this case it would have been better to add that too. (May have forestalled this conversation.)
But they still get removed and/or But people will still just ignore them, and then what? (Would we really keep checks around beyond the deprecation period? Until when? I’m not sure we have an answer there, so likely not)
I’d feel less bad about the hard line for a system check, since you couldn’t have not seen it.
Reading the PR: The list of removed settings isn’t uninteresting… (That’s not a big point, clearly)
I wonder if running this only via a flag --removals (similar to --deploy) which we’d do on upgrading resolves the issue? If the settings were grouped[1] by release, I could pass a --since-version option to limit the check to just recent versions (after 4.2 if I’m updating from the LTS next April, say). (I guess I could maintain an --ignore list if I had shadowed an old setting.)
I think doing anything here was always marginal (right?) so we just have to decide at what point the cure is more of a pain than the disease.
Above I said:
The question for me is did we hit easy/low-cost.
If settings were grouped by version, the error message could like to the appropriate docs, which might be nice. ↩︎
Right. I agree a system check for DEFAULT_FILE_STORAGE would have been more visible and probably prevented this report.
We’ve been spending a tremendous amount of engineering time responding to the report of two people who didn’t follow our upgrade instructions of reading release notes and checking for deprecation warnings. I think the proposed check is an overreaction and a needless hassle for people who continue to use old settings names for their own purposes.
If this particular case really must be fixed since it can result in data loss, add a check for DEFAULT_FILE_STORAGE that doesn’t need to remain in Django for the rest of time.
Volunteers will work on what they want, especially when it was them themselves that ran into the issue. The economics of open source are weird that way.
It was agreed here initially that trying to do something was worthwhile: these kind of things come up almost every time we remove anything. It would be a long-term win if we were to find a good solution here. I have particular memories of the fallout from moving path &co from django.conf when we removed the fallbacks — and it wasn’t just a couple of people then. There must be some point where there’s a cut-off but exploring ways of making that smoother for our users isn’t wasted effort.
I don’t see a disproportionate amount of Fellow time on the PR. A review each on an agreed new feature seems fine to me.
Adding to this: this work is not only about the past; it is also about the future. There is concrete work from @jrief to migrate the email settings to be a dict, and concrete ideas to unify security-related settings into a dict (see the post about CSP support in Django by @robhudson). In these cases, if people ignore or miss deprecation warnings, and have settings “disappearing” under them without noticing, it could lead to potential financial loss (emails not sent) and security holes (security settings reverted).
Your proposal @carltongibson about a new command line flag to show these and have some way of grouping by releases feels promising. The con is that we depend again on users reading and running this check. How about we run these checks for the current - 1 checks automatically?
I agree; there are other, less transcendental issues that have taken much more of our time for various reasons.
Regarding “how many reports are enough(ish)”: I have this rule of thumb: for every actual report we receive, there are about 5-10 other users who encountered the same issue but didn’t/couldn’t/wouldn’t take the time to make the report. Because of this, I feel our initial response, in general, should be “sure, tell me more” rather than “no, change my mind”. I consider this Fellow/Contributor time well spent.
(This does not mean, in any way, that Django will accept every request into core. I care deeply about a focused, stable, maintainable, and robust Django Framework. I stressed this point at length during the last DjangoCon.)
I think the “do something” was to recognize that settings are more effectively deprecated with a system check (as was done in the past). I think it worked well.
Check git log django/core/checks/compatibility. Would you advocate for adding any of it back in? If not, why are settings a special case that need to be checked for the rest of time?
)
— but, yes, that one has timed out