Skip to content

Commit

Permalink
fix(lossles-round-trip): Account for padding byte in numeric strings …
Browse files Browse the repository at this point in the history 
…with multiplicity (#401)

* Trim padding byte from numeric strings if they exceed VR max length

* Tweak comment

* Generalize pad byte trimming and add unit test

* Refactor with better jsdoc and move function within ValueRepresentation.js class

* Additional test cases, account for maxCharLength

* Trip padding byte from last element in multiple value arrays if it is odd length

---------

Co-authored-by: Craig Berry <craig.berry@veeva.com>
  • Loading branch information
@craigberry1
craigberry1 and Craig Berry authored Sep 4, 2024
1 parent faa0f86 commit 7b44953
Show file tree
Hide file tree
Showing 4 changed files with 256 additions and 35 deletions.
5 changes: 3 additions & 2 deletions src/DicomMessage.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -376,8 +376,9 @@ class DicomMessage {
rawValues = rawValue;
values = value
if (typeof value === "string") {
rawValues = rawValue.split(String.fromCharCode(VM_DELIMITER));
values = value.split(String.fromCharCode(VM_DELIMITER));
const delimiterChar = String.fromCharCode(VM_DELIMITER);
rawValues = vr.dropPadByte(rawValue.split(delimiterChar))
values = vr.dropPadByte(value.split(delimiterChar));
}
} else if (vr.type == "SQ") {
rawValues = rawValue;
Expand Down
80 changes: 52 additions & 28 deletions src/ValueRepresentation.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,41 @@ class ValueRepresentation {
return tag;
}

/**
* Removes padding byte, if it exists, from the last value in a multiple-value data element.
*
* This function ensures that data elements with multiple values maintain their integrity for lossless
* read/write operations. In cases where the last value of a multi-valued data element is at the maximum allowed length,
* an odd-length total can result in a padding byte being added. This padding byte, can cause a length violation
* when writing back to the file. To prevent this, we remove the padding byte if it is the only additional character
* in the last element. Otherwise, it leaves the values as-is to minimize changes to the original data.
*
* @param {string[]} values - An array of strings representing the values of a DICOM data element.
* @returns {string[]} The modified array, with the padding byte potentially removed from the last value.
*/
dropPadByte(values) {
const maxLength = this.maxLength ?? this.maxCharLength;
if (!Array.isArray(values) || !maxLength || !this.padByte) {
return values;
}

// Only consider multiple-value data elements, as max length issues arise from a delimiter
// making the total length odd and necessitating a padding byte.
if (values.length > 1) {
const padChar = String.fromCharCode(this.padByte);
const lastIdx = values.length - 1;
const lastValue = values[lastIdx];

// If the last element is odd and ends with the padding byte trim to avoid potential max length violations during write
if (lastValue.length % 2 !== 0 && lastValue.endsWith(padChar)) {
values[lastIdx] = lastValue.substring(0, lastValue.length - 1); // Trim the padding byte
}
}

return values;
}


read(stream, length, syntax, readOptions = { forceStoreRaw: false }) {
if (this.fixed && this.maxLength) {
if (!length) return this.defaultValue;
Expand Down Expand Up @@ -604,7 +639,7 @@ class CodeString extends AsciiStringRepresentation {

readBytes(stream, length) {
const BACKSLASH = String.fromCharCode(VM_DELIMITER);
return stream.readAsciiString(length).split(BACKSLASH);
return this.dropPadByte(stream.readAsciiString(length).split(BACKSLASH));
}

applyFormatting(value) {
Expand Down Expand Up @@ -654,29 +689,28 @@ class AttributeTag extends ValueRepresentation {
class DateValue extends AsciiStringRepresentation {
constructor(value) {
super("DA", value);
this.maxLength = 18;
this.maxLength = 8;
this.padByte = PADDING_SPACE;
//this.fixed = true;
this.defaultValue = "";
}
}

class DecimalString extends AsciiStringRepresentation {
constructor() {
super("DS");
this.maxLength = 16;
this.padByte = PADDING_SPACE;
}
class NumericStringRepresentation extends AsciiStringRepresentation {

readBytes(stream, length) {
const BACKSLASH = String.fromCharCode(VM_DELIMITER);
const ds = stream.readAsciiString(length);
if (ds.indexOf(BACKSLASH) !== -1) {
// handle decimal string with multiplicity
return ds.split(BACKSLASH);
}
const numStr = stream.readAsciiString(length);

return [ds];
return this.dropPadByte(numStr.split(BACKSLASH));
}
}

class DecimalString extends NumericStringRepresentation {
constructor() {
super("DS");
this.maxLength = 16;
this.padByte = PADDING_SPACE;
}

applyFormatting(value) {
Expand All @@ -694,6 +728,7 @@ class DecimalString extends AsciiStringRepresentation {

convertToString(value) {
if (value === null) return "";
if (typeof value === 'string') return value;

let str = String(value);
if (str.length > this.maxLength) {
Expand Down Expand Up @@ -798,25 +833,13 @@ class FloatingPointDouble extends ValueRepresentation {
}
}

class IntegerString extends AsciiStringRepresentation {
class IntegerString extends NumericStringRepresentation {
constructor() {
super("IS");
this.maxLength = 12;
this.padByte = PADDING_SPACE;
}

readBytes(stream, length) {
const BACKSLASH = String.fromCharCode(VM_DELIMITER);
const is = stream.readAsciiString(length);

if (is.indexOf(BACKSLASH) !== -1) {
// handle integer string with multiplicity
return is.split(BACKSLASH);
}

return [is];
}

applyFormatting(value) {
const formatNumber = (numberStr) => {
let returnVal = numberStr.trim().replace(/[^0-9.\\\-+e]/gi, "");
Expand All @@ -831,6 +854,7 @@ class IntegerString extends AsciiStringRepresentation {
}

convertToString(value) {
if (typeof value === 'string') return value;
return value === null ? "" : String(value);
}

Expand Down Expand Up @@ -1299,7 +1323,7 @@ class UniqueIdentifier extends AsciiStringRepresentation {
if (result.indexOf(BACKSLASH) === -1) {
return result
} else {
return result.split(BACKSLASH)
return this.dropPadByte(result.split(BACKSLASH));
}
}

Expand Down
2 changes: 1 addition & 1 deletion test/data.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1158,7 +1158,7 @@ describe("test_un_vr", () => {
'00041130',
'CS',
new Uint8Array([0x4F, 0x52, 0x49, 0x47, 0x49, 0x4E, 0x41, 0x4C, 0x20, 0x20, 0x5C, 0x20, 0x50, 0x52, 0x49, 0x4D, 0x41, 0x52, 0x59, 0x20]).buffer,
["ORIGINAL ", " PRIMARY "],
["ORIGINAL ", " PRIMARY"],
["ORIGINAL", "PRIMARY"]
],
[
Expand Down
204 changes: 200 additions & 4 deletions test/lossless-read-write.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@ import "regenerator-runtime/runtime.js";

import fs from "fs";
import dcmjs from "../src/index.js";
import {deepEqual} from "../src/utilities/deepEqual";
import { deepEqual } from "../src/utilities/deepEqual";

import {getTestDataset} from "./testUtils";
import { getTestDataset } from "./testUtils";

const {DicomDict, DicomMessage} = dcmjs.data;
const { DicomDict, DicomMessage } = dcmjs.data;


describe('lossless-read-write', () => {
Expand Down Expand Up @@ -128,6 +128,202 @@ describe('lossless-read-write', () => {
expect(outputDicomDict.dict['00181041'].Value).toEqual([9007199254740992])
});

test('test DS with multiplicity > 1 and added space for even padding is read and written correctly', () => {
const dataset = {
'00200037': {
vr: 'DS',
Value: [0.99924236548978, -0.0322633220972, -0.0217663285287, 0.02949870928067, 0.99267261121054, -0.1171789789306]
}
};

const dicomDict = new DicomDict({});
dicomDict.dict = dataset;

// write and re-read
const outputDicomDict = DicomMessage.readFile(dicomDict.write());

// ensure _rawValue strings have no added trailing spaces
const expectedDataset = {
'00200037': {
vr: 'DS',
Value: [0.99924236548978, -0.0322633220972, -0.0217663285287, 0.02949870928067, 0.99267261121054, -0.1171789789306],
_rawValue: ["0.99924236548978", "-0.0322633220972", "-0.0217663285287", "0.02949870928067", "0.99267261121054", "-0.1171789789306"]
}
};

expect(deepEqual(expectedDataset, outputDicomDict.dict)).toBeTruthy();

// re-write should succeeed
const outputDicomDictPass2 = DicomMessage.readFile(outputDicomDict.write());

// dataset should still be equal
expect(deepEqual(expectedDataset, outputDicomDictPass2.dict)).toBeTruthy();
});

test('test DS with multiplicity > 1 with padding byte on last element within VR max length is losslessly read', () => {
const dataset = {
'00200037': {
vr: 'DS',
Value: [0.99924236548978, -0.0322633220972, -0.0217663285287, 0],
_rawValue: ["0.99924236548978", "-0.0322633220972", "-0.0217663285287", " +0.00 "]
}
};

const dicomDict = new DicomDict({});
dicomDict.dict = dataset;

// write and re-read
const outputDicomDict = DicomMessage.readFile(dicomDict.write());

// ensure _rawValue strings have no added trailing spaces and retain original encoding details for + and spaces
const expectedDataset = {
'00200037': {
vr: 'DS',
Value: [0.99924236548978, -0.0322633220972, -0.0217663285287, 0],
_rawValue: ["0.99924236548978", "-0.0322633220972", "-0.0217663285287", " +0.00"]
}
};

expect(outputDicomDict.dict).toEqual(expectedDataset);

// re-write should succeeed
const outputDicomDictPass2 = DicomMessage.readFile(outputDicomDict.write());

// dataset should still be equal
expect(outputDicomDictPass2.dict).toEqual(expectedDataset);
});

test('test IS with multiplicity > 1 and added space for even padding is read and written correctly', () => {
const dataset = {
'00081160': {
vr: 'IS',
Value: [1234, 5678]
}
};

const dicomDict = new DicomDict({});
dicomDict.dict = dataset;

// write and re-read
const outputDicomDict = DicomMessage.readFile(dicomDict.write());

// last _rawValue strings does allow trailing space as it does not exceed max length
const expectedDataset = {
'00081160': {
vr: 'IS',
Value: [1234, 5678],
_rawValue: ["1234", "5678"]
}
};

expect(outputDicomDict.dict).toEqual(expectedDataset);

// re-write should succeeed
const outputDicomDictPass2 = DicomMessage.readFile(outputDicomDict.write());

// dataset should still be equal
expect(outputDicomDictPass2.dict).toEqual(expectedDataset);
});

describe('Multiplicity for non-binary String VRs', () => {
const maxLengthCases = [
{
vr: 'AE',
Value: ["MAX_LENGTH_CHARS", "MAX_LENGTH_CHARS"],
_rawValue: ["MAX_LENGTH_CHARS", "MAX_LENGTH_CHARS"]
},
{
vr: 'AS',
Value: ["120D", "045Y"],
_rawValue: ["120D", "045Y"]
},
{
vr: 'AT',
Value: [0x00207E14, 0x0012839A],
_rawValue: [0x00207E14, 0x0012839A],
},
{
vr: 'CS',
Value: ["MAX_LENGTH_CHARS", "MAX_LENGTH_CHARS"],
_rawValue: ["MAX_LENGTH_CHARS", "MAX_LENGTH_CHARS"]
},
{
vr: 'DA',
Value: ["20230826", "20230826"],
_rawValue: ["20230826", "20230826"]
},
{
vr: 'DS',
Value: [123456789012.345, 123456789012.345],
_rawValue: ["123456789012.345", "123456789012.345"]
},
{
vr: 'DT',
Value: ["20230826123045.123456+0100", "20230826123045.123456+0100"],
_rawValue: ["20230826123045.123456+0100", "20230826123045.123456+0100"]
},
{
vr: 'IS',
Value: [123456789012, 123456789012],
_rawValue: ["123456789012", "123456789012"]
},
{
vr: 'LO',
Value: ["ABCDEFGHIJKLMNOPQRSTUVWXABCDEFGHIJKLMNOPQRSTUVWXABCDEFGHIJKLMNOP", "ABCDEFGHIJKLMNOPQRSTUVWXABCDEFGHIJKLMNOPQRSTUVWXABCDEFGHIJKLMNOP"],
_rawValue: ["ABCDEFGHIJKLMNOPQRSTUVWXABCDEFGHIJKLMNOPQRSTUVWXABCDEFGHIJKLMNOP", "ABCDEFGHIJKLMNOPQRSTUVWXABCDEFGHIJKLMNOPQRSTUVWXABCDEFGHIJKLMNOP"]
},
{
vr: 'SH',
Value: ["ABCDEFGHIJKLMNOP", "ABCDEFGHIJKLMNOP"],
_rawValue: ["ABCDEFGHIJKLMNOP", "ABCDEFGHIJKLMNOP"]
},
{
vr: 'UI',
Value: ["1.2.840.12345678901234567890123456789012345678901234567890123456", "1.2.840.12345678901234567890123456789012345678901234567890123456"],
_rawValue: ["1.2.840.12345678901234567890123456789012345678901234567890123456", "1.2.840.12345678901234567890123456789012345678901234567890123456"]
},
{
vr: 'TM',
Value: ["142530.1234567", "142530.1234567"],
_rawValue: ["142530.1234567", "142530.1234567"],
},

];

test.each(maxLengthCases)(
`Test multiple values with VR max length handle pad byte correctly during read and write - $vr`,
(dataElement) => {
const dataset = {
'00081160': {
vr: dataElement.vr,
Value: dataElement.Value,
}
};

const dicomDict = new DicomDict({});
dicomDict.dict = dataset;

// write and re-read
const outputDicomDict = DicomMessage.readFile(dicomDict.write());

// expect full _rawValue to match following read
const expectedDataset = {
'00081160': {
...dataElement,
}
};

expect(outputDicomDict.dict).toEqual(expectedDataset);

// re-write should succeed without max length issues
const outputDicomDictPass2 = DicomMessage.readFile(outputDicomDict.write());

// dataset should still be equal
expect(expectedDataset).toEqual(outputDicomDictPass2.dict);
}
);
})

describe('Individual VR comparisons', () => {

const unchangedTestCases = [
Expand All @@ -148,7 +344,7 @@ describe('lossless-read-write', () => {
},
{
vr: "CS",
_rawValue: ["ORIGINAL ", " PRIMARY "], // spaces non-significant for interpretation but allowed
_rawValue: ["ORIGINAL ", " PRIMARY"], // spaces non-significant for interpretation but allowed
Value: ["ORIGINAL", "PRIMARY"],
},
{
Expand Down

0 comments on commit 7b44953

Please sign in to comment.