Ask for a private block by vendor name, and let a caller create one. - #83
Merged
Merged
Conversation
A private element's tag is not fixed by the vendor's documentation. PS3.5 7.8.1
makes the high byte of the element number a block the vendor reserves at write
time, by writing its name into (gggg,00xx) -- the Private Creator. GE's documented
"element 1 of GEMS_ACQU_01" is therefore (0019,1001) in one file and (0019,2001)
in the next, depending on which block was free when each was written. Code that
hardcodes (0019,1001) reads a different vendor's data the first time it meets a
file where the blocks landed differently, and reads it successfully, because there
is nothing about the bytes to object to.
Dataset.PrivateBlock existed to remove that hardcoding and was too shallow to
build on. It is now the read half of a pair, with the create half beside it, both
in private_block.go with the allocation rules they implement:
- PrivateBlock(group uint16, creator string) (*PrivateBlock, bool) reports "no
such block" instead of returning nil. A file from another manufacturer has no
such block, which is the ordinary case rather than an exceptional one, and the
old signature made it a nil dereference in the caller's next line:
ds.PrivateBlock(0x0019, "GEMS_ACQU_01").Get(0x01) panics on every file GE did
not write.
- NewPrivateBlock(group uint16, creator string) (*PrivateBlock, error) reserves
the lowest free block when the creator has none. Without it there was no way
to write private data at all: with no creator element there is no block, and a
private element written without one is an element no reader can attribute to
anyone. It writes the creator as LO, returns an existing block untouched, and
reports an error rather than reserving for an even group, an empty creator, or
a group whose 240 blocks are all taken -- the alternative to the last being
block 0x00, which PS3.5 reserves.
- PrivateCreators(group uint16) []string lists the names that have reserved a
block, ordered by block. It is where to start with a file from a manufacturer
whose documentation you do not have: the tags say nothing, the creator names
say who to ask.
- PrivateBlock.Delete(offset uint8) completes the set. It does not release the
block; the creator element stays, so an emptied block is still that vendor's.
offset is uint8 and group is uint16 where both were int. A block offset is the low
byte of an element number and nothing else, and an int let it be anything:
GetTag(0x1234) returned (0009,2234), silently a different vendor's block, and
GetTag(-1) returned (0009,0FFF), which is not a private element at all. Both were
measured before being changed rather than assumed. pydicom raises ValueError above
0xFF at run time and does not check for negatives; here the type carries the
constraint, so the check has nowhere left to fail.
Two defects pydicom shares, each with the mutation that shows the test for it
bites:
The block was picked by map iteration order, so with one creator name reserving
two blocks in a group -- malformed, and nothing in the read path rejects it -- the
tag a private element was read from could differ between runs of one program over
one file. findPrivateCreator now scans elements 0x10 through 0xFF in order and the
lowest block wins. Restoring the range-over-d.elements scan fails
TestPrivateBlockLowestBlockWins at (0009,2001) where it wants (0009,1001).
A cached block outlived the element that reserved it. Deleting a Private Creator,
or overwriting it with another vendor's name, left the block resolving and still
answering with its old base element: reads came back from whatever now occupies
those tags, and writes produced private elements attributed to a vendor the dataset
no longer names. Set and Delete now drop the cache when the tag is a Private
Creator, which covers Pop, Clear and RemovePrivateTags too. Disabling either guard
fails three tests, one of them the element-count assertion pydicom's issue #1097
was filed on. pydicom fixed the deletion half and still caches across the
overwrite.
Clone needed nothing: cloneDataset starts from NewDataset and rebuilds, so a
clone's blocks are bound to the clone rather than writing the clone's private
elements into the original -- which is what pydicom's __deepcopy__ rebuilds every
block to avoid. TestPrivateBlockCloneIsIndependent pins it.
The nil-map guard in cachePrivateBlock is gone rather than tested. Only NewDataset
constructs a Dataset, and a zero-value one panics at Set on its nil elements map
long before reaching that guard, so it protected nothing that was not already
broken.
The API shape was confirmed before it was written, as AGENTS.md requires: the ok
idiom for the read path to match Dataset.Get, a separate error-returning create
path rather than the single (block, err) of #51's sketch, uint8 offsets, and both
PrivateCreators and Delete.
19 tests in private_block_test.go, mirroring pydicom's test_private_block,
test_add_new_private_tag, test_delete_private_tag, test_private_creators,
test_non_contiguous_private_creators, test_create_private_tag_after_removing_all,
test_create_private_tag_after_removing_private_creator and
test_private_creator_from_raw_ds. Every function in private_block.go is at 100%
statement coverage. The README has a section on it with ExampleDataset_PrivateBlock
and ExampleDataset_NewPrivateBlock behind it, so the snippets are compile-checked.
Part of #51 section 18.
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.
A private element's tag is not fixed by the vendor's documentation. PS3.5 §7.8.1 makes the high byte of the element number a block the vendor reserves at write time, by writing its name into
(gggg,00xx)— the Private Creator. GE's documented "element 1 of GEMS_ACQU_01" is therefore(0019,1001)in one file and(0019,2001)in the next, depending on which block was free when each was written. Code that hardcodes(0019,1001)reads a different vendor's data the first time it meets a file where the blocks landed differently — and reads it successfully, because there is nothing about the bytes to object to.Dataset.PrivateBlockexisted to remove that hardcoding and was too shallow to build on. It is now the read half of a pair, with the create half beside it, both inprivate_block.gowith the allocation rules they implement.API
ds.PrivateBlock(group uint16, creator string) (*PrivateBlock, bool)ds.NewPrivateBlock(group uint16, creator string) (*PrivateBlock, error)ds.PrivateCreators(group uint16) []stringblock.Delete(offset uint8)Breaking, and acceptable at 0.x:
PrivateBlockreturns(*PrivateBlock, bool)rather than a bare pointer that wasnilwhen no such creator was there. A file from another manufacturer has no such block, which is the ordinary case rather than an exceptional one, and the old signature made it a nil dereference in the caller's next line:ds.PrivateBlock(0x0019, "GEMS_ACQU_01").Get(0x01)panics on every file GE did not write.groupisuint16andoffsetisuint8where both wereint. A block offset is the low byte of an element number and nothing else, and anintlet it be anything:GetTag(0x1234)returned(0009,2234), silently a different vendor's block, andGetTag(-1)returned(0009,0FFF), which is not a private element at all. Both were measured before being changed rather than assumed. pydicom raisesValueErrorabove0xFFat run time and does not check for negatives; here the type carries the constraint, so the check has nowhere left to fail.NewPrivateBlockis what writing private data needs andPrivateBlockcould not do: with no creator element there is no block, and a private element written without one is an element no reader can attribute to anyone. It writes the creator asLO, returns an existing block untouched, and reports an error rather than reserving for an even group, an empty creator, or a group whose 240 blocks are all taken — the alternative to the last being block0x00, which PS3.5 reserves.Two defects pydicom shares
Each with the mutation that shows the test for it bites, rather than a test that merely passes:
findPrivateCreatornow scans elements0x10through0xFFin order and the lowest block wins. Restoring therange d.elementsscan failsTestPrivateBlockLowestBlockWinsat(0009,2001)where it wants(0009,1001).SetandDeletenow drop the cache when the tag is a Private Creator, which coversPop,ClearandRemovePrivateTagstoo. Disabling either guard fails three tests, one of them the element-count assertion pydicom's issue #1097 was filed on. pydicom fixed the deletion half and still caches across the overwrite.Cloneneeded nothing:cloneDatasetstarts fromNewDatasetand rebuilds, so a clone's blocks are bound to the clone rather than writing the clone's private elements into the original — which is what pydicom's__deepcopy__rebuilds every block to avoid.TestPrivateBlockCloneIsIndependentpins it.The nil-map guard in
cachePrivateBlockis gone rather than tested. OnlyNewDatasetconstructs aDataset, and a zero-value one panics atSeton its nilelementsmap long before reaching that guard, so it protected nothing that was not already broken.Verification
19 tests in
private_block_test.go, mirroring pydicom'stest_private_block,test_add_new_private_tag,test_delete_private_tag,test_private_creators,test_non_contiguous_private_creators,test_create_private_tag_after_removing_all,test_create_private_tag_after_removing_private_creatorandtest_private_creator_from_raw_ds. Every function inprivate_block.gois at 100% statement coverage.Locally, mirroring
ci.yml:go build,go vet,go test -count=1 -p 4 ./...,staticcheck -checks=all(viago run ...@latest, as CI does),gofmt -lover 133 files with LF normalisation,GOARCH=386 CGO_ENABLED=0 go testas a real 32-bit run under WoW64, andgo vetfor all eight cross-build targets plusGOARM=5.The README has a section on private elements with
ExampleDataset_PrivateBlockandExampleDataset_NewPrivateBlockbehind it, so the snippets are compile-checked.API sign-off
Confirmed with the human before it was written, as AGENTS.md requires: the ok idiom for the read path to match
Dataset.Get, a separate error-returning create path rather than the single(block, err)of #51's sketch,uint8offsets, and bothPrivateCreatorsandDelete.Part of #51 §18. Leaving #51 open — it has other sections.
🤖 Generated with Claude Code