Skip to content

fix(migrations): preserve percent-encoded database URLs#2733

Open
Jordan-Jarvis wants to merge 3 commits into
vectorize-io:mainfrom
Jordan-Jarvis:fix/alembic-percent-escaping
Open

fix(migrations): preserve percent-encoded database URLs#2733
Jordan-Jarvis wants to merge 3 commits into
vectorize-io:mainfrom
Jordan-Jarvis:fix/alembic-percent-escaping

Conversation

@Jordan-Jarvis

Copy link
Copy Markdown
Contributor

Summary

  • escape % only at Alembic's Config.set_main_option boundary
  • apply the same ConfigParser-safe write path to every programmatic migration option
  • preserve the original URL returned by get_main_option
  • add regression coverage for a percent-encoded database credential

Fixes #2730.

Why

Alembic stores programmatic options through ConfigParser, where % is interpolation syntax. Valid SQLAlchemy/libpq URLs containing percent-encoded credentials can otherwise fail before a database connection is attempted.

This does not rewrite or normalize the database URL. Doubling % is the required ConfigParser representation; reading the option returns the original single-percent value.

Validation

  • Ruff lint passed on the changed files.
  • The same focused change and regression test have passed a downstream Hindsight API release-qualification lane.
  • Upstream CI is the authoritative full-suite validation for the current main base.

@benfrank241

Copy link
Copy Markdown
Member

Thanks @Jordan-Jarvis — nice catch, and the diagnosis is right: percent-encoded credentials hit ConfigParser interpolation and fail before a connection is ever attempted. I verified the fix locally: tests/test_migrations_newer_revision.py is 4/4, including your test_percent_encoded_database_url_is_preserved, and the escape-at-the-set_main_option-boundary approach is correct (get_main_option returns the original single-% URL, so the driver gets the URL unchanged).

One thing to fix before this can merge: all three changed files (hindsight_api/alembic/env.py, hindsight_api/migrations.py, tests/test_migrations_newer_revision.py) lost their trailing newline, so lint fails:

$ uv run ruff format --check <files>   ->  3 files would be reformatted
$ uv run ruff check <files>            ->  3 errors: "Add trailing newline"

That will red the lint job in CI. Easiest fix is to run the repo hook, which auto-adds them:

./scripts/hooks/lint.sh

(or uv run ruff check --fix / ruff format on the three files), then push. Once lint is green this is a clean merge — the actual fix and its regression coverage look good. Thanks!

@Jordan-Jarvis

Copy link
Copy Markdown
Contributor Author

Fixed the requested trailing-newline lint failures and merged current upstream main into the branch.

Validation on the updated head:

  • Ruff check: passed
  • Ruff format check: passed
  • tests/test_migrations_newer_revision.py: 4 passed
  • repository pre-commit lint hook: passed

@Jordan-Jarvis

Copy link
Copy Markdown
Contributor Author

@benfrank241 the requested trailing-newline fix is on the current head, focused tests and the repository lint hook are green, and the PR remains clean/mergeable. Since your review found the implementation correct with that one issue resolved, could you merge it when available?

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.

fix(migrations): escape percent signs before Alembic set_main_option

2 participants