PdfFileReader load cleanup

MatthiasValvekens commented 8 months ago

Description of the changes

Clean up some of the init logic in PdfFileReader (without fundamentally changing the fact that the xref table is processed during __init__), and in particular, for encrypted files, we now defer loading of the security handler to a later point. As a side effect, this means that problems with the security handler will be raised during object access and/or calls to .encrypt()/.decrypt() rather than during __init__, which is hopefully a bit more user-friendly. It also exposes the encryption dictionary as a property on the PdfFileReader class. Closes #332.

Caveats

Technically, the removal of the .read() method on PdfFileReader is a breaking change. Practically, it's not, since calling that method out of order would already have broken all sorts of things anyway.

Checklist

Please go over this checklist to increase the chances of your PR being worked on in a timely manner. Deviations are allowed with proper justification (see previous section).

[x] I have read the project's CoC and contribution guidelines.
[x] I understand and agree to the terms in the Developer Certificate of Origin as applied to this contribution.
[x] All new code in this PR has full test coverage.

For new features (delete if not applicable)

[x] I have discussed the implementation of this feature with the project maintainer(s) on the discussion forum or over email.
[x] I have verified that my changes do not break existing API or CLI functionality, or ensured that all breaking changes are clearly documented in this PR.
[x] All public API functionality in this PR is documented.

codecov[bot] commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (ddf26f8) 98.81% compared to head (9578947) 98.82%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #343 +/- ## ======================================= Coverage 98.81% 98.82% ======================================= Files 113 113 Lines 16259 16270 +11 ======================================= + Hits 16067 16079 +12 + Misses 192 191 -1 ``` | [Flag](https://app.codecov.io/gh/MatthiasValvekens/pyHanko/pull/343/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Matthias+Valvekens) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/MatthiasValvekens/pyHanko/pull/343/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Matthias+Valvekens) | `98.82% <100.00%> (+<0.01%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Matthias+Valvekens#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

benjamin-awd commented 8 months ago

LGTM -- I can confirm that encrypt_dict is now accessible as a class property 🎉

If you want, you could probably dump _get_encryption_params() and expose encrypt_dict directly using the property decorator like so: https://github.com/MatthiasValvekens/pyHanko/pull/346/files#diff-b31a4a5713e1dec7776e9bdc1372e73085050e9d9f7061a26d37586eb403afc5R207-R208 (seems to test fine locally, but unsure if this will have other unintended effects)

MatthiasValvekens commented 8 months ago

Yeah, fair enough. Yesterday me thought we might as well preserve the _get_encryption_params method while adding the property, but on reflection I agree it's kind of pointless.

MatthiasValvekens / pyHanko