OMEXMLMetadata: Correct indexed access to annotation references

[rleigh-codelibre]

This was a long-standing defect in the code generator which resulted in an unused parameter warning for each annotation ref function. This change makes appending and updating by index work (in the same way it works for all other object types).

• MetadataConverter always calls setters sequentially (0, 1, 2, ...)
• The old behaviour ignored the index - it was just appending, so the sequential calls happened to work, but the behaviour was divergent compared with all other object types
• The new behaviour uses the index correctly - when called sequentially, it produces identical results
• The new behaviour resolves references immediately when possible instead of deferring everything
• The existing behaviour is preserved, but it is now also possible to update references

I thought there was a ticket for this, since we were aware of this for many years but didn't know where or how to fix it at the time, but it's not an open issue so might be on an older tracker?

This should be tested by making sure that annotation references continue to be correctly preserved after passing through the MetadataConverter.

[melissalinkert]

Thanks, @rleigh-codelibre. Generally in support of adding this feature; I believe https://trello.com/c/uxA9hTub/484-model-defect-model-references-not-updated was the previous discussion on this topic.

It looks like the placeholder linkage needs a different approach though, as adding a linkage to a null object doesn't seem to be possible in all cases. Was there a specific test case you were using where linking to a null object worked as expected?

[melissalinkert]

Using a simple Image/ROI reference test:

$ cat ReferenceTest.java 
import ome.xml.meta.MetadataConverter;
import ome.xml.meta.OMEXMLMetadataImpl;

public class ReferenceTest {
  public static void main(String[] args) throws Exception {
    OMEXMLMetadataImpl meta = new OMEXMLMetadataImpl();
    meta.setImageID("Image:0", 0);
    meta.setROIID("ROI:0", 0);
    meta.setROIID("ROI:1", 1);
    meta.setImageROIRef("ROI:0", 0, 0);
    meta.setImageROIRef("ROI:1", 0, 1);

    System.out.println("ROI references: " + meta.getImageROIRefCount(0));

    OMEXMLMetadataImpl out = new OMEXMLMetadataImpl();
    MetadataConverter.convertMetadata(meta, out);

    System.out.println("ROI references after conversion: " + out.getImageROIRefCount(0));
  }
}

I see this exception:

Exception in thread "main" java.lang.NullPointerException
	at ome.xml.model.Image.linkROI(Image.java:620)
	at ome.xml.meta.OMEXMLMetadataImpl.setImageROIRef(OMEXMLMetadataImpl.java:8176)
	at ReferenceTest.main(ReferenceTest.java:10)

[rleigh-codelibre]

I've pushed an additional commit which should address this case. Extending the list and modifying the list at a specified index should now all work correctly.

[melissalinkert]

Thanks, that looks much better. Would you expect something like this to work with these changes?

OMEXMLMetadataImpl meta = new OMEXMLMetadataImpl();
meta.setImageID("Image:0", 0);
meta.setROIID("ROI:0", 0);
meta.setROIID("ROI:1", 1);
meta.setImageROIRef("ROI:1", 0, 1);
meta.setImageROIRef("ROI:0", 0, 0);

I think it's fine if that doesn't work (since that wouldn't work without these changes either), but just want to make sure I understand the intent for this case.

[rleigh-codelibre]

Hi Melissa, I've pushed an additional commit which adds a number of unit tests to exercise appending and replacing operations. As for all MetadataStore interfaces, deletion isn't supported well without disturbing the index stability. The aim here was primarily to close the gap in the implementation which was triggering a lot of static analysis warnings about unused parameters (which were completely legitimate), and also to bring these functions to equivalence with the rest of the OMEXMLMetadataStore functions. I think it's met those aims, but there may be additional flaws in the implementation which are not fully addressed with this change.

[melissalinkert]

Thanks, this looks good to me now, and the addition of unit tests is definitely helpful.

Unless any objection from @sbesson, propose to merge this before winter break and then make a release of this component (6.6.0?) in the new year.

[sbesson]

The plan reads great and I can imagine this either released as 6.6.0 or as 6.5.2. We can make that decision early 2026 as you suggest. Thanks @rleigh-codelibre for the contribution and hope everything is going well on your side.

@melissalinkert is there anything from #227 that should be preserved and added to the unit tests introduced by this PR as a follow-up?

[melissalinkert]

@melissalinkert is there anything from #227 that should be preserved and added to the unit tests introduced by this PR as a follow-up?

I've closed #227, as the last commit here adds more tests than were initially proposed there. #228 is opened as a reminder that we may wish to add more tests along this line in the future (outside the scope of this pull request).