Add isEquivalentTo method to COSBase and COSObject - #712
Conversation
Add support for object comparison from ISO32000-2:2020 Annex J
📝 WalkthroughWalkthroughThe change adds recursive equivalence checking across COS arrays, dictionaries, streams, wrappers, and scalar types, with cycle detection. Numeric values compare through ChangesCOS equivalence
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant COSArray
participant COSDictionary
participant COSBasePair
COSArray->>COSBasePair: Check and record compared pairs
COSArray->>COSDictionary: Compare nested COSBase values
COSDictionary->>COSBasePair: Check and record compared pairs
COSDictionary->>COSArray: Compare values by key
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
| boolean isEquivalentTo(Object o, List<COSBasePair> checkedObjects) { | ||
| return isEquivalentTo(o); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Preserve cycle-detection state across indirect dispatch.
The list-aware recursive callers pass checkedObjects, but the default and indirect implementations fall back to one-argument methods and restart traversal.
src/main/java/org/verapdf/cos/COSBase.java#L145-L147: do not discard the supplied list for recursive types.src/main/java/org/verapdf/cos/COSIndirect.java#L483-L486: propagate the list when comparing indirect targets.src/main/java/org/verapdf/cos/COSIndirect.java#L492-L497: propagate the list when comparing an indirect target with a COSObject.
📍 Affects 2 files
src/main/java/org/verapdf/cos/COSBase.java#L145-L147(this comment)src/main/java/org/verapdf/cos/COSIndirect.java#L483-L486src/main/java/org/verapdf/cos/COSIndirect.java#L492-L497
🤖 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/COSBase.java` around lines 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.
| @Override | ||
| public boolean isEquivalentTo(Object o) { | ||
| if (this == o) return true; | ||
| if (o == null) return false; | ||
| if (o instanceof COSObject) { | ||
| return this.isEquivalentTo(((COSObject) o).get()); | ||
| } | ||
| if (!(o instanceof COSDictionary)) return false; | ||
|
|
||
| List<COSBasePair> checkedObjects = new LinkedList<>(); | ||
| return isEquivalentTo(o, checkedObjects); | ||
| } | ||
|
|
||
| /** | ||
| * Internal recursive comparison with cycle detection. | ||
| */ | ||
| boolean isEquivalentTo(Object o, List<COSBasePair> checkedObjects) { | ||
| if (this == o) return true; | ||
| if (o == null) return false; | ||
| if (o instanceof COSObject) { | ||
| return this.isEquivalentTo(((COSObject) o).get(), checkedObjects); | ||
| } | ||
| if (!(o instanceof COSDictionary)) return false; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm COSStream's superclass
rg -n 'class COSStream' -A2 --type=javaRepository: veraPDF/veraPDF-parser
Length of output: 160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Java files named COS*.java:\n'
fd '^COS.*\.java$' . || true
printf '\nSearch symbols:\n'
rg -n 'class COSStream|COSClassCOSStream|class COSDictionary|isEquivalentTo|function equivalent|byteContent|COSStream' -S --type=java . || trueRepository: veraPDF/veraPDF-parser
Length of output: 25872
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'COSDictionary isEquivalentTo and equals:\n'
sed -n '310,420p' src/main/java/org/verapdf/cos/COSDictionary.java | cat -n
printf '\nCOSStream isEquivalentTo and equals:\n'
sed -n '290,415p' src/main/java/org/verapdf/cos/COSStream.java | cat -n
printf '\nCOSDirect and COSBase relevant data members:\n'
sed -n '1,120p' src/main/java/org/verapdf/cos/COSDirect.java | cat -n
sed -n '260,315p' src/main/java/org/verapdf/cos/COSDirect.java | cat -nRepository: veraPDF/veraPDF-parser
Length of output: 15896
Reject COSStream in COSDictionary.isEquivalentTo().
COSStream extends COSDictionary, so the current instanceof COSDictionary gate lets a plain dictionary pass equality with a stream whenever their key/values match, ignoring the stream data that COSStream.isEquivalentTo() compares. Use an explicit cross-type boundary, e.g. if (o.getClass() != COSDictionary.class && !(o instanceof COSObject)) 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/COSDictionary.java` around lines 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.
| @Override | ||
| public boolean isEquivalentTo(Object o) { | ||
| if (this == o) return true; | ||
| if (!(o instanceof COSName)) return false; | ||
|
|
||
| COSName that = (COSName) o; | ||
|
|
||
| byte[] thisBytes = this.getName().getValue().getBytes(StandardCharsets.ISO_8859_1); | ||
| byte[] thatBytes = that.getName().getValue().getBytes(StandardCharsets.ISO_8859_1); | ||
|
|
||
| return Arrays.equals(thisBytes, thatBytes); | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle null ASAtom values before byte canonicalization.
COSName.construct(ASAtom) can store a null value, but this method dereferences getName().getValue() for both operands. Comparing null-valued names therefore throws an NPE; define null semantics before converting to bytes.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 101-101: Use a randomly-generated IV
Context: byte[] thisBytes = this.getName().getValue().getBytes(StandardCharsets.ISO_8859_1);
Note: [CWE-329] Generation of Predictable IV with CBC Mode.
(random-iv)
[warning] 102-102: Use a randomly-generated IV
Context: byte[] thatBytes = that.getName().getValue().getBytes(StandardCharsets.ISO_8859_1);
Note: [CWE-329] Generation of Predictable IV with CBC Mode.
(random-iv)
🤖 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/COSName.java` around lines 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.
| public boolean isEquivalentTo(Object o) { | ||
| if (o == this) return true; | ||
| if (o instanceof COSObject) { | ||
| COSObject cosObject = (COSObject) o; | ||
| return base.isEquivalentTo(cosObject.base); | ||
| } | ||
|
|
||
| if (o instanceof COSIndirect) { | ||
| return base.isEquivalentTo(((COSIndirect) o).getDirect().base); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Define behavior for empty and unresolved wrappers before dereferencing.
Both implementations assume a non-null underlying object, but the surrounding APIs permit empty COSObjects and nullable indirect children.
src/main/java/org/verapdf/cos/COSObject.java#L497-L505: handle nullbasevalues before delegation.src/main/java/org/verapdf/cos/COSIndirect.java#L487-L494: handle null/unresolvedgetDirect()results before calling equivalence.
📍 Affects 2 files
src/main/java/org/verapdf/cos/COSObject.java#L497-L505(this comment)src/main/java/org/verapdf/cos/COSIndirect.java#L487-L494
🤖 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/COSObject.java` around lines 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.
| @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.
| if (next >= '0' && next <= '7') { | ||
| int octal = 0; | ||
| int count = 0; | ||
| while (i + 1 < value.length && count < 3) { | ||
| byte c = value[i + 1]; | ||
| if (c >= '0' && c <= '7') { | ||
| octal = octal * 8 + (c - '0'); | ||
| i++; | ||
| count++; | ||
| } else { | ||
| break; | ||
| } | ||
| } | ||
| out.write(octal); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Octal escape parsing duplicates the last digit into the output.
After consuming octal digits, i is left pointing at the last consumed digit (not past it), and continue skips the outer loop's i++, so that digit character is reprocessed as a plain literal byte on the next iteration.
Concrete trace: for value = "\101" (backslash + "101"), the loop correctly computes octal 0101 = 'A' (0x41) but then also emits the trailing '1' (0x31) as a separate literal byte — output is [0x41, 0x31] instead of just [0x41]. This duplication happens for every octal escape, breaking canonicalization (and therefore isEquivalentTo) for any literal string containing an octal escape sequence.
🐛 Proposed fix
if (next >= '0' && next <= '7') {
int octal = 0;
int count = 0;
- while (i + 1 < value.length && count < 3) {
- byte c = value[i + 1];
- if (c >= '0' && c <= '7') {
- octal = octal * 8 + (c - '0');
- i++;
- count++;
- } else {
- break;
- }
- }
+ int j = i + 1;
+ while (j < value.length && count < 3 && value[j] >= '0' && value[j] <= '7') {
+ octal = octal * 8 + (value[j] - '0');
+ j++;
+ count++;
+ }
out.write(octal);
+ i = j;
continue;
}Recommend adding a unit test covering octal escapes (e.g. \101, \7) to isEquivalentTo/getCanonicalBytes to lock in the fix.
📝 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.
| if (next >= '0' && next <= '7') { | |
| int octal = 0; | |
| int count = 0; | |
| while (i + 1 < value.length && count < 3) { | |
| byte c = value[i + 1]; | |
| if (c >= '0' && c <= '7') { | |
| octal = octal * 8 + (c - '0'); | |
| i++; | |
| count++; | |
| } else { | |
| break; | |
| } | |
| } | |
| out.write(octal); | |
| continue; | |
| } | |
| if (next >= '0' && next <= '7') { | |
| int octal = 0; | |
| int count = 0; | |
| int j = i + 1; | |
| while (j < value.length && count < 3 && value[j] >= '0' && value[j] <= '7') { | |
| octal = octal * 8 + (value[j] - '0'); | |
| j++; | |
| count++; | |
| } | |
| out.write(octal); | |
| i = j; | |
| continue; | |
| } |
🤖 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/COSString.java` around lines 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.
| @Override | ||
| public boolean isEquivalentTo(Object o) { | ||
| if (this == o) return true; | ||
| if (!(o instanceof COSString)) return false; | ||
|
|
||
| COSString that = (COSString) o; | ||
|
|
||
| byte[] thisBytes = this.getCanonicalBytes(); | ||
| byte[] thatBytes = that.getCanonicalBytes(); | ||
|
|
||
| return Arrays.equals(thisBytes, thatBytes); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Missing COSObject unwrap breaks parity with sibling isEquivalentTo implementations.
COSStream, COSDictionary, and COSArray all unwrap a COSObject argument before the type check in their public isEquivalentTo(Object). COSString doesn't, so someString.isEquivalentTo(someCOSObjectWrappingAnEquivalentString) incorrectly returns false instead of delegating to the wrapped value, even though it's a reasonable call pattern given the established contract.
🛠️ Proposed fix
`@Override`
public boolean isEquivalentTo(Object o) {
if (this == o) return true;
+ if (o instanceof COSObject) {
+ return this.isEquivalentTo(((COSObject) o).get());
+ }
if (!(o instanceof COSString)) 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.
| @Override | |
| public boolean isEquivalentTo(Object o) { | |
| if (this == o) return true; | |
| if (!(o instanceof COSString)) return false; | |
| COSString that = (COSString) o; | |
| byte[] thisBytes = this.getCanonicalBytes(); | |
| byte[] thatBytes = that.getCanonicalBytes(); | |
| return Arrays.equals(thisBytes, thatBytes); | |
| } | |
| `@Override` | |
| public boolean isEquivalentTo(Object o) { | |
| if (this == o) return true; | |
| if (o instanceof COSObject) { | |
| return this.isEquivalentTo(((COSObject) o).get()); | |
| } | |
| if (!(o instanceof COSString)) return false; | |
| COSString that = (COSString) o; | |
| byte[] thisBytes = this.getCanonicalBytes(); | |
| byte[] thatBytes = that.getCanonicalBytes(); | |
| return Arrays.equals(thisBytes, thatBytes); | |
| } |
🤖 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/COSString.java` around lines 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.
1 participant
Add support for object comparison from ISO32000-2:2020 Annex J
Summary by CodeRabbit