Skip to content
Open
Show file tree
Hide file tree
Changes from 2 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
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
- summary: |
Fix `TypeError: Cannot use 'in' operator to search for 'path' in undefined`
crash when `null` or `undefined` is passed to a file upload parameter at
runtime. The error is now a clear `TypeError` describing the expected types
instead of a cryptic `in` operator failure.
type: fix
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,11 @@ export async function toBinaryUploadRequest(

export async function toMultipartDataPart(
file: Uploadable,
): Promise<{ data: Uploadable.FileLike; filename?: string; contentType?: string }> {
): Promise<{ data: Uploadable.FileLike; filename?: string; contentType?: string } | undefined> {
if (file == null) {
console.warn(`File upload received ${file === null ? "null" : "undefined"}, skipping.`);
return undefined;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

since file is not null | undefined, I still think we should throw an error here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agreed — since file is typed as Uploadable (not nullable), if it somehow reaches here as null it's a logic error and should throw rather than silently swallow. The tolerance/skip should happen at the appendFile level (the API boundary) before this function is ever called.

}
const { data, filename, contentType } = await getFileWithMetadata(file, {
noSniffFileSize: true,
});
Expand All @@ -37,6 +41,11 @@ async function getFileWithMetadata(
file: Uploadable,
{ noSniffFileSize }: { noSniffFileSize?: boolean } = {},
): Promise<Uploadable.WithMetadata> {
if (file == null) {
throw new TypeError(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should use a different error type, not TypeError. Can you figure out what is most idiomatic in this case?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking at the rest of this file, line 90 uses throw new Error(...) for an invalid file upload. Using a plain Error would be most consistent with the existing code in this file. Will update.

`Expected file to be a Blob, Buffer, ReadableStream, or an object with a "path" or "data" property, but received ${file === null ? "null" : "undefined"}.`,
);
}
if (isFileLike(file)) {
return getFileWithMetadata(
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,11 @@ export class Node18FormData implements CrossPlatformFormData {
}

public async appendFile(key: string, value: Uploadable): Promise<void> {
const { data, filename } = await toMultipartDataPart(value);
const part = await toMultipartDataPart(value);
if (part == null) {
return;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

shouldn't be necessary, as value should never be null'ish at this point, and part should never be null'ish at this point.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agreed — moving the null guard to the top of appendFile (before calling toMultipartDataPart) makes this unnecessary. Will restructure so that appendFile guards early and these internal functions never see null.

Comment on lines +82 to +86

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should throw here. appendFile should not be allowed to be called with null.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The warn+skip behavior here was specifically requested to match the old 3.12.x SDK tolerance — the customer's optional file field passes undefined through the generated code into appendFile, and they expect it not to crash.

If the intent is to throw here instead, the fix would need to move upstream: the generated code that calls appendFile would need to guard optional file fields with if (value != null) before calling appendFile. Without that generated-code change, throwing here would reproduce the original crash (just with a clearer error message).

@cadesark — could you weigh in on the approach? Should we:

  1. Keep warn+skip in appendFile (current PR — tolerant core utility), or
  2. Throw in appendFile but also fix the generated caller code to not call appendFile with null for optional fields?

Option 2 is the cleaner design but requires changes to the TypeScript SDK generator's request body serialization.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Done — appendFile now throws an Error instead of warn+skip. The generated code already guards optional file fields with if (request.maybe_file != null) before calling appendFile, so this is safe.

const { data, filename } = part;

if (data instanceof Blob) {
this.fd?.append(key, data, filename);
Expand Down Expand Up @@ -140,7 +144,11 @@ export class Node16FormData implements CrossPlatformFormData {
}

public async appendFile(key: string, value: Uploadable): Promise<void> {
const { data, filename } = await toMultipartDataPart(value);
const part = await toMultipartDataPart(value);
if (part == null) {
return;
}
const { data, filename } = part;

let bufferedValue;
if (data instanceof Blob) {
Expand Down Expand Up @@ -181,7 +189,11 @@ export class WebFormData implements CrossPlatformFormData {
}

public async appendFile(key: string, value: Uploadable): Promise<void> {
const { data, filename, contentType } = await toMultipartDataPart(value);
const part = await toMultipartDataPart(value);
if (part == null) {
return;
}
const { data, filename, contentType } = part;

if (data instanceof Blob) {
this.fd?.append(key, data, filename);
Expand Down Expand Up @@ -221,7 +233,11 @@ export class FormDataWrapper {
}

public async appendFile(key: string, value: Uploadable): Promise<void> {
const { data, filename, contentType } = await toMultipartDataPart(value);
const part = await toMultipartDataPart(value);
if (part == null) {
return;
}
const { data, filename, contentType } = part;
const blob = await convertToBlob(data, contentType);
if (filename) {
this.fd.append(key, blob, filename);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,22 @@ describe("toBinaryUploadRequest", () => {
});
});

describe("null and undefined input", () => {
it("should throw TypeError for undefined in binary upload", async () => {
await expect(toBinaryUploadRequest(undefined as unknown as Uploadable)).rejects.toThrow(TypeError);
await expect(toBinaryUploadRequest(undefined as unknown as Uploadable)).rejects.toThrow(
/but received undefined/,
);
});

it("should throw TypeError for null in binary upload", async () => {
await expect(toBinaryUploadRequest(null as unknown as Uploadable)).rejects.toThrow(TypeError);
await expect(toBinaryUploadRequest(null as unknown as Uploadable)).rejects.toThrow(
/but received null/,
);
});
});

describe("Edge cases", () => {
it("should handle empty headers when no metadata is available", async () => {
const buffer = Buffer.from("");
Expand Down
11 changes: 10 additions & 1 deletion seed/ts-sdk/bytes-upload/src/core/file/file.ts

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 16 additions & 0 deletions seed/ts-sdk/bytes-upload/tests/unit/file/file.test.ts

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 10 additions & 1 deletion seed/ts-sdk/enum/forward-compatible-enums/src/core/file/file.ts

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 10 additions & 1 deletion seed/ts-sdk/enum/no-custom-config/src/core/file/file.ts

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 16 additions & 0 deletions seed/ts-sdk/enum/no-custom-config/tests/unit/file/file.test.ts

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 10 additions & 1 deletion seed/ts-sdk/enum/serde/src/core/file/file.ts

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 16 additions & 0 deletions seed/ts-sdk/enum/serde/tests/unit/file/file.test.ts

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading