Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
"types": "./build/bin/qasphere.d.ts",
"scripts": {
"test": "vitest run",
"test:api-manifest": "vitest run src/tests/api-manifest.spec.ts",
"test:e2e:api": "vitest run src/tests/api.e2e.spec.ts",
"test:watch": "vitest",
"build": "npm run clean && tsc && ts-add-js-extension --dir=./build && chmod +x build/bin/qasphere.js",
"clean": "rm -rf ./build",
Expand Down
96 changes: 96 additions & 0 deletions src/api/publicApi.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
import { readFile } from 'node:fs/promises'
import { basename } from 'node:path'

import { CLI_VERSION } from '../utils/version'
import { withBaseUrl, withHeaders, withHttpRetry } from './utils'

export interface PublicApiRequest {
method: 'GET' | 'POST' | 'PATCH'
pathname: string
query?: URLSearchParams
jsonBody?: unknown
filePath?: string
}

const createPublicApiFetcher = (baseUrl: string, apiKey: string) =>
withHttpRetry(
withHeaders(withBaseUrl(fetch, baseUrl), {
Authorization: `ApiKey ${apiKey}`,
Accept: 'application/json',
'User-Agent': `qas-cli/${CLI_VERSION}`,
})
)

const buildErrorFromResponse = async (response: Response) => {
const contentType = response.headers.get('content-type') ?? ''
if (contentType.includes('application/json')) {
try {
const body = (await response.json()) as Record<string, unknown>
const message = typeof body.message === 'string' ? body.message : response.statusText
throw new Error(message)
} catch (error) {
if (error instanceof Error && error.message !== 'Unexpected end of JSON input') {
throw error
}
}
Comment on lines +28 to +35
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The buildErrorFromResponse function specifically re-throws JSON parsing errors only if the message is not 'Unexpected end of JSON input'. This approach might inadvertently suppress other valid JSON parsing errors, making it harder to debug issues where the API returns malformed JSON for reasons other than an incomplete payload. It's generally better to either re-throw all parsing errors with additional context or handle them more explicitly.

}

const text = await response.text()
throw new Error(text || response.statusText || `Request failed with status ${response.status}`)
}

const parseJsonResponse = async (response: Response) => {
if (response.status === 204) {
return null
}

const contentType = response.headers.get('content-type') ?? ''
if (!contentType.includes('application/json')) {
const text = await response.text()
return text ? JSON.parse(text) : null
Comment on lines +48 to +50
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

In parseJsonResponse, if the content-type header does not include 'application/json', the code still attempts to JSON.parse(text). This can lead to runtime errors if the response body is not valid JSON, even if the content type header indicates otherwise (e.g., text/plain). JSON parsing should ideally only occur when the content type explicitly indicates JSON. Consider only attempting JSON.parse(text) if contentType.includes('application/json'). If not, and text is not empty, it might be more appropriate to return the raw text or throw an error indicating an unexpected content type.

}

const text = await response.text()
return text ? JSON.parse(text) : null
Comment on lines +42 to +54
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both the if branch (non-JSON content type) and the fallthrough (JSON content type) execute identical code: response.text() then JSON.parse. The content-type check serves no purpose. This can be simplified to:

const parseJsonResponse = async (response: Response) => {
	if (response.status === 204) {
		return null
	}
	const text = await response.text()
	return text ? JSON.parse(text) : null
}

Also, for non-JSON content types, calling JSON.parse will throw an unhelpful SyntaxError if the server returns HTML (e.g. a proxy error page). Consider wrapping in a try/catch with a descriptive error message for that case.

}

export const executePublicApiRequest = async (
baseUrl: string,
apiKey: string,
request: PublicApiRequest
) => {
const fetcher = createPublicApiFetcher(baseUrl, apiKey)
const query = request.query?.size ? `?${request.query.toString()}` : ''
const url = `${request.pathname}${query}`

if (request.filePath) {
const fileBuffer = await readFile(request.filePath)
const formData = new FormData()
formData.append('file', new Blob([fileBuffer]), basename(request.filePath))
Comment on lines +67 to +69
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For file uploads, readFile loads the entire file into memory as a Buffer, then copies it into a Blob. For large files this could cause high memory pressure. Node 19.8+ has openAsBlob() which avoids buffering the entire file. Since the project supports Node 18+, this would need a fallback, but it may be worth noting as a future improvement or documenting a practical file size limit.

const response = await fetcher(url, {
method: request.method,
body: formData,
})
if (!response.ok) {
return buildErrorFromResponse(response)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buildErrorFromResponse always throws, so return here (and on line 92) is dead code that misleads readers into thinking a value is returned. Additionally, the catch block on line 31-34 matches error messages by the exact string "Unexpected end of JSON input", which is fragile across JS runtimes. Consider matching on SyntaxError type instead.

Suggested change
return buildErrorFromResponse(response)
await buildErrorFromResponse(response)

And similarly for line 91-92:

	await buildErrorFromResponse(response)

}
return parseJsonResponse(response)
}

const response = await fetcher(url, {
method: request.method,
headers:
request.jsonBody === undefined
? undefined
: {
'Content-Type': 'application/json',
},
body: request.jsonBody === undefined ? undefined : JSON.stringify(request.jsonBody),
})

if (!response.ok) {
return buildErrorFromResponse(response)
}

return parseJsonResponse(response)
}
129 changes: 129 additions & 0 deletions src/commands/api/executor.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
import { Readable } from 'node:stream'

import { executePublicApiRequest } from '../../api/publicApi'
import { loadEnvs } from '../../utils/env'
import {
ApiValidationError,
collectQueryInput,
buildPathname,
loadJsonBodyInput,
parseIntegerValue,
serializeQueryObject,
validateBodyMode,
validateWithSchema,
} from './helpers'
import { ApiEndpointSpec, ApiOptionSpec } from './types'

export interface ExecuteApiCommandOptions {
stdin?: Readable
baseUrl?: string
apiKey?: string
}

const normalizeOptionValue = (option: ApiOptionSpec, value: unknown) => {
if (option.type !== 'integer') {
return value
}

if (option.array) {
const items = Array.isArray(value) ? value : [value]
return items.map((item, index) => parseIntegerValue(`--${option.name}[${index}]`, item))
Comment on lines +29 to +30
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

When option.array is true, items.map is used, and parseIntegerValue is called for each item. If value itself was undefined and option.array is true, items would become [undefined]. Calling parseIntegerValue with undefined will throw an ApiValidationError. It's better to filter out undefined or null values from the items array before mapping to parseIntegerValue, or ensure parseIntegerValue can handle them gracefully (e.g., by returning undefined or null).

Suggested change
const items = Array.isArray(value) ? value : [value]
return items.map((item, index) => parseIntegerValue(`--${option.name}[${index}]`, item))
const items = Array.isArray(value) ? value : [value]
return items.filter((item) => item !== undefined && item !== null).map((item, index) => parseIntegerValue("--" + option.name + "[" + index + "]", item))

}

return parseIntegerValue(`--${option.name}`, value)
}

const collectValidatedQuery = (spec: ApiEndpointSpec, args: Record<string, unknown>) => {
const query = collectQueryInput(args, spec.queryOptions, spec.supportsCustomFieldFilters)
const normalizedQuery = Object.fromEntries(
Object.entries(query).map(([key, value]) => {
const option = spec.queryOptions?.find((item) => item.name === key)
return [key, option ? normalizeOptionValue(option, value) : value]
})
)
const queryInput = spec.queryAdapter ? spec.queryAdapter(normalizedQuery) : normalizedQuery
return validateWithSchema<Record<string, unknown>>('Query', spec.querySchema, queryInput)
}

const collectPathParams = (spec: ApiEndpointSpec, args: Record<string, unknown>) => {
return Object.fromEntries(
spec.pathParams.map((param) => {
const rawValue = args[param.name]
if (param.type === 'integer') {
return [param.name, parseIntegerValue(`<${param.name}>`, rawValue)]
}
if (typeof rawValue !== 'string' || rawValue.length === 0) {
throw new ApiValidationError(`<${param.name}> is required.`)
}
return [param.name, rawValue]
})
)
}

const collectBody = async (
spec: ApiEndpointSpec,
args: Record<string, unknown>,
stdin: Readable
) => {
if (spec.bodyMode === 'none') {
validateBodyMode(spec.bodyMode, args)
return undefined
}

if (spec.bodyMode === 'file') {
validateBodyMode('none', args)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

When spec.bodyMode is 'file', the function incorrectly calls validateBodyMode('none', args). The validateBodyMode function is designed to check for conflicting body input flags based on the expected bodyMode. If the bodyMode is 'file', it should validate against 'file' mode to ensure only the --file flag is used and no JSON body flags are present. Calling it with 'none' will incorrectly disallow the --file flag.

Suggested change
validateBodyMode('none', args)
validateBodyMode('file', args)

if (typeof args.file !== 'string' || args.file.length === 0) {
throw new ApiValidationError('--file is required for this command.')
}
return { filePath: args.file }
}

validateBodyMode('json', args)
const rawBody = await loadJsonBodyInput(args, stdin)
const normalizedBody = spec.bodyAdapter ? spec.bodyAdapter(rawBody) : rawBody
const body = validateWithSchema('Request body', spec.bodySchema, normalizedBody)
return { jsonBody: body }
}

export const buildApiRequestFromArgs = async (
spec: ApiEndpointSpec,
args: Record<string, unknown>,
stdin: Readable = process.stdin
) => {
const pathParams = collectPathParams(spec, args)
const query = collectValidatedQuery(spec, args)
const body = await collectBody(spec, args, stdin)

return {
method: spec.method,
pathname: buildPathname(spec.pathTemplate, pathParams),
query: serializeQueryObject(query),
...body,
}
}

export const executeApiCommand = async (
spec: ApiEndpointSpec,
args: Record<string, unknown>,
options?: ExecuteApiCommandOptions
) => {
loadEnvs()
const request = await buildApiRequestFromArgs(spec, args, options?.stdin)
const baseUrl = options?.baseUrl ?? process.env.QAS_URL
const apiKey = options?.apiKey ?? process.env.QAS_TOKEN

if (!baseUrl || !apiKey) {
throw new ApiValidationError('QAS_URL and QAS_TOKEN are required.')
}

try {
new URL(baseUrl)
} catch {
throw new ApiValidationError(
'QAS_URL must be a valid absolute URL, for example https://qas.eu1.qasphere.com'
)
}

const response = await executePublicApiRequest(baseUrl, apiKey, request)
return response
}
Loading
Loading