feat: allow pass hostname in docker env#157
Open
Conversation
Review Summary by QodoAdd SMTP local hostname override configuration
WalkthroughsDescription• Add SMTP_LOCAL_HOSTNAME configuration to override local hostname in SMTP HELO/EHLO • Refactor SMTP client initialization to use ternary operator for cleaner code • Update all SMTP tests to accept local_hostname parameter with ANY matcher • Add configuration to Docker environment files and example configs Diagramflowchart LR
A["MailConfig"] -->|"adds SMTP_LOCAL_HOSTNAME field"| B["Feature Configuration"]
B -->|"read by"| C["SMTPClient"]
C -->|"passes to"| D["smtplib.SMTP/SMTP_SSL"]
D -->|"uses in"| E["HELO/EHLO commands"]
F["Environment Files"] -->|"define"| B
File Changes1. api/configs/feature/__init__.py
|
Code Review by Qodo
1. feature/__init__.py exceeds 800
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+952
to
+956
| SMTP_LOCAL_HOSTNAME: str | None = Field( | ||
| description="Override the local hostname used in SMTP HELO/EHLO. " | ||
| "Useful behind NAT or when the default hostname causes rejections.", | ||
| default=None, | ||
| ) |
There was a problem hiding this comment.
1. feature/init.py exceeds 800 📘 Rule violation ⛯ Reliability
api/configs/feature/__init__.py is well over the 800-line limit and this PR adds additional lines to it, further exceeding the maximum. This reduces maintainability and violates the required file size constraint for backend Python files under api/.
Agent Prompt
## Issue description
`api/configs/feature/__init__.py` exceeds the 800-line maximum for backend files under `api/`, and this PR adds more content to the file.
## Issue Context
The file already contains content beyond line 1100, which is over the 800-line limit. The new `SMTP_LOCAL_HOSTNAME` field was added into this already oversized module.
## Fix Focus Areas
- api/configs/feature/__init__.py[952-956]
- api/configs/feature/__init__.py[890-980]
- api/configs/feature/__init__.py[1095-1105]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+24
to
+36
| smtp: smtplib.SMTP | None = None | ||
| local_host = dify_config.SMTP_LOCAL_HOSTNAME or "" | ||
| try: | ||
| if self.use_tls: | ||
| if self.opportunistic_tls: | ||
| smtp = smtplib.SMTP(self.server, self.port, timeout=10) | ||
| # Send EHLO command with the HELO domain name as the server address | ||
| smtp.ehlo(self.server) | ||
| smtp.starttls() | ||
| # Resend EHLO command to identify the TLS session | ||
| smtp.ehlo(self.server) | ||
| else: | ||
| smtp = smtplib.SMTP_SSL(self.server, self.port, timeout=10) | ||
| else: | ||
| smtp = smtplib.SMTP(self.server, self.port, timeout=10) | ||
| # Use ternary to select SMTP class based on TLS mode | ||
| smtp = (smtplib.SMTP_SSL if (self.use_tls and not self.opportunistic_tls) else smtplib.SMTP)( | ||
| self.server, self.port, timeout=10, local_hostname=local_host or None | ||
| ) | ||
|
|
||
| assert smtp is not None | ||
| if self.use_tls and self.opportunistic_tls: | ||
| smtp.ehlo(local_host) | ||
| smtp.starttls() | ||
| smtp.ehlo(local_host) |
There was a problem hiding this comment.
2. Smtpclient config side effects 🐞 Bug ⛯ Reliability
libs.smtp.SMTPClient now depends on the global dify_config to determine HELO/EHLO hostname, which forces config initialization when importing the SMTP utility and makes the local hostname process-global rather than instance-level. This increases coupling and can break callers that want to use SMTPClient without the full app config stack or need different hostnames per client instance.
Agent Prompt
### Issue description
`SMTPClient` currently imports and reads global `dify_config` inside `libs/smtp.py`, introducing import-time side effects and preventing per-instance configuration of the HELO/EHLO hostname.
### Issue Context
The mail extension (`extensions/ext_mail.py`) already has access to `dify_config` when it constructs `SMTPClient`, so it can provide the hostname override explicitly without coupling the SMTP utility module to global config.
### Fix Focus Areas
- api/libs/smtp.py[6-36]
- api/extensions/ext_mail.py[42-57]
- api/configs/feature/__init__.py[952-957]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+20
to
21
| mock_smtp_cls.assert_called_once_with("smtp.example.com", 25, timeout=10, local_hostname=ANY) | ||
| mock_smtp.sendmail.assert_called_once() |
There was a problem hiding this comment.
3. Hostname override untested 🐞 Bug ⛯ Reliability
Unit tests were updated to accept any value for the new local_hostname argument, but they do not assert that SMTP_LOCAL_HOSTNAME is actually propagated into the SMTP handshake behavior. This can allow future regressions where the env/config value is ignored while tests still pass.
Agent Prompt
### Issue description
Current unit tests accept `local_hostname=ANY` and do not verify the new env/config-driven hostname override is actually used.
### Issue Context
The PR’s feature is specifically about propagating `SMTP_LOCAL_HOSTNAME` into the SMTP client behavior; tests should pin this behavior to prevent regressions.
### Fix Focus Areas
- api/tests/unit_tests/libs/test_smtp_client.py[1-100]
- api/libs/smtp.py[23-36]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Benchmark PR from agentic-review-benchmarks#6