Skip to content

fix(gh-446): accept the motherduck: prefix in the city renderer and sync - #457

Draft
JesuFemi-O wants to merge 1 commit into
fix/gh-448-raw-post-load-guardsfrom
fix/gh-446-motherduck-prefix-city-sync
Draft

JesuFemi-O wants to merge 1 commit into
fix/gh-448-raw-post-load-guardsfrom
fix/gh-446-motherduck-prefix-city-sync

Conversation

@JesuFemi-O

@JesuFemi-O JesuFemi-O commented Oct 27, 2026 •

Copy link
Copy Markdown
Collaborator

What this does

#454 taught the tycoon package to accept motherduck: as well as md:. Four places still checked startswith("md:"), so a motherduck: catalog was resolved as a local path there: the city renderer and its catalog loader (src/tycoon_city/), tycoon city, and tycoon data sync. They now accept both prefixes.

Mental model

  • Input: a catalog string such as motherduck:my_db passed to tycoon city, the city renderer or tycoon data sync.
  • Change: tycoon city and sync use the is_motherduck() helper from tycoon.project. tycoon_city must not import tycoon (docs/city/conventions.md), so catalog/loader.py carries the same two-prefix rule and pricing.py and webserve.py import it from there. The catalog-name logic strips either prefix, and the check is case-insensitive like DuckDB's (MD:my_db, MotherDuck:my_db); ./MD:my_db stays a file path.
  • Outcome: motherduck:x and md:x behave the same. A local path or an md: string behaves as before.

Known gap

The default tycoon city path, which serves the project directory, still fails on a MotherDuck warehouse with "Database file not found": src/tycoon_city/catalog/tycoon_project.py builds root / warehouse as a path. This PR already touches 8 files, so fixing it here would exceed the cap and it is left as a known remaining gap tracked by #446. tycoon city --path md:db passes the target through to the server unresolved, which a test covers; I did not run it end to end against a real catalog.

Linked issue

Refs #446

Refs #308

Depends on #456. This is the last PR for #308.

Type of change

  • Bug fix
  • New feature
  • Docs
  • Refactor / internal
  • Other:

Checklist

  • pre-commit run --all-files passes
  • Tests pass locally, and the offline csv-import pipeline still runs end to end (this is the CI gate — if it breaks, the release is blocked)
  • Added or updated tests where it made sense
  • Updated CHANGELOG.md if this is user-facing
  • Updated docs / README if behavior or commands changed

Notes for reviewers

This change does not alter layout and touches no file under web/, so there is no screenshot. The catalog-name tests in tests/tycoon_city/catalog/test_loader.py and tests/test_city_command.py show the behaviour instead.

I rewrote test_motherduck_source_is_not_treated_as_a_file_path. It called load_catalog("md:definitely_not_a_real_catalog_xyz") with nothing mocked, so it opened a real MotherDuck connection. With no token set, DuckDB answers that with a browser sign-in, so every default pytest run on a machine without a token could pop up a device confirmation. The test now mocks duckdb.connect and runs for both prefixes.

This PR and #454 each sit at the 8-file cap.

Verification:

  • uv run pytest -q at this branch gave 1666 passed and 10 skipped, with MOTHERDUCK_TOKEN=invalid-token-for-test and DLT_DATA_DIR set to a writable directory.
  • In a local script, load_catalog and the data sync ATTACH worked under both prefixes against a scratch database. That script is not committed as a test.
  • Upgrade safety: md: strings and local paths resolve as before, including paths that contain md: later in the string.
  • pre-commit run --all-files fails on a clean tree and rewrites nine unrelated files, so I ran pre-commit run --files on the changed files instead, plus ruff check and ruff format --check.

@JesuFemi-O
JesuFemi-O added this pull request to stack #458 October 27, 2026 11:04
@github-actions

Copy link
Copy Markdown

PR title check

mode: warn

fix(gh-446): accept the motherduck: prefix in the city renderer and sync

Result: ✅ title matches the convention.

@github-actions

Copy link
Copy Markdown

PR size check

mode: warn | file limit: 8

Files changed: 8 counted / 8 allowed

  • CHANGELOG.md
  • src/tycoon/commands/city.py
  • src/tycoon/sync.py
  • src/tycoon_city/catalog/loader.py
  • src/tycoon_city/pricing.py
  • src/tycoon_city/webserve.py
  • tests/test_city_command.py
  • tests/tycoon_city/catalog/test_loader.py

Result: ✅ within limits.

@JesuFemi-O
JesuFemi-O force-pushed the fix/gh-446-motherduck-prefix-city-sync branch from 8dfc848 to 2dd8994 Compare October 27, 2026 11:30
@JesuFemi-O
JesuFemi-O force-pushed the fix/gh-446-motherduck-prefix-city-sync branch 2 times, most recently from 75b251f to e0c59d6 Compare October 27, 2026 15:50

@db-tycoon-stephen db-tycoon-stephen 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.

Independent review at e0c59d6. This replaces an earlier automated approval whose summary was not based on the diff. The approval stays, but the item under "Should fix" is worth settling before this merges.

What it does: tycoon city (_serve_target) and tycoon data sync use is_motherduck() from tycoon.project. Because tycoon_city can't import tycoon, catalog/loader.py gets its own two-prefix is_motherduck, used by pricing.py and webserve.py, and the catalog-name logic strips either prefix. It also rewrites a test that used to open a real MotherDuck connection without a token.

Checked: motherduck:dogfood gets the same alias, name and serve target as md:dogfood; motherduck:_share/n/uuid names the catalog n; local catalogs are unaffected. All 11 CI checks pass.

Should fix

  • src/tycoon_city/catalog/tycoon_project.py:62: the default tycoon city, which reads the project directory, still builds warehouse_path = root / warehouse, so motherduck:mydb (and md:mydb) fails with "Database file not found". The body says Closes #308, but #308 is still live on this path. Fix it here, or change to Refs #308 and track it separately.

Nits

  • loader.py _database_name_for and sync.py _alias_for keep a ?motherduck_token= query in the display name and the ATTACH alias, and the name reaches city.json. Splitting on ? first is cheap since this PR touches both lines. This predates the PR but now applies to motherduck: too.
  • is_motherduck is now public in loader.py but not in __all__.

tycoon city and sync use the helper from tycoon.project. tycoon_city does not import tycoon (docs/city/conventions.md), so the loader carries the same two-prefix rule and pricing and webserve import it from there. The catalog name logic strips either prefix.
@JesuFemi-O
JesuFemi-O force-pushed the fix/gh-446-motherduck-prefix-city-sync branch from e0c59d6 to 69282ea Compare November 4, 2026 21:13

This branch has not been deployed

No deployments
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.

2 participants