Skip to content

fix(config): reject empty TOML sslmode before DSN fallback - #451

Merged
tianzhou merged 1 commit into
bytebase:mainfrom
tsiakoulias:fix/toml-empty-sslmode
Oct 2, 2026
Merged

tianzhou merged 1 commit into
bytebase:mainfrom
tsiakoulias:fix/toml-empty-sslmode

Conversation

@tsiakoulias

Copy link
Copy Markdown
Contributor

Problem

While checking #443, we found that the TOML loader can replace an empty sslmode with the DSN value before validation.

This affects PostgreSQL, MySQL, MariaDB, SQL Server, and Oracle. SQLite already rejects an explicit sslmode.

Change

Use the DSN value only when the TOML sslmode field is absent. The existing validation then rejects empty values.

This fix is separate from #443 because it affects the shared TOML loader.

Tests

Add two regression cases with a valid DSN mode:

  • An empty TOML sslmode.
  • An sslmode from an empty environment variable.

Copilot AI balanced review requested due to automatic review settings October 1, 2026 22:16
@tsiakoulias
tsiakoulias requested a review from tianzhou as a code owner October 1, 2026 22:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

The focused change correctly distinguishes absent and explicitly empty values, with adequate regression coverage.

Review effort: Balanced
Findings: None

What changed in this PR

Corrects TOML SSL mode handling so explicit empty values are validated instead of replaced by DSN values.

Changes:

  • Uses DSN sslmode only when the TOML field is absent.
  • Adds regression tests for literal and environment-derived empty values.
  • Restores stubbed environment variables after tests.
File Description
src/​config/​toml-loader.ts Preserves explicit empty sslmode values for validation.
src/​config/​__tests__/​toml-loader.test.ts Adds regression coverage and environment cleanup.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@tianzhou tianzhou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

Thanks for the contribution.

@tianzhou
tianzhou merged commit 4f31da0 into bytebase:main Oct 2, 2026
2 checks passed
@tsiakoulias
tsiakoulias deleted the fix/toml-empty-sslmode branch October 2, 2026 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants