orm #77
orm #77
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
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: |
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Deprecated Modules Removed src/client/base-table.ts, src/client/table-occurrence.ts, src/client/build-occurrences.ts, src/filter-types.ts |
Removed entire BaseTable class with schema/ID management, TableOccurrence with navigation support, buildOccurrences helper, and all StandardSchemaV1-based filter operator type definitions (StringOperators, NumberOperators, Filter, TypedFilter, etc.). |
New ORM Core—Field Builders & Types src/orm/field-builders.ts, src/orm/column.ts, src/orm/operators.ts, src/orm/table.ts, src/orm/index.ts |
Introduced FieldBuilder class with fluent API (primaryKey, notNull, readOnly, entityId, readValidator, writeValidator); textField, numberField, dateField, timeField, timestampField, containerField, calcField factory functions; Column class for type-safe field references with isColumn guard; FilterExpression and OrderByExpression classes with operator factories (eq, ne, gt, gte, lt, lte, contains, startsWith, endsWith, inArray, notInArray, isNull, isNotNull, and, or, not, asc, desc); fmTableOccurrence factory for building FMTable instances; comprehensive table accessors (getTableName, getTableEntityId, getDefaultSelect, getBaseTableConfig, etc.) and type utilities (InferTableSchema, InferSchemaOutputFromFMTable, InsertDataFromFMTable, UpdateDataFromFMTable). |
Refactored Client Builders—Query/Record src/client/query-builder.ts, src/client/record-builder.ts, src/client/insert-builder.ts, src/client/update-builder.ts, src/client/delete-builder.ts |
Replaced TableOccurrence generics with FMTable; updated all builder signatures to use Occ extends FMTable; replaced tableName/occurrence state with table property; reparametrized return types via InferSchemaOutputFromFMTable; integrated new ExecuteMethodOptions type; updated all internal references to use FMTable helpers (getTableName, getTableId, isUsingEntityIds, getBaseTableConfig). |
New Modular Query Infrastructure src/client/query/query-builder.ts, src/client/query/expand-builder.ts, src/client/query/response-processor.ts, src/client/query/url-builder.ts, src/client/query/types.ts, src/client/query/index.ts |
Extracted QueryBuilder into modular structure with dedicated ExpandBuilder for $expand string generation and validation configs; added response-processor for handling transformations and validation; introduced QueryUrlBuilder for OData URL construction; added TypeSafeOrderBy, ExpandConfig, ExpandedRelations, QueryReturnType types; ExpandBuilder supports entity-ID aware relation name resolution and nested expands. |
New Builder Utilities src/client/builders/shared-types.ts, src/client/builders/table-utils.ts, src/client/builders/select-utils.ts, src/client/builders/select-mixin.ts, src/client/builders/expand-builder.ts, src/client/builders/response-processor.ts, src/client/builders/default-select.ts, src/client/builders/query-string-builder.ts, src/client/builders/index.ts |
Introduced shared types (ExpandConfig, ExpandedRelations, NavigationContext, BuilderConfig); table utilities for ID resolution and option merging (resolveTableId, mergeEntityIdOptions, mergeExecuteOptions, createODataRequest); select utilities for field quoting and formatting (needsFieldQuoting, formatSelectFields); select-mixin for processing field renames with Column support; ExpandBuilder for expand construction; response-processor for OData response handling with transformations and validation; default-select helpers; query-string-builder to compose $select and $expand. |
Transform Layer Updates src/transform.ts, src/client/response-processor.ts, src/validation.ts |
Updated all transform function signatures from BaseTable/TableOccurrence to FMTable (transformFieldNamesToIds, transformFieldIdsToNames, transformFieldName, transformTableName, transformResponseFields, transformOrderByField, etc.); replaced applyFieldTransformation to accept FMTable; updated ExpandValidationConfig to use targetTable/table (FMTable) instead of occurrence/baseTable; added validateAndTransformInput function for per-field input validation using inputSchema. |
Database & Entity Set Refactoring src/client/database.ts, src/client/entity-set.ts |
Simplified Database class: removed generic Occurrences parameter, removed occurrenceMap tracking, replaced from overloads with single signature from(table: T): EntitySet supporting per-table useEntityIds override; reparametrized EntitySet to Occ extends FMTable; replaced static occurrence-based methods with FMTable-aware factories; added runtime defaultSelect/navigationPath handling via FMTable metadata; updated list/get/insert/update/delete signatures to use FMTable-derived types; added navigation validation against navigationPaths. |
Infrastructure & Types src/logger.ts, src/logger.test.ts, src/types.ts, src/filemaker-odata.ts |
Added new logger module with LogLevel, TTY_COLORS, shouldPublishLog, Logger interface, createLogger factory, InternalLogger type; added logger.test.ts with Vitest tests; updated ExecutionContext with optional _getLogger getter; added ExecuteMethodOptions and FetchHandler types; removed InsertData/UpdateData types; simplified FMServerConnection.database signature to remove Occurrences generic and add logger support with request/response logging. |
Index & Public API src/index.ts, src/orm/index.ts |
Removed exports for defineBaseTable, defineTableOccurrence, buildOccurrences, BaseTable, TableOccurrence, and filter-types; added comprehensive ORM exports (field builders, Column, operators, fmTableOccurrence, table helpers, type aliases); kept Database, EntitySet, error types; added FFetchOptions and Logger type exports. |
Documentation & Examples docs/ORM_API.md, README.md, scripts/dreams.ts, scripts/experiment-batch.ts, scripts/typegen-starter.ts, scripts/download-metadata.ts, scripts/capture-responses.ts, package.json |
Added comprehensive ORM_API.md guide with examples, migrations, best practices; updated README.md to reflect new fmTableOccurrence API with field builders and examples; added dreams.ts demo script showcasing ORM usage (users/contacts tables); updated experiment-batch.ts, publish-alpha.ts, typegen-starter.ts scripts to use new API; added download-metadata.ts script for OData metadata retrieval; updated package.json version to 0.1.0-alpha.19 and added build/test scripts and fast-xml-parser dependency. |
Test Suite Refactoring tests/*.test.ts, tests/e2e/* |
Updated all test files (batch, batch-error-messages, delete, expands, e2e, field-id-transforms, filters, errors) and setup fixtures (tests/e2e/setup.ts) to use fmTableOccurrence with field builders instead of defineBaseTable/defineTableOccurrence; replaced string table references with typed table objects; replaced filter(...) with where(...) and operator builders (eq, and, or, etc.); added type assertions via expectTypeOf; removed occurrences array from database initialization; updated test assertions to reflect new API semantics. |
Removed Plans & Documentation .cursor/plans/static-registry.md, packages/fmodata/BATCH_ERROR_FIX.md, packages/fmodata/IMPLEMENTATION_SUMMARY.md |
Deleted static-registry.md planning file; deleted BATCH_ERROR_FIX.md documenting batch error handling solution; deleted IMPLEMENTATION_SUMMARY.md detailing ORM API implementation (replaced by ORM_API.md). |
Estimated code review effort
🎯 5 (Critical) | ⏱️ ~120 minutes
Areas requiring extra attention:
- New ORM type system (src/orm/*): Complex generic type utilities and phantom types in field-builders.ts, column.ts, operators.ts, and table.ts with intricate type inference (InferTableSchema, InferSchemaOutputFromFMTable, InsertDataFromFMTable). Review conditional types, mapped types, and generic constraint interactions.
- QueryBuilder rewrite (src/client/query/*): Major refactoring of query construction logic; verify select, where, orderBy, expand implementations; validate OData string generation; check filter expression serialization and operator overloads.
- Builder refactoring (insert/update/delete-builder.ts, record-builder.ts): All builders reparametrized to FMTable; verify data transformation paths, response processing pipelines, and return type alignment; check integration with new ExecuteMethodOptions.
- Database & EntitySet refactoring: Simplified Database class with per-table useEntityIds override; verify from() method behavior and EntitySet factory logic; check navigation/expand validation against navigationPaths at runtime.
- Transform layer consistency: Verify all transform functions correctly delegate to FMTable helpers; check ExpandValidationConfig migration from TableOccurrence to FMTable; ensure validateAndTransformInput integration with builders.
- Test coverage alignment: All test files refactored; verify assertions match new API behavior; check entity-ID handling in field-id-transforms.test.ts; validate nested expand scenarios in expands.test.ts.
- Type inference correctness: Verify that type-safe select, where clauses, and return types flow correctly through QueryBuilder/RecordBuilder generics; check MapSelectToReturnType and ExtractColumnType utility functions.
Possibly related PRs
- validate multiple portals #52: Both PRs extend typegen/fmdapi portal schema handling with new generic support and portal-inference types, sharing common schema/type generation patterns.
- custom registry base #59: Directly related; that PR deletes the .cursor/plans/static-registry.md file that this PR also removes, indicating coordinated planning document cleanup.
Pre-merge checks and finishing touches
❌ Failed checks (1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Title check | ❓ Inconclusive | The title 'orm' is vague and lacks specificity. It does not clearly convey the scope or primary change of this substantial refactor introducing a new ORM API. | Consider using a more descriptive title such as 'Introduce Drizzle-inspired ORM API with typed table definitions and field builders' to clearly communicate the main change to reviewers. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 81.82% which is sufficient. The required threshold is 80.00%. |
✨ 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
12-07-orm
Comment @coderabbitai help to get the list of available commands and usage tips.
![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 (16)
packages/fmodata/package.json-31-32 (1)
31-32: Add a build step totest:watch:buildor document the expected workflow.The
test:watch:buildscript setsTEST_BUILD=true(which tests against built artifacts indist/esm) but doesn't include the rebuild step thattest:buildhas. This means source changes won't trigger a rebuild, causing tests to run against stale artifacts. Either addbuild:watchto the script, run it separately in parallel, or document the expected development workflow.packages/fmodata/README.md-95-95 (1)
95-95: Fix spelling: "occurances" → "occurrences".The word "occurrences" is misspelled twice in this line. This appears in user-facing documentation.
🔎 Proposed 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.packages/fmodata/scripts/download-metadata.ts-6-8 (1)
6-8: Documentation says JSON but output is XML.The doc comment states "saves it to a JSON file" but the script actually saves XML format (line 72:
format: "xml", line 40:metadata.xml).- * This script downloads OData metadata from a FileMaker server and saves it - * to a JSON file. The metadata can then be used with typegen-starter.ts to + * This script downloads OData metadata from a FileMaker server and saves it + * to an XML file. The metadata can then be used with typegen-starter.ts topackages/fmodata/scripts/download-metadata.ts-61-62 (1)
61-62: Comment/code mismatch on timeout value.The comment says "10 seconds" but the timeout is set to 15000ms (15 seconds).
timeout: 15000, // 10 seconds + timeout: 15000, // 15 secondspackages/fmodata/docs/ORM_API.md-212-224 (1)
212-224: Missing.list()in orderBy examples.OrderBy examples also need
.list()for consistency.packages/fmodata/docs/ORM_API.md-84-95 (1)
84-95: Missing.list()in select examples.Based on
dreams.ts(line 94:db.from(users).list().select(users.id)), the.list()method appears to be required before.select(). The documentation examples omit this step.🔎 Proposed fix
// Option 1: Typed strings (original style) -db.from(users).select("id", "name", "email"); +db.from(users).list().select("id", "name", "email"); // Option 2: Column references (new capability) -db.from(users).select(users.id, users.name, users.email); +db.from(users).list().select(users.id, users.name, users.email); // Option 3: Mix both styles -db.from(users).select(users.id, "name", users.email); +db.from(users).list().select(users.id, "name", users.email);packages/fmodata/docs/ORM_API.md-101-117 (1)
101-117: Missing.list()in filter examples.Same issue - filter examples should include
.list()before.where()for consistency with the actual API.🔎 Proposed fix (representative)
// Equal -db.from(users).where(eq(users.status, "active")); +db.from(users).list().where(eq(users.status, "active"));Apply similar changes to all filter examples (lines 105-196).
packages/fmodata/docs/ORM_API.md-344-414 (1)
344-414: Complete example needs.list()method.The complete example at lines 394-409 should include
.list()before.select()to match the actual API.🔎 Proposed fix
const result = await db .from(users) + .list() .select(users.id, users.name, users.email)packages/fmodata/scripts/dreams.ts-54-58 (1)
54-58: Missing import forFMTableBaseTableConfig.Line 58 references
FMTableBaseTableConfigin the@ts-expect-errorblock, but this symbol is not imported. The test will fail to compile or behave unexpectedly.🔎 Proposed fix
import { fmTableOccurrence, textField, numberField, dateField, timeField, timestampField, containerField, calcField, eq, gt, and, or, contains, } from "../src/orm"; +import { FMTable } from "../src/orm/table"; + +// Access the symbol for testing +const { BaseTableConfig: FMTableBaseTableConfig } = FMTable.Symbol;Alternatively, import from the table module directly if the symbol is exported.
Committable suggestion skipped: line range outside the PR's diff.
packages/fmodata/src/client/builders/response-processor.ts-252-273 (1)
252-273: Field mapping not passed toprocessODataResponse, resulting in double-renaming risk or missed renaming.The
fieldMappingis extracted from config but not passed toprocessODataResponseat line 253-261. Then it's applied separately at lines 264-273. This means:
- In the validation path inside
processODataResponse, field renaming won't occur (sincefieldMappingisn't passed)- The renaming at lines 264-273 will happen regardless
This could cause issues if the validation path in
processODataResponseis later updated to expect fieldMapping.🔎 Suggested fix - pass fieldMapping to processODataResponse
// Process the response first let processedResponse = await processODataResponse(response, { table: occurrence, schema: getSchemaFromTable(occurrence), singleMode, selectedFields, expandValidationConfigs, skipValidation, useEntityIds, + fieldMapping, }); - // Rename fields if field mapping is provided (for renamed fields in select) - if ( - processedResponse.data && - fieldMapping && - Object.keys(fieldMapping).length > 0 - ) { - processedResponse = { - ...processedResponse, - data: renameFieldsInResponse(processedResponse.data, fieldMapping), - }; - } - return processedResponse;Committable suggestion skipped: line range outside the PR's diff.
packages/fmodata/tests/delete.test.ts-169-176 (1)
169-176: Test appears incomplete - no assertions.This test creates query builders but doesn't assert anything. The test name suggests it should verify the return type, but there are no
expectorexpectTypeOfcalls.🔎 Suggested 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))); + + // Type check: result should have deletedCount + expectTypeOf(deleteBuilder.execute).returns.resolves.toMatchTypeOf< + { data?: { deletedCount: number }; error?: Error } + >(); });packages/fmodata/src/client/query/query-builder.ts-441-453 (1)
441-453:top()andskip()also mutatethisdirectly.These methods follow the same pattern of mutating
thisand returning it. For a consistent immutable builder pattern, all chainable methods should usecloneWithChanges().packages/fmodata/src/client/insert-builder.ts-392-407 (1)
392-407: UnusedvalidatedDatavariable inprocessResponse.The
validatedDatavariable is computed viavalidateAndTransformInputbut is never used afterward. The comment suggests this is for batch operations, but the validated data isn't applied to anything. This appears to be dead code or an incomplete implementation.If the intent is to validate that the input was valid (for error reporting), then the variable is correctly unused. But if it was meant to influence response processing, this is a bug.
🔎 If validation is only for error checking, add a comment
if (this.table) { const baseTableConfig = getBaseTableConfig(this.table); const inputSchema = baseTableConfig.inputSchema; try { - validatedData = await validateAndTransformInput(this.data, inputSchema); + // Validate input data to catch errors early in batch operations + // The actual transformation was applied when the request was built + await validateAndTransformInput(this.data, inputSchema); } catch (error) {packages/fmodata/scripts/typegen-starter.ts-93-107 (1)
93-107: Minor edge case inextractEntityTypeNameFromType.When
partsarray is empty after split (edge case of empty string), the function returns an empty string instead ofnull, which is inconsistent with the Collection path.Suggested fix
// Try without Collection wrapper - extract last part after last dot const parts = typeString.split("."); - return parts.length > 0 ? parts[parts.length - 1] : null; + return parts.length > 0 ? (parts[parts.length - 1] || null) : null;packages/fmodata/scripts/typegen-starter.ts-354-358 (1)
354-358: Boolean annotation parsing is inconsistent.The code checks
ann["@_Bool"] === "true"but the parser is configured withparseAttributeValue: true, which should convert"true"to booleantrue. This may cause the check to fail.Suggested fix
- } else if (term === "com.filemaker.odata.Calculation") { - isCalculation = ann["@_Bool"] === "true" || ann.Bool === "true"; - } else if (term === "com.filemaker.odata.Global") { - isGlobal = ann["@_Bool"] === "true" || ann.Bool === "true"; - } else if (term === "com.filemaker.odata.AutoGenerated") { - isAutoGenerated = ann["@_Bool"] === "true" || ann.Bool === "true"; - } else if (term === "com.filemaker.odata.Index") { - hasIndex = ann["@_Bool"] === "true" || ann.Bool === "true"; - } else if (term === "com.filemaker.odata.VersionID") { - isVersionId = ann["@_Bool"] === "true" || ann.Bool === "true"; + } else if (term === "com.filemaker.odata.Calculation") { + isCalculation = ann["@_Bool"] === "true" || ann["@_Bool"] === true || ann.Bool === "true" || ann.Bool === true; + } else if (term === "com.filemaker.odata.Global") { + isGlobal = ann["@_Bool"] === "true" || ann["@_Bool"] === true || ann.Bool === "true" || ann.Bool === true; + } else if (term === "com.filemaker.odata.AutoGenerated") { + isAutoGenerated = ann["@_Bool"] === "true" || ann["@_Bool"] === true || ann.Bool === "true" || ann.Bool === true; + } else if (term === "com.filemaker.odata.Index") { + hasIndex = ann["@_Bool"] === "true" || ann["@_Bool"] === true || ann.Bool === "true" || ann.Bool === true; + } else if (term === "com.filemaker.odata.VersionID") { + isVersionId = ann["@_Bool"] === "true" || ann["@_Bool"] === true || ann.Bool === "true" || ann.Bool === true;Committable suggestion skipped: line range outside the PR's diff.
packages/fmodata/src/client/update-builder.ts-392-407 (1)
392-407: The validation logic inprocessResponseis unused dead code.Lines 395-400 validate
this.dataintovalidatedData, but the variable is never referenced afterward. The method returnsrawResponseinstead (the API's response), making the validation pointless. Additionally, this validation duplicates the logic inexecute(lines 202-208), wherevalidatedDatais actually used for field transformation before the request is sent.For batch operations, the issue is worse: by the time
processResponsevalidates the data, the API request has already been made with unvalidated input, so the validation cannot affect the request. If validation is required in batch scenarios, it should happen earlier in the pipeline, not after receiving the response.
🧹 Nitpick comments (46)
packages/fmodata/package.json (1)
36-36: Mixed package managers:bunin pnpm project.The
pub:alphascript usesbun runwhile the rest of the project usespnpm. This requires developers to have both runtimes installed and may introduce subtle behavioral differences.If this is intentional (e.g., for performance), consider documenting it. Otherwise, consider standardizing on
pnpm:🔎 Proposed refactor
- "pub:alpha": "bun run scripts/publish-alpha.ts", + "pub:alpha": "pnpm tsx scripts/publish-alpha.ts",packages/fmodata/src/client/builders/table-utils.ts (1)
44-52: Consider refining the type assertion.The type assertion on line 51 may bypass type checking. Consider using a more precise return type or conditional types to ensure type safety without relying on
as.💡 Potential improvement
export function mergeEntityIdOptions<T extends Record<string, any>>( options: T | undefined, databaseDefault: boolean, -): T & { useEntityIds?: boolean } { - return { - ...options, - useEntityIds: (options as any)?.useEntityIds ?? databaseDefault, - } as T & { useEntityIds?: boolean }; +): (T extends undefined ? {} : T) & { useEntityIds: boolean } { + return { + ...(options ?? {}), + useEntityIds: options?.useEntityIds ?? databaseDefault, + } as any; }Alternatively, if the type assertion is unavoidable given the dynamic spread, document why it's safe.
packages/fmodata/src/orm/field-builders.ts (1)
41-46: Consider adding explicit return types to_getConfig()method.The
_clone()method and fluent methods useas anycasts to work around TypeScript limitations with generic type narrowing. While this is a common pattern in fluent builder APIs, it would be safer to add an explicit return type to_getConfig()to ensure type stability at the boundary.The current implementation is acceptable for internal use (marked
@internal), but if this ever becomes part of a public contract, the implicit return type inference could cause breaking changes.🔎 Optional: Add explicit return type
- _getConfig() { + _getConfig(): { + fieldType: string; + primaryKey: boolean; + notNull: boolean; + readOnly: boolean; + entityId: `FMFID:${string}` | undefined; + outputValidator: StandardSchemaV1<any, TOutput> | undefined; + inputValidator: StandardSchemaV1<TInput, any> | undefined; + } { return { fieldType: this._fieldType, primaryKey: this._primaryKey,Also applies to: 52-61, 67-71, 77-83, 93-99, 109-115
packages/fmodata/src/client/builders/query-string-builder.ts (1)
5-5: Use type-only import forInternalLogger.
InternalLoggeris only used as a type annotation in the function signature, so it should use a type-only import to enable better tree-shaking and clarify intent.🔎 Proposed fix
-import { InternalLogger } from "../../logger"; +import type { InternalLogger } from "../../logger";packages/fmodata/scripts/publish-alpha.ts (1)
409-409: Minor: Trailing--in npm publish command.The trailing
--after--access publicis unusual. It's typically used to separate npm arguments from script arguments, but here it appears unnecessary.- execSync("npm publish --tag alpha --access public --", { + execSync("npm publish --tag alpha --access public", {packages/fmodata/src/client/builders/select-utils.ts (1)
41-41: Redundant array check or incorrect fallback.Given the type signature
string[] | readonly string[] | undefined, after the empty check on line 39,selectis always an array. TheArray.isArraycheck will always be true, making the[select]fallback unreachable. If the intent was to also accept a single string, the type signature should be updated.- const selectArray = Array.isArray(select) ? select : [select]; + const selectArray = [...select];Or if single-string input is desired:
export function formatSelectFields( - select: string[] | readonly string[] | undefined, + select: string | string[] | readonly string[] | undefined, table?: FMTable<any, any>, useEntityIds?: boolean, ): string {packages/fmodata/src/client/builders/default-select.ts (2)
3-3: Unused import.
StandardSchemaV1is imported but not used in this file.🔎 Proposed fix
import type { FMTable } from "../../orm/table"; import { FMTable as FMTableClass } from "../../orm/table"; -import type { StandardSchemaV1 } from "@standard-schema/spec"; import { getBaseTableConfig } from "../../orm/table"; import { isColumn } from "../../orm/column";
29-29: Consider usinggetBaseTableConfigconsistently.Line 29 accesses
DefaultSelectvia(table as any)[FMTableClass.Symbol.DefaultSelect], while line 33 usesgetBaseTableConfig(table). For consistency and to avoidanycasts, consider extendinggetBaseTableConfigto exposedefaultSelector creating a dedicated getter.packages/fmodata/tests/field-id-transforms.test.ts (1)
420-422: Consider stronger typing for expand callback.The expand callback uses
(b: any)which loses type safety. If the builder type is available, use it for better inference.- .expand(usersTOWithIds, (b: any) => - b.select({ id: usersTOWithIds.id, name: usersTOWithIds.name }), - ) + .expand(usersTOWithIds, (b) => + b.select({ id: usersTOWithIds.id, name: usersTOWithIds.name }), + )packages/fmodata/src/client/filemaker-odata.ts (1)
39-46: URL normalization has redundant trailing slash removal.Line 45 removes trailing slashes from
pathname, and line 46 removes trailing slashes from the entire URL string aftertoString(). The second removal (line 46) is redundant if the first one works correctly, but it serves as a safety net for edge cases where the URL might have a trailing slash after the protocol/host portion.Consider simplifying to just one approach for clarity:
🔎 Suggested simplification
// Remove any trailing slash from pathname url.pathname = url.pathname.replace(/\/+$/, ""); - this.serverUrl = url.toString().replace(/\/+$/, ""); + this.serverUrl = url.toString();packages/fmodata/tests/batch.test.ts (1)
9-9: Unused import:zfromzod/v4is not referenced.The
zimport is not used anywhere in this test file. Consider removing it to keep imports clean.🔎 Suggested fix
import { describe, it, expect } from "vitest"; -import { z } from "zod/v4"; import {packages/fmodata/tests/batch-error-messages.test.ts (1)
12-12: Unused import:zfromzod/v4is not referenced.Similar to
batch.test.ts, thezimport is not used in this file.🔎 Suggested fix
import { describe, it, expect } from "vitest"; -import { z } from "zod/v4"; import {packages/fmodata/src/client/query/response-processor.ts (1)
16-26:loggerfield in config is unused.The
ProcessQueryResponseConfigincludes alogger: InternalLoggerfield (line 25), but it's never used withinprocessQueryResponseor any helper functions. Either remove it or add appropriate logging calls.🔎 Option 1: Remove unused logger
export interface ProcessQueryResponseConfig<T> { occurrence?: FMTable<any, any>; singleMode: "exact" | "maybe" | false; queryOptions: Partial<QueryOptions<T>>; expandConfigs: ExpandConfig[]; skipValidation?: boolean; useEntityIds?: boolean; // Mapping from field names to output keys (for renamed fields in select) fieldMapping?: Record<string, string>; - logger: InternalLogger; }🔎 Option 2: Add logging for observability
export async function processQueryResponse<T>( response: any, config: ProcessQueryResponseConfig<T>, ): Promise<Result<any>> { - const { occurrence, singleMode, skipValidation, useEntityIds, fieldMapping } = - config; + const { occurrence, singleMode, skipValidation, useEntityIds, fieldMapping, logger } = + config; // Transform response if needed let data = response; if (occurrence && useEntityIds) { + logger.debug("Transforming response fields with entity IDs"); const expandValidationConfigs = buildExpandValidationConfigs(packages/fmodata/src/client/builders/select-mixin.ts (2)
14-18: Unnecessary type assertion onfieldName.The
fieldNameproperty onColumnis already typed asstring(seepackages/fmodata/src/orm/column.tslines 17-18). Theas stringcast is redundant.🔎 Suggested simplification
const fieldNames = fields.map((field) => { if (isColumn(field)) { - return field.fieldName as string; + return field.fieldName; } return String(field); });
63-66: Minor redundancy in return statement.The conditional
Object.keys(fieldMapping).length > 0 ? fieldMapping : {}always returns an object that is functionally equivalent tofieldMappingitself (which is already{}when empty). This could be simplified.🔎 Suggested simplification
return { selectedFields, - fieldMapping: Object.keys(fieldMapping).length > 0 ? fieldMapping : {}, + fieldMapping, };packages/fmodata/tests/errors.test.ts (1)
34-34: Remove unused import.
validateHeaderValuefrom"http"is imported but not used anywhere in this test file.🔎 Suggested fix
import { simpleMock, createMockFetch } from "./utils/mock-fetch"; -import { validateHeaderValue } from "http";packages/fmodata/src/orm/index.ts (1)
52-52: Remove commented-out export to clean up barrel file.
getTableFieldsis defined but not exported or used anywhere in the codebase. Remove the commented line at line 52 to eliminate noise in the barrel export.packages/fmodata/scripts/experiment-batch.ts (2)
67-77: Inconsistent table reference style.The cleanup function uses string-based table reference (
db.from("contacts")) whileexperiment5_AllGetWithOneFailureuses the typed table occurrence (db.from(contactsTO)). While both may work, usingcontactsTOconsistently would provide better type safety.🔎 Suggested fix for consistency
async function cleanup() { console.log("\n🧹 Cleaning up created records..."); for (const id of createdRecordIds) { try { - await db.from("contacts").delete().byId(id).execute(); + await db.from(contactsTO).delete().byId(id).execute(); console.log(` Deleted: ${id}`); } catch (error) { console.log(` Failed to delete ${id}:`, error); } } }
43-52: Consider adding.primaryKey()to the PrimaryKey field.The
PrimaryKeyfield is not marked with.primaryKey(). While the script may work, adding this modifier would ensure proper type inference and enable ID-based features in the ORM.🔎 Suggested fix
const contactsTO = fmTableOccurrence("contacts", { - PrimaryKey: textField(), + PrimaryKey: textField().primaryKey(), CreationTimestamp: timestampField(), CreatedBy: textField(), ModificationTimestamp: timestampField(), ModifiedBy: textField(), name: textField(), hobby: textField(), id_user: textField(), });packages/fmodata/src/client/builders/expand-builder.ts (1)
108-137: Consider defining an interface for the builder to avoidas anycasts.The method uses
(configuredBuilder as any).queryOptionsand(configuredBuilder as any).expandConfigswhich bypasses type checking. Consider defining a minimal interface that the builder must satisfy.🔎 Suggested approach
// At the top of the file or in shared-types.ts interface ExpandableBuilder { queryOptions: Partial<QueryOptions<any>>; expandConfigs?: ExpandConfig[]; } // Then in processExpand signature: processExpand<TargetTable extends FMTable<any, any>, Builder extends ExpandableBuilder>( targetTable: TargetTable, sourceTable: FMTable<any, any> | undefined, callback?: (builder: Builder) => Builder, builderFactory?: () => Builder, ): ExpandConfigpackages/fmodata/src/client/builders/response-processor.ts (1)
54-67: Redundant error check in skip-validation path.The check at lines 58-60 is unreachable. If
result.dataexists (checked at line 57), thenresult.errorwill always beundefinedbased on howextractRecordsworks (it returns mutually exclusive data/error).🔎 Suggested 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>; - } return { data: renameFieldsInResponse(result.data, fieldMapping) as T, error: undefined, }; } return result as Result<T>; }packages/fmodata/src/client/query/query-builder.ts (1)
684-697: 204 status handled twice with different code paths.The 204 No Content case is handled at line 685-697, but there's also a catch for it in the JSON parse error handler at lines 705-708. The first handler returns
{ data: [] as any, error: undefined }for list queries but the second returns the same. This is fine but slightly redundant.packages/fmodata/src/client/query/expand-builder.ts (3)
22-23: Missing logger parameter in constructor.The
QueryBuilderand other builders accept and use a logger for warnings (e.g., table name mismatches). ThisExpandBuilderclass doesn't accept a logger, which could lead to inconsistent logging behavior. TheExpandBuilderin../builders/expand-builder.ts(per the AI summary) accepts a logger.🔎 Proposed fix
+import { createLogger, InternalLogger } from "../../logger"; + export class ExpandBuilder { - constructor(private useEntityIds: boolean) {} + constructor( + private useEntityIds: boolean, + private logger: InternalLogger = createLogger(), + ) {}
160-160: TODO comment for nested expands should be tracked.The TODO indicates incomplete functionality for nested expand validation. If nested expands are supported in
buildSingleExpand(lines 107-113), they should also be handled in validation configs.Would you like me to open an issue to track implementing nested expand validation configs?
81-89: Filter extraction using regex may fail for complex filters.The code extracts the filter value using a regex match on the built query string. This approach may fail for filters containing
&characters or URL-encoded values. Consider using the filter value directly instead of parsing it back out.🔎 Alternative approach
if (config.options.filter) { // Filter should already be transformed by the nested builder - // Use odata-query to build filter string - const filterQuery = buildQuery({ filter: config.options.filter }); - const filterMatch = filterQuery.match(/\$filter=([^&]+)/); - if (filterMatch) { - parts.push(`$filter=${filterMatch[1]}`); - } + // Use the filter value directly if it's a string, otherwise build it + const filterValue = typeof config.options.filter === "string" + ? config.options.filter + : buildQuery({ filter: config.options.filter }).replace(/^\?\$filter=/, ""); + parts.push(`$filter=${filterValue}`); }packages/fmodata/src/client/query/url-builder.ts (1)
86-94: UnusedtableIdparameter in buildEntitySetNavigation.The
tableIdparameter is passed but never used in the method body. This appears to be dead code.🔎 Proposed fix
private buildEntitySetNavigation( queryString: string, - tableId: string, navigation: NavigationConfig, ): string {Also update the call site at line 58:
- return this.buildEntitySetNavigation(queryString, tableId, navigation); + return this.buildEntitySetNavigation(queryString, navigation);packages/fmodata/tests/e2e.test.ts (1)
447-451: Hook callbacks useanytype - consider proper typing.The hook callbacks use
anyforreqandresparameters. While this may be intentional for flexibility during testing, it loses type safety. If proper types exist (likeRequestandResponse), consider using them.Also applies to: 463-466, 526-530
packages/fmodata/src/index.ts (1)
22-28: Remove or uncomment the commented-out helper function exports.These commented-out exports (
getTableFields,getDefaultSelect,getBaseTableConfig,getFieldId,getFieldName,getTableId) appear to be intentionally excluded. If they're not part of the public API, remove them entirely to avoid clutter. If they're pending implementation or planned for future exposure, consider adding a TODO comment or tracking issue.packages/fmodata/tests/e2e/setup.ts (1)
103-115: Test-specific readValidator transforms all values.The
readValidatoron line 111-113 transforms any input to"static-value". This appears intentional for testing validation behavior, but consider adding a brief comment explaining the test purpose.packages/fmodata/src/orm/operators.ts (1)
157-179: Silent validation failure may hide bugs.When input validation fails or throws, the code silently falls back to the original value with comments like "will likely cause a query error" and "maintains backward compatibility." This design choice prioritizes not breaking queries, but it could make debugging difficult when values aren't transformed as expected.
Consider at least logging a warning in development mode when validation fails, so developers are aware their validators aren't being applied.
🔎 Example: Add optional warning for validation failures
} catch (error) { // If validation throws, use the original value (will likely cause a query error) // This maintains backward compatibility and allows the server to handle validation + if (process.env.NODE_ENV === 'development') { + console.warn(`Filter validation failed for column, using original value:`, error); + } value = operand; }packages/fmodata/src/client/delete-builder.ts (1)
170-180: Consider extracting duplicated query string stripping logic.The logic for stripping the table identifier prefix from the query string is duplicated between
execute()(lines 173-178) andgetRequestConfig()(lines 224-229). Consider extracting this to a private helper method.🔎 Example extraction
+ private stripTablePrefixFromQueryString(queryString: string, tableId: string): string { + const tableName = getTableName(this.table); + if (queryString.startsWith(`/${tableId}`)) { + return queryString.slice(`/${tableId}`.length); + } + if (queryString.startsWith(`/${tableName}`)) { + return queryString.slice(`/${tableName}`.length); + } + return queryString; + }Also applies to: 222-232
packages/fmodata/src/client/insert-builder.ts (1)
240-251: Duplicated container field filtering logic.The logic to exclude container fields from the validation schema is duplicated between
execute()(lines 240-251) andprocessResponse()(lines 422-433). Consider extracting this to a private helper method.🔎 Example extraction
+ private getValidationSchema(): Record<string, any> | undefined { + if (!this.table) return undefined; + + const baseTableConfig = getBaseTableConfig(this.table); + const containerFields = baseTableConfig.containerFields || []; + const schema = { ...baseTableConfig.schema }; + + for (const containerField of containerFields) { + delete schema[containerField as string]; + } + return schema; + }Also applies to: 422-433
packages/fmodata/scripts/typegen-starter.ts (1)
266-274: XMLParserarrayMode: falsemay cause issues with single-element arrays.When
arrayMode: false, single-element XML arrays are parsed as objects instead of arrays. TheensureArrayhelper handles this, but ensure all XML array paths use it consistently.Consider using
arrayMode: (name) => ['Property', 'Annotation', 'EntityType', 'EntitySet', 'NavigationProperty', 'PropertyRef'].includes(name)for more explicit array handling.packages/fmodata/src/client/update-builder.ts (2)
201-216: Input validation error handling usesas anycast.The error return at line 214 uses
as anywhich bypasses type safety. Consider using a more explicit type assertion or restructuring the error handling.Suggested improvement
} catch (error) { // If validation fails, return error immediately return { data: undefined, error: error instanceof Error ? error : new Error(String(error)), - } as any; + } as Result< + ReturnPreference extends "minimal" + ? { updatedCount: number } + : InferSchemaOutputFromFMTable<Occ> + >; }
203-216: Redundantthis.tablecheck.At line 203,
this.tableis always truthy since it's assigned in the constructor from a required config property. The conditional is unnecessary.Suggested fix
- if (this.table) { - const baseTableConfig = getBaseTableConfig(this.table); - const inputSchema = baseTableConfig.inputSchema; - - try { - validatedData = await validateAndTransformInput(this.data, inputSchema); - } catch (error) { - // If validation fails, return error immediately - return { - data: undefined, - error: error instanceof Error ? error : new Error(String(error)), - } as any; - } + const baseTableConfig = getBaseTableConfig(this.table); + const inputSchema = baseTableConfig.inputSchema; + + if (inputSchema) { + try { + validatedData = await validateAndTransformInput(this.data, inputSchema); + } catch (error) { + // If validation fails, return error immediately + return { + data: undefined, + error: error instanceof Error ? error : new Error(String(error)), + } as any; + } }packages/fmodata/src/client/entity-set.ts (3)
103-146: Duplicate defaultSelect logic betweenlist()andget().The defaultSelect handling logic (lines 103-146 in
list()and 188-252 inget()) is nearly identical. Consider extracting this into a private helper method.Suggested refactor
private applyDefaultSelect<B extends QueryBuilder<Occ> | RecordBuilder<Occ>>( builder: B, ): B { const defaultSelectValue = getDefaultSelect(this.occurrence); if (defaultSelectValue === "schema") { const allColumns = getTableColumns(this.occurrence) as ExtractColumnsFromOcc<Occ>; return builder.select(allColumns) as B; } else if (typeof defaultSelectValue === "object" && defaultSelectValue !== null) { return builder.select(defaultSelectValue as ExtractColumnsFromOcc<Occ>) as B; } return builder; }
339-391: Navigation validation emits warning but doesn't prevent invalid navigation.The navigate method logs a warning when the target isn't in
navigationPathsbut still proceeds with the navigation. Consider whether this should throw or return a different result for invalid navigations.If strict validation is desired:
if (navigationPaths && !navigationPaths.includes(relationName)) { throw new Error( `Cannot navigate to "${relationName}". Valid navigation paths: ${navigationPaths.length > 0 ? navigationPaths.join(", ") : "none"}` ); }
66-69: Accessing private_useEntityIdsviaas anycast.The access pattern
(config.database as any)?._useEntityIdssuggests_useEntityIdsshould be exposed via a proper accessor method on the Database class rather than accessing a private property.packages/fmodata/src/orm/table.ts (4)
328-331: Accessing private_getConfig()method on field builders.The code accesses
(builder as any)._getConfig()multiple times. This pattern suggests_getConfig()should either be a public method or there should be a type-safe way to access field builder configuration.Consider exposing a public
getConfig()method or using a type that includes_getConfig:interface FieldBuilderWithConfig { _getConfig(): FieldBuilderConfig; }
746-767:getTableColumnscreates new Column instances on each call.This function creates new Column instances every time it's called, which could be inefficient if called frequently. Consider caching the columns on the table instance.
Suggested improvement
+const FMTableColumnsCache = Symbol.for("fmodata:FMTableColumnsCache"); + export function getTableColumns<T extends FMTable<any, any>>( table: T, ): ColumnMap<T[typeof FMTableFields], ExtractTableName<T>> { + // Return cached columns if available + if ((table as any)[FMTableColumnsCache]) { + return (table as any)[FMTableColumnsCache]; + } + const fields = table[FMTableFields]; const tableName = table[FMTableName]; // ... rest of implementation + + // Cache the columns + (table as any)[FMTableColumnsCache] = columns; return columns; }
465-489: Commented-out code should be removed.The commented-out
isTableOccurrencefunction (lines 465-489) should be removed if it's no longer needed, or uncommented and used if it is needed.
724-733: Missing newline betweengetFieldNameandgetTableId.There's a missing blank line between the closing brace of
getFieldName(line 724) and the JSDoc comment forgetTableId(line 725).Suggested fix
return fieldId; } + /** * Get the table ID (FMTID or name) from an FMTable instance.packages/fmodata/src/client/record-builder.ts (4)
51-56: Unused generic parameterTSchema.The
TSchemageneric parameter is declared but never referenced in the type body. Consider removing it to simplify the type definition.🔎 Suggested fix
type MapSelectToReturnType< TSelect extends Record<string, Column<any, any, any, any>>, - TSchema extends Record<string, any>, > = { [K in keyof TSelect]: ExtractColumnType<TSelect[K]>; };Note: You'll also need to update the usage at line 73 from
MapSelectToReturnType<Selected, Schema>toMapSelectToReturnType<Selected>.
343-350: Consider usingcloneWithChangesto reduce code duplication.The manual state copying here duplicates the logic in
cloneWithChanges. While both serve slightly different purposes (this creates a new builder for expand), consolidating this logic could prevent future inconsistencies if new properties are added.
489-496: Consider URL-encodingrecordIdin URL construction.The
recordIdis interpolated directly into the URL without encoding. If the record ID contains special characters (e.g., quotes, ampersands), this could cause malformed URLs or unexpected behavior.🔎 Suggested fix
if ( this.isNavigateFromEntitySet && this.navigateSourceTableName && this.navigateRelation ) { // From navigated EntitySet: /sourceTable/relation('recordId') - url = `/${this.databaseName}/${this.navigateSourceTableName}/${this.navigateRelation}('${this.recordId}')`; + url = `/${this.databaseName}/${this.navigateSourceTableName}/${this.navigateRelation}('${encodeURIComponent(String(this.recordId))}')`; } else { // Normal record: /tableName('recordId') - use FMTID if configured const tableId = this.getTableId( options?.useEntityIds ?? this.databaseUseEntityIds, ); - url = `/${this.databaseName}/${tableId}('${this.recordId}')`; + url = `/${this.databaseName}/${tableId}('${encodeURIComponent(String(this.recordId))}')`; }
562-569: Fallback branch may be dead code.The comment indicates the
operationParamfallback "shouldn't happen in normal flow". IfoperationColumnis always set whenoperation === "getSingleField", consider removing the fallback branch or adding an assertion to catch unexpected states during development.
| 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.
Critical: from() mutates database-level _useEntityIds causing side effects.
The from() method mutates this._useEntityIds when a table has FMTable.Symbol.UseEntityIds defined. This means the order of from() calls affects subsequent queries:
const db = connection.database("mydb", { useEntityIds: false });
db.from(tableWithIds); // Sets _useEntityIds = true
db.from(otherTable); // Now uses entity IDs unexpectedly!The per-table override should be passed to the EntitySet without mutating the Database instance.
🔎 Proposed fix: Pass override to EntitySet without mutation
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;
- }
- }
+ // Determine effective useEntityIds: table-level override takes precedence
+ let effectiveUseEntityIds = this._useEntityIds;
+ if (
+ Object.prototype.hasOwnProperty.call(table, FMTable.Symbol.UseEntityIds)
+ ) {
+ const tableUseEntityIds = (table as any)[FMTable.Symbol.UseEntityIds];
+ if (typeof tableUseEntityIds === "boolean") {
+ effectiveUseEntityIds = tableUseEntityIds;
+ }
+ }
return new EntitySet<T>({
occurrence: table as T,
databaseName: this.databaseName,
context: this.context,
database: this,
+ useEntityIds: effectiveUseEntityIds,
});
}Note: This requires updating EntitySet constructor to accept and use the useEntityIds parameter instead of reading it from database._useEntityIds.
🤖 Prompt for AI Agents
In packages/fmodata/src/client/database.ts around lines 29 to 46, the from()
method is mutating the Database instance by assigning this._useEntityIds when a
table declares FMTable.Symbol.UseEntityIds; change this so from() does NOT
mutate the database state but instead computes a local useEntityIds value (table
override if defined and boolean, otherwise fallback to this._useEntityIds) and
pass that value into the new EntitySet constructor. Also update EntitySet to
accept a useEntityIds option and consume it instead of reading
database._useEntityIds, so per-table overrides are applied only to that
EntitySet and do not produce side effects on the Database.
| 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.
Mutating this instead of cloning breaks builder immutability.
The where() method mutates this.queryOptions.filter directly and returns this, while other methods like select(), single(), maybeSingle(), and count() use cloneWithChanges() to return a new builder instance. This inconsistency can lead to unexpected side effects when reusing query builders.
🔎 Proposed fix: Use cloneWithChanges for consistency
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 and returns this;
change it to not mutate the existing builder but return a cloned builder via
cloneWithChanges(). For string input set const filterValue = expression; for
FilterExpression set const filterValue =
expression.toODataFilter(this.databaseUseEntityIds); then return
cloneWithChanges({ queryOptions: { ...this.queryOptions, filter: filterValue }
}) so the original builder remains immutable and behavior matches other builder
methods.
| orderBy( | ||
| ...orderByArgs: | ||
| | [ | ||
| | TypeSafeOrderBy<InferSchemaOutputFromFMTable<Occ>> | ||
| | Column<any, any, ExtractTableName<Occ>> | ||
| | OrderByExpression<ExtractTableName<Occ>>, | ||
| ] | ||
| | [ | ||
| Column<any, any, ExtractTableName<Occ>>, | ||
| ...Array< | ||
| | Column<any, any, ExtractTableName<Occ>> | ||
| | OrderByExpression<ExtractTableName<Occ>> | ||
| >, | ||
| ] | ||
| ): QueryBuilder<Occ, Selected, SingleMode, IsCount, Expands> { | ||
| const tableName = getTableName(this.occurrence); | ||
|
|
||
| // Handle variadic arguments (multiple fields) | ||
| if (orderByArgs.length > 1) { | ||
| const orderByParts = orderByArgs.map((arg) => { | ||
| if (isOrderByExpression(arg)) { | ||
| // Validate table match | ||
| if (arg.column.tableName !== tableName) { | ||
| this.logger.warn( | ||
| `Column ${arg.column.toString()} is from table "${arg.column.tableName}", but query is for table "${tableName}"`, | ||
| ); | ||
| } | ||
| const fieldName = arg.column.fieldName; | ||
| const transformedField = this.occurrence | ||
| ? transformOrderByField(fieldName, this.occurrence) | ||
| : fieldName; | ||
| return `${transformedField} ${arg.direction}`; | ||
| } else if (isColumn(arg)) { | ||
| // Validate table match | ||
| if (arg.tableName !== tableName) { | ||
| this.logger.warn( | ||
| `Column ${arg.toString()} is from table "${arg.tableName}", but query is for table "${tableName}"`, | ||
| ); | ||
| } | ||
| const fieldName = arg.fieldName; | ||
| const transformedField = this.occurrence | ||
| ? transformOrderByField(fieldName, this.occurrence) | ||
| : fieldName; | ||
| return transformedField; // Default to ascending | ||
| } else { | ||
| throw new Error( | ||
| "Variadic orderBy() only accepts Column or OrderByExpression arguments", | ||
| ); | ||
| } | ||
| }); | ||
| this.queryOptions.orderBy = orderByParts; | ||
| return this; | ||
| } | ||
|
|
||
| // Handle single argument | ||
| const orderBy = orderByArgs[0]; | ||
|
|
||
| // Handle OrderByExpression | ||
| if (isOrderByExpression(orderBy)) { | ||
| // Validate table match | ||
| if (orderBy.column.tableName !== tableName) { | ||
| this.logger.warn( | ||
| `Column ${orderBy.column.toString()} is from table "${orderBy.column.tableName}", but query is for table "${tableName}"`, | ||
| ); | ||
| } | ||
| const fieldName = orderBy.column.fieldName; | ||
| const transformedField = this.occurrence | ||
| ? transformOrderByField(fieldName, this.occurrence) | ||
| : fieldName; | ||
| this.queryOptions.orderBy = `${transformedField} ${orderBy.direction}`; | ||
| return this; | ||
| } | ||
|
|
||
| // Handle Column references | ||
| if (isColumn(orderBy)) { | ||
| // Validate table match | ||
| if (orderBy.tableName !== tableName) { | ||
| this.logger.warn( | ||
| `Column ${orderBy.toString()} is from table "${orderBy.tableName}", but query is for table "${tableName}"`, | ||
| ); | ||
| } | ||
| // Single Column reference without direction (defaults to ascending) | ||
| const fieldName = orderBy.fieldName; | ||
| this.queryOptions.orderBy = this.occurrence | ||
| ? transformOrderByField(fieldName, this.occurrence) | ||
| : fieldName; | ||
| return this; | ||
| } | ||
| // Transform field names to FMFIDs if using entity IDs | ||
| if (this.occurrence && orderBy) { | ||
| if (Array.isArray(orderBy)) { | ||
| // Check if it's a single tuple [field, direction] or array of tuples | ||
| if ( | ||
| orderBy.length === 2 && | ||
| (typeof orderBy[0] === "string" || isColumn(orderBy[0])) && | ||
| (orderBy[1] === "asc" || orderBy[1] === "desc") | ||
| ) { | ||
| // Single tuple: [field, direction] or [column, direction] | ||
| const field = isColumn(orderBy[0]) | ||
| ? orderBy[0].fieldName | ||
| : orderBy[0]; | ||
| const direction = orderBy[1] as "asc" | "desc"; | ||
| this.queryOptions.orderBy = `${transformOrderByField(field, this.occurrence)} ${direction}`; | ||
| } else { | ||
| // Array of tuples: [[field, dir], [field, dir], ...] | ||
| this.queryOptions.orderBy = ( | ||
| orderBy as Array<[any, "asc" | "desc"]> | ||
| ).map(([fieldOrCol, direction]) => { | ||
| const field = isColumn(fieldOrCol) | ||
| ? fieldOrCol.fieldName | ||
| : String(fieldOrCol); | ||
| const transformedField = transformOrderByField( | ||
| field, | ||
| this.occurrence!, | ||
| ); | ||
| return `${transformedField} ${direction}`; | ||
| }); | ||
| } | ||
| } else { | ||
| // Single field name (string) | ||
| this.queryOptions.orderBy = transformOrderByField( | ||
| String(orderBy), | ||
| this.occurrence, | ||
| ); | ||
| } | ||
| } else { | ||
| // No occurrence/baseTable - pass through as-is | ||
| if (Array.isArray(orderBy)) { | ||
| if ( | ||
| orderBy.length === 2 && | ||
| (typeof orderBy[0] === "string" || isColumn(orderBy[0])) && | ||
| (orderBy[1] === "asc" || orderBy[1] === "desc") | ||
| ) { | ||
| // Single tuple: [field, direction] or [column, direction] | ||
| const field = isColumn(orderBy[0]) | ||
| ? orderBy[0].fieldName | ||
| : orderBy[0]; | ||
| const direction = orderBy[1] as "asc" | "desc"; | ||
| this.queryOptions.orderBy = `${field} ${direction}`; | ||
| } else { | ||
| // Array of tuples | ||
| this.queryOptions.orderBy = ( | ||
| orderBy as Array<[any, "asc" | "desc"]> | ||
| ).map(([fieldOrCol, direction]) => { | ||
| const field = isColumn(fieldOrCol) | ||
| ? fieldOrCol.fieldName | ||
| : String(fieldOrCol); | ||
| return `${field} ${direction}`; | ||
| }); | ||
| } | ||
| } else { | ||
| this.queryOptions.orderBy = orderBy; | ||
| } | ||
| } | ||
| return this; | ||
| } |
There was a problem hiding this comment.
orderBy() also mutates this directly, breaking immutability.
Same issue as where() - this method mutates this.queryOptions.orderBy and returns this instead of using cloneWithChanges(). The method is also quite long (~155 lines) with significant code duplication between the entity-ID-enabled and non-entity-ID branches.
Consider:
- Using
cloneWithChanges()for consistency with other builder methods - Extracting the orderBy transformation logic into a helper function to reduce duplication
🤖 Prompt for AI Agents
In packages/fmodata/src/client/query/query-builder.ts around lines 284 to 439,
orderBy() mutates this.queryOptions.orderBy and returns this (breaking
immutability) and duplicates logic across entity-ID and non-entity-ID branches;
change it to build a new orderBy value (using a small helper to transform any
accepted input into normalized string or string[] form, handling Columns,
OrderByExpression, tuples, and entity-ID field mapping), then return
cloneWithChanges({ queryOptions: { ...this.queryOptions, orderBy: newOrderBy }
}) instead of assigning to this.queryOptions; extract the transformation into a
private helper (e.g., normalizeOrderByArg(orderByArg, occurrence, tableName,
logger)) to centralize validation/warnings and remove duplicated code paths.
| function buildExpandValidationConfigs( | ||
| configs: ExpandConfig[], | ||
| ): ExpandValidationConfig[] { | ||
| return configs.map((config) => { | ||
| // Get target table/occurrence from config (stored during expand call) | ||
| const targetTable = config.targetTable; | ||
|
|
||
| // Extract schema from target table/occurrence | ||
| let targetSchema: Record<string, StandardSchemaV1> | undefined; | ||
| if (targetTable) { | ||
| const tableSchema = (targetTable as any)[FMTableClass.Symbol.Schema]; | ||
| if (tableSchema) { | ||
| const zodSchema = tableSchema["~standard"]?.schema; | ||
| if ( | ||
| zodSchema && | ||
| typeof zodSchema === "object" && | ||
| "shape" in zodSchema | ||
| ) { | ||
| targetSchema = zodSchema.shape as Record<string, StandardSchemaV1>; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Extract selected fields from options | ||
| const selectedFields = config.options?.select | ||
| ? Array.isArray(config.options.select) | ||
| ? config.options.select.map((f) => String(f)) | ||
| : [String(config.options.select)] | ||
| : undefined; | ||
|
|
||
| return { | ||
| relation: config.relation, | ||
| targetSchema: targetSchema, | ||
| targetTable: targetTable, | ||
| table: targetTable, // For transformation | ||
| selectedFields: selectedFields, | ||
| nestedExpands: undefined, // TODO: Handle nested expands if needed | ||
| }; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how many places access FMTableClass.Symbol.Schema or similar patterns
rg -n "Symbol\.Schema" --type=tsRepository: proofgeist/proofkit
Length of output: 1156
🏁 Script executed:
#!/bin/bash
# Find the FMTableClass definition and check for schema-related methods
fd -e ts | xargs grep -l "class FMTableClass\|class FMTable" | head -10Repository: proofgeist/proofkit
Length of output: 96
🏁 Script executed:
#!/bin/bash
# Search for schema extraction/exposure helper methods
rg -n "getSchema|exposedSchema|schemaShape" --type=ts -A 2Repository: proofgeist/proofkit
Length of output: 6864
🏁 Script executed:
#!/bin/bash
# Check the getSchemaFromTable implementation in builders/response-processor.ts
sed -n '156,180p' packages/fmodata/src/client/builders/response-processor.tsRepository: proofgeist/proofkit
Length of output: 840
🏁 Script executed:
#!/bin/bash
# Check if response-processor.ts imports or uses getSchemaFromTable
head -50 packages/fmodata/src/client/query/response-processor.ts | grep -n "import\|getSchema"Repository: proofgeist/proofkit
Length of output: 862
🏁 Script executed:
#!/bin/bash
# Compare the schema extraction logic in response-processor.ts vs the helper
sed -n '40,52p' packages/fmodata/src/client/query/response-processor.tsRepository: proofgeist/proofkit
Length of output: 532
🏁 Script executed:
#!/bin/bash
# Check if getBaseTableConfig is exported and what it does
rg -n "getBaseTableConfig" packages/fmodata/src/client/builders/response-processor.ts -A 3Repository: proofgeist/proofkit
Length of output: 457
🏁 Script executed:
#!/bin/bash
# Check the actual difference between what's needed - does expand-builder.ts have similar pattern
sed -n '132,145p' packages/fmodata/src/client/query/expand-builder.tsRepository: proofgeist/proofkit
Length of output: 561
🏁 Script executed:
#!/bin/bash
# Find getBaseTableConfig definition
rg -n "function getBaseTableConfig|export.*getBaseTableConfig" --type=ts -A 5Repository: proofgeist/proofkit
Length of output: 490
🏁 Script executed:
#!/bin/bash
# Check what baseTableConfig.schema actually contains vs what zodSchema.shape is
rg -n "baseTableConfig\.schema" --type=ts -B 2 -A 2 | head -30Repository: proofgeist/proofkit
Length of output: 2534
🏁 Script executed:
#!/bin/bash
# Check if there are type definitions that explain the difference
fd -e ts | xargs grep -l "baseTableConfig\|FMTableBaseTableConfig" | head -5Repository: proofgeist/proofkit
Length of output: 312
🏁 Script executed:
#!/bin/bash
# Understand the difference - check record-builder.ts which uses getSchemaFromTable
sed -n '530,540p' packages/fmodata/src/client/record-builder.tsRepository: proofgeist/proofkit
Length of output: 407
🏁 Script executed:
#!/bin/bash
# Check how builders/expand-builder.ts uses getBaseTableConfig.schema vs query/expand-builder.ts uses Symbol.Schema
echo "=== builders/expand-builder.ts (using getBaseTableConfig) ==="
sed -n '43,56p' packages/fmodata/src/client/builders/expand-builder.ts
echo ""
echo "=== query/expand-builder.ts (using Symbol.Schema) ==="
sed -n '132,145p' packages/fmodata/src/client/query/expand-builder.tsRepository: proofgeist/proofkit
Length of output: 1173
🏁 Script executed:
#!/bin/bash
# Check type definition of baseTableConfig to understand what .schema is
rg -n "FMTableBaseTableConfig\|baseTableConfig" packages/fmodata/src/orm/table.ts -B 3 -A 10 | head -50Repository: proofgeist/proofkit
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Search for FMTableBaseTableConfig definition more specifically
rg -n "FMTableBaseTableConfig" packages/fmodata/src/orm/table.ts -B 2 -A 15 | head -80Repository: proofgeist/proofkit
Length of output: 2958
🏁 Script executed:
#!/bin/bash
# Check what FMTableBaseTableConfig symbol contains
ast-grep --pattern 'const FMTableBaseTableConfig = $_'Repository: proofgeist/proofkit
Length of output: 176
🏁 Script executed:
#!/bin/bash
# Check if there's documentation or type info about the schema property
rg -n "schema.*StandardSchemaV1\|Record.*StandardSchemaV1" packages/fmodata/src/orm/table.ts -B 2 -A 2Repository: proofgeist/proofkit
Length of output: 45
Migrate to getBaseTableConfig() instead of directly accessing Symbol.Schema.
The code accesses the internal schema structure via FMTableClass.Symbol.Schema and navigates through ["~standard"]?.schema.shape, creating tight coupling to Zod v4 internals. However, getBaseTableConfig() already provides the proper abstraction—it exposes baseTableConfig.schema as a Record<string, StandardSchemaV1>, eliminating the need to extract from the Zod schema shape.
The newer builder code in packages/fmodata/src/client/builders/expand-builder.ts already uses this pattern. Update buildExpandValidationConfigs() and the similar logic in the buildValidationConfigs() function (line 198) to use getBaseTableConfig() and import it from ../../orm/table, matching the approach in the builders directory.
🤖 Prompt for AI Agents
In packages/fmodata/src/client/query/response-processor.ts around lines 32 to 71
(and also update the similar logic around line 198 in buildValidationConfigs),
the code directly reads the table schema via FMTableClass.Symbol.Schema and the
Zod internals; replace that with the exported helper getBaseTableConfig from
../../orm/table: import getBaseTableConfig at top, call
getBaseTableConfig(targetTable) and, when present, use its
baseTableConfig.schema (which is already a Record<string, StandardSchemaV1>) for
targetSchema instead of digging into ["~standard"]?.schema.shape; apply the same
replacement in buildValidationConfigs at ~line 198 so both functions use
getBaseTableConfig and avoid coupling to Zod internals.
| private buildRecordNavigation( | ||
| queryString: string, | ||
| tableId: string, | ||
| navigation: NavigationConfig, | ||
| ): string { | ||
| const { sourceTableName, baseRelation, recordId, relation } = navigation; | ||
| const base = baseRelation | ||
| ? `${sourceTableName}/${baseRelation}('${recordId}')` | ||
| : `${sourceTableName}('${recordId}')`; | ||
| return `/${this.databaseName}/${base}/${relation}${queryString}`; | ||
| } |
There was a problem hiding this comment.
Record ID not sanitized - potential OData injection.
The recordId is interpolated directly into the URL without sanitization. If recordId contains special characters like ' or ), it could break the URL or potentially allow OData injection.
🔎 Proposed fix: Escape special characters in record ID
+ /**
+ * Escapes a record ID for use in OData URLs.
+ * Single quotes must be doubled per OData spec.
+ */
+ private escapeRecordId(recordId: string | number): string {
+ return String(recordId).replace(/'/g, "''");
+ }
+
private buildRecordNavigation(
queryString: string,
tableId: string,
navigation: NavigationConfig,
): string {
const { sourceTableName, baseRelation, recordId, relation } = navigation;
+ const escapedId = this.escapeRecordId(recordId!);
const base = baseRelation
- ? `${sourceTableName}/${baseRelation}('${recordId}')`
- : `${sourceTableName}('${recordId}')`;
+ ? `${sourceTableName}/${baseRelation}('${escapedId}')`
+ : `${sourceTableName}('${escapedId}')`;
return `/${this.databaseName}/${base}/${relation}${queryString}`;
}🤖 Prompt for AI Agents
In packages/fmodata/src/client/query/url-builder.ts around lines 70 to 80, the
recordId is interpolated directly into the OData path which allows special
characters (e.g. single quotes or parentheses) to break the URL or enable
injection; modify the code to sanitize recordId by first replacing any single
quote (') with two single quotes ('') per OData literal escaping rules and then
URL-encoding the result (e.g., via encodeURIComponent) before embedding it into
the path so the constructed base uses the escaped/encoded recordId.
| if ( | ||
| options?.isNavigateFromEntitySet && | ||
| options.navigateSourceTableName && | ||
| options.navigateRelation | ||
| ) { | ||
| // From navigated EntitySet: /sourceTable/relation('recordId') | ||
| url = `/${this.databaseName}/${options.navigateSourceTableName}/${options.navigateRelation}('${recordId}')`; | ||
| } else { | ||
| // Normal record: /tableName('recordId') - use FMTID if configured | ||
| url = `/${this.databaseName}/${tableId}('${recordId}')`; | ||
| } |
There was a problem hiding this comment.
Same record ID escaping issue in buildRecordUrl.
The recordId parameter is also interpolated without escaping in buildRecordUrl. Apply the same escaping fix here.
🤖 Prompt for AI Agents
In packages/fmodata/src/client/query/url-builder.ts around lines 161 to 171, the
recordId is interpolated into the URL without escaping; update both branches
(navigated EntitySet and normal record) to escape/sanitize recordId the same way
used elsewhere (e.g., call the existing escape/encode helper or run
encodeURIComponent and handle single quotes appropriately) before inserting it
into the template string so the generated URL is safe and correct.
| const testTable = fmTableOccurrence( | ||
| "test", | ||
| { | ||
| text: textField().primaryKey(), | ||
| textNumber: textField().writeValidator(z.number().transform(toString)), | ||
| enum: textField().writeValidator(z.enum(["a", "b", "c"])), | ||
| transform: textField().writeValidator( | ||
| z.string().transform(() => "static-value"), | ||
| ), | ||
| }, | ||
| { useEntityIds: false }, | ||
| ); |
There was a problem hiding this comment.
toString is not defined - will cause runtime error.
At line 340, toString is used as a transform function but it's not imported or defined. This will fail at runtime. You likely meant String or need to define a local function.
🔎 Suggested fix
const testTable = fmTableOccurrence(
"test",
{
text: textField().primaryKey(),
- textNumber: textField().writeValidator(z.number().transform(toString)),
+ textNumber: textField().writeValidator(z.number().transform(String)),
enum: textField().writeValidator(z.enum(["a", "b", "c"])),
transform: textField().writeValidator(
z.string().transform(() => "static-value"),
),
},
{ useEntityIds: false },
);📝 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 testTable = fmTableOccurrence( | |
| "test", | |
| { | |
| text: textField().primaryKey(), | |
| textNumber: textField().writeValidator(z.number().transform(toString)), | |
| enum: textField().writeValidator(z.enum(["a", "b", "c"])), | |
| transform: textField().writeValidator( | |
| z.string().transform(() => "static-value"), | |
| ), | |
| }, | |
| { useEntityIds: false }, | |
| ); | |
| const testTable = fmTableOccurrence( | |
| "test", | |
| { | |
| text: textField().primaryKey(), | |
| textNumber: textField().writeValidator(z.number().transform(String)), | |
| enum: textField().writeValidator(z.enum(["a", "b", "c"])), | |
| transform: textField().writeValidator( | |
| z.string().transform(() => "static-value"), | |
| ), | |
| }, | |
| { useEntityIds: false }, | |
| ); |
🤖 Prompt for AI Agents
In packages/fmodata/tests/filters.test.ts around lines 336 to 347, the transform
references toString which is not defined and will throw at runtime; replace
toString with the global String constructor (or define a local helper function)
so the z.number().transform call converts numbers to strings correctly, e.g.,
use z.number().transform(String) or add a defined function and reference it,
then run tests to verify no runtime error.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| ? string // If no occurrences, allow any string | ||
| : Occurrences[number]["name"]; // Otherwise, extract union of names | ||
|
|
||
| export class Database< |
There was a problem hiding this comment.
Bug: Database state mutation causes incorrect entity ID settings in navigation
The from() method mutates this._useEntityIds based on the table's UseEntityIds setting before creating the EntitySet. When navigate() is later called on an EntitySet, it passes database: this.database to create a new EntitySet, which reads the current _useEntityIds value. If from() was called with a different table in between, the navigated EntitySet will capture the wrong useEntityIds setting. For example, calling db.from(tableWithEntityIds) followed by db.from(tableWithoutEntityIds) would cause subsequent navigate() calls from the first EntitySet to incorrectly use useEntityIds = false. The databaseUseEntityIds value should be passed through from the original EntitySet rather than re-reading from the mutated database state.
Additional Locations (1)
| const queryBuilder = new QueryBuilder< | ||
| WithSystemFields<T>, | ||
| keyof WithSystemFields<T>, | ||
| false, |
There was a problem hiding this comment.
Bug: QueryBuilder missing entity ID setting in delete/update where methods
The where() methods in DeleteBuilder and UpdateBuilder create QueryBuilder instances without passing the databaseUseEntityIds setting from the parent builder. The QueryBuilder constructor defaults databaseUseEntityIds to false, so filter expressions created via where() would use field names instead of entity IDs even when the database is configured to use entity IDs. This causes the OData $filter parameter to contain incorrect field identifiers, leading to query failures or incorrect results when entity IDs are enabled.
Note
Migrates the test suite to the new ORM/table/field API with typed columns, operators, and navigation; refreshes fixtures/mocks; and updates test configuration/aliases and dependencies.
defineBaseTable/defineTableOccurrencewithfmTableOccurrenceand field builders (textField,numberField, etc.).select/orderBy(asc/desc) and typed filter operators (eq,gt,and,or,isNull).useEntityIdsoverride retained and validated.tests/fixtures/responses.ts(new records/ids, adjust statuses; add "list with nested expand").tests/list-methods.test.ts; newtests/orm-api.test.ts; broad updates to mock, navigate, query-strings, insert/update/validation tests.@proofkit/fmodataand build-aware tsconfig; addtests/tsconfig.build.jsonand updatetests/tsconfig.json.@proofkit/fmodata(e.g.,sanitize-json, core exports); update E2Eschema-managerusage.fast-xml-parser.Written by Cursor Bugbot for commit cc2baa6. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Release Notes
New Features
fmTableOccurrenceand field builders for streamlined schema configuration.eq,contains,startsWith, etc.) and improved filtering capabilities.Documentation
✏️ Tip: You can customize this high-level summary in your review settings.