Skip to content

feat: allow pass hostname in docker env #157

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tomerqodo wants to merge 2 commits into
base: qodo_claude_vs_qodo_base_feat_allow_pass_hostname_in_docker_env_pr6
Choose a base branch
from
Open

feat: allow pass hostname in docker env #157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c0322c4
feat: allow pass hostname in docker env
fatelei Jan 14, 2026
6facade
update pr
tomerqodo Jan 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions api/.env.example
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,8 @@ SMTP_USERNAME=123
SMTP_PASSWORD=abc
SMTP_USE_TLS=true
SMTP_OPPORTUNISTIC_TLS=false
# Optional: override the local hostname used for SMTP HELO/EHLO
SMTP_LOCAL_HOSTNAME=
# Sendgid configuration
SENDGRID_API_KEY=
# Sentry configuration
Expand Down Expand Up @@ -713,3 +715,4 @@ ANNOTATION_IMPORT_MAX_CONCURRENT=5
SANDBOX_EXPIRED_RECORDS_CLEAN_GRACEFUL_PERIOD=21
SANDBOX_EXPIRED_RECORDS_CLEAN_BATCH_SIZE=1000
SANDBOX_EXPIRED_RECORDS_RETENTION_DAYS=30

6 changes: 6 additions & 0 deletions api/configs/feature/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -949,6 +949,12 @@ class MailConfig(BaseSettings):
default=False,
)

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,
)
Comment on lines +952 to +956
Copy link
Copy Markdown

@qodo-code-review qodo-code-review bot Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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


EMAIL_SEND_IP_LIMIT_PER_MINUTE: PositiveInt = Field(
description="Maximum number of emails allowed to be sent from the same IP address in a minute",
default=50,
Expand Down
27 changes: 14 additions & 13 deletions api/libs/smtp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
from email.mime.multipart import MIMEMultipart
from email.mime.text import MIMEText

from configs import dify_config

logger = logging.getLogger(__name__)


Expand All @@ -19,20 +21,19 @@ def __init__(
self.opportunistic_tls = opportunistic_tls

def send(self, mail: dict):
smtp = None
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)
Comment on lines +24 to +36
Copy link
Copy Markdown

@qodo-code-review qodo-code-review bot Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

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


# Only authenticate if both username and password are non-empty
if self.username and self.password and self.username.strip() and self.password.strip():
Expand Down
2 changes: 2 additions & 0 deletions api/tests/integration_tests/.env.example
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,8 @@ SMTP_USERNAME=123
SMTP_PASSWORD=abc
SMTP_USE_TLS=true
SMTP_OPPORTUNISTIC_TLS=false
# Optional: override the local hostname used for SMTP HELO/EHLO
SMTP_LOCAL_HOSTNAME=

# Sentry configuration
SENTRY_DSN=
Expand Down
6 changes: 3 additions & 3 deletions api/tests/unit_tests/libs/test_smtp_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from unittest.mock import MagicMock, patch
from unittest.mock import ANY, MagicMock, patch

import pytest

Expand All @@ -17,7 +17,7 @@ def test_smtp_plain_success(mock_smtp_cls: MagicMock):
client = SMTPClient(server="smtp.example.com", port=25, username="", password="", _from="noreply@example.com")
client.send(_mail())

mock_smtp_cls.assert_called_once_with("smtp.example.com", 25, timeout=10)
mock_smtp_cls.assert_called_once_with("smtp.example.com", 25, timeout=10, local_hostname=ANY)
mock_smtp.sendmail.assert_called_once()
Comment on lines +20 to 21
Copy link
Copy Markdown

@qodo-code-review qodo-code-review bot Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Advisory comments

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

mock_smtp.quit.assert_called_once()

Expand All @@ -38,7 +38,7 @@ def test_smtp_tls_opportunistic_success(mock_smtp_cls: MagicMock):
)
client.send(_mail())

mock_smtp_cls.assert_called_once_with("smtp.example.com", 587, timeout=10)
mock_smtp_cls.assert_called_once_with("smtp.example.com", 587, timeout=10, local_hostname=ANY)
assert mock_smtp.ehlo.call_count == 2
mock_smtp.starttls.assert_called_once()
mock_smtp.login.assert_called_once_with("user", "pass")
Expand Down
8 changes: 4 additions & 4 deletions api/tests/unit_tests/tasks/test_mail_send_task.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
"""

import smtplib
from unittest.mock import MagicMock, patch
from unittest.mock import ANY, MagicMock, patch

import pytest

Expand Down Expand Up @@ -151,7 +151,7 @@ def test_smtp_send_with_tls_ssl(self, mock_smtp_ssl):
client.send(mail_data)

# Assert
mock_smtp_ssl.assert_called_once_with("smtp.example.com", 465, timeout=10)
mock_smtp_ssl.assert_called_once_with("smtp.example.com", 465, timeout=10, local_hostname=ANY)
mock_server.login.assert_called_once_with("user@example.com", "password123")
mock_server.sendmail.assert_called_once()
mock_server.quit.assert_called_once()
Expand Down Expand Up @@ -181,7 +181,7 @@ def test_smtp_send_with_opportunistic_tls(self, mock_smtp):
client.send(mail_data)

# Assert
mock_smtp.assert_called_once_with("smtp.example.com", 587, timeout=10)
mock_smtp.assert_called_once_with("smtp.example.com", 587, timeout=10, local_hostname=ANY)
mock_server.ehlo.assert_called()
mock_server.starttls.assert_called_once()
assert mock_server.ehlo.call_count == 2 # Before and after STARTTLS
Expand Down Expand Up @@ -213,7 +213,7 @@ def test_smtp_send_without_tls(self, mock_smtp):
client.send(mail_data)

# Assert
mock_smtp.assert_called_once_with("smtp.example.com", 25, timeout=10)
mock_smtp.assert_called_once_with("smtp.example.com", 25, timeout=10, local_hostname=ANY)
mock_server.login.assert_called_once()
mock_server.sendmail.assert_called_once()
mock_server.quit.assert_called_once()
Expand Down
2 changes: 2 additions & 0 deletions docker/.env.example
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -968,6 +968,8 @@ SMTP_USERNAME=
SMTP_PASSWORD=
SMTP_USE_TLS=true
SMTP_OPPORTUNISTIC_TLS=false
# Optional: override the local hostname used for SMTP HELO/EHLO
SMTP_LOCAL_HOSTNAME=

# Sendgid configuration
SENDGRID_API_KEY=
Expand Down
1 change: 1 addition & 0 deletions docker/docker-compose.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,7 @@ x-shared-env: &shared-api-worker-env
SMTP_PASSWORD: ${SMTP_PASSWORD:-}
SMTP_USE_TLS: ${SMTP_USE_TLS:-true}
SMTP_OPPORTUNISTIC_TLS: ${SMTP_OPPORTUNISTIC_TLS:-false}
SMTP_LOCAL_HOSTNAME: ${SMTP_LOCAL_HOSTNAME:-}
SENDGRID_API_KEY: ${SENDGRID_API_KEY:-}
INDEXING_MAX_SEGMENTATION_TOKENS_LENGTH: ${INDEXING_MAX_SEGMENTATION_TOKENS_LENGTH:-4000}
INVITE_EXPIRY_HOURS: ${INVITE_EXPIRY_HOURS:-72}
Expand Down