BUG: Avoid accessing invalid page when inserting blank page under some conditions

[j-t-1]

width and height are now treated symmetrically and updated only if they are not provided.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.36%. Comparing base (b9f66ab) to head (3d17c8e).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3529   +/-   ##
=======================================
  Coverage   97.36%   97.36%           
=======================================
  Files          55       55           
  Lines        9940     9945    +5     
  Branches     1822     1824    +2     
=======================================
+ Hits         9678     9683    +5     
  Misses        152      152           
  Partials      110      110           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[stefan6419846]

• Please add a test for an out-of-bounds index.
• What happens if the width is 0 or negative?
• Please update the title to reflect what the actual bug was - currently, it is not obvious which logic is meant or what was wrong.

[j-t-1]

What happens if the width is 0 or negative?
Is the code okay?

PageObject.create_blank_page(self, width, height) gets created with 0 or negative values. Same for height.

If width or height is 0 or negative change use old_page values?

[stefan6419846]

The specific case from #3337 (comment) is still not tested?

[j-t-1]

Shall I do this?
if (width is None or width <= 0 or height is None or height <= 0) and index < self.get_num_pages():

[j-t-1]

The specific case from #3337 (comment) is still not tested?

Add a test for an out-of-bounds index does this I thought.

Have I not done the test correctly?

[stefan6419846]

If would use if not (width or height) and index <= self.get_num_pages() should be sufficient - negative indices are a common edge case I would not look into and just allow negative list indexing.

Add a test for an out-of-bounds index does this I thought. Have I not done the test correctly?

Have you tested the behavior without your change to the logic? Did it fail? If not, your change most likely does not yet cover the desired case.

[j-t-1]

Have you tested the behavior without your change to the logic? Did it fail? If not, your change most likely does not yet cover the desired case.

No. So I do the same check on the old code and make sure it fails?

[stefan6419846]

Yes. This is the usual way you should assert that your tests are correct.

[j-t-1]

If would use if not (width or height) and index <= self.get_num_pages() should be sufficient - negative indices are a common edge case I would not look into and just allow negative list indexing.

Thanks.

Have you tested the behavior without your change to the logic? Did it fail? If not, your change most likely does not yet cover the desired case.

Will do. Is this the only outstanding now?

[stefan6419846]

Will do. Is this the only outstanding now?

As far as I can currently see, yes. We probably want to cover both variants (setting only width, setting only height), although only one case has previously been broken, but it makes the tests more consistent.

[j-t-1]

The old code:

        if width is None or (height is None and index < self.get_num_pages()):
            oldpage = self.pages[index]
            width = oldpage.mediabox.width
            height = oldpage.mediabox.height

Has this error:

        old_page = writer.pages[0]
        page = writer.insert_blank_page(width=10, index=0)
>       assert page.mediabox.width == 10
E       AssertionError: assert 612.0 == 10
E        +  where 612.0 = RectangleObject([0.0, 0.0, 612, 792]).width
E        +    where RectangleObject([0.0, 0.0, 612, 792]) = {'/Type': '/Page', '/Parent': NullObject, '/Resources': {}, '/MediaBox': RectangleObject([0.0, 0.0, 612, 792])}.mediabox

Because the height is None, and thus the width does not get updated. Whereas this PR passes this test.

[stefan6419846]

But we still do not have a test for the previous wrong code mentioned before?

page = writer.insert_blank_page(width=None, height=42, index=len(writer.pages))

[j-t-1]

But we still do not have a test for the previous wrong code mentioned before?

page = writer.insert_blank_page(width=None, height=42, index=len(writer.pages))

Good call. I think the code is incorrect, as it fails this test.

[j-t-1]

What do you want done if index > num_pages?

[stefan6419846]

We could theoretically remove the condition completely and would use the last page in this case (which should be documented and tested) - or raise an error as this is a user error. I would probably go with a user error, as this should never be what the user wants.

[j-t-1]

We could theoretically remove the condition completely and would use the last page in this case (which should be documented and tested) - or raise an error as this is a user error. I would probably go with a user error, as this should never be what the user wants.

Good call. I will amend so it raises an error.

[j-t-1]

@stefan6419846 this got stuck in the tests, please could you re-run it if you have that option?

[stefan6419846]

Something is odd with the PyPy 3.11 tests lately, as they regularly get stuck. I have not yet found a pattern except re-triggering it (possibly after a rebase) and hoping.