Read the alignment tags from DmarcRua's properties again - #209
Conversation
2.0.1 replaced PolicyPublishedType.Adkim/.Aspf with get-only properties computed from new AdkimRaw/AspfRaw strings, and the helper behind them called Regex.Replace on a value that can be null. Both tags are minOccurs="0", so a reporter that omits them made merely *reading* the property throw ArgumentNullException — after deserialization had already succeeded, so it surfaced in the parser rather than as a parse error. That is 1.5% of the reports vendored in the library's own test resources, Mail.Ru and Fastmail among them. MapAlignment read the raw strings instead to keep 2.0.0's behaviour. 2.1.0 null-guards the helper (danielsen/DmarcRua#11), so the properties are safe again, and the comment on the workaround set out what to check before removing it: that an absent tag returns null rather than throwing, and that the library has not changed what absent *means*. Both hold — verified against the published package rather than inferred from the diff — so MapAlignment takes the AlignmentType? directly and collapses to one line. Relaxed stays this project's reading, not the library's. DmarcRua returns null for absent, empty and unrecognised alike, and all three land on relaxed: an alignment we could not parse is not a reason to claim the stricter policy. The existing theory covers all three cases and needed no change, which is the point of it — every other parser test supplies both tags, so without those cases the suite would go green on exactly the kind of upgrade that broke 2.0.1. Its comment is updated to say it now guards the library rather than a workaround around it. No behaviour change: 846 unit tests pass, including the six alignment cases. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: QUIET Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe parser now uses DmarcRua’s typed nullable alignment properties. ChangesDMARC alignment parsing
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized parser cleanup preserves the documented alignment behavior, and no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🟢 Approval recommended
The focused change preserves documented behavior and is covered by existing alignment regression cases.
Pull request overview
Retires the obsolete DmarcRua 2.0.1 alignment workaround after the 2.1.0 null-handling fix, preserving existing parser behavior.
Changes:
- Reads alignment through
AdkimandAspfproperties. - Simplifies alignment mapping while retaining relaxed defaults.
- Updates test commentary and implementation status documentation.
File summaries
| File | Description |
|---|---|
src/api/Application/Reports/DmarcRuaReportParser.cs |
Uses nullable alignment enums directly. |
src/api.tests/DmarcRuaReportParserTests.cs |
Documents regression coverage for optional and invalid tags. |
docs/planning/status.md |
Records retirement of the workaround. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Follow-up to #204, which bumped DmarcRua to 2.1.0 and deliberately left the parser untouched. This retires the one workaround that upgrade made obsolete. No behaviour change.
Background
2.0.1 replaced
PolicyPublishedType.Adkim/.Aspfwith get-only properties computed from newAdkimRaw/AspfRawstrings, and the helper behind them calledRegex.Replaceon a value that can be null. Both tags areminOccurs="0", so a reporter that simply omits them made reading the property throwArgumentNullException— after deserialization had already succeeded, so it surfaced in the parser rather than as a parse error. That is 1.5% of the reports vendored in the library's own test resources, Mail.Ru and Fastmail among them.MapAlignmentread the raw strings instead, to keep 2.0.0's behaviour without waiting on an upstream fix.2.1.0 null-guards the helper (danielsen/DmarcRua#11).
Why it is safe to remove now
The workaround's own comment set out what to check before going back to the properties. Both conditions hold, verified against the published package rather than inferred from the diff:
Adkim = null (AdkimRaw = null), no exceptionMapAlignmenttherefore takes theAlignmentType?directly and collapses from a nine-line character-scrubbing routine to one line. The library's own trimming and case folding is why<adkim> S </adkim>still reads as strict.What deliberately did not change
Relaxed stays this project's decision, not the library's. DmarcRua returns null for absent, empty and unrecognised alike, and all three land on relaxed — an alignment we could not parse is not a reason to claim the stricter policy. Unlike
sp,adkim/aspfhave fixed RFC 7489 §6.3 defaults, so this still needs noHasSubdomainPolicyTag-style presence sniff.Tests
Parse_DefaultsAlignmentWhenTagIsAbsentOrUnusableneeded no change — all six cases pass against the properties exactly as they did against the raw strings:<adkim>s</adkim>, aspf omitted<aspf>s</aspf><adkim></adkim>,<aspf/><adkim> S </adkim>,<aspf>R</aspf><adkim>strict</adkim>,<aspf>bogus</aspf>That the theory needed no change is the point of it. Every other parser test supplies both tags, so without these cases the suite goes green on exactly the kind of upgrade that broke 2.0.1 — which is what happened. Its comment is updated to say it now guards the library rather than a workaround around it.
846/846 unit tests pass, build clean with 0 warnings.
Still in place
The DMARCbis disposition machinery stays, for the reason recorded in #204: 2.1.0 retyped
PolicyEvaluatedType.Dispositionbut never pointedrua.xsdat the new type, so a conformantpassreport deserializes correctly and then fails schema validation. Being reported upstream separately.🤖 Generated with Claude Code