Repository navigation
[US-1983] Add PDF/UA structural content rules to validate - #38
Conversation
anovik
left a comment
There was a problem hiding this comment.
No blocking bugs found, but I'd fix the first comment (mcid-in-parent-tree passing pages it couldn't read) before merging. I reproduced the validate_structure.go correctness comments (L201, L284, L295, L298) with test fixtures; the rest are from reading the code.
One more that has no line in the diff: aria-controls="validate-rules-body" (ValidateView.tsx:177) points at an element that isn't rendered while the section is collapsed.
| // finding naming every MCID of the page in ascending order. A structure tree | ||
| // root whose read failed part way, or a ParentTree node that was not read, | ||
| // leaves the rule not evaluated, since ParentTree entries may be missing. | ||
| func checkMCIDInParentTree(_ *DocumentState, rep *StructureReport) ([]ruleHit, error) { |
There was a problem hiding this comment.
This is an error rule that gates the exit code, but on a page whose content stream couldn't be decoded, or whose /Contents couldn't be read, it counts itself as evaluated with 0 findings. The MCIDs on that content were never checked.
Reproduced with uaStreamsPDF(uaSlots(1), "/StructParents 0", "/P <</MCID 0>> BDC EMC", undecodableStream), and again with a page whose /Contents is an integer. Both give mcid-in-parent-tree evaluated=true findings=0, and the only problem is a marked-content-balance warning, so the CLI exits 0. A file whose broken stream holds MCIDs with no ParentTree slot passes, though veraPDF fails it under 7.1.
The scope sentence says unreadable content is reported under marked-content-balance, but that rule doesn't gate. I'd make this rule not evaluated when any page has sp.Err or an undecoded stream, the same fail-closed handling pagesComplete gives an early page-tree stop.
There was a problem hiding this comment.
@anovik Fixed in c915f3c, though not by switching the whole rule off: one broken stream on page 40 would then hide real orphan findings on pages 1-39. Instead mcid-in-parent-tree adds an error finding for each page whose content can't be read and each stream that can't be decoded ("...; its MCIDs were not checked against the ParentTree"), and keeps checking MCIDs in the streams that did decode. Your repro now exits 1. Tests: TestValidateUA_UndecodableStreamMCIDsAreNotCheckedErrors, TestValidateUA_UnreadableContentsMCIDsAreNotCheckedError.
| // 14.7.2). A structure tree root that could not be read leaves the rule not | ||
| // evaluated; an absent root is struct-tree-root's finding, not this rule's. | ||
| func checkStructElemType(_ *DocumentState, rep *StructureReport) ([]ruleHit, error) { | ||
| if msg := rootProblem(rep, rootNotDictPrefix, rootUnreadablePrefix); msg != "" { |
There was a problem hiding this comment.
A dangling /StructTreeRoot (/StructTreeRoot 99 0 R where object 99 is free) records /StructTreeRoot is null, not a dictionary. That matches rootNotDictPrefix, so this rule is not evaluated, and because it's an error rule the CLI exits 1 with "a rule that gates the exit code could not be evaluated". Meanwhile struct-tree-root passes (verified: evaluated, 0 findings).
Under ISO 32000-1 7.3.10 a reference to a missing object is null, so the root is absent, and the comment above says an absent root is struct-tree-root's finding, not this rule's. I'd treat a null root as absent here and let struct-tree-root report it.
There was a problem hiding this comment.
@anovik Fixed in c915f3c. The walker treats a /StructTreeRoot that doesn't resolve as absent (it records a catalog problem, and HasStructTree is false), and struct-tree-root now flags a value that is null or a reference to a missing object. A dangling root is one struct-tree-root warning, struct-elem-type is evaluated with 0 findings, and the run exits 0. Test: TestValidateUA_DanglingStructTreeRootIsAbsent.
| at := fmt.Sprintf("/K index %d of %s", i, owner) | ||
| if e.S == "" { | ||
| if e.Ref != "" { | ||
| hits = append(hits, objHit(e.Ref, fmt.Sprintf("structure element %s has no usable /S (%s)", e.Ref, asciiText(e.Err)))) |
There was a problem hiding this comment.
The whole joined e.Err goes into the /S reason, so unrelated faults appear as the reason /S is unusable. Reproduced with << /S / /Pg 2 0 R >>:
structure element 12 0 R has no usable /S (/S is an empty name; /Pg 2 0 R is a dictionary that is not a page in the page tree)
Also, if readElement's guard catches a panic before S is set, this reports an error-severity "no usable /S" finding when the reader failed. It would be cleaner to keep the /S problem as its own field, and to leave the rule not evaluated (or skip the element) when the element itself couldn't be read.
There was a problem hiding this comment.
| continue | ||
| } | ||
| e := k.Element | ||
| at := fmt.Sprintf("/K index %d of %s", i, owner) |
There was a problem hiding this comment.
i is the index into the walked kids slice, but walkK skips null items in the /K array, so the index is off after a null entry. Reproduced with /K [null << /S / >>]: the message says /K index 0 of 10 0 R, but the element is at /K index 1.
| if err != nil { | ||
| t.Fatalf("golden %s missing (run with PDFDEBUG_WRITE_VERAPDF_GOLDENS=1 to capture it): %v", goldenPath, err) | ||
| } | ||
| if !bytes.Equal(got, want) { |
There was a problem hiding this comment.
This byte-for-byte comparison includes veraPDFVersion and every log message, so with any veraPDF other than 1.30.2 installed (at the default path or $VERAPDF) all six subtests fail instead of skipping. oracle_verapdf_test.go accepts other versions. I'd either skip when the version differs from the golden's, or compare only compliant and the failed rule summaries.
| } | ||
| if !strings.Contains(snippet, "pdfua-1-structural") { | ||
| t.Fatalf("%s CI example does not run the pdfua-1-structural profile, whose findings are warnings with exit 0:\n%s", | ||
| t.Fatalf("%s CI example does not run the pdfua-1-structural profile, whose catalog and /S rules report warnings that exit 0:\n%s", |
There was a problem hiding this comment.
This message says the profile's "catalog and /S rules report warnings that exit 0", but struct-elem-type (the /S rule) is now an error rule that exits 1, as the registry, CHANGELOG and docs/cli/scripting.md say.
|
|
||
| // pagesComplete returns an error when the page-tree walk stopped early, so a | ||
| // content rule is not evaluated instead of passing on the pages it saw. | ||
| func pagesComplete(rep *StructureReport) error { |
There was a problem hiding this comment.
pagesComplete, rootProblem and parentTreeTruncated detect truncation by matching Where/Message prefixes of report problems (pageTreeWhere, rootUnreadablePrefix, ntTooDeepPrefix, ntUnreadablePrefix). If one of those messages is reworded, or the walker gains a new early return, the truncated read is treated as complete. For example, /Kids is X, not an array already drops a whole subtree without matching any of them. Explicit flags set by the walker would be safer (e.g. PagesTruncated, RootReadFailed, ParentTreeTruncated on StructureReport).
There was a problem hiding this comment.
@anovik Fixed in c915f3c. The prefix matching is gone. StructureReport carries unexported flags (pagesIncomplete, rootNotDict, rootReadFailed, parentTreeIncomplete), set where the walkers drop something. The number-tree walker marks every fault that leaves entries unread, /Kids or /Nums not an array included. The page walker marks a subtree it skipped (too deep, or its read failed), which the old check also missed.
| var rep *StructureReport | ||
| var repErr error | ||
| if profileReadsStructure(profile) { | ||
| rep, repErr = buildValidateStructure(doc, newWalker) |
There was a problem hiding this comment.
Every pdfua-1-structural run builds the full structure report. decodeStream reads streamCache but never writes to it, so each run decodes every content stream again (with per-page pdfMu contention against rendering in the GUI). It also computes alignment, claims and OtherParentTree, which no rule reads. Writing decoded tokens to the cache, or letting the validate walker skip the alignment pass, would avoid this.
There was a problem hiding this comment.
@anovik Left as is. Keeping the structure walk out of streamCache was a deliberate decision when dump structure was added, and TestGetStructure_CleanFileDoesNotFillStreamCache enforces it. The cache has no eviction and holds raw, tokenized and formatted content until the tab closes, so one validate run would pin every page of the document. Skipping alignment saves little next to the decode. If repeat runs on large files turn out slow in practice, I'd add a validate-scoped cache that can be dropped, in a follow-up.
| // is kept here by tabId and restored when that document becomes active | ||
| // again. A reload mints a new tabId, so a reloaded document starts with no | ||
| // result. | ||
| const savedRef = useRef(new Map<string, SavedValidation>()); |
There was a problem hiding this comment.
savedRef keeps a full ValidateResult for every tab id seen in the session and never removes one. That includes closed tabs and the old ids reloads leave behind. A document with thousands of findings reloaded a few times keeps every old result in memory until the window closes. I'd remove an entry when its tab id leaves the tab strip.
|
|
||
| // objHit is a finding on the object with reference ref ("N G R"), or a | ||
| // document-level finding when ref is empty. | ||
| func objHit(ref, msg string) ruleHit { |
There was a problem hiding this comment.
Nit: this parses the "N G R" string back with fmt.Sscanf to rebuild the node id, although StructElem.NodeID already holds it and nodeIDForRef/nodeIDFromRef exist. That makes a second, string-based source of the node-id format that can drift. struct-elem-type could pass e.NodeID directly.
anovik
left a comment
There was a problem hiding this comment.
Thanks, everything from round 1 is addressed. I reran my original repros on 766b4f0 and all of them now behave as intended. I agree on leaving the stream cache as it is.
Non-blocking follow-up: the page-tree walker now marks a subtree as skipped when it's too deep or its read fails. A /Pages node whose /Kids isn't an array still drops its pages without marking them (pageindex.go:292, /Kids is not an array: dictionary), so the content rules don't know those pages are missing.
Repro: the root /Pages has /Kids [3 0 R 4 0 R]. Node 4 is << /Type /Pages /Kids 7 0 R /Count 1 >>, and page 7's content has /P <</MCID 3>> BDC EMC with no ParentTree entry. mcid-in-parent-tree is evaluated with 0 findings and the run exits 0.
It needs a malformed page tree and isn't a regression, so it can be a follow-up ticket. The fix is probably just w.skip(...) in place of the errorRow append at that line.
Description and links
validate --profile pdfua-1-structuralonly looked at the catalog (/MarkInfo,/StructTreeRoot,/Lang). On creator-tagged files it said nothing while veraPDF found an MCID with no ParentTree entry, an empty/S, and a table whose BDC/EMC pairs broke across a page. This adds five rules that read thedump structurereport from #37 and check the marked content of every page and the/Sof every structure element.Severities follow veraPDF 1.30.2 on the same faults. A rule is an error (exit 1) when veraPDF fails the file for it, and a warning when veraPDF only logs it:
mcid-in-parent-tree/StructParentsand the ParentTree; a BDC whose MCID can't be read, and page content or a stream that can't be read, as not checkedstruct-elem-type/Sname; an element that can't be read, as not checkedmarked-content-balance/Contents(an array counts as one stream); an undecodable content stream is reported heremcid-uniquemcid-not-nestedThe Validate tab shows the new findings without a component change. While testing it on a production build I also fixed two things there: switching documents cleared the result, and the findings sat below the scope sentence and the rule list.
Jira: US-1983
Technical changes
internal/pdfcore/validate_structure.go(new): the five checks. Findings within a page are in content order. File text in messages is ASCII-quoted. Nothing is truncated: a page with 2000 duplicate MCIDs gives 2000 findings, each listing every occurrence.internal/pdfcore/validate.go: registry entries for the five rules and acheckStructurefield next tocheck.Validatebuilds the structure report once, before it takespdfMu, under its ownrecover. A close during the walk returns the cancel error. Any other walk failure leaves only the structure rules not evaluated.runRulereads the catalog first, so an unreadable catalog gives the same message under every rule.mcid-in-parent-treeis not evaluated when the structure tree root or some ParentTree entries could not be read, because the "has no key N" verdict would then be about entries we never saw.struct-elem-typeis not evaluated when the root is not a dictionary or could not be read. A/StructTreeRootthat points to a missing object counts as absent.ProfileGateshad no other caller and is gone.pdfa-1bexit codes don't change, since every rule there is an error rule.testdata/generate_test.go:ua-structural-clean.pdfplus one file per rule that trips only that rule. veraPDF ua1 results for each are committed undertestdata/verapdf-ua1/.tests/compliance-validation/verapdf_ua1_golden_test.gore-runs veraPDF when it is installed (and rewrites withPDFDEBUG_WRITE_VERAPDF_GOLDENS=1), otherwise it skips. CI only checks the recorded mapping.tests/ua-structural-rules: exit codes per fixture,--jsonshape, and the 2000-finding document checked line by line in both outputs.frontend/src/components/ValidateView.tsx: each document keeps its own profile, result, in-flight run and rules-section state, keyed by tab id and dropped when the tab closes. A document starts on PDF/A-1b until its profile is changed. A reload mints a new tab id, so a reloaded document starts empty. The findings come first. The scope sentence and the rule list sit below in a "Rules checked (N)" section, collapsed by default, whose label also counts rules that could not be evaluated.docs/cli-usage.md,docs/cli/scripting.md,docs/cli/validate-and-diff.mdand the godoc no longer say PDF/UA rules never fail the run. CHANGELOG entries under Unreleased (Added and Changed).Considerations
veraPDF 1.30.2 passes files with unbalanced BDC/EMC, duplicate MCIDs or nested MCIDs and only logs a warning. It fails an MCID with no ParentTree entry (7.1-3) and an empty
/S(7.1-5). We follow that split. A content stream that can't be decoded is amarked-content-balancewarning, and also amcid-in-parent-treeerror, since its MCIDs were never checked against the ParentTree. So it fails the run.Only page content is scanned. Form XObjects, annotation appearances and Type3 glyphs are not, and content outside any marked-content sequence (veraPDF 7.1-3 untagged content) is out of scope. Each rule's
checkstext says this.struct-elem-typedoesn't see elements below the walker's 256-level depth cap and still reports the rule as evaluated.How was this tested?
GOFLAGS=-count=1 scripts/test-all.shon the final tree: go vet, golangci-lint 0 issues, the root module, everytests/*/suite includingtests/ua-structural-rules, frontend typecheck, lint and Vitest. All suites passed.go test -race -count=1 ./internal/pdfcore/passed.runtime.Error, cancel, unreadable catalog, early page-tree stop, partly read root, truncated ParentTree), and confirmpdfMuis free afterwards. New guard tests were checked to fail with their guard removed.task package) on macOS: ran the fixtures through the Validate tab, switched between documents, expanded and reran.What could go wrong?
Scripts that run
validate --profile pdfua-1-structuraland expect exit 0 will now get exit 1 on files with unreachable MCIDs, bad/S, or page content that can't be decoded. The CHANGELOG calls this out.A tagged file with a huge number of MCIDs per page produces one finding per fault and very long messages for duplicates. That is deliberate; nothing is truncated.
Screenshots/videos (if appropriate)
Checklist
devbranch, notmaster