From e3f0fdc1ab59c10da31b33ca4f2fd55a5cf90c73 Mon Sep 17 00:00:00 2001 From: Jonathan Bernard Date: Sat, 28 Mar 2026 19:25:45 -0500 Subject: [PATCH] Refactor vCard validation and canonicalize VERSION handling Move whole-card cardinality checks into overloaded validate helpers in the vCard 3 and vCard 4 modules, and have readVCard invoke those validators after parsing instead of hard-coding property checks in the shared parser. For vCard 3, validate property cardinality directly from propertyCardMap and add regression coverage for duplicate single-cardinality properties. For vCard 4, validate from propertyCardMap as well, while preserving ALTID semantics for single-cardinality properties so alternate representations remain valid and duplicate ungrouped properties are rejected. Rework VERSION handling to treat it as parser metadata rather than stored card content. Parsing still requires exactly one canonical VERSION line and rejects missing or duplicate occurrences, but parsed cards no longer retain VC3_Version or VC4_Version objects in content. Accessors and serializers continue to expose and emit the canonical version for the concrete card type, which keeps parsed and programmatically constructed cards on the same model. Update the vCard 3 and vCard 4 test suites to cover the new validation path, confirm VERSION is not persisted in content, and adjust fixture expectations to match the canonical VERSION model. AI-Assisted: yes AI-Tool: OpenAI Codex / gpt-5.4 xhigh --- src/vcard.nim | 8 ++++++ src/vcard/vcard3.nim | 45 ++++++++++++++++++++++++------- src/vcard/vcard4.nim | 63 +++++++++++++++++++++++++++++++++++++++++++- tests/tvcard3.nim | 23 +++++++++++++++- tests/tvcard4.nim | 12 ++++++++- 5 files changed, 139 insertions(+), 12 deletions(-) diff --git a/src/vcard.nim b/src/vcard.nim index 95ee403..08c6a7e 100644 --- a/src/vcard.nim +++ b/src/vcard.nim @@ -55,6 +55,14 @@ proc readVCard*(p: var VCardParser): VCard = if result.parsedVersion == VCardV3: while (p.skip(CRLF, true)): discard + try: + if result.parsedVersion == VCardV3: + cast[VCard3](result).validate() + else: + cast[VCard4](result).validate() + except ValueError as exc: + p.error(exc.msg) + proc initVCardParser*(input: Stream, filename = "input"): VCardParser = result.filename = filename lexer.open(result, input) diff --git a/src/vcard/vcard3.nim b/src/vcard/vcard3.nim index f2a1198..b318834 100644 --- a/src/vcard/vcard3.nim +++ b/src/vcard/vcard3.nim @@ -707,18 +707,39 @@ genPropertyAccessors(propertyCardMap.pairs.toSeq --> filter(not [pnVersion].contains(it[0]))) func version*(vc3: VCard3): VC3_Version = - ## Return the VERSION property. - let found = findFirst[VC3_Version, VC3_Property](vc3.content) - if found.isSome: return found.get - else: return VC3_Version( - propertyId: vc3.content.len + 1, - group: none[string](), - name: "VERSION", - value: "3.0") + ## Return the canonical VERSION property for a vCard 3.0 card. + result = newVC3_Version() func xTypes*(vc3: VCard3): seq[VC3_XType] = findAll[VC3_XType, VC3_Property](vc3.content) ## Return all extended properties (starting with `x-`). +func validate*(vc3: VCard3): void = + ## Validate property cardinality for a vCard 3.0 card. + for pn in VC3_PropertyName: + if pn == pnVersion: + continue + + let count = vc3.content.countIt(it.name == $pn) + + case propertyCardMap[pn] + of vpcExactlyOne: + if count != 1: + raise newException(ValueError, + "VCard should have exactly one $# property, but $# were found" % + [$pn, $count]) + of vpcAtLeastOne: + if count < 1: + raise newException(ValueError, + "VCard should have at least one $# property, but $# were found" % + [$pn, $count]) + of vpcAtMostOne: + if count > 1: + raise newException(ValueError, + "VCard should have at most one $# property, but $# were found" % + [$pn, $count]) + of vpcAny: + discard + # Setters # ============================================================================= @@ -1030,6 +1051,7 @@ proc readBinaryValue( proc parseContentLines*(p: var VCardParser): seq[VC3_Property] = result = @[] + var sawVersion = false macro assignCommon(assign: untyped): untyped = result = assign @@ -1271,9 +1293,11 @@ proc parseContentLines*(p: var VCardParser): seq[VC3_Property] = result.add(newVC3_URL(group = group, value = p.readValue)) of $pnVersion: + if sawVersion: + p.error("VCard should have exactly one VERSION property, but 2 were found") p.expect("3.0") p.validateNoParameters(params, "VERSION") - result.add(newVC3_Version(group = group)) + sawVersion = true of $pnClass: result.add(newVC3_Class(group = group, value = p.readValue)) @@ -1312,6 +1336,9 @@ proc parseContentLines*(p: var VCardParser): seq[VC3_Property] = p.expect("\r\n") + if not sawVersion: + p.error("VCard should have exactly one VERSION property, but 0 were found") + #[ Simplified Parsing Diagram diff --git a/src/vcard/vcard4.nim b/src/vcard/vcard4.nim index cb3046c..bf75eb2 100644 --- a/src/vcard/vcard4.nim +++ b/src/vcard/vcard4.nim @@ -968,6 +968,56 @@ genPidAccessors(supportedParams["PID"].toSeq()) genPrefAccessors(supportedParams["PREF"].toSeq()) genTypeAccessors(supportedParams["TYPE"].toSeq()) +func countCardinalityInstances(vc4: VCard4, propName: string): int = + var altIds = initHashSet[string]() + + for p in vc4.content: + if p.name != propName: + continue + + if p.altId.isSome: + altIds.incl(p.altId.get) + else: + inc result + + result += altIds.len + +func validate*(vc4: VCard4): void = + ## Validate property cardinality for a vCard 4.0 card. + for pn in VC4_PropertyName: + if pn == pnVersion: + continue + + if not propertyCardMap.contains(pn): + continue + + let rawCount = vc4.content.countIt(it.name == $pn) + let cardinalityCount = + case propertyCardMap[pn] + of vpcExactlyOne, vpcAtMostOne: + vc4.countCardinalityInstances($pn) + of vpcAtLeastOne, vpcAny: + rawCount + + case propertyCardMap[pn] + of vpcExactlyOne: + if cardinalityCount != 1: + raise newException(ValueError, + "VCard should have exactly one $# property, but $# were found" % + [$pn, $cardinalityCount]) + of vpcAtLeastOne: + if cardinalityCount < 1: + raise newException(ValueError, + "VCard should have at least one $# property, but $# were found" % + [$pn, $cardinalityCount]) + of vpcAtMostOne: + if cardinalityCount > 1: + raise newException(ValueError, + ("VCard should have at most one $# property, but $# " & + "distinct properties were found") % [$pn, $cardinalityCount]) + of vpcAny: + discard + # Setters # ============================================================================= @@ -1382,6 +1432,7 @@ macro genPropParsers( proc parseContentLines*(p: var VCardParser): seq[VC4_Property] = result = @[] + var sawVersion = false while true: let group = p.readGroup @@ -1392,10 +1443,20 @@ proc parseContentLines*(p: var VCardParser): seq[VC4_Property] = let params = p.readParams p.expect(":") - genPropParsers(fixedValueTypeProperties, group, name, params, result, p) + if name == $pnVersion: + if sawVersion: + p.error("VCard should have exactly one VERSION property, but 2 were found") + p.validateType(params, vtText) + p.expect("4.0") + sawVersion = true + else: + genPropParsers(fixedValueTypeProperties, group, name, params, result, p) p.expect(CRLF) + if not sawVersion: + p.error("VCard should have exactly one VERSION property, but 0 were found") + # Private Function Unit Tests # ============================================================================ diff --git a/tests/tvcard3.nim b/tests/tvcard3.nim index ca991ae..e212e56 100644 --- a/tests/tvcard3.nim +++ b/tests/tvcard3.nim @@ -1,4 +1,4 @@ -import std/[options, strutils, times, unittest] +import std/[options, sequtils, strutils, times, unittest] import zero_functional import vcard @@ -57,6 +57,7 @@ suite "vcard/vcard3": "BEGIN:vCard\r\n" & "VERSION:3.0\r\n" & "FN:Frank Dawson\r\n" & + "N:Dawson;Frank;;;\r\n" & "ORG:Lotus Development Corporation\r\n" & "ADR;TYPE=WORK,POSTAL,PARCEL:;;6544 Battleford Drive\r\n" & " ;Raleigh;NC;27613-3502;U.S.A.\r\n" & @@ -71,6 +72,7 @@ suite "vcard/vcard3": "BEGIN:vCard\r\n" & "VERSION:3.0\r\n" & "FN:Tim Howes\r\n" & + "N:Howes;Tim;;;\r\n" & "ORG:Netscape Communications Corp.\r\n" & "ADR;TYPE=WORK:;;501 E. Middlefield Rd.;Mountain View;\r\n" & " CA; 94043;U.S.A.\r\n" & @@ -104,6 +106,25 @@ suite "vcard/vcard3": "VERSION:3.0", "FN:John Smith")) + test "spec: parser rejects duplicate single-cardinality vCard 3 properties": + expect(VCardParsingError): + discard parseVCards(vcard3Doc( + "VERSION:3.0", + "FN:John Smith", + "N:Smith;John;;;", + "BDAY:2000-01-01", + "BDAY:2000-01-02")) + + test "spec: VERSION is not stored as vCard 3 content": + let parsed = parseSingleVCard3(vcard3Doc( + "VERSION:3.0", + "FN:John Smith", + "N:Smith;John;;;")) + + check: + parsed.version.value == "3.0" + parsed.content.countIt(it of VC3_Version) == 0 + test "spec: simple text values decode RFC 2426 escapes when parsing": let parsed = parseSingleVCard3(vcard3Doc( "VERSION:3.0", diff --git a/tests/tvcard4.nim b/tests/tvcard4.nim index a63ee41..e938b1c 100644 --- a/tests/tvcard4.nim +++ b/tests/tvcard4.nim @@ -1,4 +1,4 @@ -import std/[options, strutils, tables, unittest] +import std/[options, sequtils, strutils, tables, unittest] import zero_functional import vcard @@ -203,6 +203,7 @@ suite "vcard/vcard4": madeUpProp.value == "Sample value for my made-up prop." let cardWithAltBdayStr = testVCardTemplate % [( + "FN:Simon Perreault\r\n" & "BDAY;VALUE=text;ALTID=1:20th century\r\n" & "BDAY;VALUE=date-and-or-time;ALTID=1:19650321\r\n" )] @@ -210,6 +211,14 @@ suite "vcard/vcard4": test "single-cardinality properties allow multiples with ALTID": check parseVCards(cardWithAltBdayStr).len == 1 + test "single-cardinality properties reject multiples without ALTID": + expect(VCardParsingError): + discard parseVCards(testVCardTemplate % [( + "FN:Simon Perreault\r\n" & + "BDAY;VALUE=text:20th century\r\n" & + "BDAY;VALUE=date-and-or-time:19650321\r\n" + )]) + let hasAltBdays = cast[VCard4](parseVCards(cardWithAltBdayStr)[0]) test "properties with cardinality 1 and altids return the first found by default": @@ -222,6 +231,7 @@ suite "vcard/vcard4": check: hasAltBdays.content.len == 3 hasAltBdays.bday.isSome + hasAltBdays.content.countIt(it of VC4_Version) == 0 let allBdays = allAlternatives[VC4_Bday](hasAltBdays) check: