atlassian-python-api icon indicating copy to clipboard operation
atlassian-python-api copied to clipboard

[confluence] attach_file should not have to read the whole file into RAM. Right?

Open stevecj opened this issue 2 years ago • 8 comments

In the attach_file method,

        with open(filename, "rb") as infile:
            content = infile.read()
        return self.attach_content(
            content,
            name,
            content_type,
            page_id=page_id,
            title=title,
            space=space,
            comment=comment,
        )

Is there some reason that should not be

        with open(filename, "rb") as infile:
            return self.attach_content(
                infile,
                name,
                content_type,
                page_id=page_id,
                title=title,
                space=space,
                comment=comment,
            )

instead?

stevecj avatar Jun 06 '22 23:06 stevecj

Yes, the content must be a binary string and not a file handle.

Spacetown avatar Jun 07 '22 07:06 Spacetown

From what I can determine, it actually doesn't. I examined the code, and that argument is passed to requests which wants a file-like object. It is actually not obvious to me, in reading the requests docs that a Bytes object would work (though it obviously does).

I tested the change as written in my original post, and it is working for me.

stevecj avatar Jun 07 '22 22:06 stevecj

When you get a chance, please look into this. I believe you will find that it is fine (and best) to pass the file object instead of reading the file in and then passing the Bytes.

stevecj avatar Jun 20 '22 19:06 stevecj

…or I can simply submit an MR if test coverage is in place to confirm that the change works.

stevecj avatar Jun 20 '22 20:06 stevecj

The open was there since the introduction of attach_file. Feel free to create a PR.

Spacetown avatar Jun 20 '22 20:06 Spacetown

The open is needed. It's the read that's not needed.

stevecj avatar Jun 20 '22 20:06 stevecj

I mean the whole block.

Spacetown avatar Jun 20 '22 20:06 Spacetown

@stevecj I will be happy if you submit MR/PR :)

gonchik avatar Sep 17 '22 10:09 gonchik