Codecov Report

Merging #788 (2852248) into main (e7fe707) will increase coverage by 0.09%.
The diff coverage is 95.34%.

@@            Coverage Diff             @@
##             main     #788      +/-   ##
==========================================
+ Coverage   75.21%   75.31%   +0.09%     
==========================================
  Files          11       12       +1     
  Lines        3514     3609      +95     
  Branches      809      835      +26     
==========================================
+ Hits         2643     2718      +75     
- Misses        658      670      +12     
- Partials      213      221       +8     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7fe707...2852248. Read the comment docs.

#297

@MartinThoma
Thanks for your review. I do not master pytest. Test reviewed to check both Strict True and False.

I understand :-)

This might help to capture both cases:

@pytest.mark.parametrize(
    "set_strict, [True, False]
)
def test_issue297(set_strict):
    .... # the test: you pass set_strict to the PdfFileReader and you only do 'with pytest.raises' if set_strict is True

@MartinThoma ,
The expected result not being the same I've included both in the test manually isn't this good for you ?

def test_issue297():
    path = os.path.join(RESOURCE_ROOT, "issue-297.pdf")
    with pytest.raises(PdfReadWarning) as exc:
        print(exc)
        reader = PdfFileReader(path,strict=True)
        reader.getPage(0)
    assert ( exc.value.args[0].find("startxref") > 0)
    reader = PdfFileReader(path,strict=False)
    reader.getPage(0)

@pubpub-zz Thank your for pointing it out - yes, that is ok :-) The parametrize option is better as it shows clearer where an issue is (if there was one) + it avoids a bit of code duplication. But leave it as is - that is just a nitpick; I'll polish that in another commit. Thank you 🤗

I'll not merge it for now. In my roof apartment one of the windows just broke out ... I'm still shocked + cannot concentrate. I'll likely do review it again tomorrow when I can focus again 😅

@MartinThoma, good luck with your Window(...s 10 or 11 ? That is the most common window which is breaking down 🤣)

Good morning!

There seems to be something off with test_read_unknown_zero_pages. You've commented, that this is necessary due to the double-percentage in the beginning (%%PDF-1.7). However, this is a part of the percentage formatting. The actual formatted byte-string looks like this:

%PDF-1.7
1 0 obj << /Count 1 /Kids [4 0 R] /Type /Pages >> endobj
2 0 obj << >> endobj
3 0 obj << >> endobj
4 0 obj << /Contents 3 0 R /CropBox [0.0 0.0 2550.0 3508.0] /MediaBox [0.0 0.0 2550.0 3508.0] /Parent 1 0 R /Resources << /Font << >> >> /Rotate 0 /Type /Page >> endobj
5 0 obj << /Pages 0 0 R /Type /Catalog >> endobj
xref 1 5
0000000010 00000 n
0000000067 00000 n
0000000088 00000 n
0000000109 00000 n
0000000278 00000 n
trailer << /Root 5 1 R /Size 6 >>
startxref 327
%%EOF'

As you can see, the byte string starts with %PDF-1.7 and not %%PDF-1.7.

Can you help me understand why the -1 is now necessary?

(minor comment, feel free to ignore: Running black over just the test is fine: black Tests/test_reader.py. Please don't run it over the complete codebase as this would add to many changes, but I already had it running over the Tests. For this reason it will just beautify your additions)

As you can see, the byte string starts with %PDF-1.7 and not %%PDF-1.7.

Can you help me understand why the -1 is now necessary?

due to the formatting, the original formatting string includes a double '%%' that is converted to a single during formating. the point is that the .find(b"xref") applies the search on the original function. therefore the 327 points to the r and not the x of the xref.
the original code was not checking for the xref not reporting any error being tolerant to 'invalid' pdf data.
with the new code, strict=true will reject the file - not compliant with the spec - but with string=false, the xref table will be rebuilt, being able to accept the file
@MartinThoma , Do you think it would be better to accept this pointer error as it used to be without rebuilding the xref table?

the .find(b"xref") applies the search on the original

Thank you - that was what I missed 🙏 👍 Thank you for fixing the tests!

Do you think it would be better to accept this pointer error as it used to be without rebuilding the xref table?

I fear that we might now reject / fail to parse a lot of PDFs that we could parse beforehand. Fixing the xref table if we can is pretty nice - and raising an exception if it's broken as well 👍
The part that I'm worried about is broken PDFs, that we could still parse before.

I fear that we might now reject / fail to parse a lot of PDFs that we could parse beforehand. Fixing the xref table if we can is pretty nice - and raising an exception if it's broken as well 👍
The part that I'm worried about is broken PDFs, that we could still parse before.

So did I. I've extended to test to cover both nominal case and incorrect pointer. A loop has been detected and fixed in such cases.

Thank you very much for all the time and effort you've put into this! I appreciate it!

@pubpub-zz I've created a follow-up PR that improves this IMHO a little bit. Nothing substantial, but from a maintainers perspective important. I'll add the reasoning in a second: #830