Skip to content

Commit d7efd20

Browse files
jeffmakijeffmaki
authored
Cleanup HTTP client headers and testing code (#55)
This cleans up the abstraction for HTTP request header configuration in the various API clients by merging headers instead of overwriting them. This also cleans up some old HTTP testing code which resolves some potential auth token refresh issues in TDEI calls. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary This PR refactors HTTP client header handling across the frontend services to merge headers instead of overwriting them, and removes legacy testing code. The changes introduce type-safe configurations for HTTP requests and centralize header management in the base HTTP client. ## Changes by File ### services/http.ts - Added exported types `FetchConfig` (based on `RequestInit` with optional typed headers) and `HttpBody` (union of `BodyInit | object`) - Refactored `BaseHttpClient._send` pipeline to merge default headers with per-request headers from `FetchConfig` - Improved body handling: FormData strips the `Content-Type` header to allow browser defaults, and JSON stringification is determined by both resolved `Content-Type` and runtime body type - Removed the `_sendTest` helper method, simplifying the test code cleanup - Updated all request method signatures (`_get`, `_post`, `_put`, `_patch`, `_delete`) to use the new `FetchConfig` type ### services/osm.ts - Updated method signature for `_send` override to use typed `HttpBody` and `FetchConfig` parameters - Changed default `Accept` header from `text/plain` to `*/*` - Refactored `provisionUser` to pass JSON objects directly with `Content-Type: application/json` instead of manually stringifying - Removed per-request spreading of internal `_requestHeaders` across multiple methods (workspace/element/node/way retrieval, changeset operations, notes search, export) - Centralized auth handling: `_send` override now calls `tryRefreshAuth()` before sending and re-applies auth headers via `#setAuthHeader()`, then merges `credentials: 'include'` with provided config ### services/tdei.ts - Updated `_send` override signatures in both `TdeiClient` and `TdeiUserClient` to use typed `HttpBody` and `FetchConfig` parameters - Replaced `_sendTest` calls with `_get` for dataset downloads (`downloadOswDataset`, `downloadPathwaysDataset`) - Removed explicit per-request `Authorization` header injection from upload methods - Simplified dataset conversion flow by removing manual `Accept`/`Authorization` header handling - Preserved refresh-before-send behavior in both client classes ### services/workspaces.ts - Updated `WorkspacesClient._send` method signature to use typed `HttpBody` and `FetchConfig` parameters instead of loose types - Runtime behavior preserved: auth refresh, header setting, and error wrapping remain unchanged ## Key Improvements - Centralized header merging logic prevents accidental header overwriting - Type-safe HTTP configuration across all services - Removal of `_sendTest` test helper reduces code duplication - Consistent auth token refresh handling, addressing potential token refresh issues in TDEI calls <!-- review_stack_entry_start --> [![Review Change Stack](https://storage.googleapis.com/coderabbit_public_assets/review-stack-in-coderabbit-ui.svg)](https://app.coderabbit.ai/change-stack/TaskarCenterAtUW/workspaces-frontend/pull/55?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 parents 1f86006 + faec03d commit d7efd20

4 files changed

Lines changed: 114 additions & 82 deletions

File tree

‎services/http.ts‎

Lines changed: 44 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,9 @@
1+
export type FetchConfig = Omit<RequestInit, 'headers'> & {
2+
headers?: Record<string, string>;
3+
};
4+
5+
export type HttpBody = BodyInit | object;
6+
17
export class BaseHttpClientError extends Error {
28
response: Response;
39

@@ -25,59 +31,66 @@ export abstract class BaseHttpClient {
2531
return this._baseUrl + rest
2632
}
2733

28-
_get(url: string, config?: object): Promise<Response> {
34+
_get(url: string, config?: FetchConfig): Promise<Response> {
2935
return this._send(url, 'GET', undefined, config);
3036
}
3137

32-
_post(url: string, body?: any, config?: object): Promise<Response> {
38+
_post(url: string, body?: HttpBody, config?: FetchConfig): Promise<Response> {
3339
return this._send(url, 'POST', body, config);
3440
}
3541

36-
_put(url: string, body?: any, config?: object): Promise<Response> {
42+
_put(url: string, body?: HttpBody, config?: FetchConfig): Promise<Response> {
3743
return this._send(url, 'PUT', body, config);
3844
}
3945

40-
_patch(url: string, body?: any, config?: object): Promise<Response> {
46+
_patch(url: string, body?: HttpBody, config?: FetchConfig): Promise<Response> {
4147
return this._send(url, 'PATCH', body, config);
4248
}
4349

44-
_delete(url: string, config?: object): Promise<Response> {
50+
_delete(url: string, config?: FetchConfig): Promise<Response> {
4551
return this._send(url, 'DELETE', undefined, config);
4652
}
4753

48-
async _sendTest(url: string, method: string, body?: any): Promise<Response> {
49-
const response = await fetch(this.url(url), {
50-
method,
51-
body,
52-
headers: {
53-
'Accept': 'application/text',
54-
'Authorization': this._requestHeaders.Authorization
55-
}
56-
});
57-
58-
if (!response.ok) {
59-
throw new BaseHttpClientError(response);
60-
}
61-
62-
return response;
63-
}
64-
65-
async _send(url: string, method: string, body?: any, config?: object): Promise<Response> {
54+
async _send(
55+
url: string,
56+
method: string,
57+
body?: HttpBody,
58+
config?: FetchConfig,
59+
): Promise<Response> {
60+
const { headers: configHeaders, ...restConfig } = config ?? {};
61+
const mergedHeaders: Record<string, string> = {
62+
...this._requestHeaders,
63+
...configHeaders,
64+
};
6665
const requestOptions = {
6766
method,
68-
body,
69-
headers: this._requestHeaders,
67+
body: undefined as BodyInit | undefined,
68+
headers: mergedHeaders,
7069
signal: this._abortSignal,
71-
...config
70+
...restConfig,
7271
};
7372

74-
if (requestOptions.headers['Content-Type'] === 'application/json'
75-
&& !(body instanceof FormData)
76-
&& typeof body !== 'undefined'
77-
&& body !== null
78-
) {
73+
// Let the browser set Content-Type (and multipart boundary) for FormData:
74+
if (body instanceof FormData) {
75+
delete mergedHeaders['Content-Type'];
76+
requestOptions.body = body;
77+
}
78+
else if (body !== null && body !== undefined && (
79+
mergedHeaders['Content-Type'] === 'application/json'
80+
|| (
81+
typeof body === 'object'
82+
&& !(body instanceof Blob)
83+
&& !(body instanceof ArrayBuffer)
84+
&& !ArrayBuffer.isView(body)
85+
&& !(body instanceof URLSearchParams)
86+
&& !(body instanceof ReadableStream)
87+
)
88+
)) {
7989
requestOptions.body = JSON.stringify(body);
8090
}
91+
else {
92+
requestOptions.body = body as BodyInit;
93+
}
8194

8295
const response = await fetch(this.url(url), requestOptions);
8396

‎services/osm.ts‎

Lines changed: 25 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,12 @@
11
import parseOsmChangeXml from '@osmcha/osmchange-parser';
22
import type { FeatureCollection, Point } from 'geojson';
33

4-
import { BaseHttpClient, BaseHttpClientError } from "~/services/http";
4+
import {
5+
BaseHttpClient,
6+
BaseHttpClientError,
7+
type FetchConfig,
8+
type HttpBody,
9+
} from '~/services/http';
510
import * as xml from '~/util/xml';
611

712
import type { ICancelableClient } from '~/services/loading';
@@ -14,7 +19,6 @@ import type {
1419
OsmElement,
1520
OsmNode,
1621
OsmNote,
17-
OsmTags,
1822
OsmWay,
1923
} from '~/types/osm';
2024
import type { WorkspaceId } from '~/types/workspaces';
@@ -207,7 +211,9 @@ export class OsmApiClient extends BaseHttpClient implements ICancelableClient {
207211
this.#webUrl = webUrl;
208212
this.#tdeiClient = tdeiClient;
209213
this.#setAuthHeader();
210-
this._requestHeaders['Accept'] = 'text/plain';
214+
215+
// OSM API can return XML or JSON based on the header or file extension:
216+
this._requestHeaders['Accept'] = '*/*';
211217
this._requestHeaders['Content-Type'] = 'text/plain';
212218
}
213219

@@ -234,8 +240,8 @@ export class OsmApiClient extends BaseHttpClient implements ICancelableClient {
234240
display_name: this.auth.displayName
235241
};
236242

237-
await this._put(`user/${this.auth.subject}`, JSON.stringify(body), {
238-
headers: { ...this._requestHeaders, 'Content-Type': 'application/json' }
243+
await this._put(`user/${this.auth.subject}`, body, {
244+
headers: { 'Content-Type': 'application/json' }
239245
});
240246
}
241247

@@ -286,7 +292,6 @@ export class OsmApiClient extends BaseHttpClient implements ICancelableClient {
286292
): Promise<OsmElement> {
287293
const response = await this._get(`${type}/${id}/${version}`, {
288294
headers: {
289-
...this._requestHeaders,
290295
'Accept': 'application/json',
291296
'X-Workspace': workspaceId,
292297
},
@@ -301,7 +306,6 @@ export class OsmApiClient extends BaseHttpClient implements ICancelableClient {
301306
async getNodes(workspaceId: WorkspaceId, nodeIds: (number | string)[]): Promise<OsmNode[]> {
302307
const response = await this._get(`nodes?nodes=${nodeIds.join(',')}`, {
303308
headers: {
304-
...this._requestHeaders,
305309
'Accept': 'application/json',
306310
'X-Workspace': workspaceId,
307311
},
@@ -319,7 +323,6 @@ export class OsmApiClient extends BaseHttpClient implements ICancelableClient {
319323
async getWays(workspaceId: WorkspaceId, wayIds: (number | string)[]): Promise<OsmWay[]> {
320324
const response = await this._get(`ways?ways=${wayIds.join(',')}`, {
321325
headers: {
322-
...this._requestHeaders,
323326
'Accept': 'application/json',
324327
'X-Workspace': workspaceId,
325328
},
@@ -337,7 +340,6 @@ export class OsmApiClient extends BaseHttpClient implements ICancelableClient {
337340
async getWaysForNode(workspaceId: WorkspaceId, nodeId: number): Promise<OsmElement[]> {
338341
const response = await this._get(`node/${nodeId}/ways`, {
339342
headers: {
340-
...this._requestHeaders,
341343
'Accept': 'application/json',
342344
'X-Workspace': workspaceId,
343345
},
@@ -348,7 +350,7 @@ export class OsmApiClient extends BaseHttpClient implements ICancelableClient {
348350

349351
async listChangesets(workspaceId: WorkspaceId): Promise<OsmChangeset[]> {
350352
const response = await this._get(`changesets.json`, {
351-
headers: { ...this._requestHeaders, 'X-Workspace': workspaceId },
353+
headers: { 'X-Workspace': workspaceId },
352354
});
353355

354356
const changesets = (await response.json())?.changesets ?? [];
@@ -374,7 +376,6 @@ export class OsmApiClient extends BaseHttpClient implements ICancelableClient {
374376

375377
const response = await this._get(url, {
376378
headers: {
377-
...this._requestHeaders,
378379
'Accept': 'application/json',
379380
'X-Workspace': workspaceId,
380381
},
@@ -401,7 +402,6 @@ export class OsmApiClient extends BaseHttpClient implements ICancelableClient {
401402
{
402403
const response = await this._get(`changeset/${changesetId}/download`, {
403404
headers: {
404-
...this._requestHeaders,
405405
'Accept': 'application/xml',
406406
'X-Workspace': workspaceId,
407407
},
@@ -427,7 +427,7 @@ export class OsmApiClient extends BaseHttpClient implements ICancelableClient {
427427

428428
const body = xml.serialize(doc);
429429
const response = await this._put('changeset/create', body, {
430-
headers: { ...this._requestHeaders, 'X-Workspace': workspaceId },
430+
headers: { 'X-Workspace': workspaceId },
431431
});
432432

433433
return Number(await response.text());
@@ -441,7 +441,6 @@ export class OsmApiClient extends BaseHttpClient implements ICancelableClient {
441441
await this._post(`changeset/${changesetId}/upload`, changesetXml, {
442442
headers: {
443443
'Content-Type': 'application/xml',
444-
'Authorization': this._requestHeaders['Authorization'],
445444
'X-Workspace': workspaceId,
446445
},
447446
});
@@ -468,10 +467,7 @@ export class OsmApiClient extends BaseHttpClient implements ICancelableClient {
468467
body.append('text', message);
469468

470469
await this._post(`changeset/${changesetId}/comment`, body, {
471-
headers: {
472-
'Authorization': this._requestHeaders['Authorization'],
473-
'X-Workspace': workspaceId,
474-
},
470+
headers: { 'X-Workspace': workspaceId },
475471
});
476472
}
477473

@@ -484,7 +480,6 @@ export class OsmApiClient extends BaseHttpClient implements ICancelableClient {
484480

485481
const response = await this._get(`notes/search.json?${params}`, {
486482
headers: {
487-
...this._requestHeaders,
488483
'Accept': 'application/json',
489484
'X-Workspace': workspaceId,
490485
},
@@ -497,7 +492,6 @@ export class OsmApiClient extends BaseHttpClient implements ICancelableClient {
497492
const bboxParam = await this.getExportBbox(workspaceId);
498493
const response = await this._get(`map.json?bbox=${bboxParam}`, {
499494
headers: {
500-
...this._requestHeaders,
501495
'Accept': 'application/json',
502496
'X-Workspace': workspaceId
503497
}
@@ -510,7 +504,6 @@ export class OsmApiClient extends BaseHttpClient implements ICancelableClient {
510504
const bboxParam = await this.getExportBbox(workspaceId);
511505
const response = await this._get(`map?bbox=${bboxParam}`, {
512506
headers: {
513-
...this._requestHeaders,
514507
'Accept': 'application/xml',
515508
'X-Workspace': workspaceId
516509
}
@@ -525,17 +518,23 @@ export class OsmApiClient extends BaseHttpClient implements ICancelableClient {
525518
}
526519
}
527520

528-
async _send(url: string, method: string, body?: any, config?: object): Promise<Response> {
521+
override async _send(
522+
url: string,
523+
method: string,
524+
body?: HttpBody,
525+
config?: FetchConfig,
526+
): Promise<Response> {
529527
try {
530528
await this.#tdeiClient.tryRefreshAuth();
531529
this.#setAuthHeader();
532530

533-
const requestOptions = {
534-
credentials: 'include'
535-
}
531+
const requestOptions: FetchConfig = {
532+
credentials: 'include',
533+
};
536534

537535
return await super._send(url, method, body, { ...requestOptions, ...config });
538-
} catch (e: any) {
536+
}
537+
catch (e: unknown) {
539538
if (e instanceof BaseHttpClientError) {
540539
throw new OsmApiClientError(e.response);
541540
}

0 commit comments

Comments
 (0)