ckan-api-client-js #1367
ckan-api-client-js #1367
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 9 Skipped Deployments
|
🦋 Changeset detectedLatest commit: 7fe355f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughA new JavaScript/TypeScript package, Changes
Sequence Diagram(s)sequenceDiagram
participant UserApp as User Application
participant CkanRequest as CkanRequest (Client)
participant CKANAPI as CKAN API Server
UserApp->>CkanRequest: get(actionName, options)
CkanRequest->>CKANAPI: HTTP GET /api/3/action/{actionName}
CKANAPI-->>CkanRequest: JSON Response
alt Success
CkanRequest-->>UserApp: Promise resolved with result
else Error
CkanRequest-->>UserApp: Promise rejected with CkanRequestError
end
UserApp->>CkanRequest: post(actionName, options)
CkanRequest->>CKANAPI: HTTP POST /api/3/action/{actionName}
CKANAPI-->>CkanRequest: JSON Response
alt Success
CkanRequest-->>UserApp: Promise resolved with result
else Error
CkanRequest-->>UserApp: Promise rejected with CkanRequestError
end
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (15)
packages/ckan-api-client-js/src/types.ts (1)
24-28: Refine header typing for clarity
Currentlyheaders?: { [k: string]: any }is very permissive. If you expect only string values, consider usingRecord<string, string>orRecord<string, unknown>.For example:
export interface RequestOptions { headers?: Record<string, string>; apiKey?: string; ckanUrl?: string; }packages/ckan-api-client-js/rollup.config.mjs (1)
6-16: Consider emitting source maps for easier debugging
Addingsourcemap: trueto each output bundle can help consumers trace issues back to original TypeScript.output: [ { file: "dist/index.esm.js", format: "es", + sourcemap: true, }, { file: "dist/index.cjs.js", format: "cjs", + sourcemap: true, }, ],packages/ckan-api-client-js/.gitignore (1)
1-45: Prune irrelevant patterns and add Rollup cache ignore
Your.gitignoreincludes entries for Next.js and Prisma, which don’t appear in this package. It’s worth trimming those and adding the TypeScript–Rollup cache folder generated byrollup-plugin-typescript2:# ...existing entries... /dist +.rts2_cache/ # cache for rollup-plugin-typescript2This keeps the package-level
.gitignorefocused and avoids accidental commits of plugin caches.packages/ckan-api-client-js/package.json (2)
4-4: Add a descriptive package description.The description field is currently empty. Adding a concise description will improve the package's visibility and help users understand its purpose when browsing npm or other package registries.
- "description": "", + "description": "A flexible TypeScript client for interacting with CKAN APIs with support for GET and POST requests.",
1-7: Add repository, keywords, and homepage fields to package.json.Adding these metadata fields will improve discoverability and provide users with important links.
"name": "@portaljs/ckan-api-client-js", "version": "1.3.3", "description": "", + "repository": { + "type": "git", + "url": "https://github.com/datopian/portaljs.git", + "directory": "packages/ckan-api-client-js" + }, + "keywords": ["ckan", "api", "client", "typescript", "portaljs"], + "homepage": "https://github.com/datopian/portaljs/tree/main/packages/ckan-api-client-js", "main": "dist/index.cjs.js", "module": "dist/index.esm.js", "types": "dist/index.d.ts",packages/ckan-api-client-js/README.md (4)
5-5: Fix grammar in the README.There's a subject-verb agreement error.
- - **To be completely flexible in terms of which CKAN actions are supported**, so users specify which action should be called by its name rather than my importing and calling a method implemented specifically for the given action. This ensure that all core and custom actions are supported, including all possible parameters. + - **To be completely flexible in terms of which CKAN actions are supported**, so users specify which action should be called by its name rather than my importing and calling a method implemented specifically for the given action. This ensures that all core and custom actions are supported, including all possible parameters.🧰 Tools
🪛 LanguageTool
[grammar] ~5-~5: Possible subject-verb agreement error detected.
Context: ...nted specifically for the given action. This ensure that all core and custom actions are su...(THIS_THAT_AGR)
6-6: Fix typo in environment.Small spelling error in the README.
- - **To reduce repetition when calling CKAN actions**, by reading global configurations on environemnt variables (such as the CKAN URL) and having common configurations by default (e.g. all requests by default will have the "content-type" header set to "application/json", avoiding that requests are sent without it and avoing that this has to be repeated everywhere). + - **To reduce repetition when calling CKAN actions**, by reading global configurations on environment variables (such as the CKAN URL) and having common configurations by default (e.g. all requests by default will have the "content-type" header set to "application/json", avoiding that requests are sent without it and avoiding that this has to be repeated everywhere).
16-18: Add language specification to code block.Adding a language specification improves syntax highlighting and follows markdown best practices.
- ``` - npm i @portaljs/ckan-api-client-js - ``` + ```bash + npm i @portaljs/ckan-api-client-js + ```🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
16-16: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
90-90: Fix preposition usage.Small grammatical fix.
- To do so, simply build (`npm run build`) the project and then link it (`npm link ...`) on another project to test changes. + To do so, simply build (`npm run build`) the project and then link it (`npm link ...`) to another project to test changes.🧰 Tools
🪛 LanguageTool
[uncategorized] ~90-~90: The preposition ‘to’ seems more likely in this position.
Context: ...oject and then link it (npm link ...) on another project to test changes.(AI_HYDRA_LEO_REPLACE_ON_TO)
packages/ckan-api-client-js/test/main.ts (2)
22-22: Remove debug console.log statement.There's a console.log statement in the test code that should be removed before production.
try { await result; } catch (e) { - console.log(e); expect(e.message).to.be.eq("Not found"); }
9-37: Consider using test mocks instead of real API calls.The tests are making real API calls which can be slow, unreliable, and may not work in all environments. Consider using a library like nock, msw, or fetch-mock to mock the API responses.
// Example implementation with mock using fetch-mock: import fetchMock from 'fetch-mock'; // Before tests beforeEach(() => { // Set up mock responses fetchMock.get('end:/status_show', { success: true, result: { /* mock data */ } }); fetchMock.get('end:/package_show?id=1', { status: 404, body: { help: "http://example.com/api/3/action/help_show?name=package_show", success: false, error: { __type: "Not Found Error", message: "Not found" } } }); // Add more mocks as needed }); // After tests afterEach(() => { fetchMock.restore(); });packages/ckan-api-client-js/src/CkanRequest.ts (4)
117-123:customGetignores query parametersReal-world CKAN actions (e.g.
package_show) require query-string params.
Provide a typedparamsoption and append it toactionUrl, or document that callers must pre-encode the URL.Example:
-const customGet = async (actionUrl: string, options?: GetRequestOptions) => { +const customGet = async ( + actionUrl: string, + options?: GetRequestOptions & { params?: Record<string, string | number> } +) => { + if (options?.params) { + const search = new URLSearchParams(options.params as any).toString(); + actionUrl += `?${search}`; + } const response = await fetch(actionUrl, {
32-40: Variable shadowing inValidationErrormessage builder
let errorMessage = ...inside themapcallback shadows the outererrorMessage, which can be confusing during maintenance. Rename the inner variable or extract a helper to improve clarity.- let errorMessage = `"${f}": `; - errorMessage += + let fieldMsg = `"${f}": `; + fieldMsg += typeof errorField !== "string" ? errorField?.join(" ") : errorField; - return errorMessage; + return fieldMsg;
78-79: Avoid unconditionalconsole.errorin a reusable libraryLogging every caught error pollutes consumer logs and can leak sensitive data.
Either remove the log, wrap it behind a debug flag, or delegate logging to the caller.- console.error(e); + /* optionally log here if a debug flag is enabled */
108-115:fetchmay be undefined in older Node versions
fetchis global only from Node 18+ and modern browsers. For universal libraries, consider:• bundling
cross-fetch/undicipolyfill,
• or accepting afetchimplementation via options/DI.Failing gracefully avoids runtime ReferenceErrors in LTS Node 16 environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
packages/ckan-api-client-js/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
packages/ckan-api-client-js/.gitignore(1 hunks)packages/ckan-api-client-js/.mocharc.json(1 hunks)packages/ckan-api-client-js/CHANGELOG.md(1 hunks)packages/ckan-api-client-js/README.md(1 hunks)packages/ckan-api-client-js/package.json(1 hunks)packages/ckan-api-client-js/rollup.config.mjs(1 hunks)packages/ckan-api-client-js/src/CkanRequest.ts(1 hunks)packages/ckan-api-client-js/src/index.ts(1 hunks)packages/ckan-api-client-js/src/types.ts(1 hunks)packages/ckan-api-client-js/test/main.ts(1 hunks)packages/ckan-api-client-js/tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/ckan-api-client-js/test/main.ts (2)
packages/ckan-api-client-js/src/index.ts (2)
CkanRequest(4-4)CkanRequestError(4-4)packages/ckan-api-client-js/src/CkanRequest.ts (2)
CkanRequest(128-131)CkanRequestError(9-53)
🪛 LanguageTool
packages/ckan-api-client-js/README.md
[grammar] ~5-~5: Possible subject-verb agreement error detected.
Context: ...nted specifically for the given action. This ensure that all core and custom actions are su...
(THIS_THAT_AGR)
[style] ~8-~8: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...at can be shown to the final users. - **To expose the underlying request propertie...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[typographical] ~8-~8: The conjunction “so that” does not require a comma.
Context: ...pose the underlying request properties**, so that anything can be customized e.g. headers...
(SO_THAT_UNNECESSARY_COMMA)
[uncategorized] ~90-~90: The preposition ‘to’ seems more likely in this position.
Context: ...oject and then link it (npm link ...) on another project to test changes.
(AI_HYDRA_LEO_REPLACE_ON_TO)
🪛 markdownlint-cli2 (0.17.2)
packages/ckan-api-client-js/README.md
16-16: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🔇 Additional comments (7)
packages/ckan-api-client-js/src/types.ts (3)
11-15: CkanErrorType enum is clear and exhaustive
The error categories—Validation, Authorization, and Not Found—are well defined and map cleanly to CKAN API responses.
16-22: CkanErrorBody models dynamic error payloads appropriately
Using an index signature allows capturing field-specific error messages while enforcing the__typediscrimination. This aligns well with the CKAN error schema.
30-31: Empty extension interface can be omitted or documented
GetRequestOptionsextendsRequestOptionswithout additions. If you intend this for future props or for documentation clarity, it’s fine; otherwise you could alias it directly or remove it.packages/ckan-api-client-js/.mocharc.json (1)
1-5: Verify Mocha configuration for TypeScript support
Ensure that Mocha will pick up.tsfiles and apply thetsxhook correctly. You may need to add:
"extension": ["ts"]- Change
"require": "tsx"to an array (e.g."require": ["tsx"]) if Mocha expects an array.packages/ckan-api-client-js/src/index.ts (1)
1-4: Entry point exports are correctly structured
Thedefaultexport, named exports, and type-only export use idiomatic TypeScript/ESM syntax and align with your package’s public API.packages/ckan-api-client-js/tsconfig.json (1)
1-111: The TypeScript configuration looks solid.The TypeScript configuration is comprehensive with good defaults. The strict type checking will help ensure type safety, and the ES2020 target is appropriate for modern environments.
packages/ckan-api-client-js/CHANGELOG.md (1)
1-43: Well-structured changelog.The changelog follows a good format with versioning and categorized changes. It clearly documents the evolution of the package's functionality and API.
| export interface PostRequestOptions extends RequestOptions { | ||
| json?: any; // TODO: it should be either json OR formData | ||
| formData?: any; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enforce mutual exclusivity for JSON vs. formData payloads
The TODO notes this should be one or the other. You can model this with a union type to prevent misuse at compile time.
For example:
-export interface PostRequestOptions extends RequestOptions {
- json?: any; // TODO: it should be either json OR formData
- formData?: any;
-}
+export type PostRequestOptions =
+ | (RequestOptions & { json: unknown; formData?: never })
+ | (RequestOptions & { formData: FormData; json?: never });This ensures callers supply exactly one of json or formData.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export interface PostRequestOptions extends RequestOptions { | |
| json?: any; // TODO: it should be either json OR formData | |
| formData?: any; | |
| } | |
| export type PostRequestOptions = | |
| | (RequestOptions & { json: unknown; formData?: never }) | |
| | (RequestOptions & { formData: FormData; json?: never }); |
🤖 Prompt for AI Agents
In packages/ckan-api-client-js/src/types.ts around lines 32 to 35, the
PostRequestOptions interface currently allows both json and formData properties
simultaneously, which should be mutually exclusive. Refactor the interface to
use a union type that enforces exactly one of json or formData is provided,
preventing misuse at compile time by ensuring callers supply either json or
formData but not both.
| export interface CkanResponse<T> { | ||
| help: string; | ||
| success: boolean; | ||
| result: T; | ||
| error?: { | ||
| __type: string; | ||
| message: string; | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unify error type in CkanResponse for stronger typing
The error.__type is currently a plain string. For better type safety and consistency with your CkanErrorType enum, consider updating it to CkanErrorType (or reusing CkanErrorBody['error']).
You might apply a diff like:
export interface CkanResponse<T> {
help: string;
success: boolean;
result: T;
- error?: {
- __type: string;
- message: string;
- };
+ error?: {
+ __type: CkanErrorType; // use the enum instead of string
+ message: string;
+ };
}🤖 Prompt for AI Agents
In packages/ckan-api-client-js/src/types.ts lines 1 to 9, update the type of
error.__type in the CkanResponse interface from string to the CkanErrorType enum
or reuse the existing CkanErrorBody['error'] type for stronger typing and
consistency. Modify the error property definition accordingly to reflect this
change.
| "devDependencies": { | ||
| "@changesets/cli": "^2.27.7", | ||
| "@rollup/plugin-commonjs": "^26.0.1", | ||
| "@rollup/plugin-node-resolve": "^15.2.3", | ||
| "@rollup/plugin-typescript": "^11.1.6", | ||
| "@types/node": "^20.14.2", | ||
| "chai": "^5.2.0", | ||
| "chai-as-promised": "^8.0.1", | ||
| "dotenv": "^16.5.0", | ||
| "mocha": "^11.4.0", | ||
| "rimraf": "^5.0.7", | ||
| "rollup": "^4.18.0", | ||
| "rollup-plugin-typescript2": "^0.36.0", | ||
| "tsx": "^4.19.4", | ||
| "typescript": "^5.4.5" | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding a peer dependency on fetch.
The implementation (based on the test file and README) appears to use fetch API for making HTTP requests, but there's no explicit dependency specified. Consider adding either a polyfill or a peer dependency.
🏁 Script executed:
#!/bin/bash
# Check if the implementation uses fetch API
ast-grep --pattern 'fetch($$$)'Length of output: 6106
#!/bin/bash
Check for Node engine requirement and explicit fetch imports in the CKAN API client package
grep -R '"engines"' -n packages/ckan-api-client-js/package.json || echo "no engines field in package.json"
grep -R 'import fetch' -n packages/ckan-api-client-js/src || echo "no explicit fetch imports in src"
🏁 Script executed:
#!/bin/bash
echo "Checking package.json for dependencies, peerDependencies, and engines:"
jq '{dependencies, peerDependencies, engines}' packages/ckan-api-client-js/package.jsonLength of output: 314
Add a fetch polyfill or specify Node ≥18 in package.json
The CKAN API client calls fetch directly but package.json doesn’t declare any fetch polyfill or Node engine requirement. Without this, consumers on Node <18 (which lack global fetch) will break.
Please update packages/ckan-api-client-js/package.json to either:
- Add a peerDependency on a fetch polyfill (e.g.
cross-fetch), or - Declare an
engines.node: ">=18"requirement so users know they need a runtime with built-in fetch.
Suggested diff:
{
+ "engines": {
+ "node": ">=18"
+ },
+ "peerDependencies": {
+ "cross-fetch": "^3.1.5"
+ },
"devDependencies": {
"@changesets/cli": "^2.27.7",
…
}
}- If you choose a polyfill, be sure to document in README that consumers must install it.
- If you target Node ≥18 only, the engines field will prevent installs on unsupported versions.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "devDependencies": { | |
| "@changesets/cli": "^2.27.7", | |
| "@rollup/plugin-commonjs": "^26.0.1", | |
| "@rollup/plugin-node-resolve": "^15.2.3", | |
| "@rollup/plugin-typescript": "^11.1.6", | |
| "@types/node": "^20.14.2", | |
| "chai": "^5.2.0", | |
| "chai-as-promised": "^8.0.1", | |
| "dotenv": "^16.5.0", | |
| "mocha": "^11.4.0", | |
| "rimraf": "^5.0.7", | |
| "rollup": "^4.18.0", | |
| "rollup-plugin-typescript2": "^0.36.0", | |
| "tsx": "^4.19.4", | |
| "typescript": "^5.4.5" | |
| }, | |
| { | |
| "engines": { | |
| "node": ">=18" | |
| }, | |
| "peerDependencies": { | |
| "cross-fetch": "^3.1.5" | |
| }, | |
| "devDependencies": { | |
| "@changesets/cli": "^2.27.7", | |
| "@rollup/plugin-commonjs": "^26.0.1", | |
| "@rollup/plugin-node-resolve": "^15.2.3", | |
| "@rollup/plugin-typescript": "^11.1.6", | |
| "@types/node": "^20.14.2", | |
| "chai": "^5.2.0", | |
| "chai-as-promised": "^8.0.1", | |
| "dotenv": "^16.5.0", | |
| "mocha": "^11.4.0", | |
| "rimraf": "^5.0.7", | |
| "rollup": "^4.18.0", | |
| "rollup-plugin-typescript2": "^0.36.0", | |
| "tsx": "^4.19.4", | |
| "typescript": "^5.4.5" | |
| }, | |
| // …other existing fields… | |
| } |
🤖 Prompt for AI Agents
In packages/ckan-api-client-js/package.json around lines 16 to 31, the package
uses the fetch API but does not specify a fetch polyfill or a Node.js engine
version that guarantees fetch availability. To fix this, either add a
peerDependency on a fetch polyfill like cross-fetch and document this
requirement in the README, or add an "engines" field specifying "node": ">=18"
to ensure users run the package on Node versions with built-in fetch support.
| Import the client with: | ||
|
|
||
| ```javascript | ||
| import CkanRequest from "ckan-api-client-js"; |
There was a problem hiding this comment.
Update import to match package name.
The import statement uses "ckan-api-client-js" but the package name is "@portaljs/ckan-api-client-js".
- import CkanRequest from "ckan-api-client-js";
+ import CkanRequest from "@portaljs/ckan-api-client-js";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import CkanRequest from "ckan-api-client-js"; | |
| import CkanRequest from "@portaljs/ckan-api-client-js"; |
🤖 Prompt for AI Agents
In packages/ckan-api-client-js/README.md at line 29, update the import statement
to use the correct package name "@portaljs/ckan-api-client-js" instead of
"ckan-api-client-js" to match the actual package name.
| try { | ||
| const dataset = CkanRequest.get("package_show?id=123") | ||
| } catch (e) { | ||
| alert(e.message) // E.g. "Dataset not found" | ||
| } | ||
|
|
There was a problem hiding this comment.
Add await to the example code.
The Promise is not being awaited in the example, which may cause issues in the try/catch block.
try {
- const dataset = CkanRequest.get("package_show?id=123")
+ const dataset = await CkanRequest.get("package_show?id=123")
} catch (e) {
alert(e.message) // E.g. "Dataset not found"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| const dataset = CkanRequest.get("package_show?id=123") | |
| } catch (e) { | |
| alert(e.message) // E.g. "Dataset not found" | |
| } | |
| try { | |
| const dataset = await CkanRequest.get("package_show?id=123") | |
| } catch (e) { | |
| alert(e.message) // E.g. "Dataset not found" | |
| } |
🤖 Prompt for AI Agents
In packages/ckan-api-client-js/README.md around lines 78 to 83, the example code
calls CkanRequest.get without awaiting the returned Promise, which can cause the
try/catch block to not properly handle errors. Add the await keyword before
CkanRequest.get to ensure the Promise is resolved or rejected within the
try/catch block, enabling proper error handling.
| describe("POST", () => { | ||
| it("throws CkanRequestError for Authorization errors", async () => { | ||
| const result = CkanRequest.post("organization_create", { | ||
| json: { name: "test" }, | ||
| }); | ||
| await expect(result).to.be.rejectedWith(CkanRequestError); | ||
|
|
||
| try { | ||
| await result; | ||
| } catch (e) { | ||
| expect(e.message.startsWith("Access denied")).to.be.eq(true); | ||
| } | ||
| }).timeout(10000); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add more test cases for POST requests.
There's only one test case for POST requests focusing on authorization errors. Consider adding more test cases for successful POST requests and other error scenarios.
I recommend adding at least the following additional test cases:
- Successful POST request
- Validation error case
- Post with formData option (which is mentioned in the API but not tested)
Example implementation:
it("successfully creates a resource with proper authorization", async () => {
// Mock a successful response or use a real API if available
const result = await CkanRequest.post("resource_create", {
apiKey: "valid-api-key",
json: {
package_id: "test-dataset",
name: "test-resource",
url: "http://example.com/data.csv"
}
});
expect(result.success).to.be.eq(true);
}).timeout(10000);
it("throws CkanRequestError for validation errors", async () => {
const result = CkanRequest.post("resource_create", {
apiKey: "valid-api-key",
json: { /* invalid data */ }
});
await expect(result).to.be.rejectedWith(CkanRequestError);
try {
await result;
} catch (e) {
expect(e.message.startsWith("Validation error:")).to.be.eq(true);
}
}).timeout(10000);🤖 Prompt for AI Agents
In packages/ckan-api-client-js/test/main.ts around lines 39 to 52, the POST
request tests only cover authorization errors. Add additional test cases for
successful POST requests, validation error scenarios, and POST requests using
the formData option. For success, mock or call a real API with valid data and
assert the success response. For validation errors, send invalid data and assert
that CkanRequestError is thrown with a message starting with "Validation
error:". Also, add a test for POST requests using formData to ensure that code
path is covered.
| if ("json" in options) { | ||
| headers["content-type"] = "application/json"; | ||
| } else if ("formData" in options) { | ||
| headers["content-type"] = "multipart/form-data"; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Do not set Content-Type manually when sending FormData
When a FormData body is passed, the browser / node-fetch automatically adds the correct Content-Type header including the multipart boundary.
Setting it manually to a generic multipart/form-data value removes the boundary and corrupts the request, causing CKAN (and most HTTP servers) to reject the upload.
In addition, headers should use canonical casing (Content-Type) for readability and cross-runtime consistency.
- if ("json" in options) {
- headers["content-type"] = "application/json";
- } else if ("formData" in options) {
- headers["content-type"] = "multipart/form-data";
+ if ("json" in options) {
+ headers["Content-Type"] = "application/json";
+ } else if ("formData" in options) {
+ // Let fetch set the multipart boundary automatically.
+ // Do **not** set Content-Type here.
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ("json" in options) { | |
| headers["content-type"] = "application/json"; | |
| } else if ("formData" in options) { | |
| headers["content-type"] = "multipart/form-data"; | |
| } | |
| if ("json" in options) { | |
| headers["Content-Type"] = "application/json"; | |
| } else if ("formData" in options) { | |
| // Let fetch set the multipart boundary automatically. | |
| // Do **not** set Content-Type here. | |
| } |
🤖 Prompt for AI Agents
In packages/ckan-api-client-js/src/CkanRequest.ts around lines 97 to 101, do not
manually set the Content-Type header when the body is FormData, as this removes
the multipart boundary and breaks the request. Remove the line that sets
headers["content-type"] to "multipart/form-data" for FormData bodies. Also,
update the header key to use canonical casing "Content-Type" instead of
"content-type" for consistency and readability.
| const response: Response = await method(ckanActionUrl, options); | ||
| const data: FnRes = (await response.json()) as FnRes; | ||
| if (response.ok) { | ||
| return data; | ||
| } else { | ||
| throw data; | ||
| } | ||
| } catch (e) { |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Blindly parsing response.json() may throw & loses CKAN “success=false” errors
response.json()will reject for 204/205 responses and for responses with an empty body (CKAN sometimes returns 200 with no body).- CKAN embeds a
successboolean in its payload; a 200 status code withsuccess:falseshould still be treated as an error, but the current code returns it as a successful result.
Consider:
- const data: FnRes = (await response.json()) as FnRes;
- if (response.ok) {
+ const hasBody = response.headers.get("content-type")?.includes("json");
+ const data: any = hasBody ? await response.json() : undefined;
+
+ // Treat CKAN logical errors as failures even if HTTP 200
+ if (response.ok && data?.success !== false) {
return data as FnRes;
} else {
throw data ?? { message: "Empty response body" };
}This avoids runtime crashes and surfaces CKAN-level errors correctly.
Handle empty/non-JSON responses & CKAN success=false as errors
CKAN often returns a 200/204/205 with no body (so response.json() will throw), and its payload uses a success flag for logical errors. Update your request handler in packages/ckan-api-client-js/src/CkanRequest.ts (around lines 70–77) as follows:
• File: packages/ckan-api-client-js/src/CkanRequest.ts (lines 70–77)
const response: Response = await method(ckanActionUrl, options);
- const data: FnRes = (await response.json()) as FnRes;
- if (response.ok) {
- return data;
- } else {
- throw data;
- }
+ // Only parse JSON when there’s a JSON body
+ const contentType = response.headers.get("content-type") ?? "";
+ const data: any = contentType.includes("json")
+ ? await response.json()
+ : undefined;
+
+ // Treat HTTP failures or CKAN logical failures as errors
+ if (!response.ok || data?.success === false) {
+ // Surface CKAN error details or fallback to a generic message
+ throw data ?? new Error("Empty response body");
+ }
+
+ return data as FnRes;
} catch (e) {
// …
}This ensures you won’t crash on empty bodies and still surface CKAN-level errors when success is false.
🤖 Prompt for AI Agents
In packages/ckan-api-client-js/src/CkanRequest.ts around lines 70 to 77, the
code blindly calls response.json() which can throw on empty or no-content
responses, and it does not check the CKAN-specific success flag in the JSON
payload. To fix this, first check if the response has content before parsing
JSON to avoid exceptions on empty bodies, then parse the JSON safely. After
parsing, verify if the CKAN response's success property is true; if false, throw
an error to correctly handle CKAN-level failures even when HTTP status is 200.
This will prevent runtime crashes and properly surface logical errors from CKAN.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
.gitignoreto exclude common files and directories from version control.