Fix fmodata mutation Prefer header merging for special columns#212
Merged
Fix fmodata mutation Prefer header merging for special columns#212
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 7c971ff 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 |
@proofkit/better-auth
@proofkit/cli
create-proofkit
@proofkit/fmdapi
@proofkit/fmodata
@proofkit/typegen
@proofkit/webviewer
commit: |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThreads an Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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 docstrings
🧪 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 (3)
packages/fmodata/src/client/insert-builder.ts (2)
147-181:⚠️ Potential issue | 🟠 MajorCanonicalize
callerHeaderstoHeadersinstance before writing.The code reads
callerHeadersthroughnew Headers(...)but then spreadscallerHeadersas a plain object in the request. This is unsafe:
- If
callerHeadersis aHeadersinstance, the spread operator doesn't iterate its entries—caller headers are lost.- If
callerHeadersis a plain object, bothpreferandPreferkeys can coexist, and the mergedPrefervalue gets added separately, creating duplicates on the wire.Consistently use a
Headersinstance throughout to avoid data loss and conflicts.Suggested fix
+ const requestHeaders = new Headers(callerHeaders); const preferHeader = mergePreferHeaderValues( - new Headers(callerHeaders).get("Prefer") ?? undefined, + requestHeaders.get("Prefer") ?? undefined, this.returnPreference === "minimal" ? "return=minimal" : "return=representation", shouldUseIds ? "fmodata.entity-ids" : undefined, includeSpecialColumns ? "fmodata.include-specialcolumns" : undefined, ); + requestHeaders.set("Content-Type", "application/json"); + if (preferHeader) { + requestHeaders.set("Prefer", preferHeader); + } const pipeline = Effect.gen(this, function* () { // ... const responseData = yield* requestFromService<any>(url, { ...requestOptions, method: "POST", - headers: { - ...(callerHeaders || {}), - "Content-Type": "application/json", - ...(preferHeader ? { Prefer: preferHeader } : {}), - }, + headers: requestHeaders, body: JSON.stringify(transformedData), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/src/client/insert-builder.ts` around lines 147 - 181, The request currently mixes a raw callerHeaders value and a computed preferHeader string (see preferHeader, callerHeaders, mergePreferHeaderValues) which can drop or duplicate headers; fix by canonicalizing callerHeaders to a Headers instance once (e.g., const canonicalHeaders = new Headers(callerHeaders || {})), read/merge Prefer from that Headers (using mergePreferHeaderValues as done) and then pass headers to requestFromService as an object built from canonicalHeaders (e.g., Object.fromEntries(canonicalHeaders.entries()) or by iterating entries) while ensuring Prefer is set to preferHeader only once; update the code paths around preferHeader creation and the requestFromService call to use canonicalHeaders and derived plain-object headers consistently.
288-300:⚠️ Potential issue | 🟡 Minor
processResponse()return type doesn't reflect the request-level special-column override.Line 385 reads
options?.includeSpecialColumns, and line 417 passes it to validation. However, the Promise return type (line 292) is fixed toDatabaseIncludeSpecialColumns, unlike theexecute()method which usesNormalizeIncludeSpecialColumns<EO["includeSpecialColumns"], DatabaseIncludeSpecialColumns>. A caller usingprocessResponse(response, { includeSpecialColumns: true })on a DB configured withfalsewill getROWID/ROWMODIDat runtime but TypeScript won't reflect this. MakeprocessResponse()generic likeexecute():Suggested fix
- async processResponse( - response: Response, - options?: ExecuteOptions, - ): Promise< - Result< - ReturnPreference extends "minimal" - ? { ROWID: number } - : ConditionallyWithSpecialColumns< - InferSchemaOutputFromFMTable<NonNullable<Occ>>, - DatabaseIncludeSpecialColumns, - false - > - > - > { + async processResponse<EO extends ExecuteOptions>( + response: Response, + options?: EO, + ): Promise< + Result< + ReturnPreference extends "minimal" + ? { ROWID: number } + : ConditionallyWithSpecialColumns< + InferSchemaOutputFromFMTable<NonNullable<Occ>>, + NormalizeIncludeSpecialColumns<EO["includeSpecialColumns"], DatabaseIncludeSpecialColumns>, + false + > - > - > { + > + > {Also applies to: 383-418
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/src/client/insert-builder.ts` around lines 288 - 300, processResponse's return type doesn't honor the request-level includeSpecialColumns override; make processResponse generic over an ExecuteOptions type parameter (e.g. <EO extends ExecuteOptions | undefined>) like execute(), use NormalizeIncludeSpecialColumns<EO["includeSpecialColumns"], DatabaseIncludeSpecialColumns> in the resolved Promise return type, and thread EO through any internal validation calls (where options?.includeSpecialColumns is read) so the type reflects when a caller passes includeSpecialColumns: true; apply the same change to the other processResponse overloads mentioned (around lines 383–418) and ensure function signature and internal usages (options, validation calls) reference the new EO generic.packages/fmodata/src/client/update-builder.ts (1)
185-223:⚠️ Potential issue | 🟠 MajorUse a
Headersobject throughout to avoid case-variant header duplication.
Object.fromEntries(new Headers(callerHeaders).entries())normalizes the existing key to lowercaseprefer, but thenheaders.Prefer = preferHeaderadds an uppercase variant to the plain object. When both keys exist in the object passed to fetch, it can result in duplicatePreferheader values being sent, bypassing the merge deduplication logic.Suggested fix
- const headers: Record<string, string> = { "Content-Type": "application/json" }; + const requestHeaders = new Headers(callerHeaders); + requestHeaders.set("Content-Type", "application/json"); const preferHeader = mergePreferHeaderValues( - new Headers(callerHeaders).get("Prefer") ?? undefined, + requestHeaders.get("Prefer") ?? undefined, this.returnPreference === "representation" ? "return=representation" : undefined, shouldUseIds ? "fmodata.entity-ids" : undefined, includeSpecialColumns ? "fmodata.include-specialcolumns" : undefined, ); if (preferHeader) { - headers.Prefer = preferHeader; + requestHeaders.set("Prefer", preferHeader); + } else { + requestHeaders.delete("Prefer"); } ... - const requestHeaders = { - ...Object.fromEntries(new Headers(callerHeaders).entries()), - ...headers, - }; - const response = yield* requestFromService(url, { ...requestOptions, method: "PATCH", headers: requestHeaders, body: JSON.stringify(transformedData),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/src/client/update-builder.ts` around lines 185 - 223, The problem is header duplication caused by mixing a plain object and a Headers map (Object.fromEntries(new Headers(callerHeaders).entries()) plus headers.Prefer = ...). To fix, build and manipulate a Headers instance throughout: create a Headers from callerHeaders, set or update Prefer via headersObj.set("Prefer", preferHeader) (or delete if undefined), apply any other header entries from the existing plain headers map to that Headers instance, then pass that Headers instance as the request headers into requestFromService (replace requestHeaders plain-object usage with the Headers instance). Reference symbols: callerHeaders, headers (the plain headers map), preferHeader (from mergePreferHeaderValues), requestHeaders, and requestFromService.
🧹 Nitpick comments (1)
packages/fmodata/tests/include-special-columns.test.ts (1)
654-720: Add a mutation regression that merges an existing caller-suppliedPreferheader.These new cases only assert builder-generated values. The bug this PR is fixing is about preserving merged
Prefervalues, so please add at least oneexecute({ headers: { Prefer: ... } })case and assert the final header contains each token exactly once. That would cover the path most likely to regress here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/tests/include-special-columns.test.ts` around lines 654 - 720, Add a test variation to the existing "include special columns" insert and/or update tests that passes a caller-supplied Prefer header via execute({ headers: { Prefer: "..." } }) and asserts the final merged Prefer header (captured via the before hook's req.headers.get("Prefer")) contains each token exactly once (e.g., "return=representation, fmodata.include-specialcolumns" merged with an existing token like "handling=lenient" should become "return=representation, fmodata.include-specialcolumns, handling=lenient" with no duplicates). Locate the two existing tests using db.from(contactsTO).insert(...).execute(...) and db.from(contactsTO).update(...).execute(...) and add an execute call that supplies headers.Prefer with one or more tokens, then assert preferHeader equals the expected merged string to ensure caller headers are preserved and de-duplicated.
🤖 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/update-builder.ts`:
- Around line 298-310: The return type of processResponse and the UpdateResponse
alias are currently fixed to DatabaseIncludeSpecialColumns, so per-call
options?.includeSpecialColumns overrides aren't reflected in types; update both
places to use the normalized override type (the same pattern execute() uses):
replace DatabaseIncludeSpecialColumns with
NormalizeIncludeSpecialColumns<EO["includeSpecialColumns"],
DatabaseIncludeSpecialColumns> (or the correct local ExecuteOptions generic if
named differently) so processResponse's Promise and UpdateResponse alias reflect
options?.includeSpecialColumns, and keep
validateSingleResponse(options?.includeSpecialColumns) as-is.
---
Outside diff comments:
In `@packages/fmodata/src/client/insert-builder.ts`:
- Around line 147-181: The request currently mixes a raw callerHeaders value and
a computed preferHeader string (see preferHeader, callerHeaders,
mergePreferHeaderValues) which can drop or duplicate headers; fix by
canonicalizing callerHeaders to a Headers instance once (e.g., const
canonicalHeaders = new Headers(callerHeaders || {})), read/merge Prefer from
that Headers (using mergePreferHeaderValues as done) and then pass headers to
requestFromService as an object built from canonicalHeaders (e.g.,
Object.fromEntries(canonicalHeaders.entries()) or by iterating entries) while
ensuring Prefer is set to preferHeader only once; update the code paths around
preferHeader creation and the requestFromService call to use canonicalHeaders
and derived plain-object headers consistently.
- Around line 288-300: processResponse's return type doesn't honor the
request-level includeSpecialColumns override; make processResponse generic over
an ExecuteOptions type parameter (e.g. <EO extends ExecuteOptions | undefined>)
like execute(), use NormalizeIncludeSpecialColumns<EO["includeSpecialColumns"],
DatabaseIncludeSpecialColumns> in the resolved Promise return type, and thread
EO through any internal validation calls (where options?.includeSpecialColumns
is read) so the type reflects when a caller passes includeSpecialColumns: true;
apply the same change to the other processResponse overloads mentioned (around
lines 383–418) and ensure function signature and internal usages (options,
validation calls) reference the new EO generic.
In `@packages/fmodata/src/client/update-builder.ts`:
- Around line 185-223: The problem is header duplication caused by mixing a
plain object and a Headers map (Object.fromEntries(new
Headers(callerHeaders).entries()) plus headers.Prefer = ...). To fix, build and
manipulate a Headers instance throughout: create a Headers from callerHeaders,
set or update Prefer via headersObj.set("Prefer", preferHeader) (or delete if
undefined), apply any other header entries from the existing plain headers map
to that Headers instance, then pass that Headers instance as the request headers
into requestFromService (replace requestHeaders plain-object usage with the
Headers instance). Reference symbols: callerHeaders, headers (the plain headers
map), preferHeader (from mergePreferHeaderValues), requestHeaders, and
requestFromService.
---
Nitpick comments:
In `@packages/fmodata/tests/include-special-columns.test.ts`:
- Around line 654-720: Add a test variation to the existing "include special
columns" insert and/or update tests that passes a caller-supplied Prefer header
via execute({ headers: { Prefer: "..." } }) and asserts the final merged Prefer
header (captured via the before hook's req.headers.get("Prefer")) contains each
token exactly once (e.g., "return=representation,
fmodata.include-specialcolumns" merged with an existing token like
"handling=lenient" should become "return=representation,
fmodata.include-specialcolumns, handling=lenient" with no duplicates). Locate
the two existing tests using db.from(contactsTO).insert(...).execute(...) and
db.from(contactsTO).update(...).execute(...) and add an execute call that
supplies headers.Prefer with one or more tokens, then assert preferHeader equals
the expected merged string to ensure caller headers are preserved and
de-duplicated.
🪄 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: d984a048-9508-41a4-bbbe-d16e8c19700b
📒 Files selected for processing (8)
.changeset/fix-fmodata-mutation-special-columns.mdpackages/fmodata/src/client/builders/mutation-helpers.tspackages/fmodata/src/client/delete-builder.tspackages/fmodata/src/client/entity-set.tspackages/fmodata/src/client/insert-builder.tspackages/fmodata/src/client/update-builder.tspackages/fmodata/tests/include-special-columns.test.tspackages/fmodata/tests/use-entity-ids-override.test.ts
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Comment on lines
+231
to
+233
| if (!headers.has("Prefer") && preferValues.length > 0) { | ||
| headers.set("Prefer", preferValues.join(", ")); | ||
| } |
Contributor
There was a problem hiding this comment.
This logic fails to merge Prefer headers when user-provided headers already contain a Prefer value. If options?.headers includes a Prefer header, the database-level preferValues (like fmodata.include-specialcolumns) will be silently ignored instead of being merged.
This is inconsistent with how insert-builder.ts and update-builder.ts handle Prefer headers using mergePreferHeaderValues(). The fix should merge headers:
const existingPrefer = headers.get("Prefer");
const mergedPrefer = mergePreferHeaderValues(
preferValues.length > 0 ? preferValues.join(", ") : undefined,
existingPrefer ?? undefined
);
if (mergedPrefer) {
headers.set("Prefer", mergedPrefer);
}
Suggested change
| if (!headers.has("Prefer") && preferValues.length > 0) { | |
| headers.set("Prefer", preferValues.join(", ")); | |
| } | |
| const existingPrefer = headers.get("Prefer"); | |
| const mergedPrefer = mergePreferHeaderValues( | |
| preferValues.length > 0 ? preferValues.join(", ") : undefined, | |
| existingPrefer || undefined | |
| ); | |
| if (mergedPrefer) { | |
| headers.set("Prefer", mergedPrefer); | |
| } |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/filemaker-odata.ts`:
- Around line 231-233: The current logic skips setting Prefer when a
caller-supplied header exists, which drops fmodata-specific preferences; update
the block that checks headers and preferValues (the code around
headers.set("Prefer", ...) in filemaker-odata.ts) to merge rather than skip: if
headers.has("Prefer") then read the existing Prefer value, split on commas, trim
and combine with preferValues, deduplicate while preserving order, and set
headers.set("Prefer", mergedJoinedString); otherwise keep the existing behavior
of setting the joined preferValues when no Prefer header exists. Ensure you
reference and use the existing variables headers and preferValues so direct
_makeRequest callers retain their preferences plus fmodata.* preferences.
In `@packages/fmodata/src/client/insert-builder.ts`:
- Around line 148-159: The current Prefer header construction in
insert-builder.ts merges the caller-provided "Prefer" including any return=*
directive, which can override the builder-controlled response shape; change the
logic around mergePreferHeaderValues so you first parse/sanitize the caller
prefer directives from canonicalHeaders.get("Prefer") to remove any return=*
entry, merge only the non-return directives with fmodata.entity-ids /
fmodata.include-specialcolumns, and then append the builder-controlled return
value determined by this.returnPreference (e.g., "return=minimal" or
"return=representation") before setting Prefer; apply the same
sanitization/append pattern in ExecutableUpdateBuilder.execute() to prevent
caller return=* from overriding the builder.
🪄 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: 7f2483d7-374e-4050-932c-5fa6a38120e6
📒 Files selected for processing (4)
packages/fmodata/src/client/filemaker-odata.tspackages/fmodata/src/client/insert-builder.tspackages/fmodata/src/client/update-builder.tspackages/fmodata/tests/include-special-columns.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/fmodata/tests/include-special-columns.test.ts
Comment on lines
+148
to
+159
| const preferHeader = mergePreferHeaderValues( | ||
| this.returnPreference === "minimal" ? "return=minimal" : "return=representation", | ||
| shouldUseIds ? "fmodata.entity-ids" : undefined, | ||
| includeSpecialColumns ? "fmodata.include-specialcolumns" : undefined, | ||
| canonicalHeaders.get("Prefer") ?? undefined, | ||
| ); | ||
| canonicalHeaders.set("Content-Type", "application/json"); | ||
| if (preferHeader) { | ||
| canonicalHeaders.set("Prefer", preferHeader); | ||
| } else { | ||
| canonicalHeaders.delete("Prefer"); | ||
| } |
There was a problem hiding this comment.
Do not merge caller return=* preferences into the builder-controlled response shape.
Line 140 says caller options should not override the required request shape, but canonicalHeaders.get("Prefer") is merged verbatim here. A caller can send Prefer: return=minimal on a representation insert (or the inverse), and execute() will then parse the response using the wrong shape.
Apply the merge only to non-return= directives, then append the builder-selected return= value. The same sanitization is needed in ExecutableUpdateBuilder.execute().
🔧 Proposed fix
const shouldUseIds = mergedOptions.useEntityIds ?? this.config.useEntityIds;
const includeSpecialColumns = mergedOptions.includeSpecialColumns ?? this.config.includeSpecialColumns;
const canonicalHeaders = new Headers(callerHeaders || {});
+ const callerPrefer = canonicalHeaders
+ .get("Prefer")
+ ?.split(",")
+ .map((value) => value.trim())
+ .filter((value) => value && !value.startsWith("return="))
+ .join(", ");
const preferHeader = mergePreferHeaderValues(
this.returnPreference === "minimal" ? "return=minimal" : "return=representation",
shouldUseIds ? "fmodata.entity-ids" : undefined,
includeSpecialColumns ? "fmodata.include-specialcolumns" : undefined,
- canonicalHeaders.get("Prefer") ?? undefined,
+ callerPrefer || undefined,
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/fmodata/src/client/insert-builder.ts` around lines 148 - 159, The
current Prefer header construction in insert-builder.ts merges the
caller-provided "Prefer" including any return=* directive, which can override
the builder-controlled response shape; change the logic around
mergePreferHeaderValues so you first parse/sanitize the caller prefer directives
from canonicalHeaders.get("Prefer") to remove any return=* entry, merge only the
non-return directives with fmodata.entity-ids / fmodata.include-specialcolumns,
and then append the builder-controlled return value determined by
this.returnPreference (e.g., "return=minimal" or "return=representation") before
setting Prefer; apply the same sanitization/append pattern in
ExecutableUpdateBuilder.execute() to prevent caller return=* from overriding the
builder.
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
Prefervalues forinsert()andupdate(..., { returnFullRecord: true })includeSpecialColumnsthrough mutation execute-option defaults and request buildersROWID/ROWMODIDon full-record mutation responses when special columns are enabledPreferheaders and special-column mutation responses@proofkit/fmodataTesting
pnpm run ciSummary by CodeRabbit
New Features
Bug Fixes
Tests