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

files_upload_v2 does not work with io.BytesIO file parameters #1292

Closed
seratch opened this issue Oct 31, 2022 · 9 comments · Fixed by #1294
Closed

files_upload_v2 does not work with io.BytesIO file parameters #1292

seratch opened this issue Oct 31, 2022 · 9 comments · Fixed by #1294
Assignees
Labels
bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented Version: 3x web-client
Milestone

Comments

@seratch
Copy link
Member

seratch commented Oct 31, 2022

Is this type hint correct? I tried to pass in a io.BytesIO object here as the file as I thought IOBase covers that as well as a real file handle (e.g. from open("filename", "rb")) - but it failed with TypeError: object of type '_io.BytesIO' has no len() - len comes from _to_v2_file_upload_item I believe. So either the type hint is incorrect or the implementation of _to_v2_file_upload_item needs tweaking? You could probably just do something like this?

def _to_v2_file_upload_item(upload_file: Dict[str, Any]) -> Dict[str, Optional[Any]]:
    file = upload_file.get("file")
    content = upload_file.get("content")
    data: Optional[bytes] = None
    if file is not None:
        if isinstance(file, str):  # filepath
            with open(file.encode("utf-8", "ignore"), "rb") as readable:
                data = readable.read()
        elif isinstance(file, io.IOBase):
            # new stuff
            data = file.read()
            if isinstance(data, str):
                data = data.encode()
        elif isinstance(file, bytes):
            data = file
        else:
            raise SlackRequestError("The given file must be either a filepath as str or data as bytes/IOBase")
    elif content is not None:
        if isinstance(content, str):  # str content
            data = content.encode("utf-8")
        elif isinstance(content, bytes):
            data = content
        else:
            raise SlackRequestError("The given content given as str or as bytes")

    filename = upload_file.get("filename")
    if upload_file.get("filename") is None and isinstance(file, str):
        # use the local filename if filename is missing
        if upload_file.get("filename") is None:
            filename = file.split(os.path.sep)[-1]
        else:
            filename = "Uploaded file"

    title = upload_file.get("title", "Uploaded file")
    if data is None:
        raise SlackRequestError(f"File content not found for filename: {filename}, title: {title}")

    if title is None:
        title = filename  # to be consistent with files.upload API

    return {
        "filename": filename,
        "data": data,
        "length": len(data),
        "title": title,
        "alt_txt": upload_file.get("alt_txt"),
        "snippet_type": upload_file.get("snippet_type"),
    }

Originally posted by @Alex-ley-scrub in #1272 (comment)

@seratch seratch self-assigned this Oct 31, 2022
@seratch seratch added bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented web-client Version: 3x labels Oct 31, 2022
@seratch seratch added this to the 3.19.3 milestone Oct 31, 2022
@WilliamBergamin
Copy link
Contributor

Since files_upload supports oi.BytesIO I feel it would be expected behavior for files_upload_v2 to support it as well

@Alex-ley-scrub
Copy link

Alex-ley-scrub commented Nov 2, 2022

you also seem to need more permissions for upload_v2? Can that be true?

I get: SlackApiError: {'ok': False, 'error': 'missing_scope', 'needed': 'files:read', 'provided': 'incoming-webhook,chat:write,files:write,chat:write.customize,usergroups:read'}

whereas that works with upload (v1)

@eddyg
Copy link
Contributor

eddyg commented Nov 2, 2022

v3.19.2 (released a few days ago) adds a request_file_info argument that can be set to False if the files:read scope is not available.

@Alex-ley-scrub
Copy link

Alex-ley-scrub commented Nov 2, 2022

thanks @eddyg but I think that means I don't get the same info as I did with v1, right? And I need that info for my use case.

Previously I could do this with only the files:write scope:

upload = client.files_upload(**file_dict)
url = upload["file"]["permalink"]

now I can't do this?

upload = client.files_upload_v2(**file_dict, request_file_info=False)
url = upload["file"]["permalink"] # OR this upload["files"][0]["permalink"]

How can I get that same info with v2?

Note I was trying to migrate to v2 because of this warning message:

~\anaconda3\envs\py39\lib\site-packages\slack_sdk\web\internal_utils.py:441: UserWarning: client.files_upload() may cause some issues like timeouts for relatively large files. Our latest recommendation is to use client.files_upload_v2(), which is mostly compatible and much stabler, instead.

@eddyg
Copy link
Contributor

eddyg commented Nov 2, 2022

Yes, in that case you will need to add the files:read scope to the app (see this comment for an explanation), or continue to use the legacy files_upload method and accept the reliability issues.

@Alex-ley-scrub
Copy link

ok fair enough, thanks for the link to the explanation. I will use v1 or change the way I solve my use case with v2 or just add the read scope

@Alex-ley-scrub
Copy link

Alex-ley-scrub commented Nov 2, 2022

The use case is using client.files_upload or client.files_upload_v2 to upload multiple files, and then to use client.chat_postMessage to post the message to channel with multiple files (with upload v1 I was adding the permalink of each file into the markdown message) but also, to change the username and the icon_url for the bot, based on the context in my application using client.chat_postMessage.

I don't think this (bot customization) is possible using client.files_upload_v2 to directly upload the files and to post with the initial_comment is it? I've tried to look around the sdk code/docs but I can't figure it out. I was trying to see if i could use the file id in the markdown somehow like < F0441L9JWL3 | > but that didn't work

does that mean my only options are use v1 and deal with performance issues, use v2 with read scope, or use v2 and don't customize the bot?

@WilliamBergamin
Copy link
Contributor

Hi @Alex-ley-scrub from what I know there are limitations with the initial_comment not supporting markdown for the near future.

If I understand your use case correctly I would tend to use client.files_upload_v2 with the appropriate files:read scope and use client.chat_postMessage over initial_comment to post the message to a channel with multiple files. If this fails files_upload is still a valid option

@Alex-ley-scrub
Copy link

@eddyg / @seratch / @WilliamBergamin
Am I correct in assuming that Pull #1408 now solves the issue I was having above? Namely, needing to add additional read scope with file_upload_v2 that I did not need with v1?

https://github.com/slackapi/python-slack-sdk/pull/1408/files#diff-43db19ac0a29454cd1da520664806b266d75a54632e9661cc131406b3270b040R3424

https://github.com/slackapi/python-slack-sdk/pull/1408/files#diff-81c2cbebafffbfe19cb91928f7df395c29a10c271fbc681fb40d9cee1c140dfbR3415

request_file_info: bool = True,  # since v3.23, this flag is no longer necessary

If so, that is awesome 👏 - thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented Version: 3x web-client
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants