specialcolumns header #79
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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. |
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bug: Special columns injected into user select
buildSelectExpandQueryString appends ROWID/ROWMODID to config.selectedFields whenever includeSpecialColumns is true. This causes queries that explicitly use $select (e.g., via .select({ ... })) to unexpectedly request and potentially return special columns even when not requested via the new systemColumns argument, changing response shape and conflicting with the documented “no $select” behavior.
packages/fmodata/src/client/builders/query-string-builder.ts#L24-L46
proofkit/packages/fmodata/src/client/builders/query-string-builder.ts
Lines 24 to 46 in c09d7bb
@proofkit/better-auth
@proofkit/cli
create-proofkit
@proofkit/fmdapi
@proofkit/fmodata
@proofkit/typegen
@proofkit/webviewer
commit: |
There was a problem hiding this comment.
Bug: Special columns added despite user select
buildSelectExpandQueryString appends ROWID/ROWMODID to $select whenever includeSpecialColumns is true. This also affects user-supplied .select() queries, contradicting the intended “exclude special columns when $select is applied” behavior and can produce unexpected ROWID/ROWMODID fields (often as undefined) in responses.
packages/fmodata/src/client/builders/query-string-builder.ts#L24-L46
proofkit/packages/fmodata/src/client/builders/query-string-builder.ts
Lines 24 to 46 in c09d7bb
Bug: Batch response processing drops special columns
QueryBuilder.processResponse computes merged includeSpecialColumns but never passes it to processQueryResponse. In batch/manual processResponse usage, this makes validation/processing behave as if includeSpecialColumns is disabled, so ROWID/ROWMODID can be stripped or omitted unexpectedly.
packages/fmodata/src/client/query/query-builder.ts#L888-L902
proofkit/packages/fmodata/src/client/query/query-builder.ts
Lines 888 to 902 in c09d7bb
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds opt-in support for FileMaker "special columns" (ROWID, ROWMODID) propagated from FMServerConnection through Database and EntitySet generics into builders, query-string construction, response validation/processing, types, and tests; supports database-level defaults and per-request overrides. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Conn as FMServerConnection
participant DB as Database<IncludeSpecialColumns>
participant ES as EntitySet
participant QB as QueryBuilder
participant Server as FileMakerServer
participant Validator as Validation
Client->>Conn: database(name, { includeSpecialColumns: true })
Conn->>DB: create Database<true> (store flag)
Client->>DB: from(table) -> EntitySet(..., DatabaseIncludeSpecialColumns=true)
Client->>ES: .list()
ES->>QB: create QueryBuilder(databaseIncludeSpecialColumns=true)
Client->>QB: .select(fields, systemColumns={ROWID:true})
QB->>QB: append ROWID/ROWMODID when requested
Client->>QB: .execute({ includeSpecialColumns: true })
QB->>Conn: _makeRequest(url, { useEntityIds?, includeSpecialColumns:true })
Conn->>Conn: compose Prefer header (e.g., fmodata.include-specialcolumns, fmodata.entity-ids)
Conn->>Server: HTTP request with Prefer + $select
Server->>Conn: OData response (may include special columns)
Conn->>Validator: validate response (includeSpecialColumns=true)
Validator->>QB: shaped result includes ROWID/ROWMODID
QB->>Client: resolved Result typed with special columns
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/fmodata/src/client/record-builder.ts (1)
796-805: MissingincludeSpecialColumnsinprocessResponseoptions.The
executemethod passesincludeSpecialColumns: mergedOptions.includeSpecialColumnstoprocessODataResponse(line 668), butprocessResponsedoesn't include it. This inconsistency means batch operations won't handle special columns the same way as directexecute()calls.return processODataResponse(rawResponse, { table: this.table, schema: getSchemaFromTable(this.table), singleMode: "exact", selectedFields: this.selectedFields, expandValidationConfigs, skipValidation: options?.skipValidation, useEntityIds: mergedOptions.useEntityIds, + includeSpecialColumns: mergedOptions.includeSpecialColumns, fieldMapping: this.fieldMapping, });packages/fmodata/src/client/query/query-builder.ts (1)
889-902: Remove unused variablehasSelectand add missingincludeSpecialColumns.Two issues:
hasSelectis declared but never used (same as inexecute).processQueryResponseis missingincludeSpecialColumns, whichexecutedoes pass (line 785). This causes inconsistent behavior between direct execution and batch processing.const mergedOptions = this.mergeExecuteOptions(options); - // Check if select was applied (runtime check) - const hasSelect = this.queryOptions.select !== undefined; - return processQueryResponse(rawData, { occurrence: this.occurrence, singleMode: this.singleMode, queryOptions: this.queryOptions as any, expandConfigs: this.expandConfigs, skipValidation: options?.skipValidation, useEntityIds: mergedOptions.useEntityIds, + includeSpecialColumns: mergedOptions.includeSpecialColumns, fieldMapping: this.fieldMapping, logger: this.logger, });
🧹 Nitpick comments (4)
packages/fmodata/src/client/delete-builder.ts (2)
29-43: Same issue as UpdateBuilder:databaseIncludeSpecialColumnsis stored but never used.The property is added but never propagated to
ExecutableDeleteBuilderand isn't used elsewhere in this file. Delete operations return{ deletedCount }, so special columns are likely not applicable. Consider removing this dead code to avoid confusion.export class DeleteBuilder<Occ extends FMTable<any, any>> { private databaseName: string; private context: ExecutionContext; private table: Occ; private databaseUseEntityIds: boolean; - private databaseIncludeSpecialColumns: boolean; constructor(config: { occurrence: Occ; databaseName: string; context: ExecutionContext; databaseUseEntityIds?: boolean; - databaseIncludeSpecialColumns?: boolean; }) { this.table = config.occurrence; this.databaseName = config.databaseName; this.context = config.context; this.databaseUseEntityIds = config.databaseUseEntityIds ?? false; - this.databaseIncludeSpecialColumns = - config.databaseIncludeSpecialColumns ?? false; }
5-5: Unused importWithSpecialColumns.This type is imported but not used in the file.
import type { ExecutionContext, ExecutableBuilder, Result, - WithSpecialColumns, ExecuteOptions, ExecuteMethodOptions, } from "../types";packages/fmodata/tests/include-special-columns.test.ts (1)
13-13: Unused import.
firstis imported fromes-toolkit/compatbut never used in this file.-import { first } from "es-toolkit/compat";packages/fmodata/src/client/query/query-builder.ts (1)
775-777: Remove unused variablehasSelect.This variable is declared but never used. It appears to be leftover from development or intended for future use that wasn't completed.
- // Check if select was applied (runtime check) - const hasSelect = this.queryOptions.select !== undefined; - return processQueryResponse(result.data, {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
packages/fmodata/README.md(2 hunks)packages/fmodata/src/client/builders/default-select.ts(2 hunks)packages/fmodata/src/client/builders/query-string-builder.ts(1 hunks)packages/fmodata/src/client/builders/response-processor.ts(7 hunks)packages/fmodata/src/client/database.ts(3 hunks)packages/fmodata/src/client/delete-builder.ts(2 hunks)packages/fmodata/src/client/entity-set.ts(11 hunks)packages/fmodata/src/client/filemaker-odata.ts(4 hunks)packages/fmodata/src/client/insert-builder.ts(3 hunks)packages/fmodata/src/client/query/query-builder.ts(19 hunks)packages/fmodata/src/client/query/response-processor.ts(3 hunks)packages/fmodata/src/client/query/types.ts(1 hunks)packages/fmodata/src/client/record-builder.ts(19 hunks)packages/fmodata/src/client/update-builder.ts(3 hunks)packages/fmodata/src/types.ts(4 hunks)packages/fmodata/src/validation.ts(8 hunks)packages/fmodata/tests/include-special-columns.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
packages/fmodata/src/client/query/response-processor.ts (1)
packages/fmodata/src/validation.ts (1)
validateListResponse(455-513)
packages/fmodata/tests/include-special-columns.test.ts (2)
packages/fmodata/tests/utils/test-setup.ts (1)
createMockClient(211-216)packages/fmodata/tests/utils/mock-fetch.ts (1)
simpleMock(163-174)
packages/fmodata/src/client/builders/response-processor.ts (1)
packages/fmodata/src/validation.ts (1)
validateSingleResponse(518-577)
packages/fmodata/src/client/builders/default-select.ts (3)
packages/fmodata/src/index.ts (1)
FMTable(19-19)packages/fmodata/src/orm/index.ts (1)
FMTable(46-46)packages/fmodata/src/orm/table.ts (1)
FMTable(98-180)
packages/fmodata/src/client/query/types.ts (1)
packages/fmodata/src/client/builders/shared-types.ts (1)
ExpandedRelations(18-18)
packages/fmodata/src/client/builders/query-string-builder.ts (1)
packages/fmodata/src/client/builders/select-utils.ts (1)
formatSelectFields(34-56)
packages/fmodata/src/validation.ts (2)
packages/fmodata/src/types.ts (1)
ODataRecordMetadata(70-73)packages/fmodata/src/index.ts (1)
ODataRecordMetadata(83-83)
packages/fmodata/src/client/delete-builder.ts (1)
packages/fmodata/src/types.ts (1)
ExecutionContext(32-47)
packages/fmodata/src/client/entity-set.ts (5)
packages/fmodata/src/types.ts (2)
EntitySet(338-341)ExecutionContext(32-47)packages/fmodata/src/orm/table.ts (1)
FMTable(98-180)packages/fmodata/src/client/database.ts (1)
Database(9-206)packages/fmodata/src/client/query/query-builder.ts (1)
QueryBuilder(62-904)packages/fmodata/src/client/record-builder.ts (1)
RecordBuilder(88-807)
packages/fmodata/src/client/database.ts (1)
packages/fmodata/src/client/entity-set.ts (1)
EntitySet(44-444)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Cursor Bugbot
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Cursor Bugbot
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (28)
packages/fmodata/README.md (1)
1445-1475: Clear and comprehensive documentation for the new feature.The documentation effectively explains:
- How to enable special columns at database level
- How to override at request level
- The important caveat about
$selectinteractionpackages/fmodata/src/client/builders/default-select.ts (1)
36-48: Implementation looks correct for "schema" mode.The logic properly:
- Filters out container fields
- Appends ROWID and ROWMODID only when
includeSpecialColumnsis true- Returns the combined field list
Note: Special columns are only added when
defaultSelectis"schema". For explicit array or object-baseddefaultSelect, users must manually include special columns if needed. This aligns with the documentation that states special columns are included "when no$selectquery is applied."packages/fmodata/tests/include-special-columns.test.ts (1)
1-540: Comprehensive test coverage for the feature.The test suite thoroughly covers:
- Database-level configuration (enabled/disabled/default)
- Request-level overrides
- Interaction with
defaultSelectmodes- Various query methods (
list(),get(),single(),getSingleField())- Combination with
useEntityIds- Explicit selection of special columns via
select()second argument- Both compile-time type checks (
expectTypeOf) and runtime assertionsThis provides excellent confidence in the feature implementation.
packages/fmodata/src/client/builders/response-processor.ts (2)
12-23: Clean interface extension forincludeSpecialColumns.The
ProcessResponseConfiginterface is properly extended, and the property is consistently propagated throughprocessODataResponseandprocessQueryResponse.
72-82: Helpful clarifying comment.The comment on lines 72-74 provides good context about the behavioral difference between
QueryBuilder.single()andRecordBuilder.get()regarding special columns, even though both usesingleMode: "exact". This helps future maintainers understand the design decision.packages/fmodata/src/client/update-builder.ts (1)
38-56:databaseIncludeSpecialColumnsis stored but never used.The property is added to
UpdateBuilderbut is never passed toExecutableUpdateBuilderinbyId()orwhere()methods, andExecutableUpdateBuilderdoesn't have this property either. If update operations don't need special columns support, consider removing this dead code. Otherwise, propagate it through toExecutableUpdateBuilder.#!/bin/bash # Check if databaseIncludeSpecialColumns is used anywhere in update-builder.ts rg -n "databaseIncludeSpecialColumns" packages/fmodata/src/client/update-builder.tspackages/fmodata/src/client/query/response-processor.ts (1)
218-237: TheincludeSpecialColumnsconfig is ignored forsingle()method—special columns are unconditionally excluded regardless of configuration.The logic at line 220 sets
shouldIncludeSpecialColumns = falsefor all single/maybeSingle modes, forcing special columns to be filtered out byvalidateRecordregardless of theconfig.includeSpecialColumnssetting. However, the test at lines 436–477 expectssingle()to return ROWID and ROWMODID when the database is configured withincludeSpecialColumns: true. Either the config should be respected for single mode, or the test expectations should be adjusted to match the OData spec behavior mentioned in the comment.packages/fmodata/src/client/insert-builder.ts (1)
55-75: LGTM: Scaffolding for includeSpecialColumns added correctly.The
databaseIncludeSpecialColumnsfield is properly declared and initialized. As noted in the summary, this is scaffolding that isn't yet wired into the execute/processResponse logic, which is acceptable since insert operations already return ROWID via the Location header.packages/fmodata/src/client/database.ts (2)
9-11: LGTM: Generic parameter design is sound.The
IncludeSpecialColumnsgeneric parameter with defaultfalsecorrectly establishes a compile-time flag that propagates throughEntitySetand downstream builders. This enables type-safe conditional inclusion of ROWID/ROWMODID in result types.
40-58: Clean propagation of generic parameter throughfrom()method.The method signature and EntitySet instantiation correctly thread
IncludeSpecialColumnsthrough, enabling downstream type inference.packages/fmodata/src/client/query/types.ts (2)
73-89: Well-designed conditional type utilities for system columns.
SystemColumnsOptionprovides flexible per-field control, andSystemColumnsFromOptioncorrectly derives the result shape using intersection types. The conditional checks for{ ROWID: true }and{ ROWMODID: true }are properly structured.
91-128: Consistent integration of SystemCols into QueryReturnType.The new generic parameter is correctly threaded through all branches with
& SystemColumnsFromOption<SystemCols>, maintaining backward compatibility (defaults to{}when undefined) while enabling type-safe special column inclusion.packages/fmodata/src/client/builders/query-string-builder.ts (1)
26-46: Correct implementation of special columns injection into $select.The logic properly:
- Creates a copy to avoid mutating the input
- Checks for existing fields to prevent duplicates
- Only activates when
includeSpecialColumnsis true and there are selected fieldspackages/fmodata/src/client/filemaker-odata.ts (2)
122-139: Correct composite Prefer header construction.The array-based approach with
join(", ")follows HTTP spec for multiple Prefer values. The conditional pushing ensures only enabled preferences are included.
306-314: Generic database factory correctly typed.The method signature enables compile-time tracking of
includeSpecialColumnsthrough the returnedDatabase<IncludeSpecialColumns>, which then propagates to EntitySet and builders.packages/fmodata/src/types.ts (3)
55-61: Terminology alignment: WithSystemFields → WithSpecialColumns.The rename aligns with FileMaker's terminology for ROWID/ROWMODID as "special columns" rather than "system fields," improving clarity.
230-237: NormalizeIncludeSpecialColumns handles per-request overrides correctly.The tuple wrapping prevents distribution over boolean unions, and the fallback to
DatabaseDefaultenables proper precedence: explicit request value > database default.
248-270: ConditionallyWithSpecialColumns correctly implements the business rules.The type properly enforces:
- Only applies when
IncludeSpecialColumnsis true ANDHasSelectis false- Handles arrays by augmenting element types
- Excludes primitives (e.g., count queries returning
number)This aligns with the documented behavior that special columns are only included when no
$selectquery is applied.packages/fmodata/src/validation.ts (3)
116-131: Consistent handling of special columns in no-schema validation path.The pattern of extracting ROWID/ROWMODID via destructuring, conditionally populating a
specialColumnsobject, and merging at the end is clean and maintainable.
244-251: Verify: includeSpecialColumns not propagated to expanded relation validation.Recursive
validateRecordcalls for expanded relations don't passincludeSpecialColumns. This appears intentional since expanded items are nested records where ROWID/ROWMODID semantics may differ. Confirm this matches FileMaker OData behavior for expanded relations.
455-496: validateListResponse correctly propagates includeSpecialColumns.The parameter is properly threaded through to each
validateRecordcall in the loop.packages/fmodata/src/client/entity-set.ts (3)
44-77: LGTM: Generic parameter and field initialization are correct.The
DatabaseIncludeSpecialColumnsgeneric properly defaults tofalseand is initialized from the database instance at runtime. Theas anycast for accessing_includeSpecialColumnsis necessary since it's a private field.
139-162: Correct conditional inclusion of special columns in list().When
databaseIncludeSpecialColumnsis true anddefaultSelect="schema", thesystemColumnsobject is passed toselect(), enabling ROWID/ROWMODID in the query. The return type cast correctly includestypeof systemColumnsfor proper type inference.
386-442: navigate() correctly preserves DatabaseIncludeSpecialColumns through navigation chains.The generic parameter is threaded through the return type and EntitySet construction, ensuring the database-level setting applies consistently across chained navigations.
packages/fmodata/src/client/record-builder.ts (2)
1-171: Well-structured propagation of special columns configuration.The introduction of
DatabaseIncludeSpecialColumnsandSystemColsgeneric parameters, along with the updatedmergeExecuteOptionshelper, provides a clean way to handle both database-level defaults and per-request overrides for special columns. The type-level integration withConditionallyWithSpecialColumnsandNormalizeIncludeSpecialColumnsensures type safety.
303-353: Clean API for requesting system columns viaselect().The second parameter
systemColumns?: TSystemColsprovides an intuitive way to request ROWID/ROWMODID when using explicit field selection. The implementation correctly appends these tofinalSelectedFieldsand propagates the type information throughTSystemCols.packages/fmodata/src/client/query/query-builder.ts (2)
234-298: Clean implementation of system columns inselect().The approach of accepting
systemColumnsas a second parameter and appending ROWID/ROWMODID tofinalSelectedFieldsis consistent with the record-builder implementation. Type propagation throughTSystemColsensures proper inference.
62-130: Consistent generic parameter expansion across QueryBuilder.The addition of
DatabaseIncludeSpecialColumnsandSystemColsgeneric parameters follows the same pattern asRecordBuilder, maintaining consistency across the codebase. The constructor properly initializesdatabaseIncludeSpecialColumnswith a default offalse.
|
Cursor Agent can help with this pull request. Just |
There was a problem hiding this comment.
Bug: Batch query responses drop special columns
QueryBuilder.processResponse() merges includeSpecialColumns, but does not pass it into processQueryResponse(). In batch/toRequest() flows this makes validation/processing behave as if special columns were disabled, so returned ROWID/ROWMODID can be stripped or missing despite the request being configured to include them.
packages/fmodata/src/client/query/query-builder.ts#L888-L902
proofkit/packages/fmodata/src/client/query/query-builder.ts
Lines 888 to 902 in dba69ec
Bug: Batch record responses ignore special columns option
RecordBuilder.processResponse() uses mergeExecuteOptions() (use-entity-ids only) and never forwards includeSpecialColumns into processODataResponse(). In batch/toRequest() flows this causes special columns handling to be skipped, so ROWID/ROWMODID may be dropped even when enabled at the database or request level.
packages/fmodata/src/client/record-builder.ts#L746-L805
proofkit/packages/fmodata/src/client/record-builder.ts
Lines 746 to 805 in dba69ec
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/fmodata/src/client/builders/query-string-builder.ts (1)
20-20: Clarify the purpose of the unusedincludeSpecialColumnsparameter.The
includeSpecialColumnsparameter is added to the function signature but is never referenced in the function body. The comment at lines 27-31 explains that system columns should NOT be implicitly added here, suggesting the parameter may be for API consistency or documentation.Consider:
- If the parameter is only for signature consistency, document this in a JSDoc comment
- If it's not needed, consider removing it to avoid confusion
- If it's for future use, add a comment indicating this intent
#!/bin/bash # Verify where buildSelectExpandQueryString is called and whether includeSpecialColumns is passed rg -n "buildSelectExpandQueryString" --type=ts -A 10 -B 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/fmodata/src/client/builders/query-string-builder.ts(1 hunks)packages/fmodata/tests/include-special-columns.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/fmodata/src/client/builders/query-string-builder.ts (2)
packages/fmodata/src/client/builders/expand-builder.ts (1)
ExpandBuilder(21-245)packages/fmodata/src/client/query/expand-builder.ts (1)
ExpandBuilder(22-164)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Cursor Bugbot
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (4)
packages/fmodata/tests/include-special-columns.test.ts (3)
1-119: LGTM: Well-structured test setup and initial test cases.The test file setup is clean, and the first two test cases provide good coverage:
- Verifying the Prefer header is correctly set
- Validating $select parameter behavior with different defaultSelect settings
- Checking both compile-time types and runtime values
The combination of type assertions (expectTypeOf) and runtime expectations provides robust verification.
121-347: LGTM: Comprehensive coverage of configuration scenarios.Excellent test coverage for:
- Explicit disablement at database level
- Default behavior verification
- Per-request overrides (lines 197-289) with three distinct scenarios
- Integration with useEntityIds feature (lines 291-347)
The Prefer header validation at lines 327-334 properly verifies comma-separated header values, ensuring correct multi-preference formatting.
436-567: LGTM: Excellent edge case coverage.The remaining test cases provide thorough coverage of important scenarios:
- Query string generation with explicit systemColumns parameter (lines 436-462)
- single() method behavior (lines 464-505)
- getSingleField() correctly excludes special columns from single-value responses (lines 507-535)
- Explicit selection of system columns without database-level flag (lines 537-567)
These edge cases ensure the feature behaves correctly across different API usage patterns.
packages/fmodata/src/client/builders/query-string-builder.ts (1)
27-31: LGTM: Clear documentation of system column handling behavior.The comment block effectively explains the design decision that
includeSpecialColumnscontrols the Prefer header and response parsing, but does NOT implicitly mutate an explicit$selectparameter. This distinction is important and well-documented.
Co-authored-by: eric.luce <eric.luce@proofgeist.com>
eluce2
force-pushed
the
12-17-specialcolumns_header
branch
from
dba69ec to
e14a204
Compare
Co-authored-by: eric.luce <eric.luce@proofgeist.com>
| data: { ...validatedRecord, ...specialColumns, ...metadata } as T & | ||
| ODataRecordMetadata, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Bug: Selected ROWID/ROWMODID dropped during validation
validateRecord strips ROWID/ROWMODID unless includeSpecialColumns is true. When a caller explicitly requests these via $select (e.g., select(..., { ROWID: true })), selectedFields includes them but they aren’t in schema, so they get skipped and never re-added, causing the returned records to miss the requested columns.
There was a problem hiding this comment.
Bug: Batch query parsing ignores includeSpecialColumns option
In QueryBuilder.processResponse, mergedOptions is computed but includeSpecialColumns is not forwarded into processQueryResponse. This makes response validation/processing drop ROWID/ROWMODID in batch-driven flows even when the request enabled includeSpecialColumns.
packages/fmodata/src/client/query/query-builder.ts#L888-L902
proofkit/packages/fmodata/src/client/query/query-builder.ts
Lines 888 to 902 in e14a204
…fgeist/proofkit into 12-17-specialcolumns_header
There was a problem hiding this comment.
Bug: Special columns lost in query processResponse
QueryBuilder.processResponse() computes mergedOptions but does not pass includeSpecialColumns into processQueryResponse(). This makes batch/processResponse flows ignore includeSpecialColumns (and may drop/strip ROWID/ROWMODID during validation) even when database/request-level enablement is set.
packages/fmodata/src/client/query/query-builder.ts#L888-L902
proofkit/packages/fmodata/src/client/query/query-builder.ts
Lines 888 to 902 in 62429e3
Bug: Record processResponse ignores special columns option
RecordBuilder.processResponse() uses the shared mergeExecuteOptions() (which only merges useEntityIds) and then calls processODataResponse() without includeSpecialColumns. As a result, includeSpecialColumns is not honored in processResponse (notably in batch flows), diverging from RecordBuilder.execute() behavior.
packages/fmodata/src/client/record-builder.ts#L782-L805
proofkit/packages/fmodata/src/client/record-builder.ts
Lines 782 to 805 in 62429e3
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/fmodata/tests/typescript.test.ts (1)
558-573: Well-structured type-level test with a minor cleanup opportunity.The test correctly validates that primary key fields are non-nullable in the inferred schema using
@ts-expect-errorassertions. This aligns well with the changes toprimaryKey()in field-builders.ts.Apply this diff to remove the unused variable:
- const controlTest: string | null = null; - // @ts-expect-error - id should not be nullable const idData: IdField = null;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/fmodata/src/orm/field-builders.ts(1 hunks)packages/fmodata/tests/typescript.test.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/fmodata/tests/typescript.test.ts (4)
packages/fmodata/src/index.ts (3)
fmTableOccurrence(18-18)textField(9-9)InferTableSchema(21-21)packages/fmodata/src/orm/index.ts (3)
fmTableOccurrence(45-45)textField(4-4)InferTableSchema(48-48)packages/fmodata/src/orm/table.ts (2)
fmTableOccurrence(315-463)InferTableSchema(494-497)packages/fmodata/src/orm/field-builders.ts (1)
textField(166-175)
packages/fmodata/src/orm/field-builders.ts (2)
packages/fmodata/src/index.ts (1)
FieldBuilder(16-16)packages/fmodata/src/orm/index.ts (1)
FieldBuilder(3-3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Cursor Bugbot
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (2)
packages/fmodata/tests/typescript.test.ts (1)
21-21: LGTM: Appropriate imports for type-level testing.The addition of
expectTypeOfandInferTableSchemasupports the new test suite that validates primary key non-nullability at the type level.Also applies to: 31-31
packages/fmodata/src/orm/field-builders.ts (1)
39-52: LGTM: Correctly enforces primary key non-nullability.The implementation properly enforces that primary keys are non-nullable at both the type level (via
NonNullable<>wrappers) and runtime level (via the_notNullflag). This constraint aligns with standard database practices where primary keys should never be null.The updated documentation accurately reflects this behavior, and the corresponding test suite in typescript.test.ts validates the type-level enforcement.
There was a problem hiding this comment.
Bug: Batch response path ignores includeSpecialColumns
In processResponse() (batch/manual response handling), includeSpecialColumns is not forwarded into processQueryResponse() (and RecordBuilder.processResponse() doesn’t use its own merge that includes includeSpecialColumns or pass it into processODataResponse). This makes batch processing inconsistent with .execute() and can drop ROWID/ROWMODID unexpectedly.
packages/fmodata/src/client/query/query-builder.ts#L888-L902
proofkit/packages/fmodata/src/client/query/query-builder.ts
Lines 888 to 902 in b57a2a6
packages/fmodata/src/client/record-builder.ts#L782-L805
proofkit/packages/fmodata/src/client/record-builder.ts
Lines 782 to 805 in b57a2a6
- Updated the handling of ROWID and ROWMODID to use destructured values when available. - Maintained original response inclusion for other fields not in the schema.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/fmodata/src/validation.ts (1)
256-261: Critical: includeSpecialColumns not propagated to expanded relations.The
includeSpecialColumnsparameter is not passed through recursivevalidateRecordcalls for expanded relations at lines 256, 280, 406, and 430. This means special columns will always be excluded from expanded/nested records, even whenincludeSpecialColumnsis true at the top level.Apply this diff to propagate the flag through all recursive calls:
const itemValidation = await validateRecord( item, expandConfig.targetSchema, expandConfig.selectedFields as string[] | undefined, expandConfig.nestedExpands, + includeSpecialColumns, );const itemValidation = await validateRecord( expandValue, expandConfig.targetSchema, expandConfig.selectedFields as string[] | undefined, expandConfig.nestedExpands, + includeSpecialColumns, );Apply the same fix at all four locations (lines 256, 280, 406, 430).
Also applies to: 280-285, 406-411, 430-435
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/fmodata/src/validation.ts(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/fmodata/src/validation.ts (1)
packages/fmodata/src/types.ts (1)
ODataRecordMetadata(70-73)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Cursor Bugbot
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (1)
packages/fmodata/src/validation.ts (1)
192-203: LGTM! Explicit special column selection now works correctly.This code correctly addresses the previous bug where explicitly selected ROWID/ROWMODID were dropped during validation. When these fields are in
selectedFields, they are now retrieved from the destructured values (since they were removed fromrestearlier).Note: Explicit selection via
selectedFieldstakes precedence overincludeSpecialColumnsflag, which is the correct behavior.
There was a problem hiding this comment.
Bug: Special columns ignored in batch responses
QueryBuilder.processResponse() and RecordBuilder.processResponse() don’t pass includeSpecialColumns into processQueryResponse/processODataResponse. In batch execution paths, this drops the ROWID/ROWMODID fields (now stripped by validateRecord unless enabled) even when database/request options enable special columns, causing inconsistent behavior vs non-batch .execute().
packages/fmodata/src/client/query/query-builder.ts#L888-L902
proofkit/packages/fmodata/src/client/query/query-builder.ts
Lines 888 to 902 in 2d3e9ab
packages/fmodata/src/client/record-builder.ts#L782-L805
proofkit/packages/fmodata/src/client/record-builder.ts
Lines 782 to 805 in 2d3e9ab
Bug: Expanded records drop special columns
When validating expanded relations, recursive calls to validateRecord() don’t forward includeSpecialColumns. If expanded records contain ROWID/ROWMODID (e.g., when special columns are enabled and no $select applies for the expand), validation strips them, so special columns can disappear only in expanded data.
packages/fmodata/src/validation.ts#L255-L285
proofkit/packages/fmodata/src/validation.ts
Lines 255 to 285 in 2d3e9ab
packages/fmodata/src/validation.ts#L405-L435
proofkit/packages/fmodata/src/validation.ts
Lines 405 to 435 in 2d3e9ab
Bug: Database listing endpoint likely incorrect
listDatabaseNames() now requests /$metadata with Accept: application/json and still expects a JSON payload with value: [{ name }]. In OData, /$metadata is typically CSDL (XML), while the service document is at the service root. This can cause JSON parse errors or always return an empty list.
packages/fmodata/src/client/filemaker-odata.ts#L319-L330
proofkit/packages/fmodata/src/client/filemaker-odata.ts
Lines 319 to 330 in 2d3e9ab
…s; enhance validation logic by including special columns in validation functions and record builders.
| {}, | ||
| DatabaseIncludeSpecialColumns, | ||
| typeof systemColumns | ||
| >; |
There was a problem hiding this comment.
Bug: Request override ignored with default schema select
Per-request execute({ includeSpecialColumns: true }) doesn’t result in ROWID/ROWMODID being requested when defaultSelect is "schema" and the database default is off, because systemColumns is derived only from databaseIncludeSpecialColumns during builder construction. This causes the header to be sent but the implicit $select to omit the special columns, so FileMaker won’t return them.
Additional Locations (1)
There was a problem hiding this comment.
Bug: listDatabaseNames now queries metadata endpoint
listDatabaseNames() was switched to request "/$metadata" and parse a JSON { value: [{ name }] } payload. OData $metadata is typically an XML document (service root lists entity sets), so this change is likely to return non-JSON and/or not contain value, causing database listing to break at runtime.
packages/fmodata/src/client/filemaker-odata.ts#L319-L330
proofkit/packages/fmodata/src/client/filemaker-odata.ts
Lines 319 to 330 in f66f2e4
eluce2
changed the base branch from
12-13-typegen_web_ui
to
graphite-base/79
Note
Adds opt-in support to include FileMaker special columns (ROWID, ROWMODID) via database or per-request option, propagates through builders/types, and documents with comprehensive tests.
includeSpecialColumnssupport acrossFMServerConnection(Prefer header),Database,EntitySet,QueryBuilder, andRecordBuilder; respects per-request overrides and navigation.$select; allow explicit include viaselect(..., { ROWID: true, ROWMODID: true }).schemamode can appendROWID/ROWMODIDwhen enabled; excluded when$selectis present.ConditionallyWithSpecialColumns,NormalizeIncludeSpecialColumns, and system column option/types; plumb through query/record return types.FieldBuilder.primaryKey()are now non-nullable at the type level.include-special-columnssuite covering headers, selection behavior, single/list, and entity-ids interplay; TypeScript tests for non-nullable PKs.Written by Cursor Bugbot for commit f66f2e4. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes / Type improvements
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.