Split fmodata count into count-only and counted list#210
Merged
Conversation
- add count-only `db.from(table).count()`
- keep `list().count()` for `{ records, count }`
🦋 Changeset detectedLatest commit: 98d2542 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@proofkit/better-auth
@proofkit/cli
create-proofkit
@proofkit/fmdapi
@proofkit/fmodata
@proofkit/typegen
@proofkit/webviewer
commit: |
📝 WalkthroughWalkthroughSplit the fmodata count API: Changes
Sequence DiagramsequenceDiagram
participant Client
participant EntitySet
participant CountBuilder
participant QueryBuilder
participant ResponseProcessor
participant FMODataServer
rect rgba(100,200,150,0.5)
Note over Client,FMODataServer: Count-Only Flow: db.from(table).count()
Client->>EntitySet: count()
EntitySet->>CountBuilder: instantiate
Client->>CountBuilder: where(...)? / execute()
CountBuilder->>FMODataServer: GET /table/$count[?$filter=...]
FMODataServer-->>CountBuilder: "5" (plain text)
CountBuilder->>ResponseProcessor: processResponse(response)
ResponseProcessor-->>CountBuilder: { data: 5, error: undefined }
CountBuilder-->>Client: Result<number>
end
rect rgba(150,150,200,0.5)
Note over Client,FMODataServer: List-With-Count Flow: db.from(table).list().count()
Client->>EntitySet: list()
EntitySet->>QueryBuilder: instantiate
Client->>QueryBuilder: top(...)/skip(...)/count()/execute()
QueryBuilder->>FMODataServer: GET /table?$top=...&$skip=...&$count=true[&$filter=...]
FMODataServer-->>QueryBuilder: { "@odata.count": 150, "value": [ ... ] }
QueryBuilder->>ResponseProcessor: processQueryResponse(response, includeCount=true)
ResponseProcessor-->>QueryBuilder: { data: { records: [...], count: 150 }, error: undefined }
QueryBuilder-->>Client: Result<CountedListResult>
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/fmodata/src/client/builders/response-processor.ts`:
- Around line 280-309: The counted-response branch currently ignores
processedResponse.error and can return a successful CountedListResult with
records undefined; update the includeCount handling in the function that
processes processODataResponse results to first check if processedResponse.error
exists and, if so, return { data: undefined, error: processedResponse.error }
before building the { records, count } result (preserve existing singleMode and
`@odata.count` validation and ResponseStructureError behavior). Ensure you
reference processedResponse, includeCount, singleMode, and the CountedListResult
wrapper when applying the guard so original validation/structure errors
propagate.
In `@packages/fmodata/src/client/count-builder.ts`:
- Around line 101-105: The navigated count URLs are built without the "/$count"
suffix because QueryUrlBuilder.build() returns navigation paths before applying
the isCount branch; update the code paths that construct count requests (methods
execute, getRequestConfig, and getQueryString usage) to call the path-building
logic that respects isCount or to use buildPath() for navigations so the isCount
flag results in a "/$count" segment; specifically, ensure
QueryUrlBuilder.build(queryString, { isCount: true, useEntityIds: ...,
navigation: this.navigationConfig }) produces a path with "/$count" by moving
the isCount handling ahead of the navigation-return branch or by delegating
navigation cases to buildPath() which already appends "/$count".
In `@packages/fmodata/src/types.ts`:
- Around line 11-12: The interface ExecutableBuilder<_T> is erasing its generic
by returning Result<unknown>; update the signatures to use the generic type
parameter (e.g., change execute(options?: ExecuteOptions):
Promise<Result<unknown>> to return Promise<Result<T>>) and likewise update
processResponse (and any other methods on ExecutableBuilder) to use T instead of
unknown so implementations like UpdateBuilder/QueryBuilder/InsertBuilder
correctly preserve their concrete type parameter through the API boundary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f62c5b5a-6052-4fab-9d4f-a2fc4b86ba76
📒 Files selected for processing (21)
.changeset/split-fmodata-count-api.mdapps/docs/content/docs/fmodata/methods.mdxapps/docs/content/docs/fmodata/queries.mdxpackages/fmodata/README.mdpackages/fmodata/src/client/batch-builder.tspackages/fmodata/src/client/builders/read-builder-state.tspackages/fmodata/src/client/builders/response-processor.tspackages/fmodata/src/client/count-builder.tspackages/fmodata/src/client/entity-set.tspackages/fmodata/src/client/query/index.tspackages/fmodata/src/client/query/query-builder.tspackages/fmodata/src/client/query/types.tspackages/fmodata/src/client/query/url-builder.tspackages/fmodata/src/client/record-builder.tspackages/fmodata/src/index.tspackages/fmodata/src/types.tspackages/fmodata/tests/batch.test.tspackages/fmodata/tests/e2e/e2e.test.tspackages/fmodata/tests/mock.test.tspackages/fmodata/tests/query-strings.test.tspackages/fmodata/tests/typescript.test.ts
- preserve query errors before counted result shaping - keep typed execute/processResponse returns - fix navigated /$count URL generation
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| // biome-ignore lint/suspicious/noExplicitAny: Accepts any FMTable configuration | ||
| Occ extends FMTable<any, any>, | ||
| Selected, | ||
| > = Selected extends Record<string, Column<any, any, any>> // biome-ignore lint/suspicious/noExplicitAny: Generic constraint accepting any Column configuration |
Contributor
There was a problem hiding this comment.
The biome-ignore comment for noExplicitAny should be moved to the line above (line 71) instead of being at the end of line 72, so it properly suppresses the lint warning for the 'any' types in 'Column<any, any, any>'
Spotted by Graphite (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
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/query/query-builder.ts (2)
1015-1030:⚠️ Potential issue | 🟡 MinorReturn the counted-list shape from the 204 fallback.
Once
includeCountModeis on,processResponse()is typed to return{ records, count }, but this branch still returns a bare array. Any caller that round-trips a counted list throughtoRequest()/processResponse()will get a runtime shape mismatch here.🐛 Proposed fix
if (response.status === 204) { // Return empty list for list queries, null for single queries if (this.readState.singleMode !== false) { if (this.readState.singleMode === "maybe") { // biome-ignore lint/suspicious/noExplicitAny: Type assertion for generic return type return { data: null as any, error: undefined }; } return { data: undefined, error: new RecordCountMismatchError("one", 0), }; } + if (this.readState.includeCountMode) { + return { + data: { records: [], count: 0 } as any, + error: undefined, + }; + } // biome-ignore lint/suspicious/noExplicitAny: Type assertion for generic return type return { data: [] as any, error: undefined }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/src/client/query/query-builder.ts` around lines 1015 - 1030, The 204-response branch in processResponse() returns a bare array for list queries but must return the counted-list shape when includeCountMode is enabled; update the list-return branch (when this.readState.singleMode is false) to check this.readState.includeCountMode and return { records: [] , count: 0 } (typed to the generic) instead of [] so callers that expect { records, count } get the correct shape; keep existing handling for singleMode/“maybe” and the RecordCountMismatchError for the single-item case.
689-728:⚠️ Potential issue | 🟠 MajorPin
IncludeCounttofalseinexpand()callback return type.The callback return type permits
anyfor the sixth generic parameter (IncludeCount), allowingbuilder.count()to type-check inside expand callbacks. However, expanded relations are modeled as plain nested records without a{ records, count }wrapper, making this state unsupported end-to-end. This creates a silent type-safety gap where valid TypeScript code produces incorrect runtime behavior.Enforce
IncludeCount: falsein both the callback return type and the factory-created QueryBuilder to prevent count mode in nested expands unless full end-to-end support is added.♻️ Proposed fix
callback?: ( // biome-ignore lint/complexity/noBannedTypes: Empty object type represents no expands in initial builder builder: QueryBuilder<TargetTable, keyof InferSchemaOutputFromFMTable<TargetTable>, false, false, {}, false>, // biome-ignore lint/suspicious/noExplicitAny: Generic constraint accepting any QueryBuilder configuration - ) => QueryBuilder<TargetTable, TSelected, any, any, TNestedExpands, any, any>, + ) => QueryBuilder< + TargetTable, + TSelected, + any, + any, + TNestedExpands, + false, + DatabaseIncludeSpecialColumns, + any + >, ): QueryBuilder< @@ callback as ((builder: TargetBuilder) => TargetBuilder) | undefined, () => // biome-ignore lint/suspicious/noExplicitAny: Generic constraint accepting any QueryBuilder configuration - new QueryBuilder<TargetTable, any, any, any, any, any, DatabaseIncludeSpecialColumns, undefined>({ + new QueryBuilder<TargetTable, any, any, any, any, false, DatabaseIncludeSpecialColumns, undefined>({ occurrence: targetTable, layer: this.layer, }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/src/client/query/query-builder.ts` around lines 689 - 728, The expand callback currently allows the sixth generic (IncludeCount) to be any, enabling builder.count() inside expand; update the callback signature passed to expand() and the QueryBuilder constructed in the processExpand call so the sixth generic is explicitly false (i.e., change the callback type from QueryBuilder<TargetTable, TSelected, any, any, TNestedExpands, any, any> to QueryBuilder<TargetTable, TSelected, any, any, TNestedExpands, false, any> and construct new QueryBuilder<TargetTable, any, any, any, any, false, DatabaseIncludeSpecialColumns, undefined>), ensuring IncludeCount is pinned to false in both the callback return type and the factory-created QueryBuilder used in expandBuilder.processExpand.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/fmodata/src/client/query/query-builder.ts`:
- Around line 1015-1030: The 204-response branch in processResponse() returns a
bare array for list queries but must return the counted-list shape when
includeCountMode is enabled; update the list-return branch (when
this.readState.singleMode is false) to check this.readState.includeCountMode and
return { records: [] , count: 0 } (typed to the generic) instead of [] so
callers that expect { records, count } get the correct shape; keep existing
handling for singleMode/“maybe” and the RecordCountMismatchError for the
single-item case.
- Around line 689-728: The expand callback currently allows the sixth generic
(IncludeCount) to be any, enabling builder.count() inside expand; update the
callback signature passed to expand() and the QueryBuilder constructed in the
processExpand call so the sixth generic is explicitly false (i.e., change the
callback type from QueryBuilder<TargetTable, TSelected, any, any,
TNestedExpands, any, any> to QueryBuilder<TargetTable, TSelected, any, any,
TNestedExpands, false, any> and construct new QueryBuilder<TargetTable, any,
any, any, any, false, DatabaseIncludeSpecialColumns, undefined>), ensuring
IncludeCount is pinned to false in both the callback return type and the
factory-created QueryBuilder used in expandBuilder.processExpand.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 59a45ace-c721-4d3d-8114-d6a9b5d3bbd1
📒 Files selected for processing (1)
packages/fmodata/src/client/query/query-builder.ts
- simplify codex environment config - run `pnpm i` during setup
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.codex/environments/environment.toml (1)
6-6: Use deterministic dependency install in setup script.On Line 6, replace
pnpm iwithpnpm install --frozen-lockfile. The repository has lockfiles (pnpm-lock.yamlat root and in packages/), so frozen installs ensure reproducible environment setup.Suggested change
[setup] -script = "pnpm i" +script = "pnpm install --frozen-lockfile"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.codex/environments/environment.toml at line 6, The setup script currently uses the non-deterministic command "pnpm i" in the environment.toml `script` entry; change that value to use frozen installs by replacing "pnpm i" with "pnpm install --frozen-lockfile" so installs respect the repository lockfiles and produce reproducible environments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.codex/environments/environment.toml:
- Line 6: The setup script currently uses the non-deterministic command "pnpm i"
in the environment.toml `script` entry; change that value to use frozen installs
by replacing "pnpm i" with "pnpm install --frozen-lockfile" so installs respect
the repository lockfiles and produce reproducible environments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4b3ec953-c9f7-4c3f-9dbd-db5858a05e9d
📒 Files selected for processing (1)
.codex/environments/environment.toml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
db.from(table).count()as a count-only flow against/$count.db.from(table).list().count()for one-request list plus total count, returning{ records, count }.@proofkit/fmodata.Testing
Summary by CodeRabbit
New Features
Documentation
Tests
Chores