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.

Sign up for GitHub

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

Impl uploadv2 #90

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5ab4181
Generate files.uploadV2 methods.
racTaiga533 Dec 15, 2023
b735fec
Add fileUploadV2 method to BaseSlackAPIClient
racTaiga533 Dec 15, 2023
be9ee11
Refactor file upload types in files.ts.
racTaiga533 Dec 18, 2023
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
1 change: 1 addition & 0 deletions src/api-proxy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
195 changes: 195 additions & 0 deletions src/api_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});

Expand Down
78 changes: 78 additions & 0 deletions src/base-client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(
Expand Down
1 change: 1 addition & 0 deletions src/dev_deps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ export {
assertExists,
assertInstanceOf,
assertRejects,
fail,
} from "https://deno.land/[email protected]/testing/asserts.ts";
export * as mf from "https://deno.land/x/[email protected]/mod.ts";
export { isHttpError } from "https://deno.land/[email protected]/http/http_errors.ts";
Expand Down
2 changes: 2 additions & 0 deletions src/generated/method-types/api_method_types_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
2 changes: 2 additions & 0 deletions src/generated/method-types/files.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@ export type FilesAPIType = {
comments: {
delete: SlackAPIMethod;
};
completeUploadExternal: SlackAPIMethod;
delete: SlackAPIMethod;
getUploadURLExternal: SlackAPIMethod;
info: SlackAPIMethod;
list: SlackAPICursorPaginatedMethod;
remote: {
Expand Down
32 changes: 32 additions & 0 deletions src/typed-method-types/files.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import { BaseResponse } from "../types.ts";

export type FileUploadV2 = {
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about potentially implementing the type for FileUpload similar to the node sdk and then building this type from it?

Copy link
Author

Choose a reason for hiding this comment

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

That's a good idea. By having separate types for FileUpload and FileUploadV2, the FileUpload type will be useful when trying to implement the old files.upload. Thanks for the advice.
I implemented.

/** @description Description of image for screen-reader. */
alt_text?: string;
/** @description Syntax type of the snippet being uploaded. */
snippet_type?: string;
/** @description The message text introducing the file in specified channels. */
channel_id?: 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 Size in bytes of the file being uploaded. */
length: string;
/** @description Name of the file being uploaded. */
filename: string;
/** @description Filetype of the file being uploaded. */
file: Blob | ReadableStream<Uint8Array> | string | ArrayBuffer;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the content field is missing from this type 🤔 (node sdk content field) was this omitted for a specific reason?

Copy link
Author

Choose a reason for hiding this comment

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

I didn't implement the content field because it's not usable for file upload v2 (when uploading via getExternalUploadURL). In node-slack-sdk, whether you use the content or file field, the data is processed like the file field and an HTTP request is made. reference

Copy link
Author

Choose a reason for hiding this comment

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

I learned from @filmaj that the content field is an important element! I was mistaken in my previous comment, so I will implement the content field. 🙇‍♂️

#90 (comment)

};

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
View file
Open in desktop
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";

Expand Down Expand Up @@ -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
Expand Down
Loading