From 706fabaf61f7d1d85ad41c30c2da6e77d10693ac Mon Sep 17 00:00:00 2001 From: Chris Allan Date: Tue, 27 Aug 2024 22:00:11 +0000 Subject: [PATCH 1/5] Refactors #555 to allow for backward compatibility 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. --- omeroweb/webclient/tree.py | 121 +++++++++++++++++++++++-------------- 1 file changed, 75 insertions(+), 46 deletions(-) diff --git a/omeroweb/webclient/tree.py b/omeroweb/webclient/tree.py index 0faa9588ca..f2559c4ca7 100644 --- a/omeroweb/webclient/tree.py +++ b/omeroweb/webclient/tree.py @@ -531,7 +531,6 @@ def _marshal_image( """Given an Image row (list) marshals it into a dictionary. Order and type of columns in row is: * id (rlong) - * archived (boolean) * name (rstring) * details.owner.id (rlong) * details.permissions (dict) @@ -550,22 +549,74 @@ def _marshal_image( @param row_pixels The Image row pixels data to marshal @type row_pixels L{list} """ - image_id, archived, name, owner_id, permissions, fileset_id = row + image_id, name, owner_id, permissions, fileset_id = row + data = { + "id": image_id, + "name": name, + "ownerId": owner_id, + "image_details_permissions": permissions, + "filesetId": fileset_id, + } + if row_pixels: + sizeX, sizeY, sizeZ, sizeT = row_pixels + data["sizeX"] = sizeX + data["sizeY"] = sizeY + data["sizeZ"] = sizeZ + data["sizeT"] = sizeT + return _marshal_image_map( + conn, + data, + share_id=share_id, + date=date, + acqDate=acqDate, + thumbVersion=thumbVersion, + ) + + +def _marshal_image_map( + conn, + data, + share_id=None, + date=None, + acqDate=None, + thumbVersion=None, +): + """Given an Image data dictionary marshals it into a dictionary. Suppored keys are: + * id (rlong) + * archived (boolean; optional) + * name (rstring) + * ownerId (rlong) + * image_details_permissions (dict) + * filesetId (rlong) + * sizeX (rlong; optional) + * sizeY (rlong; optional) + * sizeZ (rlong; optional) + * sizeT (rlong; optional) + + @param conn OMERO gateway. + @type conn L{omero.gateway.BlitzGateway} + @param data The data to marshal + @type row L{dict} + """ image = dict() - image["id"] = unwrap(image_id) - image["archived"] = unwrap(archived) is True - image["name"] = unwrap_to_str(name) - image["ownerId"] = unwrap(owner_id) - image["permsCss"] = parse_permissions_css(permissions, unwrap(owner_id), conn) - fileset_id_val = unwrap(fileset_id) + image["id"] = unwrap(data["id"]) + image["archived"] = unwrap(data.get("archived")) is True + image["name"] = unwrap_to_str(data["name"]) + image["ownerId"] = unwrap(data["ownerId"]) + image["permsCss"] = parse_permissions_css( + data["image_details_permissions"], unwrap(data["ownerId"]), conn + ) + fileset_id_val = unwrap(data["filesetId"]) if fileset_id_val is not None: image["filesetId"] = fileset_id_val - if row_pixels: - sizeX, sizeY, sizeZ, sizeT = row_pixels - image["sizeX"] = unwrap(sizeX) - image["sizeY"] = unwrap(sizeY) - image["sizeZ"] = unwrap(sizeZ) - image["sizeT"] = unwrap(sizeT) + if "sizeX" in data: + image["sizeX"] = unwrap(data["sizeX"]) + if "sizeY" in data: + image["sizeY"] = unwrap(data["sizeY"]) + if "sizeZ" in data: + image["sizeZ"] = unwrap(data["sizeZ"]) + if "sizeT" in data: + image["sizeT"] = unwrap(data["sizeT"]) if share_id is not None: image["shareId"] = share_id if date is not None: @@ -753,31 +804,20 @@ def marshal_images( ) for e in qs.projection(q, params, service_opts): - e = unwrap(e)[0] - d = [ - e["id"], - e["archived"], - e["name"], - e["ownerId"], - e["image_details_permissions"], - e["filesetId"], - ] - kwargs = {"conn": conn, "row": d[0:6]} - if load_pixels: - d = [e["sizeX"], e["sizeY"], e["sizeZ"], e["sizeT"]] - kwargs["row_pixels"] = d + data = unwrap(e)[0] + kwargs = {} if date: - kwargs["acqDate"] = e["acqDate"] - kwargs["date"] = e["date"] + kwargs["acqDate"] = data["acqDate"] + kwargs["date"] = data["date"] # While marshalling the images, determine if there are any # images mentioned in shares that are not in the results # because they have been deleted - if share_id is not None and image_rids and e["id"] in image_rids: + if share_id is not None and image_rids and data["id"] in image_rids: image_rids.remove(e["id"]) kwargs["share_id"] = share_id - images.append(_marshal_image(**kwargs)) + images.append(_marshal_image_map(conn, data, **kwargs)) # Load thumbnails separately # We want version of most recent thumbnail (max thumbId) owned by user @@ -1536,23 +1576,12 @@ def marshal_tagged( images = [] for e in qs.projection(q, params, service_opts): - e = unwrap(e) - row = [ - e[0]["id"], - e[0]["archived"], - e[0]["name"], - e[0]["ownerId"], - e[0]["image_details_permissions"], - e[0]["filesetId"], - ] + data = unwrap(e)[0] kwargs = {} - if load_pixels: - d = [e[0]["sizeX"], e[0]["sizeY"], e[0]["sizeZ"], e[0]["sizeT"]] - kwargs["row_pixels"] = d if date: - kwargs["acqDate"] = e[0]["acqDate"] - kwargs["date"] = e[0]["date"] - images.append(_marshal_image(conn, row, **kwargs)) + kwargs["acqDate"] = data["acqDate"] + kwargs["date"] = data["date"] + images.append(_marshal_image_map(conn, data, **kwargs)) tagged["images"] = images # Screens From 3cf6fceaaa6bb6723cee1d12f117c527f305aa4f Mon Sep 17 00:00:00 2001 From: Chris Allan Date: Thu, 29 Aug 2024 10:13:14 +0000 Subject: [PATCH 2/5] Remove prints --- test/unit/test_tree.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/unit/test_tree.py b/test/unit/test_tree.py index 06ad248dfe..9e59402dd2 100644 --- a/test/unit/test_tree.py +++ b/test/unit/test_tree.py @@ -213,8 +213,6 @@ def test_marshal_plate_not_owner(self, mock_conn, owner_permissions): } marshaled = _marshal_plate(mock_conn, row) - print(marshaled) - print(expected) assert marshaled == expected # Add a lot of tests From 825af04d739b4e70ee45ce1b0cf835d7d05fe8fd Mon Sep 17 00:00:00 2001 From: Chris Allan Date: Thu, 29 Aug 2024 10:52:09 +0000 Subject: [PATCH 3/5] Add unit tests for image marshalling --- test/unit/test_tree.py | 134 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 133 insertions(+), 1 deletion(-) diff --git a/test/unit/test_tree.py b/test/unit/test_tree.py index 9e59402dd2..946390c15d 100644 --- a/test/unit/test_tree.py +++ b/test/unit/test_tree.py @@ -23,10 +23,13 @@ import pytest -from omero.rtypes import rlong, rstring, rtime +from omero.rtypes import rlong, rstring, rtime, unwrap from omeroweb.webclient.tree import ( _marshal_plate_acquisition, _marshal_dataset, + _marshal_date, + _marshal_image, + _marshal_image_map, _marshal_plate, parse_permissions_css, ) @@ -68,6 +71,43 @@ def end_time(): return rtime(1399545510 * 1000) +@pytest.fixture(scope="module") +def image_row(owner_permissions): + return [rlong(1), rstring("name"), rlong(10), owner_permissions, rlong(100)] + + +@pytest.fixture(scope="module") +def pixels_row(): + return [rlong(1), rlong(2), rlong(3), rlong(4)] + + +@pytest.fixture() +def image_data(image_row): + image_id, name, owner_id, permissions, fileset_id = image_row + return { + "id": image_id, + "archived": False, + "name": name, + "ownerId": owner_id, + "image_details_permissions": permissions, + "filesetId": fileset_id, + } + + +@pytest.fixture() +def image_data_with_pixels(image_data, pixels_row): + sizeX, sizeY, sizeZ, sizeT = pixels_row + image_data.update( + { + "sizeX": sizeX, + "sizeY": sizeY, + "sizeZ": sizeZ, + "sizeT": sizeT, + } + ) + return image_data + + class TestTree(object): """ Tests to ensure that OMERO.web "tree" infrastructure is working @@ -215,4 +255,96 @@ def test_marshal_plate_not_owner(self, mock_conn, owner_permissions): marshaled = _marshal_plate(mock_conn, row) assert marshaled == expected + def assert_image( + self, + marshaled, + with_pixels=False, + with_share=False, + date=None, + acqDate=None, + with_thumbVersion=False, + ): + expected = { + "id": 1, + "archived": False, + "ownerId": 10, + "name": "name", + "permsCss": "canEdit canAnnotate canLink canDelete canChgrp", + "filesetId": 100, + } + if with_pixels: + expected.update( + { + "sizeX": 1, + "sizeY": 2, + "sizeZ": 3, + "sizeT": 4, + } + ) + if with_share: + expected.update({"shareId": 1}) + if date: + # Cannot be static, will include local timezone + expected.update({"date": _marshal_date(unwrap(date))}) + if acqDate: + # Cannot be static, will include local timezone + expected.update({"acqDate": _marshal_date(unwrap(acqDate))}) + if with_thumbVersion: + expected.update({"thumbVersion": 1}) + assert marshaled == expected + + def test_marshal_image(self, mock_conn, image_row): + marshaled = _marshal_image(mock_conn, image_row) + self.assert_image(marshaled) + + def test_marshal_image_with_pixels(self, mock_conn, image_row, pixels_row): + marshaled = _marshal_image(mock_conn, image_row, pixels_row) + self.assert_image(marshaled, with_pixels=True) + + def test_marshal_image_with_share(self, mock_conn, image_row): + marshaled = _marshal_image(mock_conn, image_row, share_id=1) + self.assert_image(marshaled, with_share=True) + + def test_marshal_image_with_date(self, mock_conn, image_row, end_time): + marshaled = _marshal_image(mock_conn, image_row, date=end_time) + self.assert_image(marshaled, date=end_time) + + def test_marshal_image_with_acquisition_date( + self, mock_conn, image_row, start_time + ): + marshaled = _marshal_image(mock_conn, image_row, acqDate=start_time) + self.assert_image(marshaled, acqDate=start_time) + + def test_marshal_image_with_thumb_version(self, mock_conn, image_row, start_time): + marshaled = _marshal_image(mock_conn, image_row, thumbVersion=1) + self.assert_image(marshaled, with_thumbVersion=True) + + def test_marshal_image_map(self, mock_conn, image_data): + marshaled = _marshal_image_map(mock_conn, image_data) + self.assert_image(marshaled) + + def test_marshal_image_map_with_pixels(self, mock_conn, image_data_with_pixels): + marshaled = _marshal_image_map(mock_conn, image_data_with_pixels) + self.assert_image(marshaled, with_pixels=True) + + def test_marshal_image_map_with_share(self, mock_conn, image_data): + marshaled = _marshal_image_map(mock_conn, image_data, share_id=1) + self.assert_image(marshaled, with_share=True) + + def test_marshal_image_map_with_date(self, mock_conn, image_data, end_time): + marshaled = _marshal_image_map(mock_conn, image_data, date=end_time) + self.assert_image(marshaled, date=end_time) + + def test_marshal_image_map_with_acquisition_date( + self, mock_conn, image_data, start_time + ): + marshaled = _marshal_image_map(mock_conn, image_data, acqDate=start_time) + self.assert_image(marshaled, acqDate=start_time) + + def test_marshal_image_map_with_thumb_version( + self, mock_conn, image_data, start_time + ): + marshaled = _marshal_image_map(mock_conn, image_data, thumbVersion=1) + self.assert_image(marshaled, with_thumbVersion=True) + # Add a lot of tests From 68e99e5e5db446e1155d68b96abe568faa02bb27 Mon Sep 17 00:00:00 2001 From: Chris Allan Date: Thu, 29 Aug 2024 20:13:04 +0000 Subject: [PATCH 4/5] Fix incorrect variable usage in marshal_images() --- omeroweb/webclient/tree.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/omeroweb/webclient/tree.py b/omeroweb/webclient/tree.py index f2559c4ca7..beb46de53c 100644 --- a/omeroweb/webclient/tree.py +++ b/omeroweb/webclient/tree.py @@ -814,7 +814,7 @@ def marshal_images( # images mentioned in shares that are not in the results # because they have been deleted if share_id is not None and image_rids and data["id"] in image_rids: - image_rids.remove(e["id"]) + image_rids.remove(data["id"]) kwargs["share_id"] = share_id images.append(_marshal_image_map(conn, data, **kwargs)) From 7691f51049f69dd546557a7eea465024f597a983 Mon Sep 17 00:00:00 2001 From: Chris Allan Date: Thu, 29 Aug 2024 20:17:58 +0000 Subject: [PATCH 5/5] Add test case for positive archival --- test/unit/test_tree.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/test/unit/test_tree.py b/test/unit/test_tree.py index 946390c15d..8bbff31f6a 100644 --- a/test/unit/test_tree.py +++ b/test/unit/test_tree.py @@ -263,10 +263,11 @@ def assert_image( date=None, acqDate=None, with_thumbVersion=False, + is_archived=False, ): expected = { "id": 1, - "archived": False, + "archived": is_archived, "ownerId": 10, "name": "name", "permsCss": "canEdit canAnnotate canLink canDelete canChgrp", @@ -323,6 +324,11 @@ def test_marshal_image_map(self, mock_conn, image_data): marshaled = _marshal_image_map(mock_conn, image_data) self.assert_image(marshaled) + def test_marshal_image_map_archived(self, mock_conn, image_data): + image_data["archived"] = True + marshaled = _marshal_image_map(mock_conn, image_data) + self.assert_image(marshaled, is_archived=True) + def test_marshal_image_map_with_pixels(self, mock_conn, image_data_with_pixels): marshaled = _marshal_image_map(mock_conn, image_data_with_pixels) self.assert_image(marshaled, with_pixels=True)