ENH: automatic access to pointed object

[pubpub-zz]

alternative solution to #2460
fixes #2287

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.44%. Comparing base (cd705f9) to head (d881bae).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2464   +/-   ##
=======================================
  Coverage   94.43%   94.44%           
=======================================
  Files          49       49           
  Lines        8013     8024   +11     
  Branches     1618     1618           
=======================================
+ Hits         7567     7578   +11     
  Misses        276      276           
  Partials      170      170           

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

[SamStephens]

Am I correct that the reason test_indirect_object_page_dimensions passes is because

• The IndirectObject in the page dimensions is passed into the constructor of RectangleObject, which calls _ensure_is_number with it.
• As the IndirectObject is neither a NumberObject or a FloatObject, _ensure_is_number constructs a FloatObject from the IndirectObject.
• The constructor of FloatObject creates a new float using the string representation of the IndirectObject.

If so, this feels wrong; in this scenario, should we not be using the NumberObject the IndirectObject is a reference to, rather than creating a new FloatObject?

[pubpub-zz]

Am I correct that the reason test_indirect_object_page_dimensions passes is because

* The `IndirectObject` in the page dimensions is passed into the constructor of `RectangleObject`, which calls `_ensure_is_number` with it.

* As the `IndirectObject` is neither a `NumberObject` or a `FloatObject`, `_ensure_is_number` constructs a `FloatObject` from the  `IndirectObject`.

* The constructor of `FloatObject` creates a new float using the string representation of the `IndirectObject`.

If so, this feels wrong; in this scenario, should we not be using the NumberObject the IndirectObject is a reference to, rather than creating a new FloatObject?

the general objective of this PR is to flow down the call of the functions to pointed object. the "fix" about str is generic and will fix also possible issues with conversion to NumberObject or FloatObject

/Your idea of pointing the existing Object is possible but will only covers MediaBox case

[stefan6419846]

The constructor of FloatObject creates a new float using the string representation of the IndirectObject.

the general objective of this PR is to flow down the call of the functions to pointed object. the "fix" about str is generic and will fix also possible issues with conversion to NumberObject or FloatObject

Does this really involve doing some intermediate string step with this PR?

[SamStephens]

Does this really involve doing some intermediate string step with this PR?

Unless I'm misreading or misunderstanding, yeah.

The IndirectObject in the page dimensions is passed into the constructor of RectangleObject, which calls _ensure_is_number with it.

pypdf/pypdf/generic/_rectangle.py

Lines 20 to 26 in 88b8cb1

	def __init__(
	self, arr: Union["RectangleObject", Tuple[float, float, float, float]]
	) -> None:
	# must have four points
	assert len(arr) == 4
	# automatically convert arr[x] into NumberObject(arr[x]) if necessary
	ArrayObject.__init__(self, [self._ensure_is_number(x) for x in arr]) # type: ignore

As the IndirectObject is neither a NumberObject or a FloatObject, _ensure_is_number constructs a FloatObject from the IndirectObject.

pypdf/pypdf/generic/_rectangle.py

Lines 28 to 31 in 88b8cb1

	def _ensure_is_number(self, value: Any) -> Union[FloatObject, NumberObject]:
	if not isinstance(value, (NumberObject, FloatObject)):
	value = FloatObject(value)
	return value

The constructor of FloatObject creates a new float using the string representation of the IndirectObject.

pypdf/pypdf/generic/_base.py

Lines 363 to 375 in 88b8cb1

	def __new__(
	cls, value: Union[str, Any] = "0.0", context: Optional[Any] = None
	) -> "FloatObject":
	try:
	value = float(str_(value))
	return float.__new__(cls, value)
	except Exception as e:
	# If this isn't a valid decimal (happens in malformed PDFs)
	# fallback to 0
	logger_warning(
	f"{e} : FloatObject ({value}) invalid; use 0.0 instead", __name__
	)
	return float.__new__(cls, 0.0)

[stefan6419846]

I understand that this tends to solve more of the general IndirectObject issues we tend to encounter from time to time. Apparently, creating FloatObjects already involved the string conversion beforehand, thus this should not change much.

Some things which I would like to consider/clarify before merging:

• Is there a realistic way to solve the possibly infinite recursion (exceeding the maximum recursion depth) when doing nested get_object calls?
• Are there any (further) side effects which could/would arise from this general change?
• Does this allow us to further simplify some of the existing code?

[pubpub-zz]

• Is there a realistic way to solve the possibly infinite recursion (exceeding the maximum recursion depth) when doing nested get_object calls?

The only way I can image would be a IndirectObject referencing an IndirectObject which should not exist
I've added some code in case of.

• Are there any (further) side effects which could/would arise from this general change?

This should fix some other issue.

• Does this allow us to further simplify some of the existing code?

yes it should allow to remove many .get_object() some open issues should be solved too

[pubpub-zz]

there is an issue in the test but this seems not linked to the PR. any ideas ?

[stefan6419846]

The file from #1896 does not exist any more: https://www.selbst.de/paidcontent/dl/64733/72916 Do you see an easy way to replace it? Alternatively we probably have to remove this test for now. In both cases, we most likely should do this in a separate PR to keep things clean.

[stefan6419846]

This only fails on Windows as we do not use the cache there. If we want to continue using the same test file, https://github.com/stefan6419846/pypdf/actions/runs/8031448259/artifacts/1272068136 provides the file (zipped) as retrieved from the cache (will expire, thus we would have to upload this separately).

[pubpub-zz]

Ok I see, the missing file :
selbst.72916.pdf

[stefan6419846]

Thanks @pubpub-zz for your work and @SamStephens for helping with the review. As #2460 tends to cover only one case, I am going to merge #2464 for now.