Add addDeclarations method#1590
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds public constants Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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: 2
🧹 Nitpick comments (2)
xmp-core/src/main/java/org/verapdf/xmp/impl/VeraPDFMeta.java (2)
379-397: Avoid appending duplicateconformsToentries.The loop unconditionally appends a new struct per URI. Calling
addDeclarationstwice with the same URI — or calling it with a URI already present in the metadata — will produce duplicatedeclarations/*/conformsToentries, which readers may treat as malformed. The class already exposescontainsDeclaration(String); consider reusing it to skip URIs that are already declared.♻️ Proposed refactor
for (String uri : conformsToURIs) { + if (containsDeclaration(uri)) { + continue; + } this.meta.appendArrayItem(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xmp-core/src/main/java/org/verapdf/xmp/impl/VeraPDFMeta.java` around lines 379 - 397, The loop in VeraPDFMeta that appends a new declarations struct for each URI should skip URIs already present to avoid duplicate declarations: before calling meta.appendArrayItem / setStructField for each uri, call the existing containsDeclaration(String) (or the appropriate containsDeclaration method on this) and continue to the next uri when it returns true; only append and set the struct field when containsDeclaration(uri) is false, ensuring addDeclarations (the method containing this loop) does not produce duplicate PDFA_DECLARATIONS/CONFORMS_TO entries.
370-401: Simplify array creation by lettingappendArrayItemhandle it.The standard
XMPMetaImpl.appendArrayItem(schemaNS, arrayName, arrayOptions, itemValue, itemOptions)overload already locates or creates the array node. Passingnew PropertyOptions().setArray(true)asarrayOptionsmakes the pre-emptivesetProperty(...)at lines 374–377 redundant and removes one code path that can drift out of sync (e.g. future callers addingsetArrayOrdered).♻️ Proposed refactor
public VeraPDFMeta addDeclarations(Set<String> conformsToURIs) throws XMPException { if (conformsToURIs == null || conformsToURIs.isEmpty()) { throw new IllegalArgumentException("Argument conformsToURIs can not be null or empty"); } - if (getProperty(PDFA_DECLARATIONS, DECLARATIONS) == null) { - meta.setProperty(PDFA_DECLARATIONS, DECLARATIONS, null, - new PropertyOptions().setArray(true)); - } for (String uri : conformsToURIs) { this.meta.appendArrayItem( PDFA_DECLARATIONS, DECLARATIONS, - null, + new PropertyOptions().setArray(true), null, new PropertyOptions().setStruct(true) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xmp-core/src/main/java/org/verapdf/xmp/impl/VeraPDFMeta.java` around lines 370 - 401, In VeraPDFMeta.addDeclarations, remove the pre-creation of the array via setProperty(PDFA_DECLARATIONS, DECLARATIONS, null, new PropertyOptions().setArray(true)) and let meta.appendArrayItem(...) create the array itself; specifically, delete the block that checks getProperty(...) == null and the setProperty call, and stop passing an explicit array PropertyOptions to appendArrayItem (use null or appropriate itemOptions only) so appendArrayItem handles locating/creating the array node; keep the existing loop that appends struct items, the calls to meta.countArrayItems(...) and XMPPathFactory.composeArrayItemPath(DECLARATIONS, idx), and the subsequent setStructField(...) and update() unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@xmp-core/src/main/java/org/verapdf/xmp/impl/VeraPDFMeta.java`:
- Around line 22-23: The constant PDFD_Prefix uses inconsistent casing; rename
the public static final String PDFD_Prefix to PDFD_PREFIX to follow
SCREAMING_SNAKE_CASE and match other constants (e.g., DC_PREFIX, PDFAID_PREFIX).
Update all references/usages of PDFD_Prefix across the codebase to PDFD_PREFIX;
optionally keep the old PDFD_Prefix as a `@Deprecated` alias that delegates to
PDFD_PREFIX if you need to avoid an immediate breaking change for external
users. Ensure any Javadoc or comments referencing PDFD_Prefix are updated as
well.
- Around line 22-23: Remove the unused constants from the VeraPDFMeta class:
delete the public static final fields DC_PREFIX and PDFD_Prefix (the two
constants defined in VeraPDFMeta) so the class does not introduce unused API
surface; if these prefixes are intended for future use, instead postpone adding
DC_PREFIX and PDFD_Prefix until there are actual consumers.
---
Nitpick comments:
In `@xmp-core/src/main/java/org/verapdf/xmp/impl/VeraPDFMeta.java`:
- Around line 379-397: The loop in VeraPDFMeta that appends a new declarations
struct for each URI should skip URIs already present to avoid duplicate
declarations: before calling meta.appendArrayItem / setStructField for each uri,
call the existing containsDeclaration(String) (or the appropriate
containsDeclaration method on this) and continue to the next uri when it returns
true; only append and set the struct field when containsDeclaration(uri) is
false, ensuring addDeclarations (the method containing this loop) does not
produce duplicate PDFA_DECLARATIONS/CONFORMS_TO entries.
- Around line 370-401: In VeraPDFMeta.addDeclarations, remove the pre-creation
of the array via setProperty(PDFA_DECLARATIONS, DECLARATIONS, null, new
PropertyOptions().setArray(true)) and let meta.appendArrayItem(...) create the
array itself; specifically, delete the block that checks getProperty(...) ==
null and the setProperty call, and stop passing an explicit array
PropertyOptions to appendArrayItem (use null or appropriate itemOptions only) so
appendArrayItem handles locating/creating the array node; keep the existing loop
that appends struct items, the calls to meta.countArrayItems(...) and
XMPPathFactory.composeArrayItemPath(DECLARATIONS, idx), and the subsequent
setStructField(...) and update() unchanged.
🪄 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
Run ID: 062a3aad-0876-4b10-8b26-3bdbbb1bcd06
📒 Files selected for processing (1)
xmp-core/src/main/java/org/verapdf/xmp/impl/VeraPDFMeta.java
| public static final String DC_PREFIX = "dc"; | ||
| public static final String PDFD_Prefix = "pdfd"; |
There was a problem hiding this comment.
Rename PDFD_Prefix to PDFD_PREFIX for consistency.
PDFD_Prefix violates Java's SCREAMING_SNAKE_CASE convention for constants and is inconsistent with every other constant in this class (PDFAID_PREFIX, PDFUAID_PREFIX, PDFA_EXTENSION_PREFIX, DC_PREFIX, etc.). Since it is declared public static final, it becomes part of the public API; fixing the casing now avoids a breaking rename later.
♻️ Proposed fix
public static final String DC_PREFIX = "dc";
- public static final String PDFD_Prefix = "pdfd";
+ public static final String PDFD_PREFIX = "pdfd";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@xmp-core/src/main/java/org/verapdf/xmp/impl/VeraPDFMeta.java` around lines 22
- 23, The constant PDFD_Prefix uses inconsistent casing; rename the public
static final String PDFD_Prefix to PDFD_PREFIX to follow SCREAMING_SNAKE_CASE
and match other constants (e.g., DC_PREFIX, PDFAID_PREFIX). Update all
references/usages of PDFD_Prefix across the codebase to PDFD_PREFIX; optionally
keep the old PDFD_Prefix as a `@Deprecated` alias that delegates to PDFD_PREFIX if
you need to avoid an immediate breaking change for external users. Ensure any
Javadoc or comments referencing PDFD_Prefix are updated as well.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find usages of the newly added constants across the repository.
rg -nP '\b(DC_PREFIX|PDFD_Prefix|PDFD_PREFIX)\b' -C2Repository: veraPDF/veraPDF-library
Length of output: 796
🏁 Script executed:
#!/bin/bash
# Check if the constants are used anywhere in the codebase through imports or references
# 1. Look for any imports of DC_PREFIX or PDFD_Prefix
# 2. Check the entire file to see if they're used internally
# 3. Search for the literal string values "dc" and "pdfd" to see if they're being used
echo "=== Checking VeraPDFMeta.java for any usage of the constants ==="
grep -n "DC_PREFIX\|PDFD_Prefix\|PDFD_PREFIX" xmp-core/src/main/java/org/verapdf/xmp/impl/VeraPDFMeta.java
echo -e "\n=== Checking for imports of VeraPDFMeta constants in the codebase ==="
rg -l "VeraPDFMeta\.(DC_PREFIX|PDFD_Prefix|PDFD_PREFIX)" 2>/dev/null || echo "No imports found"
echo -e "\n=== Checking for direct literal usage of 'dc' and 'pdfd' namespaces in XML/RDF context ==="
rg -n '(dc"|"pdfd")' --type java -C 2 | head -50
echo -e "\n=== Listing all files in the PR context that might use these constants ==="
find . -name "*.java" -path "*/xmp-core/*" -type f | head -20Repository: veraPDF/veraPDF-library
Length of output: 3791
Remove unused constants or defer their introduction until they have consumers.
The constants DC_PREFIX and PDFD_Prefix are not referenced anywhere in the codebase. If they are not intended for immediate use, consider deferring their addition to a future change when they have actual consumers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@xmp-core/src/main/java/org/verapdf/xmp/impl/VeraPDFMeta.java` around lines 22
- 23, Remove the unused constants from the VeraPDFMeta class: delete the public
static final fields DC_PREFIX and PDFD_Prefix (the two constants defined in
VeraPDFMeta) so the class does not introduce unused API surface; if these
prefixes are intended for future use, instead postpone adding DC_PREFIX and
PDFD_Prefix until there are actual consumers.
MaximPlusov
force-pushed
the
declarations
branch
from
5410eac to
24032d7
Compare
2 participants
Summary by CodeRabbit