fix(hal): accept empty href as an RFC 3986 same-document reference - #123
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #120.
Problem
A Link Object whose
hrefis the empty string was silently dropped on deserialization.LinkObjectConvertertreated empty and whitespace-only hrefs alike as invalid and returnednull, so the relation survived as an empty collection and every other property of the dropped object —title,name,templated— was discarded with it.The empty string is a valid same-document URI reference under RFC 3986 §4.4, and HAL §5.1 defines
hrefby reference to RFC 3986. Rejecting it was an acceptance deviation against the letter of the spec, and the silent, undocumented nature of the drop was the worst part of it.Change
Read path.
LinkObjectConverter.ReadFromNodenow branches explicitly, in an order that matters:href is nullreturnsnull; a zero-lengthhrefis accepted and constructed via a new internalLinkObject.SameDocumentReference()factory; whitespace-only still returnsnull; anything else goes through the public constructor. The null check must precedehref.Length, and the length check must precede anyIsNullOrWhiteSpacecall, sinceIsNullOrWhiteSpaceis null-tolerant and.Lengthis not.Construction seam.
LinkObjectgained a private parameterless constructor assigningHref = string.Empty, exposed only throughinternal static LinkObject SameDocumentReference(). The factory takes no parameter by design: it is structurally incapable of producing a null or whitespace-only href, so the "whitespace-only is still rejected" invariant is enforced by the type system rather than by a caller-side guard. Nobool skipValidationoverload was added, which would have reopened the invariant to every internal caller. The publicLinkObject(string href)constructor and itsArgumentExceptionguard are unchanged, so the fluent builder path still rejects empty and whitespace hrefs.Sibling properties. Both accepted branches route through one shared
PopulateOptionalAttributeshelper covering all seven optional properties, so they cannot drift and an empty-href link keeps itstitle.Write path. The
if (!string.IsNullOrWhiteSpace(linkObject.Href))guard was removed sohrefis emitted unconditionally. Because the public constructor rejects null-or-whitespace,Hrefis provably either""or contains a non-whitespace character, making the old guard exactly equivalent toHref.Length != 0. Removing it changes only the""case — precisely the one now worth emitting. Without this, relaxing the read alone would have produced a silently lossy round-trip.Resulting
hreftolerance ladder"href": """href": """href": " ""href": nullor absenthrefJsonExceptionThe bare JSON string shorthand (
"self": "/path") still rejects an empty string. That asymmetry is deliberate: the shorthand is a library convenience the HAL spec does not define, so an empty shorthand is not a spec-valid Link Object, and relaxing it would additionally changeLinkConverter.Read's documented tolerance contract. It is documented with its rationale indocs/serialization.md§5.2 rather than left implicit.Versioning
Chatter.Rest.Hal2.0.0 → 2.1.0 (minor). The public API surface is strictly unchanged — every new member isprivateorinternal— and no input previously accepted is now rejected, so this is not a breaking change. But there is observable behavior change on both the read and write paths for a class of documents, which is more than a patch. Applied atomically across the csproj, theCLAUDE.mdversion table, andCHANGELOG.mdwith comparison links. No tag; CI createshal/v2.1.0after the post-merge deploy.Tests
456 passing (453 baseline + 3 added by review), plus 51 CodeGenerators tests. New coverage pins every rung of the ladder above, the lossless round-trip in both the single-object and array link shapes, sibling-attribute preservation across both the string and boolean optional-property paths, and all three bare-string shorthand sites. The pre-existing guard asserting
AddLinkObject(string.Empty)throwsArgumentExceptionstill passes, which is what proves the relaxation stayed read-path-only.Incidental fixes found during review
docs/serialization.mdclaimed every read path applied the same tolerance ladder while §5.2 of the same document said the shorthand deliberately diverges — a self-contradiction, now scoped to the object form.CHANGELOG.mdhad 2.1.0 dated 2026-08-23 above 2.0.0 dated 2026-08-24. Thehal/v2.0.0tag is dated 2026-08-23, so the pre-existing 2.0.0 date was wrong; corrected to tag ground truth.LinkConverter.Readreturnnull. A code trace shows it returns theLinkwith an emptyLinkObjectscollection. Both corrected.Linksaccess in a single asserted action, so it would have passed either way. Probing showed the throw is eager, not lazy; the assertion was narrowed and the inaccurate comment fixed.docs/architecture.mdsketch comments were audited as a set for wrong exception types.Link(string rel)claimedArgumentNullExceptionbut throwsArgumentException; the other four exception mentions were verified correct and left alone.docs/HAL_TEST_PLAN.md§3.1 cited a deleted test, and a second citation named the wrong class for a test that does exist inHalCuriesAndTemplatedTests. Both corrected rather than dropped.Known follow-up
docs/architecture.mdlinks todocs/uri-templates/architecture.md, which does not exist. Pre-existing and unrelated to this change, so deliberately left out of scope rather than widening a spec-conformance PR into doc repair.