Skip to content

Cloning errors when using context manager #2912

Open
@pubpub-zz

Description

@pubpub-zz
Collaborator

I agree with @alexaryn in that responsibility of closing a stream should rest purely on the caller of PdfWriter, and not PdfWriter itself. Calling pdf_writer.close() or using the context manager should only clean up the resources that PdfWriter itself has created as part of its operation. I apologize for not catching the current behavior when I reviewed #1193. 😬

Looking at the code, there is also the inefficiency where PdfWriter.__init__ is called twice. It's also permissible to call PdfWriter with a PdfReader as the first argument (to simplify cloning), but in doing so with contexts, will hit the following error:

>>> with PdfReader("./sample-files/001-trivial/minimal-document.pdf") as reader:
...   with PdfWriter(reader) as writer:
...     print(writer.fileobj)
...
__init__
__enter__
__init__
<pypdf._reader.PdfReader object at 0x10304e970>
Traceback (most recent call last):
  File "<stdin>", line 4, in <module>
  File "/Users/mpeveler/code/github/pypdf/pypdf/_writer.py", line 373, in __exit__
    self.write(self.fileobj)
  File "/Users/mpeveler/code/github/pypdf/pypdf/_writer.py", line 1396, in write
    self.write_stream(stream)
  File "/Users/mpeveler/code/github/pypdf/pypdf/_writer.py", line 1367, in write_stream
    object_positions, free_objects = self._write_pdf_structure(stream)
  File "/Users/mpeveler/code/github/pypdf/pypdf/_writer.py", line 1500, in _write_pdf_structure
    stream.write(self.pdf_header.encode() + b"\n")
AttributeError: 'PdfReader' object has no attribute 'write'

I'm not sure what the correct behavior here should be, especially with regards to cloning. Like, if I call:

with PdfWriter("foo.pdf") as writer:

and foo.pdf exists, do I expect that it'll be cloned into the writer? I'd think no? However, I think this does force the second __init__ call to be made as there's no way to tell in the first __init__ that we're in a context. I'd think this could be confusing for end users, but I guess not since no one has written in about this behavior.

Originally posted by @MasterOdin in #2905 (comment)

Activity

changed the title [-]errors when using context manager[/-] [+]Cloning errors when using context manager[/+] on Oct 20, 2024
added a commit that references this issue on Oct 20, 2024
fb1ee44
alexaryn

alexaryn commented on Oct 21, 2024

@alexaryn
Contributor

The API is ambiguous here, which makes a solution elusive. In a simple construction like PdfWriter(f) the semantics depend on if f is an empty file or not. Empty means write to f and non-empty means clone from f. This violates the principle of least astonishment. I would like it better if there were separate keyword arguments for "write to" and "clone from".

The other main problem is that the context manager __exit__() is going to try to write to (and close) the file object, even if it was of the "clone from" kind and not writable.

The double-init suggests that the logic and API can be tightened up.

stefan6419846

stefan6419846 commented on Oct 21, 2024

@stefan6419846
Collaborator

I have already requested changes on the corresponding PR, which is currently flawed and just introduced a magic positional argument which would guess its correct destination - while this would work if the flaws are removed, this just complicates the pypdf code unnecessarily as the user should know best what is desired in each case.

My proposal was to make the API more explicit by enforcing keyword-only arguments in the future (after a deprecation period). This way, we should at least avoid the ambiguity and force the user to set the correct value to avoid the current side effects.

deleted a comment from on Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    PdfWriterThe PdfWriter component is affected

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      Participants

      @pubpub-zz@stefan6419846@alexaryn

      Issue actions

        Cloning errors when using context manager · Issue #2912 · py-pdf/pypdf