deprecate file.upload and implement new apis #11

koonwen · 2025-01-03T07:42:58Z

Related to #5

I did some initial scaffolding work around this. Haven't done any testing yet so it probably doesn't work.

Some caveats about the new api is that it is a three step query process

get a url to upload to
actually upload the file to the url
specify where the uploaded file should appear in slack

Also with the new api, we cannot specify multiple channels in 3. and you can only call it once which I guess means once you upload it once, it gets thrown away after. https://api.slack.com/methods/files.getUploadURLExternal#markdown I thought about multiplexing the request by getting multiple upload urls but I don't know if this will work as expected and slack will give back multiple unique ids to upload to. That's also kinda inefficient to duplicate the upload work.

In any case, feel free to take over this PR

sewenthy · 2025-01-06T01:57:21Z

I feel like this change with additional upload_file_v2 is a bit hard to manage, I would rather remove the deprecated version. Honestly, not sure how to handle deprecation in opensource libraries 😅 @Khady if you have any suggestions!

lib/api_remote.ml

EmileTrotignon · 2025-02-04T09:36:18Z

lib/api.ml

+
+  val get_upload_url_external : ctx:Context.t -> req:get_upload_url_ext_req -> get_upload_url_ext_res slack_response Lwt.t
+  val complete_upload_external : ctx:Context.t -> req:complete_upload_ext_req -> complete_upload_ext_res slack_response Lwt.t
+  val upload_file_v2 : ctx:Context.t -> file:files_upload_req -> files_upload_res slack_response Lwt.t


I know other slack SDK have used the v2 naming scheme, but since we have the exact same type, I don't see the point. What downside is there to just replacing upload_file ?

EmileTrotignon · 2025-02-04T09:38:28Z

lib/api.ml

+                 there is upload_file_v2 convenience function"]
+
+  val get_upload_url_external : ctx:Context.t -> req:get_upload_url_ext_req -> get_upload_url_ext_res slack_response Lwt.t
+  val complete_upload_external : ctx:Context.t -> req:complete_upload_ext_req -> complete_upload_ext_res slack_response Lwt.t


Why expose the substeps ? If we do not, it can also be remove from the api_local implementation.

Is it in the case that you want to do multiple uploads of the same file ?

It gives us the flexibility to upload multiple files* simultaneously if that's desired in the future. But I see your point, to cut down on the extra cruft I'll hide it from the signature

EmileTrotignon · 2025-02-04T10:56:25Z

lib/api_remote.ml

+    ~name:(sprintf "files.completeUploadExternal (%s)" @@ Slack_j.string_of_files_v2 req.files)
+    ~body `POST "files.completeUploadExternal" Slack_j.read_complete_upload_ext_res
+
+(** NOTE: this api only can specify one channel_id unlike in the deprecated version *)


It can actually specify multiple channels. The channel_id field only excpects 1, but there is a channels field where you can add multiples. https://api.slack.com/methods/files.completeUploadExternal

thatportugueseguy

please revisit this code and add support for snippets that don't come from a file and better error handling.

Add tests to the backlog, if it will take you too much time

thatportugueseguy · 2025-02-07T15:54:40Z

.github/workflows/action.yml

these kinds of changes should be sent directly to master. These are unrelated to the PR

thatportugueseguy · 2025-02-07T16:02:14Z

lib/api_local.ml

+  let json =
+    req |> Slack_j.string_of_get_upload_url_ext_req |> Yojson.Basic.from_string |> Yojson.Basic.pretty_to_string
+  in
+  printf "getting upload url for %s\n" req.filename;
+  printf "%s\n" json;


isn't this debugging logic?

Are you adding tests to these functions? it seems that this logic should live in the test file?

(same below, for complete_upload_external)

thatportugueseguy · 2025-02-07T16:03:43Z

lib/api_remote.ml

+  let args = www_form_of_get_upload_url_ext req in
+  let data = Web.make_url_args args in
+  let body = `Raw ("application/x-www-form-urlencoded", data) in
+  log#info "data to upload req: %s" data;


should these be log#debug? I'm not sure that the users want to log this data for all requests

thatportugueseguy · 2025-02-07T16:05:53Z

lib/api_remote.ml

+  log#info "completing upload url for %s" @@ Slack_j.string_of_files_v2 req.files;
+  let data = Slack_j.string_of_complete_upload_ext_req req in
+  let body = `Raw ("application/json", data) in
+  log#info "data to upload req: %s" data;


same as above

thatportugueseguy · 2025-02-07T16:13:42Z

lib/api_remote.ml

+  match file.filename, file.content with
+  | None, _ | _, None -> Exn.fail "need to supply both filename and content"


i think it would make more sense to separate the concerns regarding the file itself and the upload config. These are all "crammed" in files_upload_req, but could easily be separated.

Or don't call the argument file, which it's not.

thatportugueseguy · 2025-02-07T16:26:11Z

lib/api_remote.ml

+    | Error e -> Lwt.return_error e
+    | Ok { upload_url; file_id; _ } ->
+      let raw_file_contents = In_channel.with_open_bin filename (fun ic -> input_all ic) in
+      let body = `Raw ("", raw_file_contents) in


