TypeError: Cannot redefine property: Alphabetic

[sedghi]

I'm getting the following error on latest dcmjs version, any idea? ~~I can confirm dcmjs 0.18.6 does not produce this error~~

dcmjs.es.js?3946:2743 Uncaught (in promise) TypeError: Cannot redefine property: Alphabetic
    at Function.defineProperty (<anonymous>)
    at eval (dcmjs.es.js?3946:2743)
    at Array.forEach (<anonymous>)
    at addAccessors (dcmjs.es.js?3946:2742)
    at eval (dcmjs.es.js?3946:2913)
    at Array.forEach (<anonymous>)
    at Function.naturalizeDataset (dcmjs.es.js?3946:2869)
    at Array.map (<anonymous>)

[pieper]

Thanks for the report 👍

Could you provide a PR with a test that replicates this issue?

@wayfarer3130 can you have a look?

[wayfarer3130]

Will look as soon as the test is available.

[sedghi]

Sorry It might take me a while to create a test for this issue as the repo I'm working on is private.

[sedghi]

@pieper @wayfarer3130 we debugged this today with James and Erik, and here are the results:

Apparently you cannot naturalizeDataset twice, so following code will through an error. (We were naturalizing the dataset in the internals of one of our providers)

const instance = DicomMetaDictionary.naturalizeDataset(instanceMetaData);
const another = DicomMetaDictionary.naturalizeDataset(instanceMetaData);

but the following code works

const instance = DicomMetaDictionary.naturalizeDataset(
   JSON.parse(JSON.stringify(instanceMetaData))
);
const another = DicomMetaDictionary.naturalizeDataset(
  JSON.parse(JSON.stringify(instanceMetaData))
);

Is there any where in the naturalizeDataset that it is mutating the metadata by reference?

I can confirm this issue does not exist in 0.18.1 (above i was saying 0.18.6, but right now 0.18.6 gives the same error)

[pieper]

That sounds like a regression, probably introduced by this commit or similar.

Calling naturalizeDataset should be idempotent in my opinion, especially if it was that way before.

It would be good to have a test of that along with a fix of course. @wayfarer3130 are you able to work on that?

[wayfarer3130]

Will try to get a PR for this by the end of the day - I have a fix for it already, just need to add a unit test. I might do an alternate fix for it that avoids re-using the contained objects as one can get odd side affects in other cases.

Bill

On Thu, 27 Jan 2022 at 13:38, Steve Pieper @.***> wrote:

That sounds like a regression, probably introduced by this commit https://github.com/dcmjs-org/dcmjs/commit/70b24332783d63c9db2ed21d512d9f7b526c5222 or similar.

Calling naturalizeDataset should be idempotent in my opinion, especially if it was that way before.

It would be good to have a test of that along with a fix of course. @wayfarer3130 https://github.com/wayfarer3130 are you able to work on that?

— Reply to this email directly, view it on GitHub https://github.com/dcmjs-org/dcmjs/issues/231#issuecomment-1023528570, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGT56XMYPVJCLPGU6QWGNALUYGGLVANCNFSM5H33TO3Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

[wayfarer3130]

I have a PR for this right now, but the issue exposes the fact that the normalized data refers to the original data, so modifying it will modify the original/underlying data.