Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Impl uploadv2 #90

Closed
wants to merge 3 commits into from
Closed
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
2 changes: 2 additions & 0 deletions scripts/src/public-api-methods.ts
Original file line number Diff line number Diff line change
@@ -154,6 +154,8 @@ export const getPublicAPIMethods = () => {
"files.list",
"files.revokePublicURL",
"files.sharedPublicURL",
"files.getUploadURLExternal",
"files.completeUploadExternal",
"files.upload",
"files.remote.add",
"files.remote.info",
1 change: 1 addition & 0 deletions src/api-proxy.ts
Original file line number Diff line number Diff line change
@@ -16,6 +16,7 @@ export const ProxifyAndTypeClient = (baseClient: BaseSlackAPIClient) => {
setSlackApiUrl: baseClient.setSlackApiUrl.bind(baseClient),
apiCall: baseClient.apiCall.bind(baseClient),
response: baseClient.response.bind(baseClient),
fileUploadV2: baseClient.fileUploadV2.bind(baseClient),
};

// Create our proxy, and type it w/ our api method types
195 changes: 195 additions & 0 deletions src/api_test.ts
Original file line number Diff line number Diff line change
@@ -365,6 +365,201 @@ Deno.test("SlackAPI class", async (t) => {
},
);

await t.step(
"fileUploadV2 method",
async (t) => {
const client = SlackAPI("test-token");
await t.step(
"should successfully upload a single file",
async () => {
const testFile = {
file: new Blob(["test"]),
filename: "test.txt",
length: "6",
fileId: "test_id",
};
mf.mock("POST@/api/files.getUploadURLExternal", () => {
return new Response(
JSON.stringify({
"ok": true,
"upload_url": "https://files.slack.com/test",
"file_id": "test_id",
}),
);
});
mf.mock("POST@/test", () => {
return new Response(
undefined,
{ status: 200 },
);
});
mf.mock("POST@/api/files.completeUploadExternal", () => {
return new Response(
`{"ok":true}`,
);
});
const response = await client.fileUploadV2({
file_uploads: [
testFile,
],
});
response.forEach((res) => assertEquals(res.ok, true));

mf.reset();
},
);

await t.step(
"should successfully upload multiple file",
async () => {
const testFile = {
file: new Blob(["test"]),
filename: "test.txt",
length: "6",
fileId: "test_id",
};
const testTextFile = {
file: "test",
filename: "test.txt",
length: "6",
fileId: "test_id",
};
mf.mock("POST@/api/files.getUploadURLExternal", () => {
return new Response(
JSON.stringify({
"ok": true,
"upload_url": "https://files.slack.com/test",
"file_id": "test_id",
}),
);
});
mf.mock("POST@/test", () => {
return new Response(
undefined,
{ status: 200 },
);
});
mf.mock("POST@/api/files.completeUploadExternal", () => {
return new Response(
`{"ok":true}`,
);
});
const response = await client.fileUploadV2({
file_uploads: [
testFile,
testTextFile,
],
});
response.forEach((res) => assertEquals(res.ok, true));

mf.reset();
},
);
await t.step(
"should rejects when get upload url fails",
async () => {
const testFile = {
file: new Blob(["test"]),
filename: "test.txt",
length: "6",
fileId: "test_id",
};
mf.mock("POST@/api/files.getUploadURLExternal", () => {
return new Response(
JSON.stringify({
"ok": false,
}),
);
});
await assertRejects(async () =>
await client.fileUploadV2({
file_uploads: [
testFile,
],
})
);

mf.reset();
},
);
await t.step(
"should rejects when upload fails",
async () => {
const testFile = {
file: new Blob(["test"]),
filename: "test.txt",
length: "6",
fileId: "test_id",
};
mf.mock("POST@/api/files.getUploadURLExternal", () => {
return new Response(
JSON.stringify({
"ok": true,
"upload_url": "https://files.slack.com/test",
"file_id": "test_id",
}),
);
});
mf.mock("POST@/test", () => {
return new Response(
undefined,
{ status: 500 },
);
});
await assertRejects(async () =>
await client.fileUploadV2({
file_uploads: [
testFile,
],
})
);

mf.reset();
},
);
await t.step(
"should rejects when upload complete fails",
async () => {
const testFile = {
file: new Blob(["test"]),
filename: "test.txt",
length: "6",
fileId: "test_id",
};
mf.mock("POST@/api/files.getUploadURLExternal", () => {
return new Response(
JSON.stringify({
"ok": true,
"upload_url": "https://files.slack.com/test",
"file_id": "test_id",
}),
);
});
mf.mock("POST@/test", () => {
return new Response(
undefined,
{ status: 200 },
);
});
mf.mock("POST@/api/files.completeUploadExternal", () => {
return new Response(
`{"ok":false}`,
);
});
await assertRejects(async () =>
await client.fileUploadV2({
file_uploads: [
testFile,
],
})
);

mf.reset();
},
);
},
);

mf.uninstall();
});