Suggested change

let body = `Raw ("", raw_file_contents) in

let body = `Raw ("text/plain", raw_content) in

thatportugueseguy · 2025-02-07T16:30:29Z

lib/api_remote.ml

+    let req = Slack_j.make_get_upload_url_ext_req ~filename ~length () in
+    ( match%lwt get_upload_url_external ~ctx ~req with
+    | Error e -> Lwt.return_error e
+    | Ok { upload_url; file_id; _ } ->


slack api fails with a 200 OK response but sends the ok property of the payload as false and includes an error property on the response.

Error responses aren't properly handled here or below, after the complete_upload_external response.

thatportugueseguy · 2025-02-07T16:31:38Z

lib/api_remote.ml

+        let req = Slack_j.make_complete_upload_ext_req ~files ?channels:file.channels ?thread_ts:file.thread_ts () in
+        ( match%lwt complete_upload_external ~ctx ~req with
+        | Error e -> Lwt.return_error e
+        | Ok { files; _ } ->


see comment above regarding slack failing with 200 OK

thatportugueseguy · 2025-02-07T16:38:16Z

lib/api_remote.ml

+  log#info "getting upload url for %s" req.filename;
+  let args = www_form_of_get_upload_url_ext req in
+  let data = Web.make_url_args args in
+  let body = `Raw ("application/x-www-form-urlencoded", data) in


if you want to use application/x-www-form-urlencoded here instead of text/plain, you should use `Form instead of `Raw

thatportugueseguy · 2025-02-07T16:46:32Z

lib/api_remote.ml

@@ -113,6 +100,72 @@ let get_permalink ~(ctx : Context.t) ~(req : Slack_t.get_permalink_req) =
    ~name:(sprintf "chat.getPermalink (%s, %s)" req.channel req.message_ts)
    ~ctx `GET api_path Slack_j.read_get_permalink_res

+let www_form_of_get_upload_url_ext (req : Slack_t.get_upload_url_ext_req) =


is this function used elsewhere? i don't think it's worth having this function out of get_upload_url_external. Just declare the fields variable and inside of it.

koonwen requested a review from sewenthy January 3, 2025 07:43

koonwen mentioned this pull request Jan 3, 2025

deprecating files.upload #5

Closed

EmileTrotignon reviewed Feb 6, 2025

View reviewed changes

koonwen force-pushed the new_upload_api branch from d721cda to 0435af8 Compare February 7, 2025 03:36

deprecate file upload and implement new apis

c6f9438

koonwen force-pushed the new_upload_api branch from 0435af8 to 33299ba Compare February 7, 2025 05:01

fmt + remove macos ci build error

a4f1ae5

koonwen force-pushed the new_upload_api branch 3 times, most recently from 3f022a4 to bf2d4ac Compare February 7, 2025 06:05

koonwen added 2 commits February 7, 2025 15:12

cleanup ci

f16100f

add channels param + hide substeps

dab5247

koonwen force-pushed the new_upload_api branch from bf2d4ac to dab5247 Compare February 7, 2025 07:13

koonwen added 2 commits February 7, 2025 15:16

fmt

66e57c5

check why atdgen can't be found

4cdc6f5

koonwen force-pushed the new_upload_api branch from abd61c3 to 4cdc6f5 Compare February 7, 2025 07:26

hopefully fix ci

0ad0418

koonwen merged commit 365868f into master Feb 7, 2025
8 checks passed

thatportugueseguy reviewed Feb 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deprecate file.upload and implement new apis #11

deprecate file.upload and implement new apis #11

koonwen commented Jan 3, 2025 •

edited

Loading

sewenthy commented Jan 6, 2025

EmileTrotignon Feb 4, 2025

EmileTrotignon Feb 4, 2025

koonwen Feb 7, 2025

EmileTrotignon Feb 4, 2025

thatportugueseguy left a comment

thatportugueseguy Feb 7, 2025

thatportugueseguy Feb 7, 2025

thatportugueseguy Feb 7, 2025

thatportugueseguy Feb 7, 2025

thatportugueseguy Feb 7, 2025

thatportugueseguy Feb 7, 2025

thatportugueseguy Feb 7, 2025

thatportugueseguy Feb 7, 2025

thatportugueseguy Feb 7, 2025

thatportugueseguy Feb 7, 2025

		match file.filename, file.content with
		\| None, _ \| _, None -> Exn.fail "need to supply both filename and content"

	let body = `Raw ("", raw_file_contents) in
	let body = `Raw ("text/plain", raw_content) in

deprecate file.upload and implement new apis #11

deprecate file.upload and implement new apis #11

Conversation

koonwen commented Jan 3, 2025 • edited Loading

sewenthy commented Jan 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thatportugueseguy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koonwen commented Jan 3, 2025 •

edited

Loading