Refactors #555 to allow for backward compatibility

[chris-allan]

There were certain downstream omero-web plugins which were using _marshal_image(). This refactors the changes of #555 such that the prototype of _marshal_image() is maintained and a new _marshal_image_map() is added to give us options going forward for scalability.

Alternative to #575.

/cc @Tom-TBT

[chris-allan]

I've opened this as a draft so we don't end up with build conflicts with #575 and can use this for discussion.

[sbesson]

A couple of comments. From an API construction, I think this proposal gets us into a good place:

• a new _marshal_image_map is introduced that takes a dictionary rather than an array of fixed length. This means new properties can be introduced in a backwards-compatible way in the future
• the signature of the existing _marshal_image is restored and its implementation is forwarded to the new _marshal_image_map. This should fix the behavior of existing downstream apps that were using this internal method
• I assume the high-level marshal_images and marshal_tagged API should use the _marshal_image_map API moving forward?

In terms of testing, I expect the changes introduced in ome/openmicroscopy#6389 should be unchanged as the omeroweb.webclient.tree.marshal_images API should create a dictionary including the archived key.
Should the legacy _marshal_image API be tested and/or deprecated?

[sbesson]

With OMERO.web 5.27.0 and OMERO.autotag 4.0.1, I can reproduce the original issue as toggling the Auto-tag from the centre panel triggers a 500 error.

I deployed a local build of this PR with the additional change proposed in #577 (comment), I can confirm that the auto-tag server issue is fixed. Setting the archived flag to true via the CLI also produces the expected behavior as per #555 i.e. an archived icon and Archived: True in the right-hand panel.

I would be in favor of closing #575 and moving this out of draft for inclusion in the nightly CI builds in preparation of a round of functional testing.

[knabar]

Do we also want to address the issue of the "underscore" method that indicates it is private, since we know it is not used as such?

For example, remove the leading underscore from the methods, but keep the underscore reference for compatibility:

def marshal_image(...):
    ...

_marshal_image = marshal_image

def marshal_image_map(...):
    ...

This is admittedly a bit redundant, but might deter future signature changes.

[chris-allan]

Should the legacy _marshal_image API be tested and/or deprecated?

I think at least tested. Once the current integration tests are passing I'll add some more unit tests to this project on this PR.

[chris-allan]

Do we also want to address the issue of the "underscore" method that indicates it is private, since we know it is not used as such?

For example, remove the leading underscore from the methods, but keep the underscore reference for compatibility:

def marshal_image(...):
    ...

_marshal_image = marshal_image

def marshal_image_map(...):
    ...

This is admittedly a bit redundant, but might deter future signature changes.

I don't think so. Or at least not here. What was clear from #575, especially ome/omero-mapr#84, is that almost every leading underscore marshal method from the tree package is used in that plugin and has been for nearly a decade. The whole module needs review and an assessment of what we we want to do with it as there are clearly critical plugins that are tightly coupled to nearly its entire API.

[will-moore]

Changes look good and make sense, preserving previous API. Auto-tag working locally and the archived status is shown in webclient correctly.

[chris-allan]

Unit tests have been added to cover both _marshal_image() and _marshal_image_map().

[sbesson]

Retested the last commit in a deployment with the current versions of OMERO.autotag and OMERO.mapr. Confirmed that the archived status is still displayed as expected in the webclient (left and right hand panels) and both applications work as expected.

The nightly CI integration tests have been passing and the new collection of unit tests add coverage for all supported arguments of the internal _marshal_image and _marshal_image_map APIs.

Proposing to get this released asap to reduce the impact on people who upgraded their version of OMERO.web in a deployment with any of the impacted applications. Given these APIs are marked as internal, either 5.27.1 or 5.28.0 would be acceptable version increment from my side