Take the UID registry out of callers' hands, and stop building it twice. - #84
Merged
Merged
Conversation
The uid package exported three maps: Dictionary (496 registered UIDs and their PS3.6 metadata), KeywordToUID (494 keyword -> UID), and Known (an Info per registered UID, built by a package init). All three were read-only by convention and mutable by type, with no synchronisation of any kind. `uid.Known[uid.ImplicitVRLittleEndian] = someOtherInfo` was one assignment away from making every implicit-VR file in the process decode as explicit, from anywhere in any dependency, and nothing in the package could have noticed. This is the same defect the data dictionary had before #81, in the table that decides how bytes are read. Read access is now two accessors. Lookup takes a UID and returns Info, which is what the three internal call sites already wanted -- DecodeDatasetContext, determineWriteEncoding, EncodeDataset all read IsTransferSyntax, IsImplicitVR and IsLittleEndian off it -- and it returns a copy, so the table cannot be reached through the result. LookupKeyword takes a keyword and returns a UID, which is the direction the old Lookup went, under a name that says which direction that is. Dictionary, KeywordToUID and DictEntry become unexported, in the generator as well as the generated file, so a regeneration cannot quietly export them again. Known is deleted rather than unexported. It was a second copy of the dictionary: every field in an Info is either read straight from the dictionary entry or derived from the UID in three comparisons, so Lookup derives them on demand instead. That leaves one table where there were two, nothing to keep in step, and no init work for a program that never looks a UID up. The root aliases UIDDictionary and KnownUIDs are deleted too -- an alias to another package's identifier cannot be unexported, and there is nothing left for them to name. UIDInfo and LookupUID stay. Verified by mutation, each restored and re-checked: forcing IsImplicitVR false fails TestLookup and eight root tests; answering true for an unregistered UID fails TestLookup and TestDetermineEncodingPrivateTransferSyntaxRequiresArgs; dropping Keyword fails TestLookup, TestLookupCoversEveryEntry and TestLookupKeyword. The first of those found a real gap -- nothing had pinned the implicit-VR true case -- which TestLookup now covers. Full local gate green: build, vet, gofmt, staticcheck -checks=all, tests on amd64 and GOARCH=386, all eight cross-vet targets. Lookup, LookupKeyword and entry are at 100% coverage, and ExtraInfo/IsRetired go from 66.7% to 100%. Within the organisation the only affected code is two lines in one gonetdicom test, ae/storage_sop_classes_test.go:35 and :121, recorded in the CHANGELOG for whoever bumps the module there. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.
The defect
uidexported three package-level maps:Dictionary(496 registered UIDs with their PS3.6 metadata),KeywordToUID(494 keyword → UID), andKnown(oneInfoper registered UID, filled by a packageinit). Read-only by convention, mutable by type, no synchronisation of any kind.That is one assignment, from anywhere in any dependency, and every implicit-VR file in the process decodes as explicit from then on. Same defect #81 fixed for the data dictionary — this time in the table that decides how bytes are read.
The change
Approved API shape:
uid.Lookup+uid.LookupKeyword.Lookup(u UID) (Info, bool)is what replaces the maps for readers.Infois a strict superset of a dictionary entry plus the four derived transfer-syntax flags, and it is exactly what the three internal call sites already wanted —DecodeDatasetContext,determineWriteEncodingandEncodeDatasetall readIsTransferSyntax/IsImplicitVR/IsLittleEndianoff it. It returns a copy, so the table is not reachable through the result.LookupKeyword(keyword string) (UID, bool)is new, and is the oldLookup's job under a name that says which direction it goes.Dictionary,KeywordToUIDandDictEntryare unexported — ingenerate_uid_dict.pyas well as in the generated file, so a regeneration cannot quietly export them again.Two deviations from a literal "unexport everything"
Knownis deleted, not unexported. Every field of anInfois either read straight from the dictionary entry or derived from the UID in three comparisons, soLookupderives them on demand. That is one table where there were two, nothing to keep in step, and no init work — 496Infovalues built at program start — for a program that never looks a UID up.godicom.UIDDictionaryandgodicom.KnownUIDsare deleted, not unexported. An alias to another package's identifier cannot be unexported, and there is nothing left for them to name.godicom.UIDInfoandgodicom.LookupUIDare unchanged.Both are stated in the CHANGELOG.
Evidence
Three mutations, each restored and re-verified:
info.IsImplicitVR = falseTestLookup+ 8 root-package teststrueTestLookup,TestDetermineEncodingPrivateTransferSyntaxRequiresArgsKeyword: ""TestLookup,TestLookupCoversEveryEntry,TestLookupKeywordThe first found a real gap — nothing had pinned the implicit-VR true case, only the false ones — which
TestLookupnow covers.TestLookupCoversEveryEntrywalks the whole table and checks each entry resolves with every field intact, which is the invariantlen(Known) == len(Dictionary)used to state.TestLookupReturnsCopymutates the returnedInfoand checks the registry andUID.Name()are untouched, so a future memoising*Infocache cannot regress the point of the change.Local gate green: build,
go vet, gofmt,staticcheck -checks=all,go test ./...on amd64 and underGOARCH=386 CGO_ENABLED=0, all eight cross-vet targets.Lookup,LookupKeywordandentryat 100%;ExtraInfo/IsRetiredlifted 66.7% → 100%.Downstream
Breaking, which 0.x allows. Within the organisation the only affected code is two lines in one gonetdicom test —
ae/storage_sop_classes_test.go:35and:121— recorded in the CHANGELOG for whoever bumps the module there. No non-test code anywhere in the org touched the maps.🤖 Generated with Claude Code