-
Notifications
You must be signed in to change notification settings - Fork 28
OMEXMLMetadata: prevent IndexOutOfBoundsException when setting references #229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OMEXMLMetadata: prevent IndexOutOfBoundsException when setting references #229
Conversation
|
While investigating further, I identified the cause for one of the failures in the Bio-Formats repository tests with The changes in #226 implemented a behavior similar to other setters which append an ROI link if c9966ef is one way to restore some form of backwards compatibility by deferring the resolution of references if A few additional/alternate thoughts:
|
Define a FolderFolderRef before the target Folder is declared and confirm that the reference addition is deferred
|
With some additional debugging, the sequence of calls leading to the dest.setFolderID(Folder:13,0)
dest.setFolderID(Folder:30,1)
dest.setFolderID(Folder:16,2)
dest.setFolderID(Folder:31,3)
dest.setFolderID(Folder:6,4)
dest.setFolderID(Folder:4,5)
dest.setFolderID(Folder:17,6)
dest.setFolderID(Folder:22,7)
dest.setFolderID(Folder:29,8)
dest.setFolderID(Folder:25,9)
dest.setFolderID(Folder:15,10)
dest.setFolderID(Folder:3,11)
dest.setFolderID(Folder:1,12)
dest.setFolderFolderRef(Folder:2,12,0)
dest.setFolderFolderRef(Folder:30,12,1)Here, the exception is caused by the fact that Moving this PR out of draft so that it is included in the nightly builds before assigning reviewers. |
|
See https://bf-testing-results.s3.amazonaws.com/2026/2026-01-21/build-status.log for the result of the nightly repository tests with this PR included |
| assertEquals(3, meta.getImageROIRefCount(0)); | ||
| assertEquals("ROI:0", meta.getImageROIRef(0, 0)); | ||
| assertEquals("ROI:2", meta.getImageROIRef(0, 1)); | ||
| assertEquals("ROI:1", meta.getImageROIRef(0, 2)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand this ordering is consistent with the updated set*Ref implementation, but it isn't the behavior I'd initially expect without actually reading the implementation. I would have expected either ROI:0, ROI:1, ROI:2 or ROI:2, ROI:1, ROI:0 ordering initially.
I can't think of a case where the reference order actually ends up mattering, though, so maybe it's enough to add some documentation that indicates the index will be effectively ignored if it is greater than the number of existing references.
One other test that might be worthwhile as well is something like:
setImageROIRef("ROI:1", 0, 1)setImageROIRef("ROI:0", 0, 0)setImageROIRef("ROI:2", 0, 1)
If I read correctly, the reference order would end up being ROI:0, ROI:2, ROI:1, but it would be good to verify.
Fixes the issue detected in ome/bioformats#4394 (comment) as a follow-up of #226
As shown in the failing tests https://github.com/ome/ome-model/blob/master/specification/samples/2016-06/folders-larger-taxonomy.ome.xml is a good way to reproduce the
IndexOutOfBoundsExceptionusing Bio-Formatsshowinf -ome-xmlandorg.openmicroscopy:ome-xml 6.5.2.Taking
setImageROIRef(roiReference, imageIndex, roiRefIndex)as an example, the current logic following the changes in #226 is the following,roiReferencedoes not exist, the addition of the ROI reference for imageimageIndexis deferred underresolveReferencesis calledroiReferenceexists androiRefIndex == getRoiRefCount, the ROI reference is appended to the list of references for imageimageIndexroiReferenceexists androiRefIndex < getRoiRefCount, the ROI reference for imageimageIndexatroiRefIndexis replaced byroiReferenceThis Pull Requests adds a fourth additional conditional behavior allowing to restore backwards-compatibility
roiReferenceexists androiRefIndex > getRoiRefCount, the addition of the ROI reference for imageimageIndexis deferred underresolveReferencesis calledUnit tests are also added to cover the different failing scenarios found in our repository tests i.e.:
roiRefIndexstarting with a non-zero offset