Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fix-batch-sub-request-urls.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@proofkit/fmodata": patch
---

Fix batch sub-request URLs to use canonical FileMaker OData path format. Strips the Otto proxy prefix (`/otto/`) and `.fmp12` file extension from database names in sub-request URLs inside multipart batch bodies, which are processed directly by FileMaker's OData engine. Also fix `InvalidLocationHeaderError` in batch insert/update sub-responses by gracefully handling missing Location headers (returns ROWID -1 instead of throwing).
22 changes: 20 additions & 2 deletions packages/fmodata/src/client/batch-request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ const BOUNDARY_REGEX = /boundary=([^;]+)/;
const HTTP_STATUS_LINE_REGEX = /HTTP\/\d\.\d\s+(\d+)\s*(.*)/;
const CRLF_REGEX = /\r\n/;
const CHANGESET_CONTENT_TYPE_REGEX = /Content-Type: multipart\/mixed;\s*boundary=([^\r\n]+)/;
const OTTO_PREFIX_REGEX = /^\/otto/;
const FMPRO_EXT_REGEX = /\.fmp12/;

export interface RequestConfig {
method: string;
Expand Down Expand Up @@ -62,6 +64,18 @@ async function requestToConfig(request: Request): Promise<RequestConfig> {
};
}

/**
* Transforms a full URL into the canonical path format required by FileMaker's
* OData batch processor. Strips proxy prefixes (e.g. /otto/) and the .fmp12
* file extension from the database name segment.
*/
export function toBatchSubRequestUrl(fullUrl: string): string {
const url = new URL(fullUrl);
const path = url.pathname.replace(OTTO_PREFIX_REGEX, "");
const batchPath = path.replace(FMPRO_EXT_REGEX, "");
return `${batchPath}${url.search}`;
}

/**
* Formats a single HTTP request for inclusion in a batch
* @param request - The request configuration
Expand All @@ -80,11 +94,15 @@ function formatSubRequest(request: RequestConfig, baseUrl: string): string {
lines.push("Content-Transfer-Encoding: binary");
lines.push(""); // Empty line after multipart headers

// Construct full URL (convert relative to absolute)
// Construct sub-request URL as a canonical FileMaker OData path.
// Sub-requests inside the batch body are processed directly by FileMaker's
// OData engine, so they must not include proxy prefixes (e.g. /otto/) or
// the .fmp12 file extension on the database name.
const fullUrl = request.url.startsWith("http") ? request.url : `${baseUrl}${request.url}`;
const subRequestUrl = toBatchSubRequestUrl(fullUrl);

// Add HTTP request line
lines.push(`${request.method} ${fullUrl} HTTP/1.1`);
lines.push(`${request.method} ${subRequestUrl} HTTP/1.1`);

// For requests with body, add headers
if (request.body) {
Expand Down
22 changes: 10 additions & 12 deletions packages/fmodata/src/client/insert-builder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -275,14 +275,9 @@ export class InsertBuilder<
// Check for Location header (for return=minimal)
if (this.returnPreference === "minimal") {
const locationHeader = getLocationHeader(response.headers);
if (locationHeader) {
const rowid = this.parseLocationHeader(locationHeader);
// biome-ignore lint/suspicious/noExplicitAny: Type assertion for generic return type
return { data: { ROWID: rowid } as any, error: undefined };
}
throw new InvalidLocationHeaderError(
"Location header is required when using return=minimal but was not found in response",
);
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 };
Comment on lines +278 to +280
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

}

// For 204 responses without return=minimal, FileMaker doesn't return the created entity
Expand All @@ -295,11 +290,14 @@ export class InsertBuilder<
};
}

// If we expected return=minimal but got a body, that's unexpected
// If we expected return=minimal but got a body (e.g. batch sub-responses
// where FM returns 204-with-body, converted to 200 by parsedToResponse),
// try to extract ROWID from the Location header or return -1.
if (this.returnPreference === "minimal") {
throw new InvalidLocationHeaderError(
"Expected 204 No Content for return=minimal, but received response with body",
);
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 };
}

// Use safeJsonParse to handle FileMaker's invalid JSON with unquoted ? values
Expand Down
51 changes: 51 additions & 0 deletions packages/fmodata/tests/batch-sub-request-url.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import { describe, expect, it } from "vitest";
import { formatBatchRequest, toBatchSubRequestUrl } from "../src/client/batch-request";

describe("toBatchSubRequestUrl", () => {
it("strips /otto/ prefix and .fmp12 extension", () => {
const result = toBatchSubRequestUrl(
"https://host.example.com/otto/fmi/odata/v4/GMT_Web.fmp12/bookings?$top=1&$select=_GMTNum",
);
expect(result).toBe("/fmi/odata/v4/GMT_Web/bookings?$top=1&$select=_GMTNum");
});

it("strips .fmp12 extension without /otto/ prefix", () => {
const result = toBatchSubRequestUrl("https://host.example.com/fmi/odata/v4/GMT_Web.fmp12/bookings");
expect(result).toBe("/fmi/odata/v4/GMT_Web/bookings");
});

it("handles URLs without /otto/ or .fmp12", () => {
const result = toBatchSubRequestUrl("https://host.example.com/fmi/odata/v4/MyDB/contacts");
expect(result).toBe("/fmi/odata/v4/MyDB/contacts");
});

it("preserves query parameters", () => {
const result = toBatchSubRequestUrl(
"https://host.example.com/otto/fmi/odata/v4/MyDB.fmp12/contacts?$filter=name eq 'test'&$top=10",
);
expect(result).toBe("/fmi/odata/v4/MyDB/contacts?$filter=name%20eq%20%27test%27&$top=10");
});
});

describe("formatBatchRequest sub-request URLs", () => {
it("uses canonical paths without /otto/ prefix or .fmp12 in sub-requests", () => {
const baseUrl = "https://host.example.com/otto/fmi/odata/v4/GMT_Web.fmp12";
const { body } = formatBatchRequest(
[{ method: "GET", url: `${baseUrl}/bookings?$top=1&$select=_GMTNum` }],
baseUrl,
);

// The sub-request line must use the canonical path
expect(body).toContain("GET /fmi/odata/v4/GMT_Web/bookings?$top=1&$select=_GMTNum HTTP/1.1");
// Must NOT contain the otto prefix or .fmp12 in the request line
expect(body).not.toContain("/otto/");
expect(body).not.toContain(".fmp12");
});

it("handles relative URLs by prepending baseUrl then transforming", () => {
const baseUrl = "https://host.example.com/otto/fmi/odata/v4/MyDB.fmp12";
const { body } = formatBatchRequest([{ method: "GET", url: "/contacts?$top=5" }], baseUrl);

expect(body).toContain("GET /fmi/odata/v4/MyDB/contacts?$top=5 HTTP/1.1");
});
});
Loading