diff --git a/src/simdb/database/database.py b/src/simdb/database/database.py index f8523a11..37382920 100644 --- a/src/simdb/database/database.py +++ b/src/simdb/database/database.py @@ -260,8 +260,11 @@ def _get_simulation_data( """ total_count = query.count() - if sort_by: - query = self._apply_sort_by(query, sort_by, sort_asc) + # PostgreSQL does not guarantee row order without ORDER BY; see + # https://www.postgresql.org/docs/current/queries-order.html. + # Always apply_sort_by to avoid unreliable listings and + # artefacts like duplications in pagination, etc. + query = self._apply_sort_by(query, sort_by, sort_asc) if limit: offset = (page - 1) * limit @@ -295,34 +298,46 @@ def _get_simulation_data( return total_count, results - def _apply_sort_by(self, query, sort_by: str, sort_asc: bool): + def _apply_sort_by(self, query, sort_by: str = "", sort_asc: bool = False): """ Apply ORDER BY clause to query for given sort field. :param query: SQLAlchemy query object - :param sort_by: Field name to sort by + :param sort_by: Field name to sort by, defaults to "uuid" :param sort_asc: Sort in ascending order if True, descending if False :return: Query with ORDER BY applied """ dialect = self.engine.dialect.name + # Default to uuid, the only required and unique column, + # so listings stay stable. + if not sort_by: + sort_by = "uuid" + if sort_by == "alias": return query.order_by( - Simulation.alias if sort_asc else Simulation.alias.desc() + Simulation.alias if sort_asc else Simulation.alias.desc(), + Simulation.uuid if sort_asc else Simulation.uuid.desc(), ) elif sort_by == "uuid": return query.order_by( Simulation.uuid if sort_asc else Simulation.uuid.desc() ) elif sort_by == "datetime": + # However unlikely, datetime is not unique; add uuid for reproducible order. return query.order_by( - Simulation.datetime if sort_asc else Simulation.datetime.desc() + Simulation.datetime if sort_asc else Simulation.datetime.desc(), + Simulation.uuid if sort_asc else Simulation.uuid.desc(), ) else: sort_col = self._get_json_sort_column(sort_by, dialect) if sort_col is not None: - return query.order_by(sort_col if sort_asc else sort_col.desc()) - return query + return query.order_by( + sort_col if sort_asc else sort_col.desc(), + Simulation.uuid if sort_asc else Simulation.uuid.desc(), + ) + # Could not sort. + raise DatabaseError(f"Unknown sort column: {sort_by}") def _get_json_sort_column(self, key: str, dialect: str): """ diff --git a/tests/database/test_metadata_queries.py b/tests/database/test_metadata_queries.py index f7630055..7da850ab 100644 --- a/tests/database/test_metadata_queries.py +++ b/tests/database/test_metadata_queries.py @@ -10,11 +10,36 @@ @pytest.fixture -def db(): +def db(monkeypatch): with tempfile.NamedTemporaryFile(suffix=".db", delete=False) as f: db_file = f.name database = Database(Database.DBMS.SQLITE, file=db_file) Base.metadata.create_all(database.engine) + + original_query_all = database.session.query(Simulation).__class__.all + unordered_query_calls = {"value": 0} + + def unstable_order(self): + # PostgreSQL does not guarantee row order without ORDER BY; see + # https://www.postgresql.org/docs/current/queries-order.html. + # Unless "_order_by_clauses" is set, mimic the unstable ordering. + if getattr(self, "_order_by_clauses", ()): + return list(original_query_all(self)) + + rows = list(original_query_all(self)) + if not rows: + return rows + + # Simple rotation of rows, at every call to query.all() + unordered_query_calls["value"] += 1 + rotation = (unordered_query_calls["value"] - 1) % len(rows) + rows = rows[rotation:] + rows[:rotation] + return rows + + monkeypatch.setattr( + database.session.query(Simulation).__class__, "all", unstable_order + ) + yield database database.close() @@ -296,6 +321,54 @@ def test_list_simulation_data_with_sort_by_alias(self, db): aliases = [r["alias"] for r in results] assert aliases == ["a_sim", "b_sim", "c_sim"] + def test_db_fixture_returns_unstable_order_without_sort(self, db): + sim1 = create_simulation(alias="sim1") + sim2 = create_simulation(alias="sim2") + sim3 = create_simulation(alias="sim3") + sim4 = create_simulation(alias="sim4") + + db.insert_simulation(sim1) + db.insert_simulation(sim2) + db.insert_simulation(sim3) + db.insert_simulation(sim4) + db.session.commit() + + query = db.session.query(Simulation) + page1_count = query.count() + page1 = query.all()[:2] + + query = db.session.query(Simulation) + page2_count = query.count() + page2 = query.all()[2:4] + + assert page1_count == page2_count == 4 + assert len(page1) == len(page2) == 2 + + page1_aliases = {row.alias for row in page1} + page2_aliases = {row.alias for row in page2} + + # Without a stable sort key, page 1 and page 2 can overlap. + assert len(page1_aliases & page2_aliases) == 1 + assert len(page1_aliases | page2_aliases) == 3 + + def test_list_simulation_data_is_stable_without_explicit_sort(self, db): + sim1 = create_simulation() + sim2 = create_simulation() + sim3 = create_simulation() + sim4 = create_simulation() + + db.insert_simulation(sim1) + db.insert_simulation(sim2) + db.insert_simulation(sim3) + db.insert_simulation(sim4) + db.session.commit() + + first_count, first_page = db.list_simulation_data(limit=2, page=1) + second_count, second_page = db.list_simulation_data(limit=2, page=1) + + assert first_count == second_count == 4 + assert first_page == second_page + class TestQueryMetaData: def test_query_meta_data_empty_constraints(self, db):