Add isEquivalentTo method to COSBase and COSObject - #712
Conversation
Add support for object comparison from ISO32000-2:2020 Annex J
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe change adds recursive equivalence checking across COS arrays, dictionaries, streams, wrappers, and scalar types. Numeric values compare through ChangesCOS equivalence
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant COSArray
participant COSDictionary
participant COSStream
participant COSBasePair
COSArray->>COSBasePair: Check and record compared pairs
COSArray->>COSDictionary: Compare nested COSBase values
COSDictionary->>COSBasePair: Check and record compared pairs
COSDictionary->>COSStream: Compare nested stream values
COSStream->>COSBasePair: Check and record compared pairs
COSStream->>COSDictionary: Compare decoded bytes and dictionary entries
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
src/main/java/org/verapdf/cos/COSDictionary.java (1)
302-311: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStray/inaccurate comment.
// assume getKey returns a COSBase wrapperis confusing —getKeyreturns aCOSObject(the wrapper), and.get()on it yields theCOSBase. Looks like a leftover authoring note.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/verapdf/cos/COSDictionary.java` around lines 302 - 311, Remove the inaccurate authoring comment from getNonNullKeySet while preserving the existing getKey(key).get() value retrieval and null-filtering behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/org/verapdf/cos/COSBase.java`:
- Around line 145-147: Preserve the supplied cycle-detection state during
equivalent comparisons: update COSBase.isEquivalentTo(Object, List<COSBasePair>)
to dispatch recursive comparisons with the existing checkedObjects list, and
update both COSIndirect comparison paths at
src/main/java/org/verapdf/cos/COSIndirect.java lines 483-486 and 492-497 to pass
that same list when comparing indirect targets, including COSObject targets; do
not restart traversal through one-argument methods.
In `@src/main/java/org/verapdf/cos/COSDictionary.java`:
- Around line 313-335: Update both overloads of COSDictionary.isEquivalentTo,
including the recursive variant, to reject COSStream and any other COSDictionary
subclass before comparing entries; preserve COSObject unwrapping and allow only
exact COSDictionary instances through the dictionary comparison path.
In `@src/main/java/org/verapdf/cos/COSName.java`:
- Around line 95-107: Update COSName.isEquivalentTo to handle null ASAtom values
before calling getValue() or converting to bytes. Ensure two null values compare
as equivalent, a null versus non-null value returns false, and retain the
existing byte comparison for non-null values.
In `@src/main/java/org/verapdf/cos/COSObject.java`:
- Around line 497-505: Update COSObject.isEquivalentTo in
src/main/java/org/verapdf/cos/COSObject.java:497-505 to handle null base values
before delegating, preserving equivalent empty-wrapper behavior. Also update
COSIndirect.isEquivalentTo in
src/main/java/org/verapdf/cos/COSIndirect.java:487-494 to check getDirect() for
null or unresolved results before accessing base, and define equivalence
consistently without dereferencing null objects.
In `@src/main/java/org/verapdf/cos/COSReal.java`:
- Around line 49-52: Ensure non-finite values accepted by
COSReal.setValue(double) cannot reach BigDecimal conversion: update COSReal’s
set/setReal/construct paths to reject NaN and infinities, or explicitly handle
them in COSReal.getDecimalValue() before BigDecimal.valueOf(value). Also update
COSInteger.isEquivalentTo(...) to handle non-finite COSReal values without
invoking BigDecimal conversion; apply the changes in
src/main/java/org/verapdf/cos/COSReal.java lines 49-52 and
src/main/java/org/verapdf/cos/COSInteger.java lines 88-90.
In `@src/main/java/org/verapdf/cos/COSStream.java`:
- Around line 320-329: Update the decoded-stream comparison block in
COSStream.equals to manage both getData(FilterFlags.DECODE) results with
try-with-resources, ensuring thisDecoded and thatDecoded are closed on success
and IOException while preserving the existing equalsDecodedStreams and failure
behavior.
- Around line 320-329: Update the decoded-stream comparison block in
COSStream.equals(Object, List) to guard against either this.stream or
that.stream being null before calling getData(). Return false for the
null-stream case, matching the existing stream checks in equals(Object, List),
while preserving the current decoded comparison and IOException handling for
non-null streams.
In `@src/main/java/org/verapdf/cos/COSString.java`:
- Around line 437-448: Update COSString.isEquivalentTo(Object) to unwrap a
COSObject argument before checking for COSString and comparing canonical bytes,
matching the established behavior of COSStream, COSDictionary, and COSArray.
Preserve identity handling and return false for unsupported underlying types.
- Around line 383-398: Fix octal escape handling in the parsing loop around the
octal accumulation logic so the outer iteration advances past the final consumed
octal digit before continue, preventing it from being emitted again as a
literal. Preserve correct one- to three-digit octal decoding, and add unit
coverage for escapes such as \101 and \7 through isEquivalentTo or
getCanonicalBytes.
---
Nitpick comments:
In `@src/main/java/org/verapdf/cos/COSDictionary.java`:
- Around line 302-311: Remove the inaccurate authoring comment from
getNonNullKeySet while preserving the existing getKey(key).get() value retrieval
and null-filtering behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 168acea3-5e14-4e08-ab69-3fe3b6814d78
📒 Files selected for processing (13)
src/main/java/org/verapdf/cos/COSArray.javasrc/main/java/org/verapdf/cos/COSBase.javasrc/main/java/org/verapdf/cos/COSBoolean.javasrc/main/java/org/verapdf/cos/COSDictionary.javasrc/main/java/org/verapdf/cos/COSIndirect.javasrc/main/java/org/verapdf/cos/COSInteger.javasrc/main/java/org/verapdf/cos/COSName.javasrc/main/java/org/verapdf/cos/COSNull.javasrc/main/java/org/verapdf/cos/COSNumber.javasrc/main/java/org/verapdf/cos/COSObject.javasrc/main/java/org/verapdf/cos/COSReal.javasrc/main/java/org/verapdf/cos/COSStream.javasrc/main/java/org/verapdf/cos/COSString.java
| @Override | ||
| public BigDecimal getDecimalValue() { | ||
| return BigDecimal.valueOf(value); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files:\n'
fd -a 'COSReal.java|COSInteger.java' .
printf '\nCOSReal outline:\n'
ast-grep outline src/main/java/org/verapdf/cos/COSReal.java || true
printf '\nCOSInteger outline:\n'
ast-grep outline src/main/java/org/verapdf/cos/COSInteger.java || true
printf '\nRelevant code snippets:\n'
sed -n '1,140p' src/main/java/org/verapdf/cos/COSReal.java
printf '\n---\n'
sed -n '1,125p' src/main/java/org/verapdf/cos/COSInteger.java
printf '\nSearch for COSReal/COSInteger creation and getDecimalValue usage:\n'
rg -n "new COSReal|COSReal\.|getDecimalValue|compareTo.*getDecimalValue" src/main/java/org/verapdf/cos src/main/java -g '*.java' | head -200
printf '\nJava BigDecimal.valueOf behavior probe:\n'
python3 - <<'PY'
import math
values = [float('nan'), float('inf'), float('-inf'), 42.0]
for v in values:
try:
print(f"{v}: finite={math.isfinite(v)}")
print(f" Double.toString={float_repr(v)}")
except NameError as e:
print(f"{v}: finite={math.isfinite(v)}")
PY
printf '\nBehavior note: BigDecimal.valueOf(float/double) rejects NaN/infinity via Double.toString.\n'Repository: veraPDF/veraPDF-parser
Length of output: 14662
🏁 Script executed:
#!/bin/bash
set -euo pipefail
provider=$(mvn help:evaluate -Dexpression=project.properties.pdfbox-provider -q -DforceStdout 2>/dev/null || true)
if [ -n "$provider" ]; then
echo "current pdfbox-provider=${provider}"
else
echo "pdfbox-provider not available"
fi
# Read the parser/token code around real-number construction without running repo code.
printf '\nPDFStreamParser token.real context:\n'
sed -n '120,190p' src/main/java/org/verapdf/parser/PDFStreamParser.java
printf '\nPostscript operators around division/log/sqrt context:\n'
sed -n '360,505p' src/main/java/org/verapdf/parser/postscript/PSOperator.java
printf '\nPDFStreamParser number token pattern references:\n'
rg -n "real|NUMBER|Double\\.|parseFloat" src/main/java/org/verapdf/parser src/main/java/org/verapdf/pd/function src/main/java/org/verapdf/pd/colors -g '*.java' | head -120Repository: veraPDF/veraPDF-parser
Length of output: 11491
🌐 Web query:
Java BigDecimal.valueOf double NaN Infinity IllegalArgumentException source Double.toString
💡 Result:
In Java, the BigDecimal.valueOf(double) method explicitly throws a NumberFormatException (not an IllegalArgumentException) when the provided double is NaN (Not-a-Number), Double.POSITIVE_INFINITY, or Double.NEGATIVE_INFINITY [1][2][3][4]. The rationale for this behavior and its relationship to Double.toString(double) is as follows: 1. Method Behavior: The BigDecimal.valueOf(double) method is designed to be equivalent to calling new BigDecimal(Double.toString(val)) [1][4][5]. Since Double.toString() returns the strings "NaN", "Infinity", or "-Infinity" for non-finite values, and the BigDecimal(String) constructor cannot parse these specific strings into a numeric value, a NumberFormatException is thrown [1][3][6]. 2. Explicit Validation: Historically, this exception was a consequence of passing those strings to the BigDecimal(String) constructor [5]. However, modern implementations of BigDecimal.valueOf(double) (such as in recent OpenJDK versions) include explicit checks—such as !Double.isFinite(val)—to throw a NumberFormatException with a descriptive message ("Infinite or NaN") before any string conversion takes place [6]. 3. API Distinction: This behavior aligns with the standard design of BigDecimal as a class representing finite, arbitrary-precision signed decimal numbers, which inherently does not support non-finite floating-point concepts [2].
Citations:
- 1: https://docs.oracle.com/en/java/javase/23/docs/api/java.base/java/math/BigDecimal.html
- 2: https://stackoverflow.com/questions/10080084/how-to-convert-double-positive-infinity-to-bigdecimal
- 3: https://docs.oracle.com/javase/9/docs/api/java/math/BigDecimal.html
- 4: https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/math/BigDecimal.html
- 5: https://github.com/openjdk/jdk/blob/f6feeb03bbe9024b609ae6a4795255128581d53c/src/java.base/share/classes/java/math/BigDecimal.java
- 6: openjdk/jdk@1e61352
Prevent finite-only real values from being exposed through BigDecimal conversion.
COSReal.setValue(double) also accepts NaN/infinity via set(), so getDecimalValue() will throw NumberFormatException when called from equivalent-comparison paths. Reject non-finite inputs in set / setReal / construct, or handle them explicitly before calling BigDecimal.valueOf(value) in getDecimalValue() and COSInteger.isEquivalentTo(...).
📍 Affects 2 files
src/main/java/org/verapdf/cos/COSReal.java#L49-L52(this comment)src/main/java/org/verapdf/cos/COSInteger.java#L88-L90
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/org/verapdf/cos/COSReal.java` around lines 49 - 52, Ensure
non-finite values accepted by COSReal.setValue(double) cannot reach BigDecimal
conversion: update COSReal’s set/setReal/construct paths to reject NaN and
infinities, or explicitly handle them in COSReal.getDecimalValue() before
BigDecimal.valueOf(value). Also update COSInteger.isEquivalentTo(...) to handle
non-finite COSReal values without invoking BigDecimal conversion; apply the
changes in src/main/java/org/verapdf/cos/COSReal.java lines 49-52 and
src/main/java/org/verapdf/cos/COSInteger.java lines 88-90.
| try { | ||
| ASInputStream thisDecoded = this.getData(FilterFlags.DECODE); | ||
| ASInputStream thatDecoded = that.getData(FilterFlags.DECODE); | ||
| if (!equalsDecodedStreams(thisDecoded, thatDecoded)) { | ||
| return false; | ||
| } | ||
| } catch (IOException e) { | ||
| LOGGER.log(Level.FINE, "Exception during comparing decoded streams", e); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Decoded streams from getData() are never closed.
thisDecoded/thatDecoded may be filter-pipeline streams backed by temp files (see setFilters, Line 241, which already uses try-with-resources on the same getData() result). Leaving them unclosed here risks file-handle/resource leaks when equivalence checks run across many streams in a large document.
🛠️ Proposed fix
- ASInputStream thisDecoded = this.getData(FilterFlags.DECODE);
- ASInputStream thatDecoded = that.getData(FilterFlags.DECODE);
- if (!equalsDecodedStreams(thisDecoded, thatDecoded)) {
- return false;
- }
+ try (ASInputStream thisDecoded = this.getData(FilterFlags.DECODE);
+ ASInputStream thatDecoded = that.getData(FilterFlags.DECODE)) {
+ if (!equalsDecodedStreams(thisDecoded, thatDecoded)) {
+ return false;
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| ASInputStream thisDecoded = this.getData(FilterFlags.DECODE); | |
| ASInputStream thatDecoded = that.getData(FilterFlags.DECODE); | |
| if (!equalsDecodedStreams(thisDecoded, thatDecoded)) { | |
| return false; | |
| } | |
| } catch (IOException e) { | |
| LOGGER.log(Level.FINE, "Exception during comparing decoded streams", e); | |
| return false; | |
| } | |
| try { | |
| try (ASInputStream thisDecoded = this.getData(FilterFlags.DECODE); | |
| ASInputStream thatDecoded = that.getData(FilterFlags.DECODE)) { | |
| if (!equalsDecodedStreams(thisDecoded, thatDecoded)) { | |
| return false; | |
| } | |
| } | |
| } catch (IOException e) { | |
| LOGGER.log(Level.FINE, "Exception during comparing decoded streams", e); | |
| return false; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/org/verapdf/cos/COSStream.java` around lines 320 - 329, Update
the decoded-stream comparison block in COSStream.equals to manage both
getData(FilterFlags.DECODE) results with try-with-resources, ensuring
thisDecoded and thatDecoded are closed on success and IOException while
preserving the existing equalsDecodedStreams and failure behavior.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Unguarded null stream field can throw NPE instead of returning false.
getData() calls this.stream.reset() internally with no null check. A COSStream built via the no-data constructor (protected COSStream(), Line 58) leaves stream == null until setData is called. Comparing such an object throws an uncaught NullPointerException (not IOException, so it escapes the surrounding catch). The existing equals(Object, List) (Line 426) already guards this exact case for stream/that.stream; this new method should do the same.
🛠️ Proposed fix
+ if (this.stream == null || that.stream == null) {
+ return this.stream == that.stream;
+ }
try {
ASInputStream thisDecoded = this.getData(FilterFlags.DECODE);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| ASInputStream thisDecoded = this.getData(FilterFlags.DECODE); | |
| ASInputStream thatDecoded = that.getData(FilterFlags.DECODE); | |
| if (!equalsDecodedStreams(thisDecoded, thatDecoded)) { | |
| return false; | |
| } | |
| } catch (IOException e) { | |
| LOGGER.log(Level.FINE, "Exception during comparing decoded streams", e); | |
| return false; | |
| } | |
| if (this.stream == null || that.stream == null) { | |
| return this.stream == that.stream; | |
| } | |
| try { | |
| ASInputStream thisDecoded = this.getData(FilterFlags.DECODE); | |
| ASInputStream thatDecoded = that.getData(FilterFlags.DECODE); | |
| if (!equalsDecodedStreams(thisDecoded, thatDecoded)) { | |
| return false; | |
| } | |
| } catch (IOException e) { | |
| LOGGER.log(Level.FINE, "Exception during comparing decoded streams", e); | |
| return false; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/org/verapdf/cos/COSStream.java` around lines 320 - 329, Update
the decoded-stream comparison block in COSStream.equals(Object, List) to guard
against either this.stream or that.stream being null before calling getData().
Return false for the null-stream case, matching the existing stream checks in
equals(Object, List), while preserving the current decoded comparison and
IOException handling for non-null streams.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/org/verapdf/cos/COSInteger.java`:
- Around line 81-85: Update isEquivalentTo in
src/main/java/org/verapdf/cos/COSInteger.java lines 81-85, COSBoolean.java lines
75-79, COSName.java lines 96-100, and COSString.java lines 369-373 to return
false when the operand is null before calling isIndirect(); also guard the
dereferenced result so unresolved indirect operands return false instead of
throwing.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e31f0423-195f-4a18-88d9-dd00fddb2270
📒 Files selected for processing (12)
src/main/java/org/verapdf/cos/COSArray.javasrc/main/java/org/verapdf/cos/COSBase.javasrc/main/java/org/verapdf/cos/COSBoolean.javasrc/main/java/org/verapdf/cos/COSDictionary.javasrc/main/java/org/verapdf/cos/COSIndirect.javasrc/main/java/org/verapdf/cos/COSInteger.javasrc/main/java/org/verapdf/cos/COSName.javasrc/main/java/org/verapdf/cos/COSNull.javasrc/main/java/org/verapdf/cos/COSObject.javasrc/main/java/org/verapdf/cos/COSReal.javasrc/main/java/org/verapdf/cos/COSStream.javasrc/main/java/org/verapdf/cos/COSString.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/org/verapdf/cos/COSStream.java
- src/main/java/org/verapdf/cos/COSNull.java
- src/main/java/org/verapdf/cos/COSReal.java
1 participant
Add support for object comparison from ISO32000-2:2020 Annex J
Summary by CodeRabbit