fix(fmodata): strip otto prefix and .fmp12 from batch sub-request URLs#208
Merged
fix(fmodata): strip otto prefix and .fmp12 from batch sub-request URLs#208
Conversation
Batch sub-request URLs inside the multipart body are processed directly
by FileMaker's OData engine, not through the Otto proxy. The engine
rejects URLs containing the /otto/ prefix (-1000 error) or the .fmp12
extension (-1032 error).
Add toBatchSubRequestUrl() to transform full URLs into the canonical
/fmi/odata/v4/{database}/{table} format before writing them into the
batch body.
Closes #207
🦋 Changeset detectedLatest commit: 3584bff 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: |
📝 WalkthroughWalkthroughAdds an exported helper Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
🧹 Nitpick comments (2)
packages/fmodata/tests/batch-sub-request-url.test.ts (1)
4-28: Consider adding a test case for URLs with only/otto/prefix (no.fmp12).The current tests cover both transformations together,
.fmp12only, and neither. Adding a test for/otto/prefix without.fmp12would complete the matrix.📝 Suggested additional test
it("strips /otto/ prefix without .fmp12 extension", () => { const result = toBatchSubRequestUrl("https://host.example.com/otto/fmi/odata/v4/MyDB/contacts"); expect(result).toBe("/fmi/odata/v4/MyDB/contacts"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/tests/batch-sub-request-url.test.ts` around lines 4 - 28, Add a unit test to cover the missing case where the URL contains only the "/otto/" prefix but no ".fmp12" extension by adding an it block that calls toBatchSubRequestUrl with "https://host.example.com/otto/fmi/odata/v4/MyDB/contacts" and asserts the result equals "/fmi/odata/v4/MyDB/contacts"; locate the tests in the describe("toBatchSubRequestUrl") block and follow the existing test style for consistency.packages/fmodata/src/client/batch-request.ts (1)
13-14: Consider anchoring the.fmp12regex to the database segment.The
FMPRO_EXT_REGEXwill match.fmp12anywhere in the path, not just in the database name segment. While unlikely in practice, a table or field name containing.fmp12would be incorrectly modified.A more precise approach would anchor the pattern to the expected OData path structure (e.g., matching only after
/v4/and before the next/).💡 Optional: More precise regex
-const FMPRO_EXT_REGEX = /\.fmp12/; +const FMPRO_EXT_REGEX = /(?<=\/v4\/[^/]+)\.fmp12(?=\/|$|\?)/;Alternatively, keep it simple if FileMaker database names are the only place
.fmp12can appear in practice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/src/client/batch-request.ts` around lines 13 - 14, FMPRO_EXT_REGEX currently matches `.fmp12` anywhere in the path; tighten it so it only matches the database segment (i.e., the portion immediately after the OData version segment and before the next `/` or end). Update the FMPRO_EXT_REGEX definition used in batch-request.ts to anchor the pattern to the OData path structure (match `.fmp12` only when it occurs in the database name following `/v4/` and before the next `/` or end), and adjust any replacement logic that uses FMPRO_EXT_REGEX accordingly (referencing the FMPRO_EXT_REGEX constant and any code paths that strip `.fmp12` from database names).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/fmodata/src/client/batch-request.ts`:
- Around line 13-14: FMPRO_EXT_REGEX currently matches `.fmp12` anywhere in the
path; tighten it so it only matches the database segment (i.e., the portion
immediately after the OData version segment and before the next `/` or end).
Update the FMPRO_EXT_REGEX definition used in batch-request.ts to anchor the
pattern to the OData path structure (match `.fmp12` only when it occurs in the
database name following `/v4/` and before the next `/` or end), and adjust any
replacement logic that uses FMPRO_EXT_REGEX accordingly (referencing the
FMPRO_EXT_REGEX constant and any code paths that strip `.fmp12` from database
names).
In `@packages/fmodata/tests/batch-sub-request-url.test.ts`:
- Around line 4-28: Add a unit test to cover the missing case where the URL
contains only the "/otto/" prefix but no ".fmp12" extension by adding an it
block that calls toBatchSubRequestUrl with
"https://host.example.com/otto/fmi/odata/v4/MyDB/contacts" and asserts the
result equals "/fmi/odata/v4/MyDB/contacts"; locate the tests in the
describe("toBatchSubRequestUrl") block and follow the existing test style for
consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a6a0de79-92c9-4055-afcc-234bb68d548b
📒 Files selected for processing (3)
.changeset/fix-batch-sub-request-urls.mdpackages/fmodata/src/client/batch-request.tspackages/fmodata/tests/batch-sub-request-url.test.ts
…onses FileMaker batch sub-responses may not include the Location header that standalone responses provide. Instead of throwing InvalidLocationHeaderError, return ROWID -1 when the header is absent in a batch context. This fixes the follow-up issue from #207 where batch writes (INSERT/UPDATE) failed with InvalidLocationHeaderError after the URL fix.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/insert-builder.ts`:
- Around line 278-280: processResponse() currently duplicates Location header
parsing and can throw from this.parseLocationHeader(), causing a rejected
promise instead of returning the usual { data, error } shape; create a small
helper (e.g., safeParseRowIdFromLocation or parseLocationSafe) used by both
places to return a constant FALLBACK_ROWID (replace the magic -1 with a named
constant) and to catch parse exceptions and convert them into a structured error
result rather than throwing; update calls in processResponse() and the other
branch that handles minimal responses (the code using parseLocationHeader and
returning { data: { ROWID: ... } }) to use this helper so parsing failures
become part of the { data, error } return path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 07cefb89-cf4b-4785-a09e-7f23428a5ec9
📒 Files selected for processing (2)
.changeset/fix-batch-sub-request-urls.mdpackages/fmodata/src/client/insert-builder.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/fix-batch-sub-request-urls.md
Comment on lines
+278
to
+280
| const rowid = locationHeader ? this.parseLocationHeader(locationHeader) : -1; | ||
| // biome-ignore lint/suspicious/noExplicitAny: Type assertion for generic return type | ||
| return { data: { ROWID: rowid } as any, error: undefined }; |
There was a problem hiding this comment.
Centralize minimal-response ROWID parsing and keep processResponse() non-throwing.
These two branches duplicate the same Location handling, and this.parseLocationHeader() can still throw on a malformed header. In processResponse(), that means a rejected promise instead of the usual { data, error } result. A small helper would make the -1 fallback explicit and keep parse failures on the structured error path.
♻️ Proposed refactor
+const UNKNOWN_ROWID = -1 as const;
+
+ private processMinimalInsertResponse(response: Response): Result<{ ROWID: number }> {
+ const locationHeader = getLocationHeader(response.headers);
+ if (!locationHeader) {
+ return { data: { ROWID: UNKNOWN_ROWID }, error: undefined };
+ }
+
+ try {
+ return {
+ data: { ROWID: this.parseLocationHeader(locationHeader) },
+ error: undefined,
+ };
+ } catch (error) {
+ return {
+ data: undefined,
+ error:
+ error instanceof Error
+ ? error
+ : new BuilderInvariantError("InsertBuilder.processResponse", String(error)),
+ };
+ }
+ }
...
if (this.returnPreference === "minimal") {
- const locationHeader = getLocationHeader(response.headers);
- const rowid = locationHeader ? this.parseLocationHeader(locationHeader) : -1;
- // biome-ignore lint/suspicious/noExplicitAny: Type assertion for generic return type
- return { data: { ROWID: rowid } as any, error: undefined };
+ // biome-ignore lint/suspicious/noExplicitAny: Type assertion for generic return type
+ return this.processMinimalInsertResponse(response) as any;
}
...
if (this.returnPreference === "minimal") {
- const locationHeader = getLocationHeader(response.headers);
- const rowid = locationHeader ? this.parseLocationHeader(locationHeader) : -1;
- // biome-ignore lint/suspicious/noExplicitAny: Type assertion for generic return type
- return { data: { ROWID: rowid } as any, error: undefined };
+ // biome-ignore lint/suspicious/noExplicitAny: Type assertion for generic return type
+ return this.processMinimalInsertResponse(response) as any;
}As per coding guidelines, "Use meaningful variable names instead of magic numbers - extract constants with descriptive names".
Also applies to: 293-300
🤖 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 278 - 280,
processResponse() currently duplicates Location header parsing and can throw
from this.parseLocationHeader(), causing a rejected promise instead of returning
the usual { data, error } shape; create a small helper (e.g.,
safeParseRowIdFromLocation or parseLocationSafe) used by both places to return a
constant FALLBACK_ROWID (replace the magic -1 with a named constant) and to
catch parse exceptions and convert them into a structured error result rather
than throwing; update calls in processResponse() and the other branch that
handles minimal responses (the code using parseLocationHeader and returning {
data: { ROWID: ... } }) to use this helper so parsing failures become part of
the { data, error } return path.
eluce2
approved these changes
Mar 26, 2026
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
2 participants
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
$batchsub-request URLs to use the canonical FileMaker OData path format/otto/proxy prefix and.fmp12file extension from database names in sub-request URLs inside multipart batch bodiestoBatchSubRequestUrl()helper with top-level regex constantsProblem
db.batch()fails because sub-request URLs inside the multipart body include the Otto proxy prefix (/otto/) and.fmp12extension. FileMaker's OData engine processes sub-requests directly and rejects these:/otto/prefix → error-1000(Incorrect OData service root path).fmp12extension → error-1032(Operation database does not match batch request database)Fix
Sub-request URLs are now transformed from:
to:
Test plan
toBatchSubRequestUrl()covering all URL variantsformatBatchRequestverifying sub-request linespnpm run ci)Closes #207
Summary by CodeRabbit
Bug Fixes
Tests