Skip to content

Fix: Mount v1Router to enable /api/v1/* routes#21

Merged
Uchechukwu-Ekezie merged 2 commits into
grantFoxin:mainfrom
bbjiggy:fix-issue-9-mount-v1-router
Jun 18, 2026
Merged

Fix: Mount v1Router to enable /api/v1/* routes#21
Uchechukwu-Ekezie merged 2 commits into
grantFoxin:mainfrom
bbjiggy:fix-issue-9-mount-v1-router

Conversation

@bbjiggy

@bbjiggy bbjiggy commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Closes #9

Changes

  • Imported and mounted v1Router at /api/v1 (canonical namespace)
  • Added legacyApiDeprecation middleware to /api routes (legacy namespace)
  • Both namespaces now work as documented

Testing

# Canonical v1 endpoint now works
curl http://localhost:3001/api/v1/portfolio/GABC...

# Legacy endpoint still works with deprecation headers
curl -I http://localhost:3001/api/portfolio/GABC...

@Uchechukwu-Ekezie Uchechukwu-Ekezie 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.

Good targeted fix — mounts v1Router at the canonical /api/v1 namespace and wraps the legacy /api routes with a deprecation middleware. A few things worth discussing before merge:

Route ordering looks correct. Express matches in declaration order, so /api/v1/* hits v1Router before the broader /api prefix kicks in. No issue there.

The app.use('/', portfolioRouter) mount is a concern. This was pre-existing, but with this PR now explicitly formalizing the namespace strategy it's worth addressing: any route defined in portfolioRouter (e.g. /portfolio/:address) is now reachable at both /api/portfolio/:address and /portfolio/:address (root-level). That's likely unintentional exposure and should be removed or documented.

v1Router contents aren't visible in this diff. The fix assumes v1Router already exists and is correct — but we can't verify whether it exposes the same contract as the documented /api/v1/* endpoints, or if it's a thin re-export of portfolioRouter. Worth a quick note confirming what's inside, especially since this is the canonical namespace going forward.

No test coverage added. Even a single integration test confirming GET /api/v1/portfolio/:address returns 200 and GET /api/portfolio/:address returns 200 with a deprecation header would lock this in and prevent future regressions.

The core change is minimal and the intent is sound. Clean up the root mount concern and we're good.

@bbjiggy

bbjiggy commented Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

Good targeted fix — mounts v1Router at the canonical /api/v1 namespace and wraps the legacy /api routes with a deprecation middleware. A few things worth discussing before merge:

Route ordering looks correct. Express matches in declaration order, so /api/v1/* hits v1Router before the broader /api prefix kicks in. No issue there.

The app.use('/', portfolioRouter) mount is a concern. This was pre-existing, but with this PR now explicitly formalizing the namespace strategy it's worth addressing: any route defined in portfolioRouter (e.g. /portfolio/:address) is now reachable at both /api/portfolio/:address and /portfolio/:address (root-level). That's likely unintentional exposure and should be removed or documented.

v1Router contents aren't visible in this diff. The fix assumes v1Router already exists and is correct — but we can't verify whether it exposes the same contract as the documented /api/v1/* endpoints, or if it's a thin re-export of portfolioRouter. Worth a quick note confirming what's inside, especially since this is the canonical namespace going forward.

No test coverage added. Even a single integration test confirming GET /api/v1/portfolio/:address returns 200 and GET /api/portfolio/:address returns 200 with a deprecation header would lock this in and prevent future regressions.

The core change is minimal and the intent is sound. Clean up the root mount concern and we're good.

It's done.
I've removed root mount - app.use('/', portfolioRouter).

Added integration tests - Tests verify:

  • /api/v1/prices returns 200 (canonical)
  • /api/prices returns 200 with deprecation headers (legacy)
  • Both namespaces return same data structure
  • Root-level routes are not exposed

@Uchechukwu-Ekezie

Copy link
Copy Markdown
Contributor

Thanks I will review shortly

bbjiggy added 2 commits June 18, 2026 15:14
- Import and mount v1Router at /api/v1 (canonical namespace)
- Add legacyApiDeprecation middleware to /api routes
- Fixes 404 errors on all /api/v1/* endpoints

Closes grantFoxin#9
- Remove app.use('/', portfolioRouter) to prevent unintentional root-level exposure
- Add integration tests for /api/v1/* (canonical) and /api/* (legacy)
- Tests verify both namespaces work and deprecation headers are present
@bbjiggy bbjiggy force-pushed the fix-issue-9-mount-v1-router branch from ba5d74c to cc75786 Compare June 18, 2026 14:15
@Uchechukwu-Ekezie Uchechukwu-Ekezie merged commit 9fd549e into grantFoxin:main Jun 18, 2026
1 check passed
@Uchechukwu-Ekezie

Copy link
Copy Markdown
Contributor

nice job

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.

bug: /api/v1/* routes return 404 — v1Router is defined but never mounted in Express app

2 participants