fmdata #76
fmdata #76
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
@proofkit/better-auth
@proofkit/cli
create-proofkit
@proofkit/fmdapi
@proofkit/fmodata
@proofkit/typegen
@proofkit/webviewer
commit: |
…ce for build-time variable injection. Modify release script to use turbo for building packages.
- Updated README.md to include detailed examples of the new orderBy method, showcasing single and multiple field sorting with type safety. - Introduced TypeSafeOrderBy type in query-builder.ts to enforce type safety for orderBy parameters. - Modified orderBy implementation to support tuple syntax for explicit sorting directions and multiple fields. - Updated tests to validate the new orderBy API, ensuring compile-time type checks and proper query string generation for both typed and untyped databases.
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Documentation & Configuration .cursor/plans/static-registry.md, packages/fmodata/CHANGELOG.md, packages/fmodata/IMPLEMENTATION_SUMMARY.md, packages/fmodata/README.md, packages/fmodata/REFACTORING_SUMMARY.md, packages/fmodata/docs/ORM_API.md, packages/fmodata/op.env |
Deleted static registry plan; added comprehensive package documentation (README, changelogs, implementation summaries, ORM API guide), and 1Password-based environment configuration file. |
Package Configuration root package.json, packages/cli/package.json, packages/cli/tsdown.config.ts, packages/fmodata/package.json |
Updated root release script to use turbo run build; bumped CLI to v2.0.0-beta.6 and added @rollup/plugin-replace dependency; migrated CLI constants to Rollup replace plugin; expanded fmodata package.json from minimal placeholder to comprehensive public-facing manifest with exports, scripts, dependencies, and metadata. |
Server Connection & Database packages/fmodata/src/client/filemaker-odata.ts, packages/fmodata/src/client/database.ts |
Introduced FMServerConnection class for authenticated OData API access with error handling and entity-ID toggling; added Database class for database-level operations (metadata retrieval, script execution, table listing, batch construction). |
Entity Set & CRUD Operations packages/fmodata/src/client/entity-set.ts, packages/fmodata/src/client/insert-builder.ts, packages/fmodata/src/client/update-builder.ts, packages/fmodata/src/client/delete-builder.ts |
Introduced EntitySet providing fluent API for list/get/insert/update/delete/navigate operations; added builder classes for insert, update, and delete with return-preference handling and filter-based operations. |
Query Building & Response Processing packages/fmodata/src/client/query/*, packages/fmodata/src/client/builders/*, packages/fmodata/src/client/response-processor.ts |
Implemented type-safe QueryBuilder with select/where/orderBy/expand/top/skip support; added ExpandBuilder for nested expands with validation; created response processors, select utilities, field formatters, and query string builders for OData integration. |
Batch Operations packages/fmodata/src/client/batch-builder.ts, packages/fmodata/src/client/batch-request.ts |
Added BatchBuilder for executing multiple operations atomically; implemented multipart batch request/response formatting and parsing per OData spec. |
ORM Layer packages/fmodata/src/orm/table.ts, packages/fmodata/src/orm/field-builders.ts, packages/fmodata/src/orm/operators.ts, packages/fmodata/src/orm/column.ts, packages/fmodata/src/orm/index.ts |
Introduced field builder system with fluent API (textField, numberField, etc.); implemented FMTable with Symbol-backed metadata and type-safe column references; added filter operators (comparison, logical, string functions, null checks) and order-by expressions; created Column type-safe field references. |
Error Handling & Validation packages/fmodata/src/errors.ts, packages/fmodata/src/validation.ts, packages/fmodata/src/logger.ts |
Established comprehensive error hierarchy (HTTPError, ODataError, SchemaLockedError, ValidationError, etc.) with type guards; implemented validation for records/lists with expand support; added structured, color-aware logger with configurable levels. |
Utilities & Type System packages/fmodata/src/types.ts, packages/fmodata/src/transform.ts, packages/fmodata/src/sanitize-json.ts, packages/fmodata/src/schema-manager.ts, packages/fmodata/src/index.ts |
Defined comprehensive TypeScript type system for executables, results, batches, OData metadata; implemented field ID ↔ name transformations for entity-ID mode; added JSON sanitizer for FileMaker's ? values; introduced schema management API; consolidated public barrel exports. |
Scripts & Development Tools packages/fmodata/scripts/* |
Added utility scripts for capturing live OData responses, downloading metadata, generating type definitions, testing batch requests, publishing alpha releases, and experimentation with batch/navigation APIs. |
Test Suite packages/fmodata/tests/*, packages/fmodata/tests/e2e/*, packages/fmodata/src/logger.test.ts |
Introduced comprehensive test coverage including E2E tests against live FileMaker server, batch operation tests, filter/expand tests, field ID transformation tests, delete operations, error handling, type inference validation, and logger tests. |
Estimated code review effort
🎯 5 (Critical) | ⏱️ 90+ minutes
Areas requiring extra attention:
src/orm/table.ts: Complex generic type inference using Symbol-based internal storage; interconnected type utilities for schema/field extraction and table composition with column propertiessrc/client/query/query-builder.ts: Extensive generic parameters and conditional type logic; type-safe builder pattern with select/expand/filter/orderBy composition; response type inference across multiple statessrc/client/batch-builder.ts&batch-request.ts: Multipart body formatting/parsing per OData batch spec; error handling and truncation scenarios; tuple-preserving result typingsrc/orm/operators.ts: Filter expression rendering to OData syntax; column-aware value transformation and quoting logicsrc/transform.ts: Bidirectional field ID ↔ name transformations; recursive response field mapping for expanded relations- Field ID integration: Entity-ID handling across query builders, request construction, response processing, and transform pipelines (verify consistency)
- Error parsing & propagation: SchemaLockedError detection (code "303"), ODataError extraction, and per-builder error handling paths
- Response validation pipeline: Coordinate validation, expand config handling, selected field filtering, and field renaming across builders
Possibly related PRs
- custom registry base #59: Directly related; this PR removes the
.cursor/plans/static-registry.mdplan while#59likely added or maintained static-registry planning and implementation details.
Pre-merge checks and finishing touches
❌ Failed checks (1 warning, 1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 65.65% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
|
| Title check | ❓ Inconclusive | The PR title 'fmdata' is extremely vague and does not clearly summarize the main changes. While it references the new package being introduced, it provides no meaningful information about the work or context. | Use a more descriptive title that captures the primary objective, such as 'Add type-safe OData client package (fmodata)' or 'Introduce fmodata package with QueryBuilder, EntitySet, and batch support'. |
✅ Passed checks (1 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
10-28-fmdata
Comment @coderabbitai help to get the list of available commands and usage tips.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| name: "list-basic", | ||
| description: "Basic list query without filters or options", | ||
| execute: async (client) => { | ||
| const path = "/contacts$top=10"; |
There was a problem hiding this comment.
Bug: Missing query separator in OData URL path
The OData query path /contacts$top=10 is missing the ? separator before the query parameter. It should be /contacts?$top=10. Without the ?, the entire string is interpreted as a single path segment rather than a query to the contacts collection with a $top parameter. All other similar queries in this file correctly use ? (e.g., /contacts?$select=... on line 209, /contacts?$orderby=... on line 219). The documentation comment at line 23 even shows the correct format.
| if (!ver1.prerelease && ver2.prerelease) return 1; | ||
| if (ver1.prerelease && ver2.prerelease) { | ||
| // Compare prerelease strings (e.g., "alpha.0" vs "alpha.1") | ||
| return ver1.prerelease.localeCompare(ver2.prerelease); |
There was a problem hiding this comment.
Bug: Prerelease version comparison uses lexicographic string ordering
The compareVersions function uses localeCompare to compare prerelease strings, which performs lexicographic comparison rather than numeric. This causes incorrect ordering for multi-digit alpha versions. For example, "alpha.19".localeCompare("alpha.9") returns -1 (meaning alpha.19 < alpha.9), because "1" < "9" lexicographically. This would cause the publish script to incorrectly determine that a newer version like 0.1.0-alpha.19 is older than 0.1.0-alpha.9, potentially leading to incorrect version bumping decisions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (22)
packages/fmodata/op.env-8-8 (1)
8-8: Minor typo in comment."commited" → "committed"
-# This file is intentionally commited to source control. +# This file is intentionally committed to source control.packages/fmodata/src/client/builders/default-select.ts-32-37 (1)
32-37: Potential undefined access onbaseTableConfig.schema.If
getBaseTableConfig(table)returns a config without aschemaproperty,Object.keys(baseTableConfig.schema)would throw. Consider adding a guard.🔎 Proposed defensive check
if (defaultSelect === "schema") { const baseTableConfig = getBaseTableConfig(table); + if (!baseTableConfig?.schema) { + return undefined; + } const allFields = Object.keys(baseTableConfig.schema); // Filter out container fields return [...new Set(allFields.filter((f) => !containerFields.includes(f)))]; }packages/fmodata/src/logger.test.ts-10-27 (1)
10-27: Missing test coverage for "success" log level.The
LogLeveltype includes"success"(perlogger.ts), but the test cases only coverdebug,info,warn, anderror. Consider adding test cases for thesuccesslevel to ensure complete coverage.🔎 Example additional test cases
{ currentLogLevel: "debug", logLevel: "error", expected: true }, + { currentLogLevel: "debug", logLevel: "success", expected: true }, { currentLogLevel: "info", logLevel: "debug", expected: false }, + { currentLogLevel: "info", logLevel: "success", expected: true }, + { currentLogLevel: "success", logLevel: "debug", expected: false }, + { currentLogLevel: "success", logLevel: "info", expected: false }, + { currentLogLevel: "success", logLevel: "success", expected: true }, + { currentLogLevel: "success", logLevel: "warn", expected: true }, + { currentLogLevel: "success", logLevel: "error", expected: true },Committable suggestion skipped: line range outside the PR's diff.
packages/fmodata/scripts/publish-alpha.ts-206-209 (1)
206-209: Prerelease comparison vialocaleComparemay yield incorrect results.Using
localeComparefor prerelease strings doesn't handle numeric suffixes correctly. For example,"alpha.9".localeCompare("alpha.10")returns1(indicating 9 > 10), which is semantically incorrect for version ordering.🔎 Proposed fix with numeric-aware prerelease comparison
if (ver1.prerelease && ver2.prerelease) { - // Compare prerelease strings (e.g., "alpha.0" vs "alpha.1") - return ver1.prerelease.localeCompare(ver2.prerelease); + // Compare prerelease strings with numeric awareness (e.g., "alpha.9" vs "alpha.10") + const parts1 = ver1.prerelease.split("."); + const parts2 = ver2.prerelease.split("."); + for (let i = 0; i < Math.max(parts1.length, parts2.length); i++) { + const p1 = parts1[i] ?? ""; + const p2 = parts2[i] ?? ""; + const n1 = parseInt(p1, 10); + const n2 = parseInt(p2, 10); + if (!isNaN(n1) && !isNaN(n2)) { + if (n1 !== n2) return n1 > n2 ? 1 : -1; + } else { + const cmp = p1.localeCompare(p2); + if (cmp !== 0) return cmp; + } + } + return 0; }packages/fmodata/scripts/dreams.ts-57-58 (1)
57-58:FMTableBaseTableConfigis not imported, causing a compile error.The symbol
FMTableBaseTableConfigis referenced but never imported. This will fail TypeScript compilation regardless of the@ts-expect-errordirective.🔎 Proposed fix
If the intent is to test that the symbol is inaccessible, you need to import it first:
import { fmTableOccurrence, textField, numberField, dateField, timeField, timestampField, containerField, calcField, eq, gt, and, or, contains, + FMTableBaseTableConfig, } from "../src/orm";Or, if the symbol is intentionally not exported, consider removing this test line or using a different approach to validate encapsulation.
Committable suggestion skipped: line range outside the PR's diff.
packages/fmodata/scripts/publish-alpha.ts-154-159 (1)
154-159: Potential command injection via version string.The
messageparameter (which isversion) is interpolated directly into the shell command. While version strings are typically safe, if the version were somehow malformed or manipulated, this could lead to command injection.🔎 Safer approach using execSync with array arguments
- // Commit with the provided message - execSync(`git commit -m "${message.replace(/"/g, '\\"')}"`, { - cwd: repoRoot, - encoding: "utf-8", - }); + // Commit with the provided message + const { spawnSync } = require("child_process"); + const result = spawnSync("git", ["commit", "-m", message], { + cwd: repoRoot, + encoding: "utf-8", + }); + if (result.status !== 0) { + throw new Error(`Git commit failed: ${result.stderr}`); + }Alternatively, use a library like
execafor safer subprocess handling.Committable suggestion skipped: line range outside the PR's diff.
packages/fmodata/README.md-95-95 (1)
95-95: Fix typo: "occurances" → "occurrences"The word "occurrences" is misspelled twice in this paragraph.
🔎 Suggested fix
-OData relies entirely on the table occurances in the relationship graph for data access. Relationships between table occurrences are also used, but maybe not as you expect (in short, only the simplest relationships are supported). Given these constraints, it may be best for you to have a seperate FileMaker file for your OData connection, using external data sources to link to your actual data file. We've found this especially helpful for larger projects that have very large graphs with lots of redundant table occurances compared to actual number of base tables. +OData relies entirely on the table occurrences in the relationship graph for data access. Relationships between table occurrences are also used, but maybe not as you expect (in short, only the simplest relationships are supported). Given these constraints, it may be best for you to have a separate FileMaker file for your OData connection, using external data sources to link to your actual data file. We've found this especially helpful for larger projects that have very large graphs with lots of redundant table occurrences compared to actual number of base tables.Note: Also corrected "seperate" → "separate".
packages/fmodata/tests/errors.test.ts-34-34 (1)
34-34: Remove unused import.
validateHeaderValuefrom"http"is imported but never used in this test file.🔎 Suggested fix
-import { validateHeaderValue } from "http";packages/fmodata/IMPLEMENTATION_SUMMARY.md-156-182 (1)
156-182: Add language specifier to the fenced code block.The file structure block is missing a language identifier. Per static analysis (MD040), fenced code blocks should specify a language.
🔎 Proposed fix
-``` +```text src/ ├── orm/packages/fmodata/tests/delete.test.ts-169-176 (1)
169-176: Test has no assertions.Similar to line 92-96, this test creates builders but doesn't assert or verify anything. The redundant
db.from(usersTO)at line 171 appears to be leftover code.🔎 Proposed fix
it("should return deletedCount result type for filter-based delete", async () => { const db = client.database("test_db"); - db.from(usersTO); - db.from(usersTO) + const deleteBuilder = db.from(usersTO) .delete() .where((q) => q.where(eq(usersTO.active, 0))); + + expectTypeOf(deleteBuilder.execute).returns.resolves.toMatchTypeOf<{ + data?: { deletedCount: number }; + error?: Error; + }>(); });packages/fmodata/tests/delete.test.ts-92-96 (1)
92-96: Test has no assertions.This test creates a delete builder but doesn't assert anything. Either add type assertions using
expectTypeOfor remove this test if it's redundant with line 114-116.🔎 Proposed fix - add type assertion or remove
it("should return deletedCount result type", async () => { const db = client.database("test_db"); - db.from(usersTO).delete().byId("user-123"); + const deleteBuilder = db.from(usersTO).delete().byId("user-123"); + // Verify the builder has execute method that returns expected type + expectTypeOf(deleteBuilder.execute).returns.resolves.toMatchTypeOf<{ + data?: { deletedCount: number }; + error?: Error; + }>(); });packages/fmodata/scripts/download-metadata.ts-61-62 (1)
61-62: Comment is inconsistent with the actual timeout value.The comment says "10 seconds" but the timeout is set to 15000ms (15 seconds).
🔎 Proposed fix
fetchClientOptions: { - timeout: 15000, // 10 seconds + timeout: 15000, // 15 seconds retries: 2, },packages/fmodata/scripts/capture-responses.ts-194-204 (1)
194-204: Missing query parameter separator in path.The path is missing the
?separator before$top, which will result in a 404 or malformed request.🔎 Proposed fix
{ name: "list-basic", description: "Basic list query without filters or options", execute: async (client) => { - const path = "/contacts$top=10"; + const path = "/contacts?$top=10"; const response = await client(path); // Get the full URL from the response const url = response.url; return { url, response }; }, },packages/fmodata/scripts/capture-responses.ts-567-580 (1)
567-580: HTTP method is hardcoded to "GET" for all captured responses.The captured response always records
method: "GET"even when the actual request uses POST (e.g., insert operations at lines 237-271). This could cause mock matching issues during tests if the mock server validates HTTP methods.🔎 Proposed fix
Consider passing the method from the query definition or extracting it from the request:
+ // Determine the actual HTTP method used + const method = (queryDef as any).method || "GET"; + // Store captured response (including error responses) capturedResponses[queryDef.name] = { url: sanitizedUrl, - method: "GET", + method, status,And update query definitions to include the method:
{ name: "insert-return-minimal", description: "Insert query with return=minimal", + method: "POST", execute: async (client) => {packages/fmodata/src/client/delete-builder.ts-161-163 (1)
161-163: Record ID not URL-encoded in path.The
recordIdis directly interpolated into the URL without encoding. If the ID contains special characters (e.g.,',),%, or non-ASCII characters), this could break the URL parsing or potentially allow injection if IDs are user-controlled.Suggested fix
if (this.mode === "byId") { // Delete single record by ID: DELETE /{database}/{table}('id') - url = `/${this.databaseName}/${tableId}('${this.recordId}')`; + url = `/${this.databaseName}/${tableId}('${encodeURIComponent(String(this.recordId))}')`; } else {Note: You'll also need to apply the same fix in
getRequestConfig()at line 217.packages/fmodata/src/client/delete-builder.ts-274-289 (1)
274-289: Unhandled JSON parse error in processResponse.
JSON.parse(text)at line 275 can throw aSyntaxErrorif the response body is malformed JSON. This exception would propagate uncaught, potentially breaking batch processing.Suggested fix
- const rawResponse = JSON.parse(text); + let rawResponse: any; + try { + rawResponse = JSON.parse(text); + } catch { + // If not valid JSON, treat as if no count was returned + return { data: { deletedCount: 0 }, error: undefined }; + }packages/fmodata/src/client/update-builder.ts-342-354 (1)
342-354: MissingPreferheader intoRequest()for batch operations.The
execute()method conditionally addsPrefer: return=representation(lines 257-259) whenreturnPreferenceis "representation", buttoRequest()doesn't include this header. This may cause inconsistent behavior in batch operations.Suggested fix
toRequest(baseUrl: string, options?: ExecuteOptions): Request { const config = this.getRequestConfig(); const fullUrl = `${baseUrl}${config.url}`; + const headers: Record<string, string> = { + "Content-Type": "application/json", + Accept: getAcceptHeader(options?.includeODataAnnotations), + }; + + if (this.returnPreference === "representation") { + headers["Prefer"] = "return=representation"; + } + return new Request(fullUrl, { method: config.method, - headers: { - "Content-Type": "application/json", - Accept: getAcceptHeader(options?.includeODataAnnotations), - }, + headers, body: config.body, }); }packages/fmodata/src/client/update-builder.ts-390-390 (1)
390-390: UsesafeJsonParsefor consistency with other builders.
InsertBuilder.processResponse()usessafeJsonParseto handle FileMaker's potentially invalid JSON with unquoted?values, but hereJSON.parseis used directly. This inconsistency could cause parse failures that would succeed in other builders.Suggested fix
+import { safeJsonParse } from "./sanitize-json";- const rawResponse = JSON.parse(text); + const rawResponse = await safeJsonParse({ text: () => Promise.resolve(text) } as Response);Or refactor to parse from the response directly before consuming the text.
Committable suggestion skipped: line range outside the PR's diff.
packages/fmodata/src/client/insert-builder.ts-392-407 (1)
392-407: Dead code: input validation result is unused in processResponse.The
validatedDatavariable is computed from the original input data but is never used afterward. InprocessResponse, you're processing the server's response, not the original input. This validation block appears to be copy-pasted fromexecute()and serves no purpose here.Suggested fix - remove unused validation
- // Validate and transform input data using input validators (writeValidators) - // This is needed for processResponse because it's called from batch operations - // where the data hasn't been validated yet - let validatedData = this.data; - if (this.table) { - const baseTableConfig = getBaseTableConfig(this.table); - const inputSchema = baseTableConfig.inputSchema; - try { - validatedData = await validateAndTransformInput(this.data, inputSchema); - } catch (error) { - return { - data: undefined, - error: error instanceof Error ? error : new Error(String(error)), - } as any; - } - } - // Transform response field IDs back to names if using entity IDspackages/fmodata/src/client/insert-builder.ts-354-360 (1)
354-360: Returning empty object for 204 may surprise callers expecting full record.When
returnPreferenceis "representation" but the server returns 204 No Content, returning an empty object{}may cause downstream issues. Consider returning an error or at least documenting this edge case, as callers might expect actual record data.Suggested improvement
// For 204 responses without return=minimal, FileMaker doesn't return the created entity // This is valid OData behavior for changeset operations - // We return a success indicator but no actual data - return { - data: {} as any, - error: undefined, - }; + // Return an error since the caller expected a full record + return { + data: undefined, + error: new Error( + "Server returned 204 No Content but return=representation was requested. " + + "This may occur in batch/changeset operations where FileMaker uses minimal returns." + ), + };packages/fmodata/src/client/update-builder.ts-392-407 (1)
392-407: Dead code: input validation result is unused in processResponse.Same issue as in
InsertBuilder- thevalidatedDatavariable is computed but never used. Input validation inprocessResponse(which processes the server's response) is unnecessary.Suggested fix - remove unused validation
- // Validate and transform input data using input validators (writeValidators) - // This is needed for processResponse because it's called from batch operations - // where the data hasn't been validated yet - let validatedData = this.data; - if (this.table) { - const baseTableConfig = getBaseTableConfig(this.table); - const inputSchema = baseTableConfig.inputSchema; - try { - validatedData = await validateAndTransformInput(this.data, inputSchema); - } catch (error) { - return { - data: undefined, - error: error instanceof Error ? error : new Error(String(error)), - } as any; - } - } - // Handle based on return preferencepackages/fmodata/src/errors.ts-136-139 (1)
136-139: Redundant variable assignment inRecordCountMismatchError.
expectedStris assignedexpectedregardless of the typeof check, making the conditional pointless.🔎 Proposed fix
constructor(expected: number | "one" | "at-most-one", received: number) { - const expectedStr = typeof expected === "number" ? expected : expected; - super(`Expected ${expectedStr} record(s), but received ${received}`); + super(`Expected ${expected} record(s), but received ${received}`); this.expected = expected; this.received = received; }
🧹 Nitpick comments (55)
packages/fmodata/src/client/sanitize-json.ts (1)
56-65: Consider including original text for debugging.The
rawTextfield contains the sanitized text, not the original response. For debugging purposes, having access to the original (pre-sanitization) text could be helpful to see what characters caused the issue. This is a minor consideration.🔎 Optional: include both original and sanitized text
export async function safeJsonParse<T = unknown>( response: Response, ): Promise<T> { const text = await response.text(); const sanitized = sanitizeFileMakerJson(text); try { return JSON.parse(sanitized) as T; } catch (err) { throw new ResponseParseError( response.url, `Failed to parse response as JSON: ${err instanceof Error ? err.message : "Unknown error"}`, { - rawText: sanitized, + rawText: text, // Original text for debugging cause: err instanceof Error ? err : undefined, }, ); } }packages/fmodata/scripts/publish-alpha.ts (1)
129-132: Silently returninghasChanges: falseon error may mask issues.If
git statuscommands fail, the function returnshasChanges: false, which could lead to the script proceeding when it shouldn't (e.g., attempting to republish identical code). Consider propagating the error or returning a more explicit "unknown" state.packages/fmodata/scripts/dreams.ts (1)
4-18: Unused imports:dateField,timeField,containerField,calcField,gt.These imports are declared but never used in the file. If this is intentional for documentation purposes, consider adding a comment. Otherwise, remove them to keep the example clean.
packages/fmodata/src/client/builders/table-utils.ts (1)
44-52: Type casting could be slightly improved.The cast to
anyon line 50 works but loses type safety. Consider using a type guard or narrowing the type instead.🔎 Minor improvement
export function mergeEntityIdOptions<T extends Record<string, any>>( options: T | undefined, databaseDefault: boolean, ): T & { useEntityIds?: boolean } { + const useEntityIds = options && 'useEntityIds' in options + ? (options.useEntityIds as boolean | undefined) + : undefined; return { ...options, - useEntityIds: (options as any)?.useEntityIds ?? databaseDefault, + useEntityIds: useEntityIds ?? databaseDefault, } as T & { useEntityIds?: boolean }; }packages/fmodata/src/client/query/expand-builder.ts (3)
81-89: Extracting filter via regex from odata-query output is indirect.Building a full query string with
buildQueryand then extracting the filter portion via regex is fragile. Ifodata-querychanges its output format, this could break silently.Consider directly using the filter value if
odata-queryprovides a lower-level API, or document this as a known limitation.
158-161: TODO: Nested expand validation is not implemented.This TODO indicates that nested expand validation (
nestedExpands) is not yet handled. If nested expands are supported at the query level, validation gaps could lead to runtime errors or incorrect data parsing.Would you like me to help implement nested expand validation, or should I open an issue to track this?
132-145: Deep property access via symbols is fragile.Accessing
(targetTable as any)[FMTable.Symbol.Schema]and then navigating through["~standard"]?.schema?.shaperelies on internal implementation details of the schema library (likely Zod). This could break if the schema library's internals change.Consider adding a utility function in the table module to safely extract the schema shape, providing a single point of maintenance.
packages/fmodata/src/orm/column.ts (1)
93-106: Factory function marked as@internalbut exported.The
createColumnfunction is documented as@internalbut is publicly exported. If it's truly internal, consider either:
- Not exporting it from the package's public API
- Removing the
@internaltag if external usage is expectedpackages/fmodata/src/client/query/response-processor.ts (1)
68-68: TODO for nested expands support.The comment indicates nested expand validation is not yet implemented. If nested expands are supported in other parts of the query builder, this could lead to unvalidated nested data.
Do you want me to help implement nested expand validation, or should this be tracked as a separate issue?
packages/fmodata/src/validation.ts (1)
178-274: Significant code duplication in expand validation logic.The expand validation logic (lines 178-274) is nearly identical to lines 328-423. This duplication (~90 lines) makes the code harder to maintain and increases the risk of divergent behavior if one block is updated without the other.
Consider extracting the common expand validation logic into a helper function.
🔎 Proposed refactor to extract expand validation
+async function validateExpandedRelations( + rest: any, + expandConfigs: ExpandValidationConfig[], + validatedRecord: Record<string, any>, +): Promise<{ valid: true } | { valid: false; error: ValidationError }> { + for (const expandConfig of expandConfigs) { + const expandValue = rest[expandConfig.relation]; + + if (expandValue === undefined) { + // Check for inline error array + if (Array.isArray(rest.error) && rest.error.length > 0) { + const errorDetail = rest.error[0]?.error; + if (errorDetail?.message) { + const errorMessage = errorDetail.message; + const isRelatedToExpand = + errorMessage.toLowerCase().includes(expandConfig.relation.toLowerCase()) || + (expandConfig.selectedFields?.some((field) => + errorMessage.toLowerCase().includes(field.toLowerCase()) + )); + + if (isRelatedToExpand) { + return { + valid: false, + error: new ValidationError( + `Validation failed for expanded relation '${expandConfig.relation}': ${errorMessage}`, + [], + { field: expandConfig.relation }, + ), + }; + } + } + } + } else if (Array.isArray(expandValue)) { + const validatedItems: any[] = []; + for (let i = 0; i < expandValue.length; i++) { + const itemValidation = await validateRecord( + expandValue[i], + expandConfig.targetSchema, + expandConfig.selectedFields as string[] | undefined, + expandConfig.nestedExpands, + ); + if (!itemValidation.valid) { + return { + valid: false, + error: new ValidationError( + `Validation failed for expanded relation '${expandConfig.relation}' at index ${i}: ${itemValidation.error.message}`, + itemValidation.error.issues, + { field: expandConfig.relation, cause: itemValidation.error.cause }, + ), + }; + } + validatedItems.push(itemValidation.data); + } + validatedRecord[expandConfig.relation] = validatedItems; + } else { + const itemValidation = await validateRecord( + expandValue, + expandConfig.targetSchema, + expandConfig.selectedFields as string[] | undefined, + expandConfig.nestedExpands, + ); + if (!itemValidation.valid) { + return { + valid: false, + error: new ValidationError( + `Validation failed for expanded relation '${expandConfig.relation}': ${itemValidation.error.message}`, + itemValidation.error.issues, + { field: expandConfig.relation, cause: itemValidation.error.cause }, + ), + }; + } + validatedRecord[expandConfig.relation] = itemValidation.data; + } + } + return { valid: true }; +}Then replace both duplicated blocks with:
if (expandConfigs && expandConfigs.length > 0) { const expandResult = await validateExpandedRelations(rest, expandConfigs, validatedRecord); if (!expandResult.valid) { return expandResult; } }packages/fmodata/tests/batch-error-messages.test.ts (1)
141-144: Consider removing or conditionalizing console.log statements.The console.log statements are useful for demonstration but may clutter test output in CI/CD pipelines. Consider removing them or wrapping them in a conditional based on a verbose flag.
🔎 Optional cleanup
- console.log("\n✅ Fixed Error Message:"); - console.log(` Code: ${r2.error.code}`); - console.log(` Message: ${r2.error.message}`); - console.log(` Kind: ${r2.error.kind}\n`);Alternatively, if these logs are valuable for debugging, consider using a test utility or only logging when a specific environment variable is set.
packages/fmodata/src/client/builders/index.ts (1)
1-11: LGTM!Clean barrel file for consolidating builder utilities. Wildcard re-exports are appropriate for internal modules.
Minor: Line 11 has an extra blank line that could be removed for consistency, though this is purely cosmetic.
packages/fmodata/scripts/download-metadata.ts (2)
46-46: Stale comment:__dirnameis already defined above.This comment is orphaned since the
__dirnameequivalent is defined at lines 25-26. Consider removing this stale comment.🔎 Proposed fix
-// Get __dirname equivalent in ES modules - async function downloadMetadata(): Promise<void> {
89-102: Redundant error handling paths.The
try-catchblock insidedownloadMetadata(lines 89-95) handles errors and exits, but there's also a.catch()at lines 99-102 on the promise. If an error occurs in lines 71-88, it will be caught andprocess.exit(1)called. The outer.catch()would only catch errors from lines 48-55 (before the try block). This is fine but consider consolidating to a single error handling strategy for clarity.packages/fmodata/scripts/test-batch.ts (1)
22-39: Consider using validated constants to avoid non-null assertions.After validation, you still need non-null assertions (
serverUrl!) at lines 53 and 141. Consider restructuring to use validated constants:🔎 Proposed refactor
-const serverUrl = process.env.FMODATA_SERVER_URL; -const username = process.env.FMODATA_USERNAME; -const password = process.env.FMODATA_PASSWORD; -const database = process.env.FMODATA_DATABASE; - -if (!serverUrl) { - throw new Error("FMODATA_SERVER_URL environment variable is required"); -} - -if (!username || !password) { - throw new Error( - "FMODATA_USERNAME and FMODATA_PASSWORD environment variables are required", - ); -} - -if (!database) { - throw new Error("FMODATA_DATABASE environment variable is required"); -} +function getRequiredEnv(name: string): string { + const value = process.env[name]; + if (!value) { + throw new Error(`${name} environment variable is required`); + } + return value; +} + +const serverUrl = getRequiredEnv("FMODATA_SERVER_URL"); +const username = getRequiredEnv("FMODATA_USERNAME"); +const password = getRequiredEnv("FMODATA_PASSWORD"); +const database = getRequiredEnv("FMODATA_DATABASE");This eliminates the need for non-null assertions later in the code.
packages/fmodata/tests/delete.test.ts (1)
228-241: Consider typing the mock instead of usingas any.The
as anycast on line 236 bypasses type checking. Consider properly typing the mock to match the expected fetch handler signature.🔎 Proposed fix
- const mockFetch = vi.fn().mockRejectedValue(new Error("Network error")); + const mockFetch = vi.fn<Parameters<typeof fetch>, ReturnType<typeof fetch>>() + .mockRejectedValue(new Error("Network error")); // ... - .execute({ fetchHandler: mockFetch as any }); + .execute({ fetchHandler: mockFetch });Alternatively, if the fetch handler has a specific type, use that type for the mock.
packages/fmodata/tests/expands.test.ts (2)
1-8: Stale documentation comment.The header states "DO NOT RUN THESE TESTS YET - they define the API we want to build" (line 7), but the tests appear complete with runtime assertions and mock fetch handlers. Consider updating or removing this comment to avoid confusion.
18-18: Shadowed imports create confusion.The
usersandcontactsimports from./utils/test-setup(line 18) are shadowed by local definitions (lines 38-50, 52-65). Since the tests use the locally-defined schemas, consider removing these unused imports to avoid confusion:🔎 Proposed fix
-import { createMockClient, users, contacts } from "./utils/test-setup"; +import { createMockClient } from "./utils/test-setup";Also applies to: 38-65
packages/fmodata/tests/filters.test.ts (2)
335-549: Consider splitting this large test for maintainability.This single test case spans over 200 lines and tests multiple concerns (type constraints for each operator + runtime query generation). Consider breaking it into smaller, focused tests:
- One test for type-level constraints per operator category
- One test for runtime validator application
This would improve readability and make failures easier to diagnose.
297-307: Weak assertion for URL encoding test.The test comment states "Special characters should be properly encoded" (line 305), but the assertion only checks that
queryStringis defined. Consider adding a more specific assertion to verify the encoding behavior:🔎 Suggested improvement
expect(queryString).toContain("$filter"); // Special characters should be properly encoded - expect(queryString).toBeDefined(); + // Verify that the value is properly quoted/escaped in the filter + expect(queryString).toContain("'John & Jane'");packages/fmodata/scripts/typegen-starter.ts (2)
144-162: ID field detection fallback may select an unintended field.When no explicit key, auto-generated, or id-containing field is found, the code falls back to
fieldNames[0](line 161), which selects an arbitrary field. This could lead to incorrect primary key assignment.Consider logging a warning when using this fallback, or requiring an explicit key in the metadata:
🔎 Suggested improvement
- idField = autoGenField || idFieldName || fieldNames[0]; + idField = autoGenField || idFieldName || fieldNames[0]; + if (!autoGenField && !idFieldName) { + console.warn(` ⚠ No suitable ID field found for ${entitySetName}, using first field: ${idField}`); + }
31-52: Consider adding explicit support forEdm.TimeOfDaytype.Edm.TimeOfDay is an OData V4 primitive type that currently falls through to the
textField()default. If FileMaker OData APIs expose time-of-day values, add an explicit case to map this to an appropriate field builder (likelytextField()or a dedicated time field). The current type mappings for string, numeric, boolean, date, and timestamp types appear reasonable, but verify the boolean mapping matches your field builder's expectations.packages/fmodata/scripts/capture-responses.ts (1)
520-531: Redundant environment variable validation.The environment variables are already validated at lines 52-62 with proper error throwing. This second validation in
main()is redundant since the script would have already exited if any were missing.🔎 Proposed fix
async function main() { console.log("Starting response capture...\n"); - if (!database) { - throw new Error("FMODATA_DATABASE environment variable is required"); - } - if (!serverUrl) { - throw new Error("FMODATA_SERVER_URL environment variable is required"); - } - if (!apiKey) { - throw new Error("FMODATA_API_KEY environment variable is required"); - } - // Create authenticated client with baseUrl and database configured const client = createAuthenticatedClient(serverUrl, database, apiKey);Note: You may need to add non-null assertions (
!) where these variables are used, or narrow their types at the top-level validation.packages/fmodata/src/client/builders/expand-builder.ts (1)
115-137: Unsafe casts rely on internal builder structure.The code uses
(configuredBuilder as any).queryOptionsand(configuredBuilder as any).expandConfigswhich couples this class to the internal structure of the builder. Consider defining an interface that builders must implement to make this contract explicit.🔎 Proposed fix
Define an interface for builders that can be used with
processExpand:interface ExpandableBuilder { queryOptions: Partial<QueryOptions<any>>; expandConfigs?: ExpandConfig[]; }Then update the method signature:
processExpand<TargetTable extends FMTable<any, any>, Builder extends ExpandableBuilder>( targetTable: TargetTable, sourceTable: FMTable<any, any> | undefined, - callback?: (builder: Builder) => Builder, + callback?: (builder: Builder) => ExpandableBuilder, builderFactory?: () => Builder, ): ExpandConfig {packages/fmodata/src/client/builders/select-mixin.ts (1)
63-66: Minor: Redundant empty object handling.The condition
Object.keys(fieldMapping).length > 0 ? fieldMapping : {}is redundant sincefieldMappingis already initialized as{}and would simply be returned as-is when empty.🔎 Proposed fix
return { selectedFields, - fieldMapping: Object.keys(fieldMapping).length > 0 ? fieldMapping : {}, + fieldMapping, };packages/fmodata/src/client/response-processor.ts (2)
1-7: Unused import:ExecuteOptions.The
ExecuteOptionstype is imported but never used in this file.🔎 Proposed fix
import type { StandardSchemaV1 } from "@standard-schema/spec"; import type { FMTable } from "../orm/table"; -import type { ExecuteOptions } from "../types"; import type { ExpandValidationConfig } from "../validation";
81-89: Type cast may include OData metadata in returned type.When the response is a single record,
response as Twill include OData metadata fields (@odata.context,@id,@editLink) in the returned array element, but the genericTdoesn't reflect this. Consider stripping metadata or adjusting the return type.🔎 Potential approaches
- Strip OData metadata before returning:
export function extractListValue<T>( response: ODataListResponse<T> | ODataRecordResponse<T>, ): T[] { if ("value" in response && Array.isArray(response.value)) { return response.value; } // Strip OData metadata from single record const { "@odata.context": _, "@id": __, "@editLink": ___, ...record } = response; return [record as T]; }
- Or document that returned records may contain OData metadata.
packages/fmodata/src/client/query/types.ts (1)
20-24: Potential duplicate type definition.
ExpandConfigis also defined inpackages/fmodata/src/client/builders/shared-types.ts(per AI summary) andpackages/fmodata/src/client/query/expand-builder.ts(per code snippets). Consider consolidating to a single source of truth to avoid drift.#!/bin/bash # Find all ExpandConfig type definitions rg -n "^export type ExpandConfig" packages/fmodata/src/packages/fmodata/scripts/experiment-batch.ts (2)
311-311: Remove unused variable.The
timestampvariable is declared but never used in this experiment.🔎 Proposed fix
async function experiment6_RawResponseInspection() { console.log("\n" + "=".repeat(60)); console.log("EXPERIMENT 6: Raw Response Inspection - Direct Fetch"); console.log("=".repeat(60)); // Make a direct batch request to see raw response - const timestamp = Date.now(); const boundary = "batch_direct_test_123";
486-551: Consider using a constant for the third changeset boundary.Lines 488-489 define
cs1andcs2as constants, but the third changeset uses a hardcoded string"changeset_3"at lines 537 and 539. For consistency, consider definingconst cs3 = "changeset_3".🔎 Proposed fix
const timestamp = Date.now(); const boundary = "batch_fail_test"; const cs1 = "changeset_1"; const cs2 = "changeset_2"; + const cs3 = "changeset_3"; // ... later in the batch body ... - `Content-Type: multipart/mixed; boundary=changeset_3`, + `Content-Type: multipart/mixed; boundary=${cs3}`, "", - `--changeset_3`, + `--${cs3}`, // ... - `--changeset_3--`, + `--${cs3}--`,packages/fmodata/tests/e2e.test.ts (2)
297-317: Consider properly typing the expand builder parameter.The expand callback uses
(b: any)which loses type safety. If the types are difficult to express, this may be acceptable, but consider if proper typing can be achieved.
654-667: Consider improving type handling for dynamic batch results.The double cast
as unknown as any[]at line 661 works around type inference limitations. This is acceptable given the dynamic nature ofaddRequest, but a comment explaining why would help future maintainers.packages/fmodata/src/client/query/url-builder.ts (1)
86-94: UnusedtableIdparameter inbuildEntitySetNavigation.The
tableIdparameter is passed to this method but never used in the function body. Consider removing it or documenting why it's kept for future use.🔎 Proposed fix
private buildEntitySetNavigation( queryString: string, - tableId: string, navigation: NavigationConfig, ): string { const { sourceTableName, basePath, relation } = navigation; const base = basePath || sourceTableName; return `/${this.databaseName}/${base}/${relation}${queryString}`; }And update the caller at line 58:
- return this.buildEntitySetNavigation(queryString, tableId, navigation); + return this.buildEntitySetNavigation(queryString, navigation);packages/fmodata/src/client/batch-builder.ts (1)
129-143: Stub implementation with non-standard error type.The
processResponsemethod returns a hardcoded error object that's cast toanyto bypass type checking. While this is documented as an internal method, consider using a proper error class (likeBatchErroror a newNotImplementedError) for consistency.packages/fmodata/src/client/builders/response-processor.ts (1)
54-67: Unreachable error check in fast path.At line 58, the check
if (result.error)is unreachable because line 57 already verifiedresult.datais truthy. Given theResult<T>union type, ifdataexists,errormust beundefined. Consider removing the redundant check or restructuring for clarity.🔎 Proposed fix
// Fast path: skip validation if (skipValidation) { const result = extractRecords(response, singleMode); - // Rename fields AFTER extraction (but before returning) - if (result.data && fieldMapping && Object.keys(fieldMapping).length > 0) { - if (result.error) { - return { data: undefined, error: result.error } as Result<T>; - } + if (result.error) { + return { data: undefined, error: result.error } as Result<T>; + } + // Rename fields AFTER extraction (but before returning) + if (result.data && fieldMapping && Object.keys(fieldMapping).length > 0) { return { data: renameFieldsInResponse(result.data, fieldMapping) as T, error: undefined, }; } return result as Result<T>; }packages/fmodata/REFACTORING_SUMMARY.md (1)
160-195: Add language identifier to the fenced code block.The ASCII architecture diagram code block lacks a language identifier. While it's not executable code, adding a language identifier (e.g.,
textorplaintext) improves markdown linting compliance.Suggested fix
-``` +```text Before: ┌─────────────────┐packages/fmodata/src/client/database.ts (1)
141-160: Variable shadowing:resultvariable is redefined.The variable
resultat line 146 shadows the outerresultfrom line 124. While this works, it reduces readability and could lead to confusion during maintenance.Suggested fix
// Handle both sync and async validation - const result = + const validationOutput = validationResult instanceof Promise ? await validationResult : validationResult; - if (result.issues) { + if (validationOutput.issues) { throw new Error( - `Script result validation failed: ${JSON.stringify(result.issues)}`, + `Script result validation failed: ${JSON.stringify(validationOutput.issues)}`, ); } return { resultCode: response.scriptResult.code, - result: result.value, + result: validationOutput.value, } as any;packages/fmodata/src/client/filemaker-odata.ts (2)
88-91: Duplicated base URL construction.The
baseUrlis computed inline at line 90, but_getBaseUrl()at line 70-72 does the same thing. Consider reusing the method to avoid duplication.Suggested fix
async _makeRequest<T>( url: string, options?: RequestInit & FFetchOptions & { useEntityIds?: boolean }, ): Promise<Result<T>> { const logger = this._getLogger(); - const baseUrl = `${this.serverUrl}${"apiKey" in this.auth ? `/otto` : ""}/fmi/odata/v4`; + const baseUrl = this._getBaseUrl(); const fullUrl = baseUrl + url;
144-150: Consider logging JSON parse failures in error path.When parsing error response bodies fails, the error is silently swallowed. A debug-level log here would help troubleshoot malformed error responses from the server.
Suggested fix
try { if (resp.headers.get("content-type")?.includes("application/json")) { errorBody = await safeJsonParse<typeof errorBody>(resp); } - } catch { - // Ignore JSON parse errors + } catch (parseErr) { + logger.debug("Failed to parse error response body:", parseErr); }packages/fmodata/src/client/entity-set.ts (2)
66-69: Accessing private property via cast is fragile.The pattern
(config.database as any)?._useEntityIdsrelies on accessing a private property ofDatabase. IfDatabaserefactors this property, this code will silently break. Consider exposing a public getter onDatabasefor this value.Suggested approach
Add a public method to
Databaseclass:// In database.ts getUseEntityIds(): boolean { return this._useEntityIds; }Then update EntitySet:
// Get useEntityIds from database if available, otherwise default to false - this.databaseUseEntityIds = - (config.database as any)?._useEntityIds ?? false; + this.databaseUseEntityIds = + config.database?.getUseEntityIds?.() ?? false;
102-148: Consider extracting shared defaultSelect logic.The defaultSelect handling logic (lines 102-148) is largely duplicated in the
get()method (lines 187-252). While the return types differ, the schema extraction and defaultSelectValue checks could be extracted into a private helper to reduce duplication.packages/fmodata/tests/e2e/setup.ts (1)
19-24: Environment variables exported without validation.These variables could be
undefinedif the.env.localfile is missing or incomplete, leading to confusing test failures. Consider adding early validation or documenting the required environment variables.Suggested improvement
// Load environment variables -export const serverUrl = process.env.FMODATA_SERVER_URL; -export const apiKey = process.env.FMODATA_API_KEY; -export const username = process.env.FMODATA_USERNAME; -export const password = process.env.FMODATA_PASSWORD; -export const database = process.env.FMODATA_DATABASE; +export const serverUrl = process.env.FMODATA_SERVER_URL; +export const apiKey = process.env.FMODATA_API_KEY; +export const username = process.env.FMODATA_USERNAME; +export const password = process.env.FMODATA_PASSWORD; +export const database = process.env.FMODATA_DATABASE; + +// Validate required E2E env vars at import time +const requiredVars = { serverUrl, database }; +const missing = Object.entries(requiredVars) + .filter(([_, v]) => !v) + .map(([k]) => k); +if (missing.length > 0 && process.env.NODE_ENV !== 'test') { + console.warn(`E2E setup: Missing env vars: ${missing.join(', ')}`); +}packages/fmodata/src/client/query/query-builder.ts (2)
704-707: Dead code: 204 status is already handled before JSON parsing.Lines 685-697 return early for 204 No Content responses, so this catch block condition checking
response.status === 204will never be true.🔎 Proposed fix
} catch (err) { - // Check if it's an empty body error (common with 204 responses) - if (err instanceof SyntaxError && response.status === 204) { - // Handled above, but just in case - return { data: [] as any, error: undefined }; - } return { data: undefined, error: {
284-438:orderBy()also mutatesthisdirectly.For consistency with the immutable pattern used by
select(),single(),maybeSingle(), andcount(), consider refactoringorderBy()to usecloneWithChanges(). This method is particularly complex and would benefit from the clone pattern to avoid side effects.packages/fmodata/src/orm/operators.ts (1)
159-179: Silent fallback for async validators may mask issues.When an async validator is encountered or validation fails, the code silently falls back to the original value. While the comment explains the limitation, this could lead to subtle bugs where invalid values reach the server without any indication to the developer.
Consider logging a warning when async validators are encountered or validation fails.
🔎 Proposed enhancement
if (result instanceof Promise) { // For filters, we can't use async validators, so skip transformation // This is a limitation - async validators won't work in filters + console.warn?.( + `Async validators are not supported in filter expressions for column "${column.fieldName}"` + ); value = operand; } else if ("issues" in result && result.issues) { // Validation failed, use original value value = operand;packages/fmodata/src/index.ts (1)
23-28: Remove or document commented-out exports.These commented-out helper function exports create noise. If they're intentionally not exported, remove them. If they're planned for future export, add a TODO comment explaining when.
🔎 Proposed fix
// Table helper functions - // getTableFields, - // getDefaultSelect, - // getBaseTableConfig, - // getFieldId, - // getFieldName, - // getTableId, getTableColumns,packages/fmodata/src/client/record-builder.ts (2)
396-402: Navigation path validation only logs a warning, allowing invalid navigation.When
navigate()is called with an invalid target table, it only logs a warning but proceeds anyway. This could lead to runtime errors from the server that are harder to debug.Consider throwing an error or at least making the behavior configurable.
🔎 Proposed enhancement
if (this.table) { const navigationPaths = getNavigationPaths(this.table); if (navigationPaths && !navigationPaths.includes(relationName)) { - this.logger.warn( - `Cannot navigate to "${relationName}". Valid navigation paths: ${navigationPaths.length > 0 ? navigationPaths.join(", ") : "none"}`, - ); + const message = `Cannot navigate to "${relationName}". Valid navigation paths: ${navigationPaths.length > 0 ? navigationPaths.join(", ") : "none"}`; + this.logger.warn(message); + // Consider: throw new Error(message); } }
368-370: Redundant nullish coalescing:this.table ?? undefined
this.tableis always set in the constructor and typed asOcc, so the?? undefinedis unnecessary.🔎 Proposed fix
- targetTable, - this.table ?? undefined, + targetTable, + this.table,packages/fmodata/src/orm/table.ts (7)
4-4: Unused import:createColumnThe
createColumnfunction is imported but never used in this file. The code usesnew Column({...})directly instead.Proposed fix
-import { Column, createColumn } from "./column"; +import { Column } from "./column";
328-331: Consider caching field configs to avoid repeated_getConfig()calls.The
_getConfig()method is called multiple times for the same builders: once at line 330, and again at line 426 for each field. This creates unnecessary overhead and duplicated type assertions.Proposed optimization
// Extract configuration from field builders - const fieldConfigs = Object.entries(fields).map(([fieldName, builder]) => ({ - fieldName, - config: (builder as any)._getConfig(), - })); + const fieldConfigs = Object.entries(fields).map(([fieldName, builder]) => { + const config = (builder as any)._getConfig(); + return { fieldName, builder, config }; + });Then reuse
fieldConfigswhen creating columns (line 425-434) instead of calling_getConfig()again.
370-386: Type safety lost withanyvalidator.The
let validator: any;declaration loses type information. While this works at runtime, using a more specific type or Zod's type inference would improve maintainability.Proposed improvement
- let validator: any; + let validator: z.ZodType; switch (config.fieldType) {Note: This assumes all branches return compatible Zod types. If not, you may need a union type or keep
anybut add a comment explaining why.
465-489: Remove or document commented-out code.This commented-out
isTableOccurrencetype guard spans 25 lines. If it's planned for future implementation, add a TODO comment with context. Otherwise, remove it to reduce maintenance burden—version control preserves history if needed later.
494-506: Potential duplicate types:InferTableSchemaandInferSchemaOutputFromFMTable.These two types appear to serve the same purpose. Consider consolidating them to avoid confusion. If there's a semantic distinction (e.g.,
InferTableSchemafor internal use,InferSchemaOutputFromFMTablefor public API), document it.Option: Remove duplicate and alias
export type InferTableSchema<T> = T extends FMTable<infer TFields, any> ? InferSchemaFromFields<TFields> : never; -/** - * Extract the schema type from an FMTable instance. - * This is used to infer the schema from table objects passed to db.from(), expand(), etc. - */ -export type InferSchemaOutputFromFMTable<T extends FMTable<any, any>> = - T extends FMTable<infer TFields, any> - ? InferSchemaFromFields<TFields> - : never; +/** @deprecated Use InferTableSchema instead */ +export type InferSchemaOutputFromFMTable<T extends FMTable<any, any>> = InferTableSchema<T>;
746-766:getTableColumnsrecreates columns that already exist on the table.The
fmTableOccurrencefunction assigns columns directly to the table instance viaObject.assign(table, columns)at line 460. This function creates new Column instances instead of returning the existing ones. This is both redundant and potentially inconsistent if Column instances carry mutable state.Consider returning the existing columns from the table instance instead:
Proposed optimization
export function getTableColumns<T extends FMTable<any, any>>( table: T, ): ColumnMap<T[typeof FMTableFields], ExtractTableName<T>> { - const fields = table[FMTableFields]; - const tableName = table[FMTableName]; - const tableEntityId = table[FMTableEntityId]; - const baseConfig = table[FMTableBaseTableConfig]; - - const columns = {} as ColumnMap<T[typeof FMTableFields], ExtractTableName<T>>; - for (const [fieldName, builder] of Object.entries(fields)) { - const config = (builder as any)._getConfig(); - (columns as any)[fieldName] = new Column({ - fieldName: String(fieldName), - entityId: baseConfig.fmfIds?.[fieldName], - tableName: tableName, - tableEntityId: tableEntityId, - inputValidator: config.inputValidator, - }); - } - - return columns; + // Extract existing columns from the table instance + // They were assigned via Object.assign in fmTableOccurrence + const fields = table[FMTableFields]; + const columns = {} as ColumnMap<T[typeof FMTableFields], ExtractTableName<T>>; + for (const fieldName of Object.keys(fields)) { + (columns as any)[fieldName] = (table as any)[fieldName]; + } + return columns; }
711-724: O(n) reverse lookup ingetFieldNamecould be optimized.The function iterates over all
fmfIdsentries to find a matching field name. For tables with many fields called frequently, consider caching a reverse lookup map. This is a minor concern unless profiling shows it's a hot path.
| if (!hasContentLength) { | ||
| lines.push(`Content-Length: ${request.body.length}`); | ||
| } |
There was a problem hiding this comment.
Content-Length calculation uses character count instead of byte length.
request.body.length returns the number of JavaScript characters, not bytes. For multi-byte UTF-8 characters (e.g., emojis, non-ASCII text), this will produce an incorrect Content-Length header, potentially causing the server to truncate or misread the body.
Suggested fix using TextEncoder
if (!hasContentLength) {
- lines.push(`Content-Length: ${request.body.length}`);
+ lines.push(`Content-Length: ${new TextEncoder().encode(request.body).length}`);
}📝 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 (!hasContentLength) { | |
| lines.push(`Content-Length: ${request.body.length}`); | |
| } | |
| if (!hasContentLength) { | |
| lines.push(`Content-Length: ${new TextEncoder().encode(request.body).length}`); | |
| } |
🤖 Prompt for AI Agents
packages/fmodata/src/client/batch-request.ts around lines 116-118: the code uses
request.body.length (character count) for Content-Length which is wrong for
multi-byte UTF-8; replace it with a byte-length calculation: if body is a
string, compute byte length via TextEncoder().encode(request.body).length (or
Buffer.byteLength(request.body, 'utf8') in Node), if body is a Buffer/Uint8Array
use its byteLength/length, then push `Content-Length: ${byteLength}`; do not
mutate request.body and keep support for existing body types.
| @@ -0,0 +1,69 @@ | |||
| import type { FMTable } from "../../orm/table"; | |||
| import { FMTable as FMTableClass } from "../../orm/table"; | |||
| import type { StandardSchemaV1 } from "@standard-schema/spec"; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused import.
StandardSchemaV1 is imported but never used in this file.
-import type { StandardSchemaV1 } from "@standard-schema/spec";📝 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 type { StandardSchemaV1 } from "@standard-schema/spec"; |
🤖 Prompt for AI Agents
In packages/fmodata/src/client/builders/default-select.ts around line 3, the
file imports StandardSchemaV1 which is not used anywhere; remove the unused
import statement to clean up the code and avoid linter warnings (delete the
import line or remove StandardSchemaV1 from the import clause if other imports
remain).
| from<T extends FMTable<any, any>>(table: T): EntitySet<T> { | ||
| // Only override database-level useEntityIds if table explicitly sets it | ||
| // (not if it's undefined, which would override the database setting) | ||
| if ( | ||
| Object.prototype.hasOwnProperty.call(table, FMTable.Symbol.UseEntityIds) | ||
| ) { | ||
| const tableUseEntityIds = (table as any)[FMTable.Symbol.UseEntityIds]; | ||
| if (typeof tableUseEntityIds === "boolean") { | ||
| this._useEntityIds = tableUseEntityIds; | ||
| } | ||
| } | ||
| return new EntitySet<T>({ | ||
| occurrence: table as T, | ||
| databaseName: this.databaseName, | ||
| context: this.context, | ||
| database: this, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Side effect: from() mutates database-level _useEntityIds.
The from() method modifies this._useEntityIds based on the table's setting. This creates a side effect where calling from() with one table changes the behavior for subsequent calls with other tables. This is likely unintended and could cause subtle bugs.
Consider passing the resolved useEntityIds value directly to EntitySet without mutating the database instance.
Suggested fix
from<T extends FMTable<any, any>>(table: T): EntitySet<T> {
- // Only override database-level useEntityIds if table explicitly sets it
- // (not if it's undefined, which would override the database setting)
- if (
- Object.prototype.hasOwnProperty.call(table, FMTable.Symbol.UseEntityIds)
- ) {
- const tableUseEntityIds = (table as any)[FMTable.Symbol.UseEntityIds];
- if (typeof tableUseEntityIds === "boolean") {
- this._useEntityIds = tableUseEntityIds;
- }
- }
+ // Resolve useEntityIds: table-level overrides database-level
+ let resolvedUseEntityIds = this._useEntityIds;
+ if (
+ Object.prototype.hasOwnProperty.call(table, FMTable.Symbol.UseEntityIds)
+ ) {
+ const tableUseEntityIds = (table as any)[FMTable.Symbol.UseEntityIds];
+ if (typeof tableUseEntityIds === "boolean") {
+ resolvedUseEntityIds = tableUseEntityIds;
+ }
+ }
return new EntitySet<T>({
occurrence: table as T,
databaseName: this.databaseName,
context: this.context,
database: this,
+ // Pass resolved value to EntitySet
});
}Note: You'll also need to update the EntitySet constructor to accept the resolved useEntityIds value instead of reading it from the database instance.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/fmodata/src/client/database.ts around lines 29 to 46, from()
currently mutates the database instance by assigning this._useEntityIds from a
table-level setting, causing a side effect that affects subsequent from() calls;
instead compute the resolved useEntityIds locally and pass that value into the
new EntitySet without modifying this._useEntityIds. Update from() to determine
tableUseEntityIds (respecting the hasOwnProperty check and boolean type) into a
local const and forward it as an explicit option to the EntitySet constructor,
and update EntitySet to accept and use that option rather than reading
useEntityIds from the database instance.
| if (args?.format === "json") { | ||
| const data = result.data as Record<string, Metadata>; | ||
| const metadata = data[this.databaseName]; | ||
| if (!metadata) { | ||
| throw new Error( | ||
| `Metadata for database "${this.databaseName}" not found in response`, | ||
| ); | ||
| } | ||
| return metadata; | ||
| } | ||
| return result.data as string; | ||
| } |
There was a problem hiding this comment.
Logic issue: JSON metadata extraction skipped when format is undefined.
The condition args?.format === "json" at line 70 is false when args is undefined (the default case). This means the JSON extraction logic is bypassed for the default case, and result.data is returned as-is (likely the wrapped object, not the extracted metadata).
Suggested fix
- if (args?.format === "json") {
+ if (args?.format !== "xml") {
const data = result.data as Record<string, Metadata>;
const metadata = data[this.databaseName];
if (!metadata) {
throw new Error(
`Metadata for database "${this.databaseName}" not found in response`,
);
}
return metadata;
}
return result.data as string;📝 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 (args?.format === "json") { | |
| const data = result.data as Record<string, Metadata>; | |
| const metadata = data[this.databaseName]; | |
| if (!metadata) { | |
| throw new Error( | |
| `Metadata for database "${this.databaseName}" not found in response`, | |
| ); | |
| } | |
| return metadata; | |
| } | |
| return result.data as string; | |
| } | |
| if (args?.format !== "xml") { | |
| const data = result.data as Record<string, Metadata>; | |
| const metadata = data[this.databaseName]; | |
| if (!metadata) { | |
| throw new Error( | |
| `Metadata for database "${this.databaseName}" not found in response`, | |
| ); | |
| } | |
| return metadata; | |
| } | |
| return result.data as string; |
🤖 Prompt for AI Agents
In packages/fmodata/src/client/database.ts around lines 70 to 81, the current
guard only runs JSON metadata extraction when args?.format === "json", which
skips extraction when args is undefined (the default); change the condition to
treat the default as JSON by checking if args?.format is undefined or explicitly
"json" (e.g., if (args?.format === undefined || args.format === "json")), then
extract result.data as Record<string, Metadata>, look up this.databaseName,
throw the same error if missing, and return the metadata; otherwise fall back to
returning result.data as string.
| where( | ||
| expression: FilterExpression | string, | ||
| ): QueryBuilder<Occ, Selected, SingleMode, IsCount, Expands> { | ||
| // Handle raw string filters (escape hatch) | ||
| if (typeof expression === "string") { | ||
| this.queryOptions.filter = expression; | ||
| return this; | ||
| } | ||
| // Convert FilterExpression to OData filter string | ||
| const filterString = expression.toODataFilter(this.databaseUseEntityIds); | ||
| this.queryOptions.filter = filterString; | ||
| return this; | ||
| } |
There was a problem hiding this comment.
The where() method mutates this instead of returning a new instance, breaking immutability.
Unlike select(), single(), maybeSingle(), and count() which use cloneWithChanges() to return new builder instances, where() directly mutates this.queryOptions.filter. This inconsistency can lead to unexpected behavior when chaining or reusing builders.
🔎 Proposed fix to maintain immutability
where(
expression: FilterExpression | string,
): QueryBuilder<Occ, Selected, SingleMode, IsCount, Expands> {
// Handle raw string filters (escape hatch)
if (typeof expression === "string") {
- this.queryOptions.filter = expression;
- return this;
+ return this.cloneWithChanges({
+ queryOptions: { filter: expression },
+ });
}
// Convert FilterExpression to OData filter string
const filterString = expression.toODataFilter(this.databaseUseEntityIds);
- this.queryOptions.filter = filterString;
- return this;
+ return this.cloneWithChanges({
+ queryOptions: { filter: filterString },
+ });
}🤖 Prompt for AI Agents
In packages/fmodata/src/client/query/query-builder.ts around lines 246 to 258,
the where() method currently mutates this.queryOptions.filter instead of
returning a new QueryBuilder instance; change it to use cloneWithChanges() so it
returns an immutable clone with the updated filter. Specifically, when
expression is a string, call cloneWithChanges({ queryOptions: {
...this.queryOptions, filter: expression } }) and return that; when expression
is a FilterExpression, compute filterString =
expression.toODataFilter(this.databaseUseEntityIds) and return
cloneWithChanges({ queryOptions: { ...this.queryOptions, filter: filterString }
}); ensure no in-place mutation of this.queryOptions and preserve the same
generic return type.
| top( | ||
| count: number, | ||
| ): QueryBuilder<Occ, Selected, SingleMode, IsCount, Expands> { | ||
| this.queryOptions.top = count; | ||
| return this; | ||
| } | ||
|
|
||
| skip( | ||
| count: number, | ||
| ): QueryBuilder<Occ, Selected, SingleMode, IsCount, Expands> { | ||
| this.queryOptions.skip = count; | ||
| return this; | ||
| } |
There was a problem hiding this comment.
top() and skip() also mutate this instead of cloning.
These methods have the same immutability issue as where(). For a consistent fluent API, they should use cloneWithChanges().
🔎 Proposed fix
top(
count: number,
): QueryBuilder<Occ, Selected, SingleMode, IsCount, Expands> {
- this.queryOptions.top = count;
- return this;
+ return this.cloneWithChanges({
+ queryOptions: { top: count },
+ });
}
skip(
count: number,
): QueryBuilder<Occ, Selected, SingleMode, IsCount, Expands> {
- this.queryOptions.skip = count;
- return this;
+ return this.cloneWithChanges({
+ queryOptions: { skip: count },
+ });
}🤖 Prompt for AI Agents
In packages/fmodata/src/client/query/query-builder.ts around lines 441 to 453,
the top() and skip() methods currently mutate this.queryOptions directly; change
them to use cloneWithChanges() so the builder is immutable and the fluent API is
consistent. For each method, call this.cloneWithChanges({ queryOptions: {
...this.queryOptions, top: count } }) (and similarly for skip) and return that
result instead of mutating this and returning this.
| it("should list records with entity IDs", async () => { | ||
| let rawResponseData: any; | ||
|
|
||
| let capturedPreferHeader: string | null = null; | ||
| db.from(contactsTOWithIds) | ||
| .list() | ||
| .top(1) | ||
| .execute({ | ||
| hooks: { | ||
| before: async (req: any) => { | ||
| const headers = req.headers; | ||
| capturedPreferHeader = headers.get("Prefer"); | ||
| return; | ||
| }, | ||
| }, | ||
| fetchHandler: simpleMock({ status: 200, body: { value: [{}] } }), | ||
| }); | ||
| expect(capturedPreferHeader).toBe("fmodata.entity-ids"); | ||
|
|
There was a problem hiding this comment.
Missing await on execute call may cause test flakiness.
The execute() call at lines 442-454 is not awaited, so the assertion at line 455 may run before the before hook has set capturedPreferHeader. This could cause intermittent test failures.
🔎 Proposed fix
let capturedPreferHeader: string | null = null;
- db.from(contactsTOWithIds)
+ await db.from(contactsTOWithIds)
.list()
.top(1)
.execute({
hooks: {
before: async (req: any) => {
const headers = req.headers;
capturedPreferHeader = headers.get("Prefer");
return;
},
},
fetchHandler: simpleMock({ status: 200, body: { value: [{}] } }),
});
expect(capturedPreferHeader).toBe("fmodata.entity-ids");📝 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.
| it("should list records with entity IDs", async () => { | |
| let rawResponseData: any; | |
| let capturedPreferHeader: string | null = null; | |
| db.from(contactsTOWithIds) | |
| .list() | |
| .top(1) | |
| .execute({ | |
| hooks: { | |
| before: async (req: any) => { | |
| const headers = req.headers; | |
| capturedPreferHeader = headers.get("Prefer"); | |
| return; | |
| }, | |
| }, | |
| fetchHandler: simpleMock({ status: 200, body: { value: [{}] } }), | |
| }); | |
| expect(capturedPreferHeader).toBe("fmodata.entity-ids"); | |
| it("should list records with entity IDs", async () => { | |
| let rawResponseData: any; | |
| let capturedPreferHeader: string | null = null; | |
| await db.from(contactsTOWithIds) | |
| .list() | |
| .top(1) | |
| .execute({ | |
| hooks: { | |
| before: async (req: any) => { | |
| const headers = req.headers; | |
| capturedPreferHeader = headers.get("Prefer"); | |
| return; | |
| }, | |
| }, | |
| fetchHandler: simpleMock({ status: 200, body: { value: [{}] } }), | |
| }); | |
| expect(capturedPreferHeader).toBe("fmodata.entity-ids"); |
🤖 Prompt for AI Agents
In packages/fmodata/tests/e2e.test.ts around lines 438 to 456, the test calls
db.from(...).list().top(1).execute(...) without awaiting it, so the assertion
may run before the before-hook sets capturedPreferHeader; fix by awaiting the
execute call (or returning its promise) so the request hooks complete before the
expect, ensuring capturedPreferHeader is set before the assertion.
fmdata
update next
Update CLI version to 2.0.0-beta.6 and integrate @rollup/plugin-replace for build-time variable injection. Modify release script to use turbo for building packages.
Enhance orderBy functionality with type-safe API in fmodata
Note
Adds a full Vitest-based test setup with mocks/fixtures and build tooling for fmodata, including extensive unit/integration tests and configs.
tests/fixtures/responses.ts) and sample metadata/occurrences.vitest.config.ts, packagetsconfig.jsonand test tsconfigs.vite.config.ts) and package build settings.Written by Cursor Bugbot for commit cc2baa6. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.