Fixed #35514 -- Allows to configure multiple email backends. #18421
Conversation
a7b331c to
57df692
Compare
|
I think an EmailBackend's init is going to need to know whether or not it was initialized from an EMAIL_PROVIDERS OPTIONS dict, to decide whether it's OK to use deprecated settings. See this review comment. My suggestion would be that get_connection() should pass a new def get_connection(backend=None, fail_silently=False, *, provider=None, **kwds):
+ provider_alias = provider
if backend:
...
- return klass(fail_silently=fail_silently, **kwds)
return klass(
fail_silently=fail_silently,
+ provider_alias=provider_alias,
**kwds,
)
provider = provider or "default"
klass = import_string(settings.EMAIL_PROVIDERS[provider]["BACKEND"])
return klass(
fail_silently=fail_silently,
+ provider_alias=provider_alias,
**(settings.EMAIL_PROVIDERS[provider]["OPTIONS"] | **kwds),
)Then EmailBackend init methods can check (The new arg could just be called |
|
(Also, I think this is missing tests that the deprecated settings still work for now.) |
a3a1f31 to
feae9bc
Compare
|
Many thanks to your suggestions, after reading them I believe that it makes sense to disallow the deprecated Having a compatibility layer makes the code really complicated for a minimal payoff. I will propose this problem to the community chat on Discord and ask for advise there. |
8740d89 to
e2f0ce8
Compare
|
@jrief thanks for the followup. I added some more info about the deprecated settings and compatibility, but agree a forum discussion would be easier. There are a handful of other comments you may not have seen. In particular, I think any new args to (GitHub has an annoying habit of hiding review comments behind a "load more" link. Search for "hidden conversations" in the thread above.) |
e2f0ce8 to
9d52974
Compare
8c14804 to
5e4a3a1
Compare
|
@medmunds a question about the deprecation warnings. In the comments and release notes I now changed them to version 6.1. This means that starting from version 6.0 a user would get a warning that for instance Btw. who is going to remove those settings and the associated unit tests then in the future? |
Very close. Yes, the old behavior will be removed altogether in version 7.0. But it is considered deprecated immediately in version 6.0, not 6.1. (The warning message only says when the feature will be removed, not when it was deprecated.)
See Deprecating a feature in the contributing docs for more info and examples.
A Django fellow will do that as part of preparing the codebase for Django 7.1. They just search for |
|
@jrief one other thing: there's no such thing as RemovedInDjango62Warning. That had confused me too: it jumps from RemovedInDjango61Warning directly to RemovedInDjango70Warning. It turns out that's because Django's deprecation period lasts until the next major release or at least two minor releases, whichever is longer. |
ba8fc16 to
b26b71c
Compare
|
@medmunds hopefully now everything is fixed. I added a comment I added a short section on the new configuration directive in |
|
@medmunds in your opinion, is this pull request now ready to be reviewed by the fellows? |
@jrief there are a couple of design-level questions that need to be resolved. I'm going to leave a new review focusing solely on those. Once we figure them out, I think there are a handful of other things the fellows might want cleaned up (e.g., in the docs and testing deprecated behavior). It'll be easier to look at those once the big design questions are settled. Also, since I wasn't present for the design discussion at PyCon (and haven't seen any notes from it), it's quite possible I'm misunderstanding some things. If there was anyone in that discussion who could be helpful here, I think it would be fine to @ them in. |
There was a problem hiding this comment.
@jrief There are a couple of design-decision questions we need to resolve before this can move forward.
(I'm going to refer back to these questions and options for solutions in my inline review comments: Q1, Q2b, etc.)
Q1: Should settings.EMAIL_BACKEND be readable from EMAIL_PROVIDERS?
If settings.py defines EMAIL_PROVIDERS but not deprecated EMAIL_BACKEND, what happens with code that tries to use django.conf.settings.EMAIL_BACKEND?
There are two reasonable options here:
Q1a. It doesn't work: if EMAIL_BACKEND isn't defined in settings.py, code that tries to read settings.EMAIL_BACKEND gets an AttributeError. That means users cannot change their settings.py to EMAIL_PROVIDERS until they update all their code that tries to read settings.EMAIL_BACKEND to be compatible with EMAIL_PROVIDERS. And we document this.
Q1b. It works with a deprecation warning: reading settings.EMAIL_BACKEND provides the value of the default EMAIL_PROVIDER's BACKEND. You've implemented that in LazySettings.__getattr__. If we want to stick with this approach, there are a couple of things to fix (see review comments in the code).
To decide, we need to know whether this is an important case to support. You've added a specific test for this behavior. Is it based on known third-party code? Or do you have an example scenario where this seems likely? (That is, the user must change settings.py to EMAIL_PROVIDERS but also keep using third-party code that reads settings.EMAIL_BACKEND?)
I think option Q1a is much simpler, both to implement and to explain. But Q1b is possible if there's a good reason for it.
Q2: Should settings.EMAIL_HOST and other deprecated EMAIL settings be readable from EMAIL_PROVIDERS?
This is similar to the first question, but for all the other deprecated EMAIL settings.
You've tried to implement reading these in LazySettings.__getattr__, by instantiating the default EMAIL_PROVIDER's backend and accessing the corresponding attribute—e.g., reading settings.EMAIL_USER from backend.username.
But that code assumes the default backend is Django's smtp.EmailBackend. It simply won't work in general. E.g., what happens when third-party code tries to read settings.EMAIL_HOST if the user has these settings:
EMAIL_PROVIDERS = {
"default": {
"BACKEND": "some.non_django.EmailBackend",
"OPTIONS": {
"api_key": "123",
},
},
}I don't think there's any way to make LazySettings work as you intend. So we should consider alternatives. I see two options:
Q2a. It doesn't work: if EMAIL_HOST isn't defined in settings.py, code that tries to read settings.EMAIL_HOST gets an AttributeError. That means users cannot change their settings.py to EMAIL_PROVIDERS until they update all code that tries to read settings.EMAIL_HOST and other deprecated settings to be compatible with EMAIL_PROVIDERS. And we document this.
Q2b. We allow settings.py to define both EMAIL_PROVIDERS and deprecated EMAIL_HOST at the same time. So code that reads settings.EMAIL_HOST works (but should get a deprecation warning). Users are responsible for keeping their deprecated EMAIL settings in sync with EMAIL_PROVIDERS until all their code that reads deprecated settings has been updated to use EMAIL_PROVIDERS instead.
Again, is this a case we really need to handle? I'm not aware of third party code that borrows Django's EMAIL settings like this, but you may have some example in mind?
If not, Q2a is much simpler. If so, there's probably a way to make Q2b work, but there's a high potential for user confusion if their settings get out of sync.
| if name == "EMAIL_BACKEND": | ||
| return self.EMAIL_PROVIDERS["default"]["BACKEND"] |
There was a problem hiding this comment.
If Q1a: remove this code.
If Q1b:
- Replace hard-coded "default" with DEFAULT_EMAIL_PROVIDER_ALIAS.
- Needs a deprecation warning. This warns code that tries to use the deprecated setting. (It's the counterpart to your existing warning to users who define EMAIL_BACKEND in settings.py.)
| if name == "EMAIL_BACKEND": | |
| return self.EMAIL_PROVIDERS["default"]["BACKEND"] | |
| if name == "EMAIL_BACKEND": | |
| warnings.warn("...", RemovedInDjango70Warning) | |
| return self.EMAIL_PROVIDERS[self.DEFAULT_EMAIL_PROVIDER_ALIAS]["BACKEND"] |
The warning message is maybe something like, "Using settings.EMAIL_BACKEND is deprecated. Redesign code to work with EMAIL_PROVIDERS."
(I don't think there's a specific replacement to suggest. The warning should not recommend switching to settings.EMAIL_PROVIDERS["default"]["BACKEND"]. Code that is reading EMAIL_BACKEND will need to be re-thought for a world with multiple providers.)
| if name in DEPRECATED_EMAIL_SETTINGS: | ||
| backend = import_string(getattr(self, "EMAIL_BACKEND"))( | ||
| provider="default" | ||
| ) | ||
| if name == "EMAIL_HOST_USER": | ||
| return backend.username | ||
| if name == "EMAIL_HOST_PASSWORD": | ||
| return backend.password | ||
| return getattr(backend, f"{name[6:].lower()}") |
There was a problem hiding this comment.
Q2: There is no way to make this work as intended. Remove it.
| if is_overridden_EMAIL_PROVIDERS: | ||
| raise EmailImproperlyConfigured(deprecated_setting) |
There was a problem hiding this comment.
If Q2b: remove the error here. (Maybe add a warning about potential confusion between EMAIL_PROVIDERS and the deprecated setting?)
| if name == "EMAIL_PROVIDERS" and any( | ||
| self.is_overridden(f) for f in DEPRECATED_EMAIL_SETTINGS | ||
| ): | ||
| raise EmailImproperlyConfigured(name) | ||
| if name in DEPRECATED_EMAIL_SETTINGS: | ||
| if self.is_overridden("EMAIL_PROVIDERS"): | ||
| raise EmailImproperlyConfigured(name) | ||
| warn_about_deprecated_email_setting(name) |
There was a problem hiding this comment.
If Q2b: Remove the errors here. (Maybe add a warning about potential confusion between EMAIL_PROVIDERS and the deprecated setting?)
| # The email provider settings. If left empty, will default to the SMTP backend. | ||
| # For possible shortcuts see django.core.mail. | ||
| EMAIL_PROVIDERS = { | ||
| "default": { | ||
| "BACKEND": "django.core.mail.backends.smtp.EmailBackend", | ||
| }, | ||
| } |
There was a problem hiding this comment.
Need to define (and use) global default for DEFAULT_EMAIL_PROVIDER_ALIAS.
| # The email provider settings. If left empty, will default to the SMTP backend. | |
| # For possible shortcuts see django.core.mail. | |
| EMAIL_PROVIDERS = { | |
| "default": { | |
| "BACKEND": "django.core.mail.backends.smtp.EmailBackend", | |
| }, | |
| } | |
| # The EMAIL_PROVIDERS key to use for sending if no provider is specified. | |
| DEFAULT_EMAIL_PROVIDER_ALIAS = "default" | |
| # The email provider settings. If left empty, will default to the SMTP backend. | |
| # For possible shortcuts see django.core.mail. | |
| EMAIL_PROVIDERS = { | |
| DEFAULT_EMAIL_PROVIDER_ALIAS: { | |
| "BACKEND": "django.core.mail.backends.smtp.EmailBackend", | |
| }, | |
| } |
| if provider is not None: | ||
| # Being initialized from EMAIL_PROVIDERS. | ||
| options = settings.EMAIL_PROVIDERS[provider].get("OPTIONS", {}) | ||
| file_path = options.get("file_path") |
There was a problem hiding this comment.
An EmailBackend should not examine settings.EMAIL_PROVIDERS directly. (That is mail.get_connection()'s responsibility. See also this and this earlier review comments.)
| if provider is not None: | |
| # Being initialized from EMAIL_PROVIDERS. | |
| options = settings.EMAIL_PROVIDERS[provider].get("OPTIONS", {}) | |
| file_path = options.get("file_path") | |
| if provider is not None: | |
| # Being initialized from EMAIL_PROVIDERS. | |
| pass | |
| # file_path arg is already set by mail.get_connection(). Do not override it here. | |
| # (If you want to raise ImproperlyConfigured for missing file_path, do that here.) |
| if provider is not None: | ||
| # Being initialized from EMAIL_PROVIDERS. | ||
| options = settings.EMAIL_PROVIDERS[provider].get("OPTIONS", {}) | ||
| self.host = host or options.get("host", self.DEFAULT_HOST) | ||
| self.port = port or options.get("port", self.DEFAULT_PORT) | ||
| self.username = options.get("username") if username is None else username | ||
| self.password = options.get("password") if password is None else password | ||
| self.use_tls = options.get("use_tls") if use_tls is None else use_tls | ||
| self.use_ssl = options.get("use_ssl") if use_ssl is None else use_ssl | ||
| self.ssl_keyfile = ( | ||
| options.get("ssl_keyfile") if ssl_keyfile is None else ssl_keyfile | ||
| ) | ||
| self.ssl_certfile = ( | ||
| options.get("ssl_certfile") if ssl_certfile is None else ssl_certfile | ||
| ) | ||
| self.timeout = options.get("timeout") if timeout is None else timeout |
There was a problem hiding this comment.
An EmailBackend should not examine settings.EMAIL_PROVIDERS directly. (That is mail.get_connection()'s responsibility.)
| if provider is not None: | |
| # Being initialized from EMAIL_PROVIDERS. | |
| options = settings.EMAIL_PROVIDERS[provider].get("OPTIONS", {}) | |
| self.host = host or options.get("host", self.DEFAULT_HOST) | |
| self.port = port or options.get("port", self.DEFAULT_PORT) | |
| self.username = options.get("username") if username is None else username | |
| self.password = options.get("password") if password is None else password | |
| self.use_tls = options.get("use_tls") if use_tls is None else use_tls | |
| self.use_ssl = options.get("use_ssl") if use_ssl is None else use_ssl | |
| self.ssl_keyfile = ( | |
| options.get("ssl_keyfile") if ssl_keyfile is None else ssl_keyfile | |
| ) | |
| self.ssl_certfile = ( | |
| options.get("ssl_certfile") if ssl_certfile is None else ssl_certfile | |
| ) | |
| self.timeout = options.get("timeout") if timeout is None else timeout | |
| if provider is not None: | |
| # Being initialized from EMAIL_PROVIDERS. | |
| self.host = self.DEFAULT_HOST if host is None else host | |
| self.port = self.DEFAULT_PORT if port is None else port | |
| self.username = username | |
| self.password = password | |
| self.use_tls = False if (use_tls is None) else use_tls | |
| self.use_ssl = False if (use_ssl is None) else use_ssl | |
| self.ssl_keyfile = ssl_keyfile | |
| self.ssl_certfile = ssl_certfile | |
| self.timeout = timeout |
| self.assertEqual( | ||
| settings.EMAIL_BACKEND, | ||
| "django.core.mail.backends.smtp.EmailBackend", | ||
| ) |
There was a problem hiding this comment.
If Q1a: remove - this test is not valid.
If Q1b: check that accessing settings.EMAIL_BACKEND issues a deprecation warning
| self.assertEqual(settings.EMAIL_HOST, "smtp.example.com") | ||
| self.assertEqual(settings.EMAIL_PORT, 487) | ||
| self.assertEqual(settings.EMAIL_HOST_USER, "noreply") | ||
| self.assertEqual(settings.EMAIL_HOST_PASSWORD, "secret") | ||
| self.assertTrue(settings.EMAIL_USE_TLS) | ||
| self.assertFalse(settings.EMAIL_USE_SSL) | ||
| self.assertEqual(settings.EMAIL_TIMEOUT, 42) |
There was a problem hiding this comment.
Q2: There is no reason to expect these deprecated EMAIL_ settings should be resolved from EMAIL_PROVIDERS.
If Q2a: Remove these tests.
If Q2b: Add deprecated EMAIL_HOST, EMAIL_PORT, etc. settings to override_settings above, and then check that accessing them issues a deprecation warning
| def test_host_configuration_mismatch(self): | ||
| with self.assertRaisesMessage( | ||
| ImproperlyConfigured, | ||
| "EMAIL_HOST and EMAIL_PROVIDERS are mutually exclusive.", | ||
| ): | ||
| with self.settings( | ||
| EMAIL_HOST="other.example.com", | ||
| ): | ||
| pass |
There was a problem hiding this comment.
If Q2b: remove this and following tests
After poking around in GitHub code search, my votes are Q1a and Q2a. That is, with a settings.py that defines EMAIL_PROVIDERS but not any deprecated EMAIL_* settings, we would expect: >>> from django.conf import settings
>>> settings.EMAIL_BACKEND # (error per Q1a)
AttributeError: 'Settings' object has no attribute 'EMAIL_BACKEND'
>>> settings.EMAIL_HOST_USER # (error per Q2a)
AttributeError: 'Settings' object has no attribute 'EMAIL_HOST_USER'The code searches find plenty of uses of the deprecated settings. But we only need to consider cases that appear to be libraries—where library users would be prevented from using EMAIL_PROVIDERS (at all) until the libraries stop using the deprecated settings. (Complete server implementations and other non-library projects—which include their own settings.py—can fix their own usage of the deprecated settings at the same time they change to EMAIL_PROVIDERS.)
settings.EMAIL_BACKEND code search: I found two examples of library code that reads settings.EMAIL_BACKEND:
These seem like reasonable uses, and their existence slightly tips the scales toward Q1b (LazySettings magic) for me. But not enough to justify the additional complexity. Put another way: the response to, "I'm using django-mailer and get an AttributeError when I try to configure it using EMAIL_PROVIDERS," should be, "Then keep using EMAIL_BACKEND as shown in django-mailer's docs, until django-mailer is updated to work with EMAIL_PROVIDERS."
settings.EMAIL_<others> code search: I found only one example of library code that reads other deprecated settings:
I didn't see any libraries trying to read the other deprecated settings.1 Same argument as above… Complaint: "I'm getting an AttributeError in healthchecks when I remove EMAIL_HOST from my settings.py." Solution: "Then don't remove EMAIL_HOST until healthchecks is updated. That means you'll need to wait to switch to EMAIL_PROVIDERS, too." [Standard code search disclaimer: these are illustrative examples, not a complete list of projects that might be affected. Not all projects are in GitHub; not all GitHub projects are in public code search; my searches miss things like Footnotes
|
|
@jrief sorry, there is one other case to consider. Imagine a user has just upgraded to Django 6.0 and has not modified their settings file at all yet. (So they don't have EMAIL_PROVIDERS defined in their settings.) In that case, any user or library code that tries to access any deprecated setting must continue to work, but should get a deprecation warning. Once the user opts in to using EMAIL_PROVIDERS, the compatibility guarantee no longer applies. If EMAIL_PROVIDERS is defined in settings.py, then:
But if EMAIL_PROVIDERS is not defined, we need to make all the deprecated settings readable with their original default values. I think the # django/conf/__init__.py
class LazySettings(LazyObject):
def __getattr__(self, name):
...
try:
val = getattr(_wrapped, name)
except AttributeError:
# RemovedInDjango70Warning: If EMAIL_PROVIDERS is not being used,
# provide original default values for deprecated EMAIL settings.
if name in DEPRECATED_EMAIL_SETTINGS and not self.is_overridden(
"EMAIL_PROVIDERS"
):
self._show_deprecation_warning(
f"Using the {name} setting is deprecated."
" Update code to work with EMAIL_PROVIDERS.",
RemovedInDjango70Warning,
)
val = getattr(_wrapped, f"dep_{name}")
else:
raise
...# django/conf/global_settings.py
# RemovedInDjango70Warning: Default values for deprecated EMAIL settings.
# (Used in LazySettings.__getattr__.)
dep_EMAIL_BACKEND = "django.core.mail.backends.smtp.EmailBackend"
dep_EMAIL_HOST = "localhost"
dep_EMAIL_PORT = 25
dep_EMAIL_HOST_USER = ""
dep_EMAIL_HOST_PASSWORD = ""
dep_EMAIL_USE_TLS = False
dep_EMAIL_USE_SSL = False
dep_EMAIL_SSL_CERTFILE = None
dep_EMAIL_SSL_KEYFILE = None
dep_EMAIL_TIMEOUT = None |
| myemailbackend = MyEmailBackend() | ||
| myemailbackend.open() | ||
| self.assertEqual(myemailbackend.timeout, 42) | ||
| self.assertEqual(myemailbackend.connection.timeout, 42) |
There was a problem hiding this comment.
test_connection_timeout_custom() is invalid, because it directly instantiates a (subclass of) smtp.EmailBackend rather than using mail.get_connection():
def test_connection_timeout_custom(self):
...
- myemailbackend = MyEmailBackend()
+ myemailbackend = mail.get_connection()To make this test work, you'll need to make the custom MyEmailBackend importable and then refer to its import path in EMAIL_PROVIDERS. There are a several ways to do that:
- The cleanest is probably to move it into to custombackend.py and rename it something like CustomTimeoutBackend. Then override EMAIL_PROVIDERS to set BACKEND to
"mail.custombackend.CustomTimeoutBackend". - Or if you want to keep MyEmailBackend defined inside this test case, then temporarily install it in the current module:
sys.modules[self.__module__].MyEmailBackend = MyEmailBackendand override EMAIL_PROVIDERS to set BACKEND tof"{self.__module__}.MyEmailBackend". (And delattr it out of the module when you're done, in a try-finally block. The other option seems cleaner.)
Please don't try to make this invalid test work by reading EMAIL_PROVIDERS directly inside the smtp EmailBackend implementation. (That's the responsibility of mail.get_connection().)
There was a problem hiding this comment.
that's fixed. For the test to work, I actually have to pass the temporarily opened SMTP port to the get_connection() call. Tests for this fix succeed.
|
@jrief Is there anything I could do to help move this forward? For example, I might be able to open a PR on your PR (against your PR branch in your fork of Django), which you could review and merge into your PR or request that I make changes. |
|
@medmunds I'm so sorry for not proceeding with this. I just didn't have time anymore to work on it. If you can work on it, please feel free to do so. I usually finish a job that I have started, so whenever I have some extra time, I will proceed. |
|
@jrief no apologies necessary—I completely understand. I'll try to get back into it next week and let you know if I make any progress. |
ceb8fa5 to
cfdb110
Compare
|
I retested the concern you mentioned in your previous comment. Actually, accessing |
|
@medmunds Is there still anything to be fixed on this issue (except rebasing), to make this a pull request for review by the Django fellows? |
|
@jrief It doesn't look like any of the comments from my mid-Feb review have been addressed. Per the follow-up comment I suggest the "a" option for each of the design decisions. (Also, it seems GitHub has introduced a "new PR experience" that hides even more current comments than the old experience. You may need to disable that near the top of the Files tab.)
The concern is about reading deprecated EMAIL_ settings where the user has not provided a settings.py value: # No @override_settings()
def test_deprecated_settings_have_default_values(self):
# Django 6.0 default values for deprecated email settings:
self.assertEqual(
settings.EMAIL_BACKEND,
"django.core.mail.backends.smtp.EmailBackend",
)
self.assertEqual(settings.EMAIL_HOST, "localhost")
self.assertEqual(settings.EMAIL_PORT, 25)
self.assertEqual(settings.EMAIL_HOST_USER, "")
self.assertEqual(settings.EMAIL_HOST_PASSWORD, "")
# Using assertIsFalse(...) rather than assertEqual(..., False)
# (despite coding guidelines) to ensure exact Django 6.0 values.
self.assertIsFalse(settings.EMAIL_USE_TLS)
self.assertIsFalse(settings.EMAIL_USE_SSL)
self.assertIsNone(settings.EMAIL_SSL_CERTFILE)
self.assertIsNone(settings.EMAIL_SSL_KEYFILE)
self.assertIsNone(settings.EMAIL_TIMEOUT) |
Trac ticket number
ticket-35514
Branch description
This is a draft pull request. It just replaces the the configuration settings starting with
EMAIL_…with a dictionary based configuration. More features have to be added.Checklist
mainbranch.