Skip to content

Fix upload_attachments: specify file name in parameter dict#15

Open
raph-topo wants to merge 1 commit intoricpol:mainfrom
raph-topo:fix-upload-attachments
Open

Fix upload_attachments: specify file name in parameter dict#15
raph-topo wants to merge 1 commit intoricpol:mainfrom
raph-topo:fix-upload-attachments

Conversation

@raph-topo
Copy link
Copy Markdown

Rationale: path on disk may not exist or may not be significant (as with NamedTemporaryFile()).

Rationale: path on disk may not exist or may not be significant (as with `NamedTemporaryFile()`).
@ricpol
Copy link
Copy Markdown
Owner

ricpol commented Feb 18, 2026

Non sure about this - a NamedTemporaryFile is just a file that happens to be managed by tempfile... but you may still get its path with "f.name".
It would be different if you wanted to stream the attachment upload, like this - this, however, involves calling the Requests api with a "data" parameter, which we don't use at the moment.
Support for streaming may be useful though - don't know if the underlying Grist api allows this. I'll add this to my todo list.

or - maybe I didn't understand what you are trying to do here? What use case do you have in mind?

@raph-topo
Copy link
Copy Markdown
Author

raph-topo commented Feb 21, 2026

Non sure about this - a NamedTemporaryFile is just a file that happens to be managed by tempfile... but you may still get its path with "f.name".

Yes, a random name, which it makes no sense to store in Grist. (But Pygrister download_attachment & upload_attachments seem to need NamedTemporaryFile instead of TemporaryFile if I recall my first tests.)

or - maybe I didn't understand what you are trying to do here? What use case do you have in mind?

I'm using my pygrister fork including this PR to copy attachments from one Grist doc to another, without loosing their name:

        ...
        for att in fr.attachments:
            with NamedTemporaryFile() as file:
                fr.grist.download_attachment(
                    filename=Path(file.name),
                    attachment_id=att.id,
                )
                to_id: int = to.grist.upload_attachments(
                    {att.fields.fileName: Path(file.name)}
                )[1][0]
                ...

(fr.grist & to.grist are instances pygrister.api.GristApi with .open_session() to one Grist doc each)

NamedTemporaryFile has the huge advantage of cleaning itself up when the with block ends. But without the PR, upload_attachments() would store and have Grist display that random name.

@ricpol
Copy link
Copy Markdown
Owner

ricpol commented Feb 22, 2026

Oh, I see the point now, but your specific use case seem a little too fringe to me... Basically, you are corrupting the file names at download time, then you want to restore them when uploading... But you are only saving a single "os.remove" call which should be enough to clean up your unwanted temporary files...
Something like this should work just fine:

>>> grist = GristApi()
>>> st, attachments = grist.list_attachments(doc_id='source_doc_id')
>>> att_ids = [att['id'] for att in attachments]
>>> att_files = [att['fields']['fileName'] for att in attachments]
>>> for id, name in zip(att_ids, att_files):
...     st, res = grist.download_attachment(name, id, doc_id='source_doc_id')
...     st, res = grist.upload_attachments([name], doc_id='dest_doc_it')
...     os.remove(name)

That said, it is true that Pygrister allows for changing the file name only at download time, and not when uploading, which is rather odd... Righ now I'm +0 about this change but I'll take a closer look and see if there are any drawbacks...
Thanks for clarifying the issue!

@raph-topo
Copy link
Copy Markdown
Author

Basically, you are corrupting the file names at download time, then you want to restore them when uploading...

That's true in the described scenario indeed. But one could also imagine a scenario where the script also changes the name based on some other properties.

But you are only saving a single "os.remove" call

Indeed. Though Python code style is pushing towards less os and more with: structures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants