Scene load broken due to new volume unit attribute

Development
1

py_SubjectHierarchyGenericSelfTest started failing due to a failure of loading a scene. Digging into it, this error message (Error parsing XML in stream at line 38, column 151, byte index 10406: not well-formed (invalid token)) points to this line:

 <Volume
  id="vtkMRMLScalarVolumeNode1" name="303: Unnamed Series" hideFromEditors="false" selectable="true" selected="false" attributes="DICOM.QuantityCode:{"CodeMeaning": "Attenuation Coefficient", "CodingSchemeDesignator": "DCM", "CodeValue": "110852"};DICOM.UnitsCode:{"CodeMeaning": "Hounsfield unit", "CodingSchemeDesignator": "UCUM", "CodeValue": "[hnsf'U]"};DICOM.instanceUIDs:1.2.826.0.1.3680043.8.274.1.1.2895132669.2819.7830930470.104 1.2.826.0.1.3680043.8.274.1.1.7405746770.5479.5810198465.165 1.2.826.0.1.3680043.8.274.1.1.3116843726.8233.3216805822.278 1.2.826.0.1.3680043.8.274.1.1.3263562271.1129.3152372828.828 1.2.826.0.1.3680043.8.274.1.1.4153098248.6996.3632241357.584 1.2.826.0.1.3680043.8.274.1.1.4770490127.9140.3834798946.219 1.2.826.0.1.3680043.8.274.1.1.6553947155.9980.4127073411.708 1.2.826.0.1.3680043.8.274.1.1.4554129680.8666.4619454551.121 1.2.826.0.1.3680043.8.274.1.1.5613951088.5396.9288000555.812 1.2.826.0.1.3680043.8.274.1.1.5710109910.8056.9834733307.864" displayNodeRef="vtkMRMLScalarVolumeDisplayNode1" references="display:vtkMRMLScalarVolumeDisplayNode1;" userTags="" ijkToRASDirections="-1   0   0 0   -1   0 0 0 1 " spacing="49 49 23" origin="248.844 248.289 -123.75" ></Volume>

Specifically the quotation mark before “CodeMeaning”. As this attribute is added when loading a volume from DICOM, I suspect that now all saved scenes are invalid that contain a DICOM volume. It would be good either to revert this commit
https://github.com/Slicer/Slicer/commit/9df49830897e2d0d6e458e7390eec7807a817b41
while fixing this, or fixing this quickly.

2

I’ll take care of this by this information into a member variable. We could encode the attribute to be able to store special characters, such as ", but we’ll use the unit for many things, so it’s better not to store it in just a generic metadata.

Scene corruption is a serious error that our development process should have captured. The pull request was open for a few days but just by looking at the code nobody noticed the error. SH test did capture the error (see failed test this morning, it even showed the relevant error message on the dashboard “Error parsing XML in stream at line 38, column 151, byte index 10404: not well-formed (invalid token)”) and the dashboard is normally green (we don’t have any failing test), so it should not be too difficult to spot new errors.
@jcfr do you know why the tests were not executed automatically by the pull request (https://github.com/Slicer/Slicer/pull/750)?
@Fedorov do you remember if you’ve run the automatic tests manually and if you saw the new test failure?

3

Sorry for the trouble guys. I admit I did not run the tests manually, so it is my fault!

@lassoan will you implement the solution you mentioned, or should I revert the commit?

4

Definitively, storing a complex document (e.g json) as a MRML attribute is a stretch.

Tests are not yet run automatically, we need to finalize integration of the work of Mayeul. For now, it is only the build.

As soon as this is enabled, we will capture such regression beforehand.

5

I revert the commit now and implement member variable storage within a few hours.

2 Likes
6

Thanks @lassoan for tackling this.

On my side, I will look into adding the testing to the PR later this week

7

Reverted in rev r26176.

8

Thanks guys!

Do you think it would be worth sanitizing an attribute when setting (and restoring it when getting)? Users might add attributes with quotation marks and they might wonder what broke. Not sure if simply escaping the quotation mark would solve this, but if it does, then it’s a very simple solution.

9

Yes, currently special characters in any node properties (name, description, attributes, …) can lead to invalid scenes - see https://issues.slicer.org/view.php?id=3406. There is a function in MRML node already for encoding/decoding, we could probably use that in read/write XML of MRML node to fix this.

1 Like