feat: add QTI 3.0 schema validator - #6027
Conversation
rtibbles
left a comment
There was a problem hiding this comment.
This looks broadly correct to me - one question about the errors, one blocking comment about ongoing maintainability.
| access at runtime — required since bulk publish and ricecooker upload validate many items | ||
| per run. Do not restore the absolute URLs. | ||
|
|
||
| To refresh to a newer QTI point release: re-download each file above, re-apply the |
There was a problem hiding this comment.
This documentation points to an uncommitted file to tell us how to do this. It would be far better to have a small Python script that can be rerun to do this repeatably and reliably.
There was a problem hiding this comment.
Good point — added refresh_schema.py alongside the vendored schema. It downloads the QTI-item XSD and its W3C/MathML/SSML dependencies and reapplies the schemaLocation rewrites, so refreshing to a newer QTI point release is just running the script and re-running the test suite. Updated the README to point at it instead of the plan file. (5b56e52)
| is_valid = schema.validate(doc) | ||
| errors = [ | ||
| QTIValidationError(message=err.message, line=err.line, column=err.column) | ||
| for err in schema.error_log |
There was a problem hiding this comment.
How does the schema.error_log get reset between validation runs? Is that built into the validate call?
There was a problem hiding this comment.
Confirmed empirically: error_log is reset by validate() itself — each call clears the log before writing that call's results, whether it passes or fails (verified: validate(bad) → 1 error, validate(good) → empty log, validate(bad) again → the same 1 error, no accumulation). So no manual reset is needed between calls.
The remaining risk was concurrency, not staleness: since the schema instance is cached and shared, two threads calling validate() on it at the same time could race — one thread's validate() resets the log before the other has read its own results. That's exactly what the lock added in 257aa28 (already on this branch, in response to a self-review pass before your comment) serializes: validate() and the error_log read now happen atomically per call, so results stay correctly attributed under concurrent use. No further change needed here.
Schema is compiled once and cached module-wide across calls. Parsing uses a hardened lxml parser (entities disabled, no network access): QTI reaches this validator from ricecooker uploads and direct sync-API writes, not just the editor, so the input is untrusted.
lru_cache on _secure_parser() reused a single mutable XMLParser instance across calls; build a fresh one per call instead, since parsing is cheap and parser reuse across concurrent calls is unsafe. XMLSchema.validate() writes to the schema object's shared error_log, so concurrent validate() calls on the cached schema instance could read each other's errors. Serialize validate + error_log read with a lock to keep results correctly attributed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
README.md pointed at .plans/index.md, which isn't committed to the repo. Add refresh_schema.py to download the vendored QTI/W3C/MathML/SSML XSDs and reapply the local schemaLocation rewrites repeatably, and point the README at it instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
5b56e52 to
c07cfc6
Compare
rtibbles
left a comment
There was a problem hiding this comment.
A bit more needed to make this fully maintainable.
| @@ -0,0 +1,91 @@ | |||
| """Re-vendor the QTI 3.0 item XSD schema tree. | |||
There was a problem hiding this comment.
Rerunning this script, while it produces nearly the same output as committed here, there are formatting differences that suggest that additional processing happened to them that isn't happening in this script.
After running them through pre-commit most of the differences disappeared, but XInclude.xsd still showed a binary difference, not sure what the diff was.
I think it would be helpful for this script to run pre-commit on the schema files after the download, and also nail down why XInclude.xsd is showing a diff?
There was a problem hiding this comment.
Found it — XInclude.xsd is served by purl.imsglobal.org as UTF-16 with no XML declaration, unlike every other source file here (which are all plain UTF-8). That's why it kept showing a binary diff: the script wrote the raw UTF-16 bytes straight to disk while the committed copy had been normalized to UTF-8 by hand at some point.
Fixed in 804a8e4: refresh_schema.py now transcodes any UTF-16 download to UTF-8 (decode/re-encode, not a round-trip through lxml, so the original formatting isn't reflowed), and runs pre-commit run --files on the vendored .xsd tree after download+rewrite — that only triggers trailing-whitespace/end-of-file-fixer for these files, matching what had evidently been applied to the committed copies.
Verified: re-running the script now produces a byte-for-byte empty git diff against every committed vendored file, including XInclude.xsd.
…sh_schema XInclude.xsd is served by purl.imsglobal.org as UTF-16 with no XML declaration, unlike every other vendored source (plain UTF-8). Re-running the script always produced a binary diff on that one file. Transcode any UTF-16 download to UTF-8 (preserving formatting rather than reflowing via lxml) and run the repo's trailing-whitespace/end-of-file-fixer hooks on the vendored tree after download, matching what was applied to the committed copies. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
rtibbles
left a comment
There was a problem hiding this comment.
Excellent, the refresh script now runs and lands clean! Great work.
Summary
Adds
validate_qti_item(), a standalone backend function that validates a QTI item's raw XML against the official QTI 3.0 item XSD schema (imsqti_itemv3p0p1_v1p0.xsd), independent of whether the XML came from the editor, ricecooker, or a direct sync-API write. Returns aQTIValidationResult(pass/fail plus structured errors with message/line/column) instead of raising.The XSD tree (QTI item schema + its W3C/MathML/SSML dependencies) is vendored into the repo with all
schemaLocationreferences rewritten to local files, so the schema compiles with zero network access — bulk publish and ricecooker upload validate many items per run.schema/refresh_schema.pyre-downloads and re-applies those rewrites repeatably for future QTI point releases.The compiled
lxml.etree.XMLSchemais built once behind anlru_cacheand reused across calls.XMLSchema.validate()writes to a sharederror_log, so a lock serializes validate-and-read across concurrent calls to keep errors correctly attributed.XML parsing uses a hardened
lxml.etree.XMLParser(resolve_entities=False, no_network=True, load_dtd=False, huge_tree=False), built fresh per call, since this validator sits in the path of untrusted input (ricecooker uploads, direct sync-API writes), not just editor-authored XML.References
Closes #6005
Reviewer guidance
Run
pytest contentcuration/contentcuration/tests/utils/qti/ -v. New coverage:qti-match-interaction,qti-order-interaction)validate_qti_item()callsOther areas worth a close look:
_secure_parser()incontentcuration/contentcuration/utils/assessment/qti/validation.py— the entity/network/DTD hardening, given untrusted input reaches this from ricecooker/sync-API..pre-commit-config.yaml— added an exclude tocheck-added-large-filesfor the vendoredschema/xsd/*.xsdfiles. The officialimsqti_itemv3p0p1_v1p0.xsdis ~18MB (verified genuine spec content — per-attribute schematron assertions generated by IMS's own tooling, not accidental duplication).lxml==6.1.1inrequirements.in/requirements.txt.AI usage
Implemented with Claude Code from a maintainer-approved plan (
.plans/index.md, issue #6005), using TDD per task. One vendored dependency,XInclude.xsd, turned out to be UTF-16 encoded. The repo's whitespace-fixing pre-commit hooks corrupted it on first commit attempt. I re-encoded it to UTF-8 and re-verified the schema still compiles fully offline. Ran the full QTI test suite and confirmed flake8/black compliance before opening this PR.@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Status: 🟡 Waiting for feedback · updated 2026-07-03 02:21 UTC