From 4ea118584da2bcc2e4be0080abdac56032308faa Mon Sep 17 00:00:00 2001 From: Sergey Golitsynskiy Date: Thu, 18 Nov 2021 20:52:29 -0500 Subject: [PATCH 1/4] Expire hid_counter on db_next_hid() call --- lib/galaxy/model/mapping.py | 1 + test/unit/data/test_galaxy_mapping.py | 15 +++++++++++++++ 2 files changed, 16 insertions(+) diff --git a/lib/galaxy/model/mapping.py b/lib/galaxy/model/mapping.py index de6414c4ab40..de1de7418351 100644 --- a/lib/galaxy/model/mapping.py +++ b/lib/galaxy/model/mapping.py @@ -51,6 +51,7 @@ def db_next_hid(self, n=1): else: stmt = table.update().where(table.c.id == model.cached_id(self)).values(hid_counter=(table.c.hid_counter + n)).returning(table.c.hid_counter) next_hid = session.execute(stmt).scalar() - n + session.expire(self, ['hid_counter']) return next_hid diff --git a/test/unit/data/test_galaxy_mapping.py b/test/unit/data/test_galaxy_mapping.py index e5fee44ec00d..98c2cdc478a8 100644 --- a/test/unit/data/test_galaxy_mapping.py +++ b/test/unit/data/test_galaxy_mapping.py @@ -920,6 +920,21 @@ def test_can_manage_private_dataset(self): assert security_agent.can_manage_dataset(u_from.all_roles(), d1.dataset) assert not security_agent.can_manage_dataset(u_other.all_roles(), d1.dataset) + def test_history_hid_counter_is_expired_after_next_hid_call(self): + u = model.User(email="hid_abuser@example.com", password="password") + h = model.History(name="History for hid testing", user=u) + self.persist(u, h) + state = inspect(h) + assert h.hid_counter == 1 + assert 'hid_counter' not in state.unloaded + assert 'id' not in state.unloaded + + h._next_hid() + + assert 'hid_counter' in state.unloaded # this attribute has been expired + assert 'id' not in state.unloaded # but other attributes have NOT been expired + assert h.hid_counter == 2 # check this last: this causes thie hid_counter to be reloaded + def _three_users(self, suffix): email_from = f"user_{suffix}e1@example.com" email_to = f"user_{suffix}e2@example.com" From cb757ed66d6cba13e26fefb73325b3e1ec4ed641 Mon Sep 17 00:00:00 2001 From: Sergey Golitsynskiy Date: Fri, 19 Nov 2021 02:02:37 -0500 Subject: [PATCH 2/4] Add unit test for db_next_hid --- test/unit/data/test_galaxy_mapping.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/unit/data/test_galaxy_mapping.py b/test/unit/data/test_galaxy_mapping.py index 98c2cdc478a8..afa34931413f 100644 --- a/test/unit/data/test_galaxy_mapping.py +++ b/test/unit/data/test_galaxy_mapping.py @@ -935,6 +935,16 @@ def test_history_hid_counter_is_expired_after_next_hid_call(self): assert 'id' not in state.unloaded # but other attributes have NOT been expired assert h.hid_counter == 2 # check this last: this causes thie hid_counter to be reloaded + def test_next_hid(self): + u = model.User(email="hid_abuser@example.com", password="password") + h = model.History(name="History for hid testing", user=u) + self.persist(u, h) + assert h.hid_counter == 1 + h._next_hid() + assert h.hid_counter == 2 + h._next_hid(n=3) + assert h.hid_counter == 5 + def _three_users(self, suffix): email_from = f"user_{suffix}e1@example.com" email_to = f"user_{suffix}e2@example.com" From 837332f6d1564c755b01225363b920444f34ec28 Mon Sep 17 00:00:00 2001 From: Sergey Golitsynskiy Date: Fri, 19 Nov 2021 02:03:03 -0500 Subject: [PATCH 3/4] Update db_next_hid statements; use engine.begin --- lib/galaxy/model/mapping.py | 41 ++++++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/lib/galaxy/model/mapping.py b/lib/galaxy/model/mapping.py index de1de7418351..1e298e3c3a8b 100644 --- a/lib/galaxy/model/mapping.py +++ b/lib/galaxy/model/mapping.py @@ -11,6 +11,7 @@ from sqlalchemy import ( and_, select, + update, ) from sqlalchemy.orm import class_mapper, object_session, relation @@ -32,27 +33,33 @@ # Helper methods. -def db_next_hid(self, n=1): +def db_next_hid(history, n=1): """ - db_next_hid( self ) - - Override __next_hid to generate from the database in a concurrency safe way. - Loads the next history ID from the DB and returns it. - It also saves the future next_id into the DB. + Generate from the database in a concurrency safe way. + Load the next history ID from the database and return it. + Save the future next_id in the database. :rtype: int - :returns: the next history id + :returns: the next hid """ - session = object_session(self) - table = self.table - if "postgres" not in session.bind.dialect.name: - next_hid = select([table.c.hid_counter], table.c.id == model.cached_id(self)).with_for_update().scalar() - table.update(table.c.id == self.id).execute(hid_counter=(next_hid + n)) - else: - stmt = table.update().where(table.c.id == model.cached_id(self)).values(hid_counter=(table.c.hid_counter + n)).returning(table.c.hid_counter) - next_hid = session.execute(stmt).scalar() - n - session.expire(self, ['hid_counter']) - return next_hid + session = object_session(history) + engine = session.bind + table = history.__table__ + history_id = model.cached_id(history) + update_stmt = update(table).where(table.c.id == history_id).values(hid_counter=table.c.hid_counter + n) + + with engine.begin() as conn: + if engine.name in ['postgres', 'postgresql']: + stmt = update_stmt.returning(table.c.hid_counter) + hid = conn.execute(stmt).scalar() + hid -= n + else: + select_stmt = select(table.c.hid_counter).where(table.c.id == history_id).with_for_update() + hid = conn.execute(select_stmt).scalar() + conn.execute(update_stmt) + + session.expire(history, ['hid_counter']) + return hid model.History._next_hid = db_next_hid # type: ignore From 9f6a2cca407e2e052aa8c1a42fa5cf6be2b5238b Mon Sep 17 00:00:00 2001 From: Sergey Golitsynskiy Date: Fri, 19 Nov 2021 11:19:29 -0500 Subject: [PATCH 4/4] Do not override History._next_id --- lib/galaxy/model/__init__.py | 37 ++++++++++++++++++------- lib/galaxy/model/mapping.py | 39 +-------------------------- test/unit/data/test_galaxy_mapping.py | 2 +- 3 files changed, 30 insertions(+), 48 deletions(-) diff --git a/lib/galaxy/model/__init__.py b/lib/galaxy/model/__init__.py index 5b0b6dc2c4bd..edb5b2d2cd95 100644 --- a/lib/galaxy/model/__init__.py +++ b/lib/galaxy/model/__init__.py @@ -68,6 +68,7 @@ type_coerce, Unicode, UniqueConstraint, + update, VARCHAR, ) from sqlalchemy.exc import OperationalError @@ -2440,15 +2441,33 @@ def add_pending_items(self, set_output_hid=True): self._pending_additions = [] def _next_hid(self, n=1): - # this is overriden in mapping.py db_next_hid() method - if len(self.datasets) == 0: - return n - else: - last_hid = 0 - for dataset in self.datasets: - if dataset.hid > last_hid: - last_hid = dataset.hid - return last_hid + n + """ + Generate next_hid from the database in a concurrency safe way: + 1. Retrieve hid_counter from database + 2. Increment hid_counter by n and store in database + 3. Return retrieved hid_counter. + + Handle with SQLAlchemy Core to keep this independent from current session state, except: + expire hid_counter attribute, since its value in the session is no longer valid. + """ + session = object_session(self) + engine = session.bind + table = self.__table__ + history_id = cached_id(self) + update_stmt = update(table).where(table.c.id == history_id).values(hid_counter=table.c.hid_counter + n) + + with engine.begin() as conn: + if engine.name in ['postgres', 'postgresql']: + stmt = update_stmt.returning(table.c.hid_counter) + updated_hid = conn.execute(stmt).scalar() + hid = updated_hid - n + else: + select_stmt = select(table.c.hid_counter).where(table.c.id == history_id).with_for_update() + hid = conn.execute(select_stmt).scalar() + conn.execute(update_stmt) + + session.expire(self, ['hid_counter']) + return hid def add_galaxy_session(self, galaxy_session, association=None): if association is None: diff --git a/lib/galaxy/model/mapping.py b/lib/galaxy/model/mapping.py index 1e298e3c3a8b..5d0e73177dbb 100644 --- a/lib/galaxy/model/mapping.py +++ b/lib/galaxy/model/mapping.py @@ -8,11 +8,7 @@ from threading import local from typing import Optional, Type -from sqlalchemy import ( - and_, - select, - update, -) +from sqlalchemy import and_ from sqlalchemy.orm import class_mapper, object_session, relation from galaxy import model @@ -32,39 +28,6 @@ "creating_job_associations", relation(model.JobToOutputDatasetCollectionAssociation, viewonly=True)) -# Helper methods. -def db_next_hid(history, n=1): - """ - Generate from the database in a concurrency safe way. - Load the next history ID from the database and return it. - Save the future next_id in the database. - - :rtype: int - :returns: the next hid - """ - session = object_session(history) - engine = session.bind - table = history.__table__ - history_id = model.cached_id(history) - update_stmt = update(table).where(table.c.id == history_id).values(hid_counter=table.c.hid_counter + n) - - with engine.begin() as conn: - if engine.name in ['postgres', 'postgresql']: - stmt = update_stmt.returning(table.c.hid_counter) - hid = conn.execute(stmt).scalar() - hid -= n - else: - select_stmt = select(table.c.hid_counter).where(table.c.id == history_id).with_for_update() - hid = conn.execute(select_stmt).scalar() - conn.execute(update_stmt) - - session.expire(history, ['hid_counter']) - return hid - - -model.History._next_hid = db_next_hid # type: ignore - - def _workflow_invocation_update(self): session = object_session(self) table = self.table diff --git a/test/unit/data/test_galaxy_mapping.py b/test/unit/data/test_galaxy_mapping.py index afa34931413f..0ddf699195ec 100644 --- a/test/unit/data/test_galaxy_mapping.py +++ b/test/unit/data/test_galaxy_mapping.py @@ -621,7 +621,7 @@ def get_latest_entry(entries): self.new_hda(h1, name="1") self.new_hda(h2, name="2") self.session().flush() - # db_next_hid modifies history, plus trigger on HDA means 2 additional audit rows per history + # _next_hid modifies history, plus trigger on HDA means 2 additional audit rows per history h1_audits = get_audit_table_entries(h1) h2_audits = get_audit_table_entries(h2)