-
Notifications
You must be signed in to change notification settings - Fork 247
fix(region): route projects in any cloud region to their subdomain (DISCORD-15265968) #3119
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| import { describe, expect, it } from 'vitest'; | ||
| import { | ||
| buildRegionalV1Endpoint, | ||
| getRegionSubdomain, | ||
| stripLeadingRegionSubdomain | ||
| } from '$lib/helpers/apiEndpoint'; | ||
|
|
||
| describe('getRegionSubdomain', () => { | ||
| it('maps the well-known regions to their subdomains', () => { | ||
| expect(getRegionSubdomain('fra')).toBe('fra.'); | ||
| expect(getRegionSubdomain('syd')).toBe('syd.'); | ||
| expect(getRegionSubdomain('nyc')).toBe('nyc.'); | ||
| expect(getRegionSubdomain('sfo')).toBe('sfo.'); | ||
| expect(getRegionSubdomain('sgp')).toBe('sgp.'); | ||
| expect(getRegionSubdomain('tor')).toBe('tor.'); | ||
| }); | ||
|
|
||
| it('derives a subdomain for regions that are not hardcoded', () => { | ||
| // A project living in a newer Appwrite Cloud region (e.g. Bangalore) | ||
| // must still be routed to its own regional subdomain. Previously these | ||
| // fell through to the empty default, so the SDK hit the apex domain and | ||
| // navigation got stuck with CORS errors. | ||
| expect(getRegionSubdomain('blr')).toBe('blr.'); | ||
| expect(getRegionSubdomain('lon')).toBe('lon.'); | ||
| }); | ||
|
|
||
| it('resolves the default placeholder and empty values to the apex domain', () => { | ||
| expect(getRegionSubdomain('default')).toBe(''); | ||
| expect(getRegionSubdomain('')).toBe(''); | ||
| expect(getRegionSubdomain(undefined)).toBe(''); | ||
| }); | ||
| }); | ||
|
|
||
| describe('buildRegionalV1Endpoint', () => { | ||
| it('leaves the host untouched when multi-region is disabled', () => { | ||
| expect(buildRegionalV1Endpoint('https:', 'cloud.appwrite.io', 'blr', false)).toBe( | ||
| 'https://cloud.appwrite.io/v1' | ||
| ); | ||
| }); | ||
|
|
||
| it('leaves the host untouched for the default/unknown placeholder region', () => { | ||
| expect(buildRegionalV1Endpoint('https:', 'fra.cloud.appwrite.io', 'default', true)).toBe( | ||
| 'https://fra.cloud.appwrite.io/v1' | ||
| ); | ||
| }); | ||
|
|
||
| it('prepends the regional subdomain for a hardcoded region', () => { | ||
| expect(buildRegionalV1Endpoint('https:', 'fra.cloud.appwrite.io', 'nyc', true)).toBe( | ||
| 'https://nyc.cloud.appwrite.io/v1' | ||
| ); | ||
| }); | ||
|
|
||
| it('prepends the regional subdomain for a region outside the hardcoded set', () => { | ||
| // apex host | ||
| expect(buildRegionalV1Endpoint('https:', 'cloud.appwrite.io', 'blr', true)).toBe( | ||
| 'https://blr.cloud.appwrite.io/v1' | ||
| ); | ||
| // host that already carries a known region label | ||
| expect(buildRegionalV1Endpoint('https:', 'fra.cloud.appwrite.io', 'blr', true)).toBe( | ||
| 'https://blr.cloud.appwrite.io/v1' | ||
| ); | ||
| }); | ||
|
|
||
| it('does not double the subdomain when the host already carries the target region', () => { | ||
| expect(buildRegionalV1Endpoint('https:', 'blr.cloud.appwrite.io', 'blr', true)).toBe( | ||
| 'https://blr.cloud.appwrite.io/v1' | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| describe('stripLeadingRegionSubdomain', () => { | ||
| it('strips a known leading region label', () => { | ||
| expect(stripLeadingRegionSubdomain('fra.cloud.appwrite.io')).toBe('cloud.appwrite.io'); | ||
| }); | ||
|
|
||
| it('collapses doubled region labels', () => { | ||
| expect(stripLeadingRegionSubdomain('fra.fra.cloud.appwrite.io')).toBe('cloud.appwrite.io'); | ||
| }); | ||
|
|
||
| it('leaves a bare apex host untouched', () => { | ||
| expect(stripLeadingRegionSubdomain('cloud.appwrite.io')).toBe('cloud.appwrite.io'); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,4 @@ | ||
| import { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @claudear very bad way of fixing it here. No requests were made out of these regions. So error is somewhere else |
||
| REGION_FRA, | ||
| REGION_NYC, | ||
| REGION_SFO, | ||
| REGION_SGP, | ||
| REGION_SYD, | ||
| REGION_TOR, | ||
| SUBDOMAIN_FRA, | ||
| SUBDOMAIN_NYC, | ||
| SUBDOMAIN_SFO, | ||
|
|
@@ -13,6 +7,12 @@ import { | |
| SUBDOMAIN_TOR | ||
| } from '$lib/constants'; | ||
|
|
||
| /** | ||
| * The placeholder region used for region-less projects. It maps to the apex | ||
| * domain (no subdomain) and must never be turned into a subdomain. | ||
| */ | ||
| const DEFAULT_REGION = 'default'; | ||
|
|
||
| /** Ordered list of region DNS prefixes (e.g. `fra.`) for stripping from API hostnames. */ | ||
| const REGION_SUBDOMAIN_PREFIXES: readonly string[] = [ | ||
| SUBDOMAIN_FRA, | ||
|
|
@@ -26,14 +26,21 @@ const REGION_SUBDOMAIN_PREFIXES: readonly string[] = [ | |
| /** | ||
| * Removes leading Appwrite Cloud region label(s) from `hostname` (e.g. `fra.`). | ||
| * Strips repeatedly so a doubled prefix (`fra.fra.cloud...`) does not become | ||
| * `nyc.fra.cloud...` after prepending another region. | ||
| * `nyc.fra.cloud...` after prepending another region. An optional `extraPrefix` | ||
| * (the region about to be prepended) is stripped too, so regions outside the | ||
| * well-known set do not get doubled up. | ||
| */ | ||
| export function stripLeadingRegionSubdomain(hostname: string): string { | ||
| export function stripLeadingRegionSubdomain(hostname: string, extraPrefix?: string): string { | ||
| const prefixes = | ||
| extraPrefix && !REGION_SUBDOMAIN_PREFIXES.includes(extraPrefix) | ||
| ? [extraPrefix, ...REGION_SUBDOMAIN_PREFIXES] | ||
| : REGION_SUBDOMAIN_PREFIXES; | ||
|
Comment on lines
+34
to
+37
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/lib/helpers/apiEndpoint.ts
Line: 34-37
Comment:
**Unlisted source-region prefix not stripped when navigating to a different region**
`extraPrefix` is the subdomain being *prepended* (the target region), so it only prevents doubling when the hostname already carries the same unlisted region as the destination. If the console is served from an unlisted region (e.g. `blr.cloud.appwrite.io`) and the user navigates to a *different* region (e.g. `nyc`), `extraPrefix = 'nyc.'` is already in `REGION_SUBDOMAIN_PREFIXES`, so the original list is used unchanged and `blr.` is never stripped — the result is `nyc.blr.cloud.appwrite.io/v1` instead of `nyc.cloud.appwrite.io/v1`. This scenario is real when `APPWRITE_ENDPOINT` is unset and `getApiEndpoint` in `sdk.ts` falls back to `globalThis.location`, picking up the console's own host, which can itself carry a new-region prefix.
How can I resolve this? If you propose a fix, please make it concise. |
||
|
|
||
| let host = hostname; | ||
| let changed = true; | ||
| while (changed) { | ||
| changed = false; | ||
| for (const prefix of REGION_SUBDOMAIN_PREFIXES) { | ||
| for (const prefix of prefixes) { | ||
| if (host.startsWith(prefix)) { | ||
| host = host.slice(prefix.length); | ||
| changed = true; | ||
|
|
@@ -44,24 +51,17 @@ export function stripLeadingRegionSubdomain(hostname: string): string { | |
| return host; | ||
| } | ||
|
|
||
| /** Region prefix (e.g. `fra.`) used before the API hostname when multi-region is enabled. */ | ||
| /** | ||
| * Region prefix (e.g. `fra.`) used before the API hostname when multi-region is | ||
| * enabled. The set of Appwrite Cloud regions is dynamic and grows over time, so | ||
| * the prefix is derived directly from the region id rather than a hardcoded | ||
| * list — otherwise projects in a newly added region resolve to no subdomain, | ||
| * hit the apex domain, and fail with CORS errors. The `default` placeholder | ||
| * (and empty/undefined values) resolve to the apex domain. | ||
| */ | ||
| export function getRegionSubdomain(region?: string): string { | ||
| switch (region) { | ||
| case REGION_FRA: | ||
| return SUBDOMAIN_FRA; | ||
| case REGION_SYD: | ||
| return SUBDOMAIN_SYD; | ||
| case REGION_NYC: | ||
| return SUBDOMAIN_NYC; | ||
| case REGION_SFO: | ||
| return SUBDOMAIN_SFO; | ||
| case REGION_SGP: | ||
| return SUBDOMAIN_SGP; | ||
| case REGION_TOR: | ||
| return SUBDOMAIN_TOR; | ||
| default: | ||
| return ''; | ||
| } | ||
| if (!region || region === DEFAULT_REGION) return ''; | ||
| return `${region}.`; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -85,6 +85,6 @@ export function buildRegionalV1Endpoint( | |
| return `${protocol}//${hostname}/v1`; | ||
| } | ||
|
|
||
| const hostWithoutRegion = stripLeadingRegionSubdomain(hostname); | ||
| const hostWithoutRegion = stripLeadingRegionSubdomain(hostname, subdomain); | ||
| return `${protocol}//${subdomain}${hostWithoutRegion}/v1`; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing
buildRegionalV1Endpointtests coverfra → blrandblr → blr, but notblr → nyc(unlisted source region, different listed target region). Adding a case likebuildRegionalV1Endpoint('https:', 'blr.cloud.appwrite.io', 'nyc', true)and asserting it returnshttps://nyc.cloud.appwrite.io/v1would pin down the behaviour described above and catch any future regression in the stripping logic.Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!