Odata #75
Odata #75
Conversation
- Updated package name to @proofkit/fmodata and version to 0.1.0 in package.json. - Added description, repository, author, license, and various scripts for build and testing. - Specified dependencies and devDependencies for the fmodata package. - Updated pnpm-lock.yaml to reflect changes in package dependencies and versions. - Enhanced exports configuration for module compatibility.
- Increased test timeout in vitest.config.ts to 30 seconds for integration tests. - Introduced encodeODataFilter function in utils.ts for proper OData filter encoding. - Updated buildQueryString function to return a string instead of URLSearchParams, accommodating custom encoding for OData queries. - Added buildFileMakerTablesPath function for constructing paths to the FileMaker_Tables system table. - Modified BaseFetchAdapter to handle query as a string or URLSearchParams, ensuring compatibility with OData requirements. - Updated integration tests to include timeout handling and improved error logging. - Enhanced MCP server configuration to support command-line arguments for better flexibility.
- Added comprehensive integration test script to package.json for improved testing coverage. - Exported encodeODataFilter function from utils.ts for better OData filter handling. - Updated buildQueryString function to ensure proper formatting of $select fields. - Improved response handling in BaseFetchAdapter to manage empty responses and enhance error parsing. - Refined encodeKey function to ensure correct URL encoding for string keys in OData paths.
- Introduced enhanceErrorMessage function to provide detailed context for known FileMaker error codes, improving error reporting. - Updated parseErrorResponse to utilize the new error enhancement function for clearer error messages. - Removed unnecessary encodeKey calls in BaseFetchAdapter, simplifying key handling. - Enhanced integration tests with better error handling and logging for primary key configuration issues.
…ation - Introduced FileMakerFieldType type for valid field types in schema modifications. - Added VALID_FIELD_TYPES constant for runtime validation of field types. - Updated CreateTableOptions and AddFieldsOptions to use FieldDefinition for better type safety. - Enhanced schema validation in handleSchemaTool to ensure correct field types are used. - Updated README.md to improve installation instructions and configuration examples for global and local usage.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
@proofkit/better-auth
@proofkit/cli
create-proofkit
@proofkit/fmdapi
@proofkit/fmodata
@proofkit/fmodata-mcp
@proofkit/typegen
@proofkit/webviewer
commit: |
WalkthroughThis pull request introduces two new packages: Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI
participant Server
participant MCP as MCP Server<br/>(Express/Stdio)
participant Tools
participant Adapter as OData Adapter<br/>(Fetch/Otto)
User->>CLI: fmodata-mcp --db mydb<br/>--host server.com --http
alt HTTP Mode
CLI->>Server: startServer(config)
Server->>MCP: Express + StreamableHTTPServerTransport
MCP->>MCP: Create session on first /mcp request
else Stdio Mode (default)
CLI->>Server: createServer(config)
Server->>MCP: StdioServerTransport
end
MCP->>Adapter: Initialize with config<br/>(FetchAdapter or OttoAdapter)
User->>MCP: POST /mcp<br/>{"method":"tools/call","params":{"name":"fmodata_query_records",...}}
MCP->>Tools: handleQueryTool(client, name, args)
Tools->>Adapter: client.getRecords(table, options)
Adapter->>Adapter: Build OData query URL,<br/>Add auth header
Adapter-->>Tools: ODataResponse<T>
Tools-->>MCP: JSON result
MCP-->>User: {"content":[{"type":"text","text":JSON result}]}
sequenceDiagram
participant App as Application
participant Client as ODataApi<br/>(Factory)
participant Adapter as Adapter<br/>(Interface)
participant FetchAdapter
participant OttoAdapter
participant Fetch as Fetch API
App->>Client: ODataApi({adapter: fetchAdapter})
alt FetchAdapter Path
App->>FetchAdapter: new FetchAdapter({server,database,auth:{username,password}})
FetchAdapter->>FetchAdapter: Base64 encode credentials
else OttoAdapter Path
App->>OttoAdapter: new OttoAdapter({server,database,auth:{apiKey}})
OttoAdapter->>OttoAdapter: Validate KEY_* or dk_* format
end
App->>Client: client.getRecords("Customers",{$filter:"...",callback:"..."})
Client->>Adapter: adapter.getRecords(table, options)
alt FetchAdapter
FetchAdapter->>FetchAdapter: getAuthHeader() → Basic header
else OttoAdapter
OttoAdapter->>OttoAdapter: getAuthHeader() → Bearer header
end
Adapter->>Adapter: buildQueryString(options)<br/>buildRecordPath(db,table,...)
Adapter->>Fetch: fetch(url, {headers:...,method:...})
Fetch-->>Adapter: Response
Adapter->>Adapter: parseErrorResponse if error
Adapter-->>Client: ODataResponse<T>
Client-->>App: Promise<ODataResponse<T>>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas requiring extra attention:
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (17)
packages/fmodata/README.md (2)
106-112: Vary sentence openings to improve readability.Three successive method descriptions (lines 108–112) begin with "Get," creating repetitive phrasing. Consider rephrasing at least one or two for better flow.
Suggested revision:
- #### `getRecord(table, key, options?)` - Get a single record by primary key. + #### `getRecord(table, key, options?)` + Retrieve a single record by primary key. #### `getRecordCount(table, options?)` - Get count of records, optionally filtered. + Count records in a table, optionally filtered. #### `getFieldValue(table, key, field, options?)` - Get the value of a specific field. + Retrieve a field's value from a specific record.
122-127: Simplify wording for conciseness."Create a new record" is more wordy than necessary. Shorten to "Create a record" for consistency with the rest of the API documentation.
- #### `createRecord(table, options)` - Create a new record. + #### `createRecord(table, options)` + Create a record.packages/fmodata/package.json (1)
5-5: Use HTTPS for repository URL.The SSH URL (
git@github.com:proofgeist/proofkit.git) may fail for users without SSH keys configured. Use HTTPS for better compatibility.- "repository": "git@github.com:proofgeist/proofkit.git", + "repository": "https://github.com/proofgeist/proofkit.git",packages/fmodata/vite.config.ts (1)
4-6: Consider removing the empty base config.The base config contains only an empty plugins array and adds no value. You can simplify by directly exporting the TanStack config.
Apply this diff to simplify:
-import { defineConfig, mergeConfig } from "vite"; +import { defineConfig } from "vite"; import { tanstackViteConfig } from "@tanstack/vite-config"; -const config = defineConfig({ - plugins: [], -}); - -export default mergeConfig( - config, - tanstackViteConfig({ +export default tanstackViteConfig({ entry: "./src/index.ts", srcDir: "./src", cjs: false, outDir: "./dist", - }), -); + });packages/fmodata-mcp/package.json (2)
16-16: Consider extracting the inline Node.js script to a separate build script file.The inline Node.js code uses CommonJS (
require) in an ESM package and is difficult to test, debug, or maintain. Consider creating a dedicatedscripts/post-build.jsfile to handle the directory restructuring.Example
scripts/post-build.js:import fs from 'fs'; import path from 'path'; const src = 'dist/fmodata-mcp/src'; const dest = 'dist'; if (fs.existsSync(src)) { fs.cpSync(src, dest, { recursive: true }); fs.rmSync('dist/fmodata-mcp', { recursive: true }); }Then update the build script:
-"build": "tsc && node -e \"const fs = require('fs'); const path = require('path'); if (fs.existsSync('dist/fmodata-mcp/src')) { const src = 'dist/fmodata-mcp/src'; const dest = 'dist'; fs.cpSync(src, dest, { recursive: true }); fs.rmSync('dist/fmodata-mcp', { recursive: true }); }\" && publint --strict", +"build": "tsc && node scripts/post-build.js && publint --strict",
24-27: Remove redundantpnpm buildcalls from publish scripts.The
prepublishOnlyscript already ensures the package is built before any publish operation. The explicitpnpm build &&in lines 25-27 is redundant and causes unnecessary double-building.Apply this diff:
- "pub:beta": "pnpm build && npm publish --tag beta --access public", - "pub:next": "pnpm build && npm publish --tag next --access public", - "pub:release": "pnpm build && npm publish --access public" + "pub:beta": "npm publish --tag beta --access public", + "pub:next": "npm publish --tag next --access public", + "pub:release": "npm publish --access public"packages/fmodata/tsconfig.json (1)
30-31: Clarify include/exclude pattern for config files.There's a potential contradiction:
vite.config.tsis explicitly included (line 31) while*.config.tsis excluded (line 30). While the explicit include takes precedence, this pattern could be confusing for maintainers. Consider adding a comment explaining whyvite.config.tsneeds special treatment, or restructure the patterns for clarity.packages/fmodata-mcp/README.md (1)
312-324: Add language specifier to fenced code block.The fenced code block at line 312 should specify a language for proper syntax highlighting and better accessibility.
Apply this diff:
-``` +```plaintext User: List all tables in the database Assistant: [calls fmodata_list_tables] Found 5 tables: Customers, Orders, Products, Suppliers, CategoriesBased on static analysis hints.
packages/fmodata-mcp/tsconfig.json (1)
30-31: Clarify include/exclude pattern for config files.There's a potential contradiction:
vite.config.tsis explicitly included (line 31) while*.config.tsis excluded (line 30). While the explicit include takes precedence, this pattern could be confusing for maintainers. Consider adding a comment explaining whyvite.config.tsneeds special treatment, or restructure the patterns for clarity.packages/fmodata/tests/integration-comprehensive.test.ts (3)
36-40: Consider a more targeted approach to TLS certificate handling.Setting
NODE_TLS_REJECT_UNAUTHORIZED = "0"globally affects all HTTPS requests in the process, not just the test connections. This could mask other SSL/TLS issues during testing.Consider passing the
rejectUnauthorized: falseoption directly to the adapter configurations (which you're already doing on lines 56, 65, 77) and removing the global environment variable modification. The adapters should handle this internally without requiring a global process-level change.- // Disable SSL verification for localhost/development - if (host && host.includes("localhost")) { - process.env.NODE_TLS_REJECT_UNAUTHORIZED = "0"; - } -
199-221: Document the expected ID extraction strategy.The multiple fallback strategies for extracting record IDs suggest uncertainty about the response format. This could lead to brittle tests that break when FileMaker OData behavior changes.
Consider creating a helper function like
extractRecordId(record: ODataRecord)that encapsulates this logic and documents the expected response formats. This would make the extraction strategy reusable and easier to maintain.function extractRecordId(record: ODataRecord): string | number { // Try direct id field if (record.id && (typeof record.id === "string" || typeof record.id === "number")) { return record.id; } // Try parsing @odata.id URL const odataId = (record as { "@odata.id"?: string })["@odata.id"]; if (odataId) { const match = odataId.match(/\(([^)]+)\)/); if (match?.[1]) { return match[1].replace(/^'|'$/g, ""); } } throw new Error("Failed to extract record ID from OData response"); }
231-234: Extract repeated primary key error handling.The pattern of catching "Primary key configuration error" and skipping tests is duplicated across five test cases. This duplication makes the tests harder to maintain and suggests a missing test precondition check.
Consider either:
- Adding a
beforeAllcheck that verifies primary key configuration and skips the entire CRUD suite if misconfigured- Creating a test helper wrapper that handles this pattern
async function withPrimaryKeyCheck<T>( testFn: () => Promise<T>, testName: string ): Promise<T | undefined> { try { return await testFn(); } catch (error) { if (error instanceof Error && error.message.includes("Primary key configuration error")) { console.log(`⚠️ Skipping ${testName}: Primary key field not configured`); return undefined; } throw error; } } // Usage: it("should create a record", async () => { await withPrimaryKeyCheck(async () => { // test logic }, "createRecord test"); });Also applies to: 252-255, 281-284, 300-303, 323-326
packages/fmodata/src/adapters/otto.ts (2)
53-67: Validate URL pathname modification for OttoFMS.The pathname replacement on line 59-62 uses a regex that assumes the path starts with
/fmi/. If the base URL format changes or is unexpected, the replacement silently fails without error.Consider validating the replacement succeeded:
} else if (isOttoFMSAPIKey(this.apiKey)) { // OttoFMS uses /otto prefix in path // Insert /otto before /fmi in the pathname - this.baseUrl.pathname = this.baseUrl.pathname.replace( + const newPathname = this.baseUrl.pathname.replace( /^(\/fmi\/)/, "/otto$1", ); + if (newPathname === this.baseUrl.pathname) { + throw new Error( + `Invalid base URL pathname: expected path starting with /fmi/ but got ${this.baseUrl.pathname}` + ); + } + this.baseUrl.pathname = newPathname; } else {
73-91: Consider logging when path modification occurs.The path modification for OttoFMS is conditional and uses regex replacement. For debugging purposes, it might be helpful to log when paths are modified, especially since the modification is silent if the path doesn't match the expected pattern.
if (isOttoFMSAPIKey(this.apiKey) && params.path.startsWith("/fmi/")) { const modifiedPath = params.path.replace(/^(\/fmi\/)/, "/otto$1"); + // Consider adding debug logging: + // console.debug(`OttoFMS path modified: ${params.path} -> ${modifiedPath}`); return super.request({ ...params, path: modifiedPath }); }packages/fmodata-mcp/src/server.ts (2)
196-226: Simplify tool routing with a category map.The current if-else chain checking tool name prefixes is repetitive and requires modification every time a new tool category is added. Consider using a routing map for better maintainability.
const toolHandlers: Record<string, (client: ODataApiClient, name: string, args: unknown) => Promise<unknown>> = { list_tables: handleQueryTool, get_metadata: handleQueryTool, query_records: handleQueryTool, get_record: handleQueryTool, get_field_value: handleQueryTool, navigate_related: handleQueryTool, cross_join: handleQueryTool, create_record: handleCrudTool, update_record: handleCrudTool, delete_record: handleCrudTool, create_table: handleSchemaTool, add_fields: handleSchemaTool, delete_table: handleSchemaTool, delete_field: handleSchemaTool, run_script: handleScriptTool, batch: handleScriptTool, }; // Then in the handler: const toolKey = name.replace(/^fmodata_/, ""); const handler = toolHandlers[toolKey]; if (!handler) { throw new Error(`Unknown tool: ${name}`); } result = await handler(client, name, args);
256-261: Add session cleanup to prevent memory leaks.The sessions are stored in memory without any timeout or cleanup mechanism. In a long-running server, this could lead to memory leaks as closed or abandoned sessions accumulate.
Consider adding session cleanup:
// After line 260: const SESSION_TIMEOUT = 30 * 60 * 1000; // 30 minutes const sessionTimestamps: Record<string, number> = {}; // Periodic cleanup setInterval(() => { const now = Date.now(); for (const [sessionId, timestamp] of Object.entries(sessionTimestamps)) { if (now - timestamp > SESSION_TIMEOUT) { console.log(`Cleaning up expired session: ${sessionId}`); delete sessions[sessionId]; delete sessionTimestamps[sessionId]; } } }, 5 * 60 * 1000); // Check every 5 minutes // Update timestamp on each request (after line 277): if (session) { sessionTimestamps[sessionId] = Date.now(); }Also applies to: 268-333
packages/fmodata/src/utils.ts (1)
141-147: Simplify table list construction.Line 145 uses
.map((t) => t)which is a no-op. You can directly join the tables array.export function buildCrossJoinPath( databaseName: string, tables: string[], ): string { - const tablesList = tables.map((t) => t).join(","); + const tablesList = tables.join(","); return `/fmi/odata/v4/${databaseName}/CrossJoin(${tablesList})`; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (29)
packages/fmodata-mcp/README.md(1 hunks)packages/fmodata-mcp/package.json(1 hunks)packages/fmodata-mcp/src/index.ts(1 hunks)packages/fmodata-mcp/src/server.ts(1 hunks)packages/fmodata-mcp/src/tools/crud.ts(1 hunks)packages/fmodata-mcp/src/tools/index.ts(1 hunks)packages/fmodata-mcp/src/tools/query.ts(1 hunks)packages/fmodata-mcp/src/tools/schema.ts(1 hunks)packages/fmodata-mcp/src/tools/scripts.ts(1 hunks)packages/fmodata-mcp/src/types.ts(1 hunks)packages/fmodata-mcp/tsconfig.json(1 hunks)packages/fmodata/README.md(1 hunks)packages/fmodata/package.json(1 hunks)packages/fmodata/src/adapters/core.ts(1 hunks)packages/fmodata/src/adapters/fetch-base-types.ts(1 hunks)packages/fmodata/src/adapters/fetch-base.ts(1 hunks)packages/fmodata/src/adapters/fetch.ts(1 hunks)packages/fmodata/src/adapters/otto.ts(1 hunks)packages/fmodata/src/client-types.ts(1 hunks)packages/fmodata/src/client.ts(1 hunks)packages/fmodata/src/index.ts(1 hunks)packages/fmodata/src/utils.ts(1 hunks)packages/fmodata/tests/fetch-base.test.ts(1 hunks)packages/fmodata/tests/integration-comprehensive.test.ts(1 hunks)packages/fmodata/tests/integration.test.ts(1 hunks)packages/fmodata/tests/setup.ts(1 hunks)packages/fmodata/tsconfig.json(1 hunks)packages/fmodata/vite.config.ts(1 hunks)packages/fmodata/vitest.config.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (17)
packages/fmodata-mcp/src/tools/scripts.ts (2)
packages/fmodata/src/client.ts (1)
ODataApiClient(266-266)packages/fmodata-mcp/src/types.ts (2)
RunScriptSchema(354-372)BatchRequestsSchema(374-400)
packages/fmodata-mcp/src/tools/schema.ts (3)
packages/fmodata/src/client.ts (1)
ODataApiClient(266-266)packages/fmodata-mcp/src/types.ts (4)
CreateTableSchema(264-293)AddFieldsSchema(295-324)DeleteTableSchema(326-336)DeleteFieldSchema(338-352)packages/fmodata/src/client-types.ts (2)
FieldDefinition(193-198)VALID_FIELD_TYPES(175-188)
packages/fmodata/src/adapters/fetch.ts (2)
packages/fmodata/src/adapters/fetch-base-types.ts (1)
BaseFetchAdapterOptions(4-12)packages/fmdapi/src/adapters/fetch-base.ts (1)
BaseFetchAdapter(32-335)
packages/fmodata-mcp/src/tools/crud.ts (2)
packages/fmodata/src/client.ts (1)
ODataApiClient(266-266)packages/fmodata-mcp/src/types.ts (3)
CreateRecordSchema(122-137)UpdateRecordSchema(139-159)DeleteRecordSchema(161-176)
packages/fmodata-mcp/src/tools/query.ts (2)
packages/fmodata/src/client.ts (1)
ODataApiClient(266-266)packages/fmodata-mcp/src/types.ts (8)
ListTablesSchema(8-12)GetMetadataSchema(14-18)QueryRecordsSchema(20-58)GetRecordSchema(60-83)GetRecordCountSchema(85-99)GetFieldValueSchema(101-120)NavigateRelatedSchema(178-213)CrossJoinSchema(215-243)
packages/fmodata-mcp/src/types.ts (1)
packages/fmodata/src/client-types.ts (1)
VALID_FIELD_TYPES(175-188)
packages/fmodata-mcp/src/server.ts (8)
packages/fmodata/src/client.ts (2)
ODataApiClient(266-266)ODataApi(41-259)packages/fmodata/src/index.ts (5)
ODataApiClient(16-16)isOttoAPIKey(23-23)ODataApi(15-15)OttoAdapter(19-19)FetchAdapter(17-17)packages/fmodata/src/adapters/otto.ts (3)
isOttoAPIKey(16-18)OttoAdapter(40-97)request(73-91)packages/fmodata/src/adapters/fetch.ts (1)
FetchAdapter(15-41)packages/fmodata-mcp/src/tools/query.ts (2)
createQueryTools(17-60)handleQueryTool(65-162)packages/fmodata-mcp/src/tools/crud.ts (2)
createCrudTools(12-30)handleCrudTool(35-67)packages/fmodata-mcp/src/tools/schema.ts (2)
createSchemaTools(14-37)handleSchemaTool(42-126)packages/fmodata-mcp/src/tools/scripts.ts (2)
createScriptTools(8-21)handleScriptTool(26-54)
packages/fmodata/tests/integration-comprehensive.test.ts (1)
packages/fmodata/src/client-types.ts (1)
ODataRecord(47-47)
packages/fmodata/tests/fetch-base.test.ts (3)
packages/fmodata/src/adapters/fetch.ts (1)
FetchAdapter(15-41)packages/fmodata/tests/setup.ts (3)
createODataResponse(26-36)createMockResponse(6-24)createODataErrorResponse(38-50)packages/fmodata/src/client-types.ts (1)
FileMakerODataError(260-280)
packages/fmodata-mcp/src/index.ts (1)
packages/fmodata-mcp/src/server.ts (3)
ODataConfig(33-40)startServer(250-348)createServer(85-245)
packages/fmodata/src/index.ts (1)
packages/fmodata/src/client.ts (1)
ODataApi(41-259)
packages/fmodata/src/adapters/core.ts (1)
packages/fmodata/src/client-types.ts (22)
ODataResponse(8-12)ODataTable(38-42)ODataMetadata(24-33)ODataRecord(47-47)GetRecordsOptions(103-103)GetRecordOptions(108-108)ODataEntityResponse(17-19)GetRecordCountOptions(113-115)GetFieldValueOptions(120-120)CreateRecordOptions(84-86)UpdateRecordOptions(91-93)DeleteRecordOptions(98-98)UpdateRecordReferencesOptions(135-138)NavigateRelatedOptions(125-125)CrossJoinOptions(130-130)BatchOptions(150-152)CreateTableOptions(203-206)AddFieldsOptions(211-213)DeleteTableOptions(218-218)DeleteFieldOptions(223-223)RunScriptOptions(228-231)UploadContainerOptions(236-239)
packages/fmodata/src/client.ts (2)
packages/fmodata/src/adapters/core.ts (1)
Adapter(39-171)packages/fmodata/src/client-types.ts (23)
RequestOptions(76-79)ODataResponse(8-12)ODataTable(38-42)ODataMetadata(24-33)ODataRecord(47-47)GetRecordsOptions(103-103)GetRecordOptions(108-108)ODataEntityResponse(17-19)GetRecordCountOptions(113-115)GetFieldValueOptions(120-120)CreateRecordOptions(84-86)UpdateRecordOptions(91-93)DeleteRecordOptions(98-98)UpdateRecordReferencesOptions(135-138)NavigateRelatedOptions(125-125)CrossJoinOptions(130-130)BatchOptions(150-152)CreateTableOptions(203-206)AddFieldsOptions(211-213)DeleteTableOptions(218-218)DeleteFieldOptions(223-223)RunScriptOptions(228-231)UploadContainerOptions(236-239)
packages/fmodata/src/adapters/fetch-base.ts (4)
packages/fmodata/src/adapters/core.ts (2)
Adapter(39-171)BaseRequestOptions(30-33)packages/fmodata/src/adapters/fetch-base-types.ts (1)
BaseFetchAdapterOptions(4-12)packages/fmodata/src/utils.ts (14)
buildAcceptHeader(159-178)buildContentTypeHeader(183-194)parseErrorResponse(246-272)buildTablesPath(121-123)buildMetadataPath(114-116)buildTablePath(71-73)buildQueryString(24-66)buildRecordPath(89-96)encodeODataFilter(8-16)buildFieldValuePath(101-109)buildNavigationPath(128-136)buildCrossJoinPath(141-147)buildBatchPath(152-154)buildFileMakerTablesPath(78-84)packages/fmodata/src/client-types.ts (23)
FileMakerODataError(260-280)ODataResponse(8-12)ODataTable(38-42)ODataMetadata(24-33)ODataRecord(47-47)GetRecordsOptions(103-103)GetRecordOptions(108-108)ODataEntityResponse(17-19)GetRecordCountOptions(113-115)GetFieldValueOptions(120-120)CreateRecordOptions(84-86)UpdateRecordOptions(91-93)DeleteRecordOptions(98-98)UpdateRecordReferencesOptions(135-138)NavigateRelatedOptions(125-125)CrossJoinOptions(130-130)BatchOptions(150-152)CreateTableOptions(203-206)AddFieldsOptions(211-213)DeleteTableOptions(218-218)DeleteFieldOptions(223-223)RunScriptOptions(228-231)UploadContainerOptions(236-239)
packages/fmodata/src/adapters/otto.ts (2)
packages/fmodata/src/adapters/fetch-base-types.ts (1)
BaseFetchAdapterOptions(4-12)packages/fmdapi/src/adapters/fetch-base.ts (1)
BaseFetchAdapter(32-335)
packages/fmodata/src/utils.ts (1)
packages/fmodata/src/client-types.ts (1)
QueryOptions(52-71)
packages/fmodata/tests/integration.test.ts (3)
packages/fmodata/src/client.ts (1)
ODataApi(41-259)packages/fmodata/src/adapters/otto.ts (2)
isOttoAPIKey(16-18)OttoAdapter(40-97)packages/fmodata/src/adapters/fetch.ts (1)
FetchAdapter(15-41)
🪛 Biome (2.1.2)
packages/fmodata/tests/integration.test.ts
[error] 155-200: Disallow duplicate setup and teardown hooks.
Disallow beforeAll duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
🪛 LanguageTool
packages/fmodata/README.md
[style] ~111-~111: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ieldValue(table, key, field, options?)Get the value of a specific field. ####n...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~123-~123: ‘new record’ might be wordy. Consider a shorter alternative.
Context: ...createRecord(table, options) Create a new record. Options: - data: Record data as...
(EN_WORDINESS_PREMIUM_NEW_RECORD)
packages/fmodata-mcp/README.md
[style] ~285-~285: ‘new record’ might be wordy. Consider a shorter alternative.
Context: ... - fmodata_create_record - Create new record - Parameters: table, data - **`fm...
(EN_WORDINESS_PREMIUM_NEW_RECORD)
🪛 markdownlint-cli2 (0.18.1)
packages/fmodata-mcp/README.md
312-312: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (19)
packages/fmodata/README.md (4)
15-63: Well-structured quick start with practical examples.The Basic Authentication example clearly demonstrates initialization and all major CRUD operations in a realistic flow. Good balance of detail and brevity.
65-80: Clear alternative authentication example.The Otto Authentication section is concise and includes helpful inline comments about API key formats and optional Otto v3 port configuration.
175-189: Solid error handling documentation.The try-catch pattern with
FileMakerODataErrorinstanceof check is practical and clearly shows how to access error details.
191-203: TypeScript generics support is well-demonstrated.The example clearly shows how to define and apply a custom interface type to a query result.
packages/fmodata/package.json (2)
12-26: Verify conditional exports pattern works as expected.The wildcard pattern in
./adapters/*is relatively modern. Ensure this is tested with consumers and works as intended on the npm distribution. Consider adding"sideEffects": falseif applicable to enable tree-shaking.
27-40: LGTM for build and release workflow.The script configuration aligns well with monorepo standards. The
prepublishOnlyhook andciscript provide good safeguards for release.packages/fmodata/vite.config.ts (2)
1-2: LGTM!The imports are appropriate for a Vite configuration using TanStack's build tooling.
8-16: ESM-only output is intentional and appropriate.The package is already configured as ESM-only in
package.jsonwith"type": "module"and no CommonJS export conditions defined. All imports across the monorepo use ESM syntax. Settingcjs: falseformalizes this existing configuration and does not introduce breaking changes.packages/fmodata/vitest.config.ts (1)
1-8: LGTM!The Vitest configuration is appropriate for integration tests. The 30-second timeout accommodates potentially slower integration tests, and enabling globals is a common Vitest practice.
packages/fmodata-mcp/src/tools/index.ts (1)
1-4: LGTM!Clean barrel export pattern with correct
.jsextensions for ESM module resolution.packages/fmodata/src/adapters/fetch-base-types.ts (1)
1-12: LGTM!Clean type definition with appropriate documentation. The security warning for
rejectUnauthorizedis important and well-placed.packages/fmodata/tests/integration-comprehensive.test.ts (2)
457-466: Good defensive handling of missing IDs.The test correctly handles the case where records might not have identifiable IDs (e.g., when primary key isn't configured) and validates pagination works even without comparing specific record IDs.
487-504: Verify field name spelling against your FileMaker database schema.The field name
totol_sales(used on lines 489, 491, and 498) appears to be a typo—the double 'o' suggests it should betotal_sales. Since this integration test queries an external FileMaker database, confirm that the field name in your actual database matches what's written here. If the correct field name istotal_sales, all three references need to be corrected.packages/fmodata-mcp/src/server.ts (1)
284-295: LGTM: Session ID generation pattern is correct.The session ID is generated once per session creation (line 284) and the
sessionIdGeneratorcallback correctly returns that same ID. This ensures consistency between the session storage key and the ID used by the transport.packages/fmodata/src/utils.ts (4)
8-16: LGTM: Correct encoding order to prevent double-encoding.The function correctly encodes
%first before other characters, which prevents double-encoding issues. The minimal encoding approach aligns with FileMaker OData's expectations.
32-42: Well-handled field list encoding.The implementation correctly handles comma-separated field lists by encoding individual field names while preserving commas as OData syntax delimiters.
203-210: Correct OData key encoding implementation.The function properly handles both numeric and string keys according to OData URL conventions, including the correct escape mechanism for single quotes within string keys.
215-241: Good error enhancement pattern.The error message enhancement for code 8309 provides helpful context about primary key configuration requirements. This pattern can be extended as more FileMaker error codes are discovered.
Consider documenting this pattern in a comment so future maintainers know where to add new error code enhancements.
packages/fmodata/src/adapters/core.ts (1)
27-172: Well-designed adapter interface.The
Adapterinterface provides a comprehensive and type-safe contract for OData operations. The method signatures are clear, consistently structured, and well-documented. The use of generics for record types ensures type safety while maintaining flexibility.
| const isHttpMode = | ||
| process.argv.includes("--http") || process.argv.includes("--server"); | ||
| const config = parseArgs(); |
There was a problem hiding this comment.
Fix HTTP mode detection flag collision.
process.argv.includes("--server") fires whenever the host alias is supplied as --server https://..., so the CLI flips into HTTP mode even when the user just wanted to set the host (stdio mode should have remained the default). That makes common invocations like fmodata-mcp --server https://example --database Foo behave incorrectly. Please gate HTTP mode behind a dedicated flag (e.g. only --http or another unique token) so the host alias stays usable.
-const isHttpMode =
- process.argv.includes("--http") || process.argv.includes("--server");
+const isHttpMode = process.argv.includes("--http");📝 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.
| const isHttpMode = | |
| process.argv.includes("--http") || process.argv.includes("--server"); | |
| const config = parseArgs(); | |
| const isHttpMode = process.argv.includes("--http"); | |
| const config = parseArgs(); |
| const VALID_FIELD_TYPES = [ | ||
| "NUMERIC", | ||
| "DECIMAL", | ||
| "INT", | ||
| "DATE", | ||
| "TIME", | ||
| "TIMESTAMP", | ||
| "VARCHAR", | ||
| "CHARACTER VARYING", | ||
| "BLOB", | ||
| "VARBINARY", | ||
| "LONGVARBINARY", | ||
| "BINARY VARYING", | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Eliminate code duplication by importing VALID_FIELD_TYPES.
The VALID_FIELD_TYPES array duplicates the constant defined in packages/fmodata/src/client-types.ts (lines 174-187). This creates a maintenance burden and risks the two definitions diverging.
Import the constant from the shared package:
import type { JSONSchema7 } from "json-schema";
+import { VALID_FIELD_TYPES } from "@proofkit/fmodata";
-/**
- * Valid field types for FileMaker_Tables schema modifications
- * These are the only types accepted by the FileMaker OData API
- */
-const VALID_FIELD_TYPES = [
- "NUMERIC",
- "DECIMAL",
- "INT",
- "DATE",
- "TIME",
- "TIMESTAMP",
- "VARCHAR",
- "CHARACTER VARYING",
- "BLOB",
- "VARBINARY",
- "LONGVARBINARY",
- "BINARY VARYING",
-];Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/fmodata-mcp/src/types.ts around lines 249 to 262, the local
VALID_FIELD_TYPES array duplicates the constant defined in
packages/fmodata/src/client-types.ts (lines 174-187); remove the local array and
import VALID_FIELD_TYPES from the shared module instead (adjusting the import
path to packages/fmodata/src/client-types.ts), update any references to use the
imported constant, and run TypeScript build/tests to ensure the import path and
exported name match and no linter/type errors remain.
| "dependencies": { | ||
| "@tanstack/vite-config": "^0.2.0", | ||
| "vite": "^6.3.4", | ||
| "zod": "3.25.64" |
There was a problem hiding this comment.
Move build tooling to devDependencies.
vite and @tanstack/vite-config are build-time tools and should not be published with the package. They belong in devDependencies.
Apply this diff:
"dependencies": {
- "@tanstack/vite-config": "^0.2.0",
- "vite": "^6.3.4",
"zod": "3.25.64"
},
"devDependencies": {
+ "@tanstack/vite-config": "^0.2.0",
+ "vite": "^6.3.4",
"@types/node": "^22.17.1",📝 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.
| "dependencies": { | |
| "@tanstack/vite-config": "^0.2.0", | |
| "vite": "^6.3.4", | |
| "zod": "3.25.64" | |
| "dependencies": { | |
| "zod": "3.25.64" | |
| }, | |
| "devDependencies": { | |
| "@tanstack/vite-config": "^0.2.0", | |
| "vite": "^6.3.4", | |
| "@types/node": "^22.17.1", |
🤖 Prompt for AI Agents
In packages/fmodata/package.json around lines 42 to 45, the build tools "vite"
and "@tanstack/vite-config" are incorrectly listed under "dependencies"; remove
those two entries from "dependencies" and add them with the same version strings
under "devDependencies" instead (leaving "zod" in "dependencies"); update
package.json accordingly and run the install (npm/yarn/pnpm) to update
lockfiles.
| // Note: This is a security risk and should only be used in development | ||
| let originalRejectUnauthorized: string | undefined; | ||
| if (!this.rejectUnauthorized && typeof process !== "undefined") { | ||
| // Save original value and disable SSL verification | ||
| originalRejectUnauthorized = process.env.NODE_TLS_REJECT_UNAUTHORIZED; | ||
| process.env.NODE_TLS_REJECT_UNAUTHORIZED = "0"; | ||
| } | ||
|
|
||
| const response = await fetch(fetchUrl, { | ||
| ...fetchOptions, | ||
| method, | ||
| headers: requestHeaders, | ||
| body: body ? JSON.stringify(body) : undefined, | ||
| signal: controller.signal, | ||
| }); | ||
|
|
||
| // Restore original SSL verification setting | ||
| if (!this.rejectUnauthorized && typeof process !== "undefined") { | ||
| if (originalRejectUnauthorized === undefined) { | ||
| delete process.env.NODE_TLS_REJECT_UNAUTHORIZED; | ||
| } else { | ||
| process.env.NODE_TLS_REJECT_UNAUTHORIZED = originalRejectUnauthorized; | ||
| } | ||
| } |
There was a problem hiding this comment.
Restore the TLS env override on failure paths.
When fetch throws (network error, abort, etc.), the code never runs the block that restores process.env.NODE_TLS_REJECT_UNAUTHORIZED, so the process stays in insecure mode for all subsequent requests. Wrap the override in a finally (or explicitly restore inside the catch) so the original value is always reinstated before returning or rethrowing. citeturn0code0
Apply this diff to ensure consistent restoration:
- const controller = new AbortController();
- let timeoutId: NodeJS.Timeout | null = null;
+ const controller = new AbortController();
+ let timeoutId: NodeJS.Timeout | null = null;
+ let originalRejectUnauthorized: string | undefined;
@@
- try {
- // For Node.js, if rejectUnauthorized is false, temporarily disable SSL verification
- // Note: This is a security risk and should only be used in development
- let originalRejectUnauthorized: string | undefined;
+ const shouldDisableTlsCheck = !this.rejectUnauthorized && typeof process !== "undefined";
+
+ try {
+ // For Node.js, if rejectUnauthorized is false, temporarily disable SSL verification
+ // Note: This is a security risk and should only be used in development
- if (!this.rejectUnauthorized && typeof process !== "undefined") {
+ if (shouldDisableTlsCheck) {
// Save original value and disable SSL verification
originalRejectUnauthorized = process.env.NODE_TLS_REJECT_UNAUTHORIZED;
process.env.NODE_TLS_REJECT_UNAUTHORIZED = "0";
}
@@
- // Restore original SSL verification setting
- if (!this.rejectUnauthorized && typeof process !== "undefined") {
- if (originalRejectUnauthorized === undefined) {
- delete process.env.NODE_TLS_REJECT_UNAUTHORIZED;
- } else {
- process.env.NODE_TLS_REJECT_UNAUTHORIZED = originalRejectUnauthorized;
- }
- }
-
- if (timeoutId) {
- clearTimeout(timeoutId);
- }
-
- let responseData: unknown;
+ let responseData: unknown;
@@
- } catch (error) {
- if (timeoutId) {
- clearTimeout(timeoutId);
- }
+ } catch (error) {
if (error instanceof FileMakerODataError) {
throw error;
}
@@
- throw error;
- }
+ throw error;
+ } finally {
+ if (timeoutId) {
+ clearTimeout(timeoutId);
+ }
+ if (shouldDisableTlsCheck) {
+ if (originalRejectUnauthorized === undefined) {
+ delete process.env.NODE_TLS_REJECT_UNAUTHORIZED;
+ } else {
+ process.env.NODE_TLS_REJECT_UNAUTHORIZED = originalRejectUnauthorized;
+ }
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/fmodata/src/adapters/fetch-base.ts around lines 156 to 179, the code
that disables SSL verification by setting
process.env.NODE_TLS_REJECT_UNAUTHORIZED = "0" does not guarantee restoration if
fetch throws, leaving the process insecure; move the fetch call and the
restoration logic into a try/finally (declare originalRejectUnauthorized before
the try), perform the fetch inside the try, and always restore
process.env.NODE_TLS_REJECT_UNAUTHORIZED in the finally block (deleting it if
original was undefined or resetting to the original value) so the environment is
reinstated on success or error.
| const path = buildBatchPath(this.database); | ||
|
|
||
| // Construct batch request body | ||
| const batchBody = { | ||
| requests: options.requests.map((req: BatchRequest) => ({ | ||
| id: crypto.randomUUID(), | ||
| method: req.method, | ||
| url: req.url, | ||
| headers: req.headers ?? {}, | ||
| body: req.body, | ||
| })), | ||
| }; | ||
|
|
||
| const response = await this.request<{ | ||
| responses: Array<{ id: string; status: number; body: unknown }>; | ||
| }>({ | ||
| path, | ||
| method: "POST", | ||
| body: batchBody, | ||
| timeout: options.timeout, | ||
| fetchOptions: options.fetch, | ||
| headers: { | ||
| "Content-Type": "multipart/mixed; boundary=batch", | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Fix the $batch request payload format.
batchRequests advertises Content-Type: multipart/mixed; boundary=batch, but the body you send is just JSON (because request() always JSON.stringifys it). FileMaker’s $batch endpoint expects a multipart payload that mirrors HTTP requests; sending JSON under a multipart header will 415/500 every time. Either build the multipart body (with the required boundaries and request parts) or switch entirely to the JSON batching format and set Content-Type: application/json. As written, $batch calls cannot succeed. citeturn0code0
| super({ | ||
| server: options.server, | ||
| database: options.database, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Propagate rejectUnauthorized to BaseFetchAdapter.
FetchAdapterOptions inherits rejectUnauthorized, but we drop it when calling super, so anyone trying to connect to a dev/self-signed host can’t disable TLS verification anymore. Please forward the option so existing behaviour keeps working.
constructor(options: FetchAdapterOptions) {
super({
server: options.server,
database: options.database,
+ rejectUnauthorized: options.rejectUnauthorized,
});📝 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.
| super({ | |
| server: options.server, | |
| database: options.database, | |
| }); | |
| constructor(options: FetchAdapterOptions) { | |
| super({ | |
| server: options.server, | |
| database: options.database, | |
| rejectUnauthorized: options.rejectUnauthorized, | |
| }); |
🤖 Prompt for AI Agents
In packages/fmodata/src/adapters/fetch.ts around lines 20 to 24, the constructor
calls super({ server: options.server, database: options.database }) and drops
the rejectUnauthorized flag from FetchAdapterOptions; forward
options.rejectUnauthorized into the super call (e.g. include rejectUnauthorized:
options.rejectUnauthorized or spread the relevant option) so TLS verification
behavior is preserved for dev/self-signed hosts, and ensure the value matches
the expected type on BaseFetchAdapter.
| export function isOttoAuth(auth: unknown): auth is OttoAuth { | ||
| if (typeof auth !== "object" || auth === null) return false; | ||
| return "apiKey" in auth; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Strengthen the isOttoAuth type guard.
The type guard only checks for the presence of an apiKey property but doesn't validate that it's actually a valid Otto API key format. This could lead to runtime errors when an invalid auth object is passed.
export function isOttoAuth(auth: unknown): auth is OttoAuth {
if (typeof auth !== "object" || auth === null) return false;
- return "apiKey" in auth;
+ if (!("apiKey" in auth)) return false;
+ const apiKey = (auth as { apiKey: unknown }).apiKey;
+ return typeof apiKey === "string" && isOttoAPIKey(apiKey);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/fmodata/src/adapters/otto.ts around lines 20 to 23, the isOttoAuth
type guard only checks for the presence of an apiKey property; update it to
verify the property is a non-empty string (and if you know the Otto key pattern,
validate it with a regex, e.g. prefix/format) so the guard actually narrows to a
usable OttoAuth; replace the simple "apiKey" in auth check with checks like
typeof apiKey === "string" and apiKey.trim().length > 0 (and apply a regex check
if applicable).
| beforeAll(() => { | ||
| // Disable SSL verification for localhost/development | ||
| // This must be set before any TLS connections are made | ||
| if (host && host.includes("localhost")) { | ||
| process.env.NODE_TLS_REJECT_UNAUTHORIZED = "0"; | ||
| console.log("⚠️ SSL verification disabled for localhost (development only)"); | ||
| } | ||
|
|
||
| // Replace global fetch with logging version | ||
| if (typeof globalThis.fetch !== "undefined") { | ||
| globalThis.fetch = createLoggingFetch(globalThis.fetch); | ||
| } | ||
| }); | ||
|
|
||
| beforeAll(() => { | ||
| if (!host || !database) { | ||
| throw new Error( | ||
| "Integration tests require FMODATA_HOST and FMODATA_DATABASE environment variables", | ||
| ); | ||
| } | ||
|
|
||
| if (ottoApiKey && isOttoAPIKey(ottoApiKey)) { | ||
| if (ottoApiKey.startsWith("KEY_")) { | ||
| client = ODataApi({ | ||
| adapter: new OttoAdapter({ | ||
| server: host, | ||
| database, | ||
| auth: { apiKey: ottoApiKey as `KEY_${string}`, ottoPort }, | ||
| rejectUnauthorized: false, // SSL verification handled via env var | ||
| }), | ||
| }); | ||
| } else if (ottoApiKey.startsWith("dk_")) { | ||
| client = ODataApi({ | ||
| adapter: new OttoAdapter({ | ||
| server: host, | ||
| database, | ||
| auth: { apiKey: ottoApiKey as `dk_${string}` }, | ||
| rejectUnauthorized: false, // SSL verification handled via env var | ||
| }), | ||
| }); | ||
| } else { | ||
| throw new Error("Invalid Otto API key format"); | ||
| } | ||
| } else if (username && password) { | ||
| client = ODataApi({ | ||
| adapter: new FetchAdapter({ | ||
| server: host, | ||
| database, | ||
| auth: { username, password }, | ||
| rejectUnauthorized: false, // SSL verification handled via env var | ||
| }), | ||
| }); | ||
| } else { | ||
| throw new Error( | ||
| "Integration tests require either FMODATA_OTTO_API_KEY or both FMODATA_USERNAME and FMODATA_PASSWORD", | ||
| ); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Resolve duplicate beforeAll hooks
Biome flags this as a lint error (noDuplicateTestHooks). Merge the setup logic into a single beforeAll so the suite passes lint.
@@
-beforeAll(() => {
- // Disable SSL verification for localhost/development
- // This must be set before any TLS connections are made
- if (host && host.includes("localhost")) {
- process.env.NODE_TLS_REJECT_UNAUTHORIZED = "0";
- console.log("⚠️ SSL verification disabled for localhost (development only)");
- }
-
- // Replace global fetch with logging version
- if (typeof globalThis.fetch !== "undefined") {
- globalThis.fetch = createLoggingFetch(globalThis.fetch);
- }
-});
-
-beforeAll(() => {
+beforeAll(() => {
+ // Disable SSL verification for localhost/development
+ // This must be set before any TLS connections are made
+ if (host && host.includes("localhost")) {
+ process.env.NODE_TLS_REJECT_UNAUTHORIZED = "0";
+ console.log("⚠️ SSL verification disabled for localhost (development only)");
+ }
+
+ // Replace global fetch with logging version
+ if (typeof globalThis.fetch !== "undefined") {
+ globalThis.fetch = createLoggingFetch(globalThis.fetch);
+ }
+
if (!host || !database) {
throw new Error(
"Integration tests require FMODATA_HOST and FMODATA_DATABASE environment variables",
);
}
@@
} else {
throw new Error(
"Integration tests require either FMODATA_OTTO_API_KEY or both FMODATA_USERNAME and FMODATA_PASSWORD",
);
}
-});
+});📝 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.
| beforeAll(() => { | |
| // Disable SSL verification for localhost/development | |
| // This must be set before any TLS connections are made | |
| if (host && host.includes("localhost")) { | |
| process.env.NODE_TLS_REJECT_UNAUTHORIZED = "0"; | |
| console.log("⚠️ SSL verification disabled for localhost (development only)"); | |
| } | |
| // Replace global fetch with logging version | |
| if (typeof globalThis.fetch !== "undefined") { | |
| globalThis.fetch = createLoggingFetch(globalThis.fetch); | |
| } | |
| }); | |
| beforeAll(() => { | |
| if (!host || !database) { | |
| throw new Error( | |
| "Integration tests require FMODATA_HOST and FMODATA_DATABASE environment variables", | |
| ); | |
| } | |
| if (ottoApiKey && isOttoAPIKey(ottoApiKey)) { | |
| if (ottoApiKey.startsWith("KEY_")) { | |
| client = ODataApi({ | |
| adapter: new OttoAdapter({ | |
| server: host, | |
| database, | |
| auth: { apiKey: ottoApiKey as `KEY_${string}`, ottoPort }, | |
| rejectUnauthorized: false, // SSL verification handled via env var | |
| }), | |
| }); | |
| } else if (ottoApiKey.startsWith("dk_")) { | |
| client = ODataApi({ | |
| adapter: new OttoAdapter({ | |
| server: host, | |
| database, | |
| auth: { apiKey: ottoApiKey as `dk_${string}` }, | |
| rejectUnauthorized: false, // SSL verification handled via env var | |
| }), | |
| }); | |
| } else { | |
| throw new Error("Invalid Otto API key format"); | |
| } | |
| } else if (username && password) { | |
| client = ODataApi({ | |
| adapter: new FetchAdapter({ | |
| server: host, | |
| database, | |
| auth: { username, password }, | |
| rejectUnauthorized: false, // SSL verification handled via env var | |
| }), | |
| }); | |
| } else { | |
| throw new Error( | |
| "Integration tests require either FMODATA_OTTO_API_KEY or both FMODATA_USERNAME and FMODATA_PASSWORD", | |
| ); | |
| } | |
| }); | |
| beforeAll(() => { | |
| // Disable SSL verification for localhost/development | |
| // This must be set before any TLS connections are made | |
| if (host && host.includes("localhost")) { | |
| process.env.NODE_TLS_REJECT_UNAUTHORIZED = "0"; | |
| console.log("⚠️ SSL verification disabled for localhost (development only)"); | |
| } | |
| // Replace global fetch with logging version | |
| if (typeof globalThis.fetch !== "undefined") { | |
| globalThis.fetch = createLoggingFetch(globalThis.fetch); | |
| } | |
| if (!host || !database) { | |
| throw new Error( | |
| "Integration tests require FMODATA_HOST and FMODATA_DATABASE environment variables", | |
| ); | |
| } | |
| if (ottoApiKey && isOttoAPIKey(ottoApiKey)) { | |
| if (ottoApiKey.startsWith("KEY_")) { | |
| client = ODataApi({ | |
| adapter: new OttoAdapter({ | |
| server: host, | |
| database, | |
| auth: { apiKey: ottoApiKey as `KEY_${string}`, ottoPort }, | |
| rejectUnauthorized: false, // SSL verification handled via env var | |
| }), | |
| }); | |
| } else if (ottoApiKey.startsWith("dk_")) { | |
| client = ODataApi({ | |
| adapter: new OttoAdapter({ | |
| server: host, | |
| database, | |
| auth: { apiKey: ottoApiKey as `dk_${string}` }, | |
| rejectUnauthorized: false, // SSL verification handled via env var | |
| }), | |
| }); | |
| } else { | |
| throw new Error("Invalid Otto API key format"); | |
| } | |
| } else if (username && password) { | |
| client = ODataApi({ | |
| adapter: new FetchAdapter({ | |
| server: host, | |
| database, | |
| auth: { username, password }, | |
| rejectUnauthorized: false, // SSL verification handled via env var | |
| }), | |
| }); | |
| } else { | |
| throw new Error( | |
| "Integration tests require either FMODATA_OTTO_API_KEY or both FMODATA_USERNAME and FMODATA_PASSWORD", | |
| ); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In packages/fmodata/tests/integration.test.ts around lines 141 to 198, there are
two beforeAll hooks causing a noDuplicateTestHooks lint error; merge them into a
single beforeAll that runs the SSL-disable logic (set
NODE_TLS_REJECT_UNAUTHORIZED when host includes "localhost" and log), replaces
globalThis.fetch with createLoggingFetch if defined, then performs the env
validation and client initialization: check host and database, branch on
ottoApiKey (validate format KEY_ vs dk_ and instantiate ODataApi with
OttoAdapter and rejectUnauthorized: false) or fallback to username/password
(instantiate ODataApi with FetchAdapter and rejectUnauthorized: false), and
throw the same errors for missing env or invalid key format; remove the extra
beforeAll so only one setup hook remains.
| adapter: new OttoAdapter({ | ||
| server: host, | ||
| database, | ||
| auth: { apiKey: ottoApiKey as `KEY_${string}`, ottoPort }, | ||
| rejectUnauthorized: false, // SSL verification handled via env var | ||
| }), | ||
| }); | ||
| } else if (ottoApiKey.startsWith("dk_")) { | ||
| client = ODataApi({ | ||
| adapter: new OttoAdapter({ | ||
| server: host, | ||
| database, | ||
| auth: { apiKey: ottoApiKey as `dk_${string}` }, | ||
| rejectUnauthorized: false, // SSL verification handled via env var | ||
| }), | ||
| }); | ||
| } else { | ||
| throw new Error("Invalid Otto API key format"); | ||
| } | ||
| } else if (username && password) { | ||
| client = ODataApi({ | ||
| adapter: new FetchAdapter({ | ||
| server: host, | ||
| database, | ||
| auth: { username, password }, | ||
| rejectUnauthorized: false, // SSL verification handled via env var | ||
| }), | ||
| }); |
There was a problem hiding this comment.
Don't disable TLS for non-local hosts
Passing rejectUnauthorized: false unconditionally disables TLS verification even when talking to remote servers. That’s a major security risk (MITM exposure) and defeats the purpose of the integration test against real services. Limit the override to localhost scenarios only.
@@
-const ottoPort = process.env.FMODATA_OTTO_PORT
- ? parseInt(process.env.FMODATA_OTTO_PORT.trim(), 10)
- : undefined;
+const ottoPort = process.env.FMODATA_OTTO_PORT
+ ? parseInt(process.env.FMODATA_OTTO_PORT.trim(), 10)
+ : undefined;
+const allowInsecure = Boolean(host && host.includes("localhost"));
@@
-beforeAll(() => {
- // Disable SSL verification for localhost/development
- // This must be set before any TLS connections are made
- if (host && host.includes("localhost")) {
+beforeAll(() => {
+ // Disable SSL verification for localhost/development
+ // This must be set before any TLS connections are made
+ if (allowInsecure) {
process.env.NODE_TLS_REJECT_UNAUTHORIZED = "0";
console.log("⚠️ SSL verification disabled for localhost (development only)");
}
@@
- auth: { apiKey: ottoApiKey as `KEY_${string}`, ottoPort },
- rejectUnauthorized: false, // SSL verification handled via env var
+ auth: { apiKey: ottoApiKey as `KEY_${string}`, ottoPort },
+ ...(allowInsecure ? { rejectUnauthorized: false } : {}),
}),
});
@@
- auth: { apiKey: ottoApiKey as `dk_${string}` },
- rejectUnauthorized: false, // SSL verification handled via env var
+ auth: { apiKey: ottoApiKey as `dk_${string}` },
+ ...(allowInsecure ? { rejectUnauthorized: false } : {}),
}),
});
@@
- adapter: new FetchAdapter({
- server: host,
- database,
- auth: { username, password },
- rejectUnauthorized: false, // SSL verification handled via env var
- }),
+ adapter: new FetchAdapter({
+ server: host,
+ database,
+ auth: { username, password },
+ ...(allowInsecure ? { rejectUnauthorized: false } : {}),
+ }),Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/fmodata/tests/integration.test.ts around lines 165-192, the test
code unconditionally sets rejectUnauthorized: false which disables TLS
verification for all hosts; change this so rejectUnauthorized is set to false
only for local hosts. Implement a small check (e.g., isLocalHost if host
contains "localhost" or equals "127.0.0.1" or "::1") and pass
rejectUnauthorized: !isLocalHost ? true : false (or simply omit the flag for
non-local hosts) for each adapter instantiation (both OttoAdapter branches and
the FetchAdapter) so remote servers keep TLS verification enabled.
Summary by CodeRabbit
Release Notes