Is it ok for PyPDF2 to throw non-PyPDF2 exceptions?

[MartinThoma]

Some PDF documents are so severely broken, that we cannot fix them. And issues can occur at many places.

Assume there is an issue which we cannot fix. We have to raise an exception. Is it ok to raise non-PyPDF2 exceptions like a ValueError when int(foo) fails?

How do other non-core libraries handle it?

Similar discussions

This topic was essentially discussed in #584, but the focus was not clearly on that question. It popped up again in #1279.

Other issues:

• KeyError on malformed PDF #583
• Using PdfReader causes a crash #2761

Our options

Allow non-PyPDF2 exceptions (currently the case)

Pro:

• We don't need to change anything

Only have PyPDF2 exceptions

If we want to throw e.g. PyPDF2.errors.PdfReadError for any potential issue that might happen, we might need pretty big try-except blocks internally.

Pro:

• At least some users seem to expect it
• It would force us to document that the exception can happen
• If we want to cover those cases with unit tests, it would become easier in future to check if robustness improvements actually work

Con:

• Hidden PyPDF2 bugs: Depending on how we implement it, it is very likely that we hide PyPDF2 internal errors (e.g. if we catch too broad exceptions). Hence PyPDF2 might actually do something wrong (potentially in addition to the PDF being broken).

Open questions

• Does it make things simpler for the user if they know that they can expect either no exception or a PdfReadError and not e.g. ValueError or similar?
• Does this have a performance impact

Comparisons

Some comparisons to other popular Python libraries:

JSON: Custom exceptions

>>> import json
>>> json.loads("{")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/moose/.pyenv/versions/3.11.1/lib/python3.11/json/__init__.py", line 346, in loads
    return _default_decoder.decode(s)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/moose/.pyenv/versions/3.11.1/lib/python3.11/json/decoder.py", line 337, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/moose/.pyenv/versions/3.11.1/lib/python3.11/json/decoder.py", line 353, in raw_decode
    obj, end = self.scan_once(s, idx)
               ^^^^^^^^^^^^^^^^^^^^^^
json.decoder.JSONDecodeError: Expecting property name enclosed in double quotes: line 1 column 2 (char 1)

PyYAML: Standard python exceptions

>>> import yaml
>>> yaml.load("{")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: load() missing 1 required positional argument: 'Loader'

Zipfile: Custom exceptions

In [1]: import zipfile

In [2]: import zipfile
   ...: 
   ...: def list_zip_contents(zip_path):
   ...:     with zipfile.ZipFile(zip_path, 'r') as zip_ref:
   ...:         zip_contents = zip_ref.namelist()
   ...:         for file in zip_contents:
   ...:             print(file)
In [4]: list_zip_contents("foo.html")
...
   1364     raise BadZipFile("File is not a zip file")
   1365 if not endrec:
-> 1366     raise BadZipFile("File is not a zip file")
   1367 if self.debug > 1:
   1368     print(endrec)

BadZipFile: File is not a zip file

[DL6ER]

My understanding of exceptions is that they should be (at least to some degree) predictable for the user. For instance, when I pass a dict with some mandatory fields to a function, it's okay for the function to raise KeyError when such a mandatory field is missing in my input.

On the other hand, when I pass an invalid PDF file to PyPDF2, the story is different. PyPDF2 creates some dict internally (the user has zero control over this) and when it tries to access it internally, it fails with KeyError. This is something I could not have predicted from the outside as I haven't supplied the failing dict myself.

My comments to your questions:

Does it make things simpler for the user if they know that they can expect either no exception or a PdfReadError and not e.g. ValueError or similar?

Yes. When I see a PdfReadError I can be fairly sure that this is an expected error = the PDF file is really damaged. However, I've seen a few exceptions from PDFs files that could still be viewed (PDF reader) normally but could not be read with PyPDF2. So when I see some other exception (like KeyError or ValueError) I can be sure that this is worth debugging and reporting so PyPDF2 can be improved.

Does this have a performance impact

No, try blocks come at zero costs. Actually, I remember from one of the larger Python books that relying on try/except can be a lot faster than having individual if-blocks for all kind of checks. Exception handling is of course some work but once you are in the exception, you'll likely stop processing the file anyway so that shouldn't be a concern here.

[ediamondscience]

I think you make a really good point here. The end user of a python package shouldn't be held responsible for errors popping up inside the package with no explanation because it doesn't give them a good indication of what's going wrong with their inputs. The average user of a package like this one isn't going to look deeply enough into the different files in this package to trace an error all the way back to the pdf file they input and they shouldn't have to. If a package is going to throw an error, it should at least come with some semblance of why it's thrown to the user, and we can relatively easily add these exceptions and explanations to the code base.

I hate to say it, but faced with the argument that @DL6ER just made, the only pro of leaving external exceptions in this package ("We don't need to change anything") kind of falls flat. This should be a solution for reading most pdf files. If we're still finding a ton where our outputs are messed up then it really doesn't seem like this package is living up to it's end goal.

[MartinThoma]

One severe problem of the "only PyPDF2 Exceptions" approach is that it will become very likely that we hide PyPDF2 internal errors (e.g. if we catch too broad exceptions). Hence PyPDF2 might actually do something wrong (potentially in addition to the PDF being broken).

I've added this as a disadvantage.

[MartinThoma]

Thank you for making your point, @DL6ER ! (Don't misunderstand my comment; I'm still undecided on the topic - I want to hear others opinions / see how we would actually implement it)

[MartinThoma]

Looking at the examples / issues / discussions so far, I have a preference for custom pypdf exceptions.

Of course, the stacktrace should still show where exactly the exception was thrown so that we could capture pypdf bugs / investigate what exactly is broken for robustness fixes.

Give this a 👍 if you agree and 👎 if you think we should keep the default exceptions. We already have:

• PyPdfError: Base class for all exceptions raised by pypdf.
• PdfReadError: Raised when there is an issue reading a PDF file
• ParseError: Raised when there is an issue parsing (analyzing and understanding the structure and meaning of) a PDF file.
• and more in https://github.com/py-pdf/pypdf/blob/main/pypdf/errors.py

[prodeveloper]

How I see it, allowing this other errors to bubble up are valuable if you are debugging for a specific file. Having this standard set of errors would give us much better readability of the code and less unexpected errors.

I think it makes sense to go harder on readability here. As such I am in agreement