From 49e46c5b55ccf1a3212490364fdea0d81ac525aa Mon Sep 17 00:00:00 2001 From: John Davis Date: Thu, 18 Apr 2024 17:51:13 -0400 Subject: [PATCH 01/12] Rename method, add comment to explain side effects --- lib/galaxy/model/tags.py | 13 ++++++++++--- test/unit/app/managers/test_TagHandler.py | 11 +++++++++++ 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/lib/galaxy/model/tags.py b/lib/galaxy/model/tags.py index a62051b79eef..eefc252f919c 100644 --- a/lib/galaxy/model/tags.py +++ b/lib/galaxy/model/tags.py @@ -294,8 +294,15 @@ def get_tag_by_name(self, tag_name): return self.sa_session.scalars(select(galaxy.model.Tag).filter_by(name=tag_name.lower()).limit(1)).first() return None - def _create_tag(self, tag_str: str): - """Create a Tag object from a tag string.""" + def _create_tags(self, tag_str: str): + """ + Create or retrieve one of more Tag objects from a tag string. If there are multiple + hierarchical tags in the tag string, the string will be split along `self.hierarchy_separator` chars. + A Tag instance will be created for each non-empty prefix. If a prefix corresponds to the + name of an existing tag, that tag will be retrieved; otherwise, a new Tag object will be created. + For example, for the tag string `a.b.c` 3 Tag isntances will be created: `a`, `a.b`, `a.b.c`. + Return the last tag created (`a.b.c`). + """ tag_hierarchy = tag_str.split(self.hierarchy_separator) tag_prefix = "" parent_tag = None @@ -344,7 +351,7 @@ def _get_or_create_tag(self, tag_str): tag = self.get_tag_by_name(scrubbed_tag_str) # Create tag if necessary. if tag is None: - tag = self._create_tag(scrubbed_tag_str) + tag = self._create_tags(scrubbed_tag_str) return tag def _get_item_tag_assoc(self, user, item, tag_name): diff --git a/test/unit/app/managers/test_TagHandler.py b/test/unit/app/managers/test_TagHandler.py index 97b476c0e715..f249e4dfa63d 100644 --- a/test/unit/app/managers/test_TagHandler.py +++ b/test/unit/app/managers/test_TagHandler.py @@ -112,3 +112,14 @@ def test_item_has_tag(self): # Tag assert self.tag_handler.item_has_tag(self.user, item=hda, tag=hda.tags[0].tag) assert not self.tag_handler.item_has_tag(self.user, item=hda, tag="tag2") + + def test_get_name_value_pair(self): + """Test different combinations of tag and name/value delimiters via parse_tags().""" + assert self.tag_handler.parse_tags("a") == [("a", None)] + assert self.tag_handler.parse_tags("a.b") == [("a.b", None)] + assert self.tag_handler.parse_tags("a.b:c") == [("a.b", "c")] + assert self.tag_handler.parse_tags("a.b:c.d") == [("a.b", "c.d")] + assert self.tag_handler.parse_tags("a.b:c.d:e.f") == [("a.b", "c.d:e.f")] + assert self.tag_handler.parse_tags("a.b:c.d:e.f.") == [("a.b", "c.d:e.f.")] + assert self.tag_handler.parse_tags("a.b:c.d:e.f:") == [("a.b", "c.d:e.f:")] + assert self.tag_handler.parse_tags("a.b:c.d:e.f:::") == [("a.b", "c.d:e.f:::")] From b356b0cca3b658fcda62e19c6d99886e4c194ca8 Mon Sep 17 00:00:00 2001 From: John Davis Date: Thu, 18 Apr 2024 18:29:43 -0400 Subject: [PATCH 02/12] Factor out tag pattern into a const --- lib/galaxy/schema/schema.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/galaxy/schema/schema.py b/lib/galaxy/schema/schema.py index 34fe24988095..853148272753 100644 --- a/lib/galaxy/schema/schema.py +++ b/lib/galaxy/schema/schema.py @@ -66,6 +66,8 @@ OptionalNumberT = Annotated[Optional[Union[int, float]], Field(None)] +TAG_ITEM_PATTERN = r"^([^\s.:])+(.[^\s.:]+)*(:[^\s.:]+)?$" + class DatasetState(str, Enum): NEW = "new" @@ -527,7 +529,7 @@ class HistoryContentSource(str, Enum): DatasetCollectionInstanceType = Literal["history", "library"] -TagItem = Annotated[str, Field(..., pattern=r"^([^\s.:])+(.[^\s.:]+)*(:[^\s.:]+)?$")] +TagItem = Annotated[str, Field(..., pattern=TAG_ITEM_PATTERN)] class TagCollection(RootModel): From 972f91824163047b9d25d5d1abd9812f3b410a63 Mon Sep 17 00:00:00 2001 From: John Davis Date: Thu, 18 Apr 2024 18:31:45 -0400 Subject: [PATCH 03/12] Add unit test (failing) for tag string regex --- test/unit/schema/test_schema.py | 46 ++++++++++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/test/unit/schema/test_schema.py b/test/unit/schema/test_schema.py index 570469ed6fc2..e139a52544c4 100644 --- a/test/unit/schema/test_schema.py +++ b/test/unit/schema/test_schema.py @@ -1,8 +1,12 @@ +import re from uuid import uuid4 from pydantic import BaseModel -from galaxy.schema.schema import DatasetStateField +from galaxy.schema.schema import ( + DatasetStateField, + TAG_ITEM_PATTERN, +) from galaxy.schema.tasks import ( GenerateInvocationDownload, RequestUser, @@ -34,3 +38,43 @@ class StateModel(BaseModel): def test_dataset_state_coercion(): assert StateModel(state="ok").state == "ok" assert StateModel(state="deleted").state == "discarded" + + +class TestTagPattern: + + def test_valid(self): + tag_strings = [ + "a", + "aa", + "aa.aa", + "aa.aa.aa", + "~!@#$%^&*()_+`-=[]{};'\",./<>?", + "a.b:c", + "a.b:c.d:e.f", + "a.b:c.d:e..f", + "a.b:c.d:e.f:g", + "a.b:c.d:e.f::g", + "a.b:c.d:e.f::g:h", + "a::a", # leading colon for tag value + "a:.a", # leading period for tag value + "a:a:", # training colon OK for tag value + "a:a.", # training period OK for tag value + ] + for t in tag_strings: + assert re.match(TAG_ITEM_PATTERN, t) + + def test_invalid(self): + tag_strings = [ + " a", # leading space for tag name + ":a", # leading colon for tag name + ".a", # leading period for tag name + "a ", # trailing space for tag name + "a a", # space inside tag name + "a: a", # leading space for tag value + "a:a a", # space inside tag value + "a:", # trailing colon for tag name + "a.", # trailing period for tag name + "a:b ", # trailing space for tag value + ] + for t in tag_strings: + assert not re.match(TAG_ITEM_PATTERN, t) From 7ec131156eb5ac635e90bd644201ae3f2f9503a7 Mon Sep 17 00:00:00 2001 From: John Davis Date: Thu, 18 Apr 2024 18:34:01 -0400 Subject: [PATCH 04/12] Add unit test to verify spllitting tag string into name/value --- test/unit/app/managers/test_TagHandler.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/unit/app/managers/test_TagHandler.py b/test/unit/app/managers/test_TagHandler.py index f249e4dfa63d..7f0e3ad21ce8 100644 --- a/test/unit/app/managers/test_TagHandler.py +++ b/test/unit/app/managers/test_TagHandler.py @@ -114,12 +114,13 @@ def test_item_has_tag(self): assert not self.tag_handler.item_has_tag(self.user, item=hda, tag="tag2") def test_get_name_value_pair(self): - """Test different combinations of tag and name/value delimiters via parse_tags().""" + """Verify that parsing a single tag string correctly splits it into name/value pairs.""" assert self.tag_handler.parse_tags("a") == [("a", None)] assert self.tag_handler.parse_tags("a.b") == [("a.b", None)] assert self.tag_handler.parse_tags("a.b:c") == [("a.b", "c")] assert self.tag_handler.parse_tags("a.b:c.d") == [("a.b", "c.d")] assert self.tag_handler.parse_tags("a.b:c.d:e.f") == [("a.b", "c.d:e.f")] assert self.tag_handler.parse_tags("a.b:c.d:e.f.") == [("a.b", "c.d:e.f.")] + assert self.tag_handler.parse_tags("a.b:c.d:e.f..") == [("a.b", "c.d:e.f..")] assert self.tag_handler.parse_tags("a.b:c.d:e.f:") == [("a.b", "c.d:e.f:")] - assert self.tag_handler.parse_tags("a.b:c.d:e.f:::") == [("a.b", "c.d:e.f:::")] + assert self.tag_handler.parse_tags("a.b:c.d:e.f::") == [("a.b", "c.d:e.f::")] From c31bccf712ff206f4679bde52da2969779fff0b3 Mon Sep 17 00:00:00 2001 From: John Davis Date: Thu, 18 Apr 2024 19:05:06 -0400 Subject: [PATCH 05/12] Fix regex pattern --- lib/galaxy/schema/schema.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/schema/schema.py b/lib/galaxy/schema/schema.py index 853148272753..277bb82f481e 100644 --- a/lib/galaxy/schema/schema.py +++ b/lib/galaxy/schema/schema.py @@ -66,7 +66,7 @@ OptionalNumberT = Annotated[Optional[Union[int, float]], Field(None)] -TAG_ITEM_PATTERN = r"^([^\s.:])+(.[^\s.:]+)*(:[^\s.:]+)?$" +TAG_ITEM_PATTERN = r"^([^\s.:])+(\.[^\s.:]+)*(:\S+)?$" class DatasetState(str, Enum): From ee64fc414daa5b807c2d3305e600f3b75a72fe98 Mon Sep 17 00:00:00 2001 From: John Davis Date: Thu, 18 Apr 2024 21:45:39 -0400 Subject: [PATCH 06/12] Update client valid tag regex --- client/src/components/Tags/model.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/src/components/Tags/model.js b/client/src/components/Tags/model.js index ced4f9896553..43dbc05d74ee 100644 --- a/client/src/components/Tags/model.js +++ b/client/src/components/Tags/model.js @@ -7,7 +7,7 @@ import { keyedColorScheme } from "utils/color"; // Valid tag regex. The basic format here is a tag name with optional subtags // separated by a period, and then an optional value after a colon. -export const VALID_TAG_RE = /^([^\s.:])+(.[^\s.:]+)*(:[^\s.:]+)?$/; +export const VALID_TAG_RE = /^([^\s.:])+(\.[^\s.:]+)*(:\S+)?$/; export class TagModel { /** From b32480c95cef2cf5ff9609ea71e08a3ece7f3579 Mon Sep 17 00:00:00 2001 From: John Davis Date: Fri, 19 Apr 2024 09:46:48 -0400 Subject: [PATCH 07/12] Update lib/galaxy/model/tags.py Co-authored-by: Dannon --- lib/galaxy/model/tags.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/model/tags.py b/lib/galaxy/model/tags.py index eefc252f919c..8772a10900a4 100644 --- a/lib/galaxy/model/tags.py +++ b/lib/galaxy/model/tags.py @@ -296,7 +296,7 @@ def get_tag_by_name(self, tag_name): def _create_tags(self, tag_str: str): """ - Create or retrieve one of more Tag objects from a tag string. If there are multiple + Create or retrieve one or more Tag objects from a tag string. If there are multiple hierarchical tags in the tag string, the string will be split along `self.hierarchy_separator` chars. A Tag instance will be created for each non-empty prefix. If a prefix corresponds to the name of an existing tag, that tag will be retrieved; otherwise, a new Tag object will be created. From 645ff002ecf5bdc0e057f245712f4c9a9ebb1647 Mon Sep 17 00:00:00 2001 From: John Davis Date: Fri, 19 Apr 2024 09:47:00 -0400 Subject: [PATCH 08/12] Update lib/galaxy/model/tags.py Co-authored-by: Dannon --- lib/galaxy/model/tags.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/model/tags.py b/lib/galaxy/model/tags.py index 8772a10900a4..a642a4a5a34a 100644 --- a/lib/galaxy/model/tags.py +++ b/lib/galaxy/model/tags.py @@ -300,7 +300,7 @@ def _create_tags(self, tag_str: str): hierarchical tags in the tag string, the string will be split along `self.hierarchy_separator` chars. A Tag instance will be created for each non-empty prefix. If a prefix corresponds to the name of an existing tag, that tag will be retrieved; otherwise, a new Tag object will be created. - For example, for the tag string `a.b.c` 3 Tag isntances will be created: `a`, `a.b`, `a.b.c`. + For example, for the tag string `a.b.c` 3 Tag instances will be created: `a`, `a.b`, `a.b.c`. Return the last tag created (`a.b.c`). """ tag_hierarchy = tag_str.split(self.hierarchy_separator) From b5a93f0ef7ea55a501f941812c5a54422a65e970 Mon Sep 17 00:00:00 2001 From: John Davis Date: Fri, 19 Apr 2024 09:47:11 -0400 Subject: [PATCH 09/12] Update lib/galaxy/model/tags.py Co-authored-by: Dannon --- lib/galaxy/model/tags.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/model/tags.py b/lib/galaxy/model/tags.py index a642a4a5a34a..15abfb6ab967 100644 --- a/lib/galaxy/model/tags.py +++ b/lib/galaxy/model/tags.py @@ -351,7 +351,7 @@ def _get_or_create_tag(self, tag_str): tag = self.get_tag_by_name(scrubbed_tag_str) # Create tag if necessary. if tag is None: - tag = self._create_tags(scrubbed_tag_str) + tag = self._create_tag(scrubbed_tag_str) return tag def _get_item_tag_assoc(self, user, item, tag_name): From b798109589a6e0b3cd5492d021f9c3ad825a97f5 Mon Sep 17 00:00:00 2001 From: John Davis Date: Fri, 19 Apr 2024 09:47:18 -0400 Subject: [PATCH 10/12] Update lib/galaxy/model/tags.py Co-authored-by: Dannon --- lib/galaxy/model/tags.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/model/tags.py b/lib/galaxy/model/tags.py index 15abfb6ab967..22e7ae63ef75 100644 --- a/lib/galaxy/model/tags.py +++ b/lib/galaxy/model/tags.py @@ -294,7 +294,7 @@ def get_tag_by_name(self, tag_name): return self.sa_session.scalars(select(galaxy.model.Tag).filter_by(name=tag_name.lower()).limit(1)).first() return None - def _create_tags(self, tag_str: str): + def _create_tag(self, tag_str: str): """ Create or retrieve one or more Tag objects from a tag string. If there are multiple hierarchical tags in the tag string, the string will be split along `self.hierarchy_separator` chars. From 90bb4c93341e972955e757bff4db72ee0bdc62c9 Mon Sep 17 00:00:00 2001 From: John Davis Date: Fri, 19 Apr 2024 09:47:31 -0400 Subject: [PATCH 11/12] Update test/unit/schema/test_schema.py Co-authored-by: Dannon --- test/unit/schema/test_schema.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/schema/test_schema.py b/test/unit/schema/test_schema.py index e139a52544c4..69d7660c20d4 100644 --- a/test/unit/schema/test_schema.py +++ b/test/unit/schema/test_schema.py @@ -57,7 +57,7 @@ def test_valid(self): "a.b:c.d:e.f::g:h", "a::a", # leading colon for tag value "a:.a", # leading period for tag value - "a:a:", # training colon OK for tag value + "a:a:", # trailing colon OK for tag value "a:a.", # training period OK for tag value ] for t in tag_strings: From ac8338f64df9f6c527222dfb045fd010c00f787f Mon Sep 17 00:00:00 2001 From: John Davis Date: Fri, 19 Apr 2024 09:47:38 -0400 Subject: [PATCH 12/12] Update test/unit/schema/test_schema.py Co-authored-by: Dannon --- test/unit/schema/test_schema.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/schema/test_schema.py b/test/unit/schema/test_schema.py index 69d7660c20d4..21b37096d131 100644 --- a/test/unit/schema/test_schema.py +++ b/test/unit/schema/test_schema.py @@ -58,7 +58,7 @@ def test_valid(self): "a::a", # leading colon for tag value "a:.a", # leading period for tag value "a:a:", # trailing colon OK for tag value - "a:a.", # training period OK for tag value + "a:a.", # trailing period OK for tag value ] for t in tag_strings: assert re.match(TAG_ITEM_PATTERN, t)