78 changes: 78 additions & 0 deletions src/base-client.ts
Original file line number Diff line number Diff line change
@@ -6,6 +6,7 @@ import {
} from "./types.ts";
import { createHttpError, HttpError } from "./deps.ts";
import { getUserAgent, serializeData } from "./base-client-helpers.ts";
import { FileUploadV2, FileUploadV2Args } from "./typed-method-types/files.ts";

export class BaseSlackAPIClient implements BaseSlackClient {
#token?: string;
@@ -72,6 +73,83 @@ export class BaseSlackAPIClient implements BaseSlackClient {
return await this.createBaseResponse(response);
}

async fileUploadV2(
args: FileUploadV2Args,
) {
const { file_uploads } = args;
const uploadUrls = await Promise.all(
file_uploads.map((file) => this.getFileUploadUrl(file)),
);

await Promise.all(
uploadUrls.map((uploadUrl, index) =>
this.uploadFile(uploadUrl.upload_url, file_uploads[index].file)
),
);

return await Promise.all(
uploadUrls.map((uploadUrl, index) =>
this.completeFileUpload(uploadUrl.file_id, file_uploads[index])
),
);
}

private async getFileUploadUrl(file: FileUploadV2) {
const fileMetaData = {
filename: file.filename,
length: file.length,
alt_text: file.alt_text,
snippet_type: file.snippet_type,
};
const response = await this.apiCall(
"files.getUploadURLExternal",
fileMetaData,
);

if (!response.ok) {
throw new Error(JSON.stringify(response.response_metadata));
}
return response;
}

private async completeFileUpload(fileID: string, file: FileUploadV2) {
const fileMetaData = {
files: JSON.stringify([{ id: fileID, title: file.title }]),
channel_id: file.channel_id,
initial_comment: file.initial_comment,
thread_ts: file.thread_ts,
};
const response = await this.apiCall(
"files.completeUploadExternal",
fileMetaData,
);
if (!response.ok) {
throw new Error(JSON.stringify(response.response_metadata));
}
return response;
}

private async uploadFile(
uploadUrl: string,
file: FileUploadV2["file"],
) {
const response = await fetch(uploadUrl, {
headers: {
Copy link
Contributor

Choose a reason for hiding this comment

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

When actually POSTing the file contents to the URL provided by getUploadURLExternal, we post the data as binary (raw bytes), or multipart-form-encoded (see the docs on this here).

As a result, I don't think we should be providing headers like Content-Type. The file-upload-external edge URL that we POST file data to will infer the content type based on the data uploaded.

In the node SDK, the headers are empty other than the token.
Similarly, in the python SDK, same thing.

I think the trick in implementing this API is to ensure we encode the POST data in the appropriate format, based on what is provided by the user. Assuming we support the content parameter, like the other SDKs do, then the content parameter, which is a string, should be encoded as UTF-8 bytes. Otherwise, if file is a string, we should interpret that string as a file path, and read the raw bytes directly from that path. Finally, file may also be a buffer-like object. In this situation, we should provide the raw buffer directly to fetch as its data payload.

Copy link
Author

Choose a reason for hiding this comment

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

OK. Indeed, delegating the setting of Content-Type to the fetch function is appropriate. Thank you for the detailed explanation 😊. Based on this, I will make the following modifications to the source code:

  • When uploading a file to the URL obtained from getUploadURLExternal, do not set the Content-Type.
  • Implement a content field, read its value as UTF-8, and pass it to the request body of the fetch function.
  • When a string is passed to the file field, interpret it as a file path and read the file using Deno.readFileSync.

"Content-Type": typeof file === "string"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this works the same way as the node SDK. If file is a string in the node SDK, then the assumption is that the file argument is a file path to the file data. This is why the content parameter exists on the node SDK: so that users who do want to provide string-based file contents can do so.

Can we make this work the same way as the node SDK?

Also for reference: the code for how the python SDK handles these parameters.

Copy link
Author

Choose a reason for hiding this comment

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

I had overlooked how the node SDK behaves when a string is passed to the file field 😮. It's definitely not good for the behavior to differ between SDKs. I will make corrections so that the handling of the content field and file field behaves the same as in other SDKs.

? "text/plain"
: "application/octet-stream",
"User-Agent": getUserAgent(),
},
method: "POST",
body: file,
});

if (!response.ok) {
throw await this.createHttpError(response);
}
return;
}

private async createHttpError(response: Response): Promise<HttpError> {
const text = await response.text();
return createHttpError(
1 change: 1 addition & 0 deletions src/dev_deps.ts
Original file line number Diff line number Diff line change
@@ -4,6 +4,7 @@ export {
assertExists,
assertInstanceOf,
assertRejects,
fail,
} from "https://deno.land/std@0.132.0/testing/asserts.ts";
export * as mf from "https://deno.land/x/mock_fetch@0.3.0/mod.ts";
export { isHttpError } from "https://deno.land/std@0.182.0/http/http_errors.ts";
2 changes: 2 additions & 0 deletions src/generated/method-types/api_method_types_test.ts
Original file line number Diff line number Diff line change
@@ -184,7 +184,9 @@ Deno.test("SlackAPIMethodsType generated types", () => {
assertEquals(typeof client.enterprise.auth.idpconfig.remove, "function");
assertEquals(typeof client.enterprise.auth.idpconfig.set, "function");
assertEquals(typeof client.files.comments.delete, "function");
assertEquals(typeof client.files.completeUploadExternal, "function");
assertEquals(typeof client.files.delete, "function");
assertEquals(typeof client.files.getUploadURLExternal, "function");
assertEquals(typeof client.files.info, "function");
assertEquals(typeof client.files.list, "function");
assertEquals(typeof client.files.remote.add, "function");
2 changes: 2 additions & 0 deletions src/generated/method-types/files.ts
Original file line number Diff line number Diff line change
@@ -7,7 +7,9 @@ export type FilesAPIType = {
comments: {
delete: SlackAPIMethod;
};
completeUploadExternal: SlackAPIMethod;
delete: SlackAPIMethod;
getUploadURLExternal: SlackAPIMethod;
info: SlackAPIMethod;
list: SlackAPICursorPaginatedMethod;
remote: {
44 changes: 44 additions & 0 deletions src/typed-method-types/files.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import { BaseResponse } from "../types.ts";

interface FileUpload {
/** @description Comma-separated list of channel names or IDs where the file will be shared. */
channels?: string;
/** @description If omitting this parameter, you must provide a file. */
content?: string;
/** @description A file type identifier. */
filetype?: string;
/** @description Provide another message's ts value to upload this file as a reply. Never use a reply's ts value; use its parent instead. */
thread_ts?: string;
/** @description The message text introducing the file in specified channels. */
initial_comment?: string;
/** @description Title of the file being uploaded */
title?: string;
/** @description Name of the file being uploaded. */
filename?: string;
/** @description Filetype of the file being uploaded. */
file: Exclude<BodyInit, FormData | URLSearchParams>;
}

// Channels and filetype is no longer a supported field and filename is required for file.uploadV2.
export type FileUploadV2 =
& Omit<FileUpload, "channels" | "filetype" | "content">
& {
channel_id?: string;
/** @description Description of image for screen-reader. */
alt_text?: string;
/** @description Syntax type of the snippet being uploaded. */
snippet_type?: string;
/** @description Size in bytes of the file being uploaded. */
length: string;
/** @description Name of the file being uploaded. */
filename: string;
};

export type FileUploadV2Args = {
file_uploads: FileUploadV2[];
Copy link
Contributor

Choose a reason for hiding this comment

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

@filmaj seems like the node-sdk has a deprecated additional field request_file_info since it does not appear here would we want to do something like FileUploadV2Args = FileUploadV2[]?

Copy link
Author

Choose a reason for hiding this comment

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

FileUploadV2Args might indeed be better as a FileUploadV2[] type. Is it okay to modify it that way? 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

While it may seem awkward to nest it under an additional property name, I believe having consistency across all SDKs is more important than a sense of property cleanliness. My vote would be to keep the structure as-is, to allow for easier conceptual movement from one language / bolt framework to another.

};

export type GetUploadURLExternalResponse = BaseResponse & {
file_id: string;
upload_url: string;
};
4 changes: 4 additions & 0 deletions src/types.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { TypedSlackAPIMethodsType } from "./typed-method-types/mod.ts";
import { SlackAPIMethodsType } from "./generated/method-types/mod.ts";
import { FileUploadV2Args } from "./typed-method-types/files.ts";

export type { DatastoreItem } from "./typed-method-types/apps.ts";

@@ -52,6 +53,9 @@ export type BaseSlackClient = {
setSlackApiUrl: (slackApiUrl: string) => BaseSlackClient;
apiCall: BaseClientCall;
response: BaseClientResponse;
fileUploadV2: (
args: FileUploadV2Args,
) => Promise<BaseResponse[]>;
};

// TODO: [brk-chg] return a `Promise<Response>` object