From b42fc309192e11664878730fd791731cb3c7d1a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcel=20M=C3=BCller?= Date: Sat, 5 Oct 2024 16:39:40 +0200 Subject: [PATCH 1/8] Rename incorrectly named variable --- goosebit/ui/bff/software/responses.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/goosebit/ui/bff/software/responses.py b/goosebit/ui/bff/software/responses.py index 7c3cd55b..53b96312 100644 --- a/goosebit/ui/bff/software/responses.py +++ b/goosebit/ui/bff/software/responses.py @@ -31,7 +31,7 @@ async def convert(cls, dt_query: DataTableRequest, query: QuerySet, search_filte if not dt_query.length == 0: query = query.limit(dt_query.length) - devices = await query.all() - data = [SoftwareSchema.model_validate(d) for d in devices] + software = await query.all() + data = [SoftwareSchema.model_validate(s) for s in software] return cls(data=data, draw=dt_query.draw, records_total=total_records, records_filtered=filtered_records) From 007eb556ef35112d60620357c5a50ec8c45f7035 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcel=20M=C3=BCller?= Date: Sat, 5 Oct 2024 16:48:08 +0200 Subject: [PATCH 2/8] Align convert functions --- goosebit/ui/bff/common/requests.py | 2 +- goosebit/ui/bff/devices/responses.py | 8 ++++++-- goosebit/ui/bff/rollouts/responses.py | 8 ++++++-- goosebit/ui/bff/software/responses.py | 10 ++++------ 4 files changed, 17 insertions(+), 11 deletions(-) diff --git a/goosebit/ui/bff/common/requests.py b/goosebit/ui/bff/common/requests.py index f19ac7f8..0ef6b26e 100644 --- a/goosebit/ui/bff/common/requests.py +++ b/goosebit/ui/bff/common/requests.py @@ -39,7 +39,7 @@ class DataTableRequest(BaseModel): columns: list[DataTableColumnSchema] = list() order: list[DataTableOrderSchema] = list() start: int = 0 - length: int = 0 + length: int | None = None search: DataTableSearchSchema = DataTableSearchSchema() @computed_field # type: ignore[misc] diff --git a/goosebit/ui/bff/devices/responses.py b/goosebit/ui/bff/devices/responses.py index 631c9608..9a06f9bc 100644 --- a/goosebit/ui/bff/devices/responses.py +++ b/goosebit/ui/bff/devices/responses.py @@ -21,11 +21,15 @@ async def convert(cls, dt_query: DataTableRequest, query: QuerySet, search_filte if dt_query.search.value: query = query.filter(search_filter(dt_query.search.value)) + filtered_records = await query.count() + if dt_query.order_query: query = query.order_by(dt_query.order_query) - filtered_records = await query.count() - devices = await query.offset(dt_query.start).limit(dt_query.length).all() + if dt_query.length is not None: + query = query.limit(dt_query.length) + + devices = await query.offset(dt_query.start).all() data = [DeviceSchema.model_validate(d) for d in devices] return cls(data=data, draw=dt_query.draw, records_total=total_records, records_filtered=filtered_records) diff --git a/goosebit/ui/bff/rollouts/responses.py b/goosebit/ui/bff/rollouts/responses.py index afa2c5e7..68211f12 100644 --- a/goosebit/ui/bff/rollouts/responses.py +++ b/goosebit/ui/bff/rollouts/responses.py @@ -19,11 +19,15 @@ async def convert(cls, dt_query: DataTableRequest, query: QuerySet, search_filte if dt_query.search.value: query = query.filter(search_filter(dt_query.search.value)) + filtered_records = await query.count() + if dt_query.order_query: query = query.order_by(dt_query.order_query) - filtered_records = await query.count() - rollouts = await query.offset(dt_query.start).limit(dt_query.length).all() + if dt_query.length is not None: + query = query.limit(dt_query.length) + + rollouts = await query.offset(dt_query.start).all() data = [RolloutSchema.model_validate(r) for r in rollouts] return cls(data=data, draw=dt_query.draw, records_total=total_records, records_filtered=filtered_records) diff --git a/goosebit/ui/bff/software/responses.py b/goosebit/ui/bff/software/responses.py index 53b96312..863d283c 100644 --- a/goosebit/ui/bff/software/responses.py +++ b/goosebit/ui/bff/software/responses.py @@ -21,17 +21,15 @@ async def convert(cls, dt_query: DataTableRequest, query: QuerySet, search_filte if dt_query.search.value: query = query.filter(search_filter(dt_query.search.value)) - if dt_query.order_query: - query = query.order_by(dt_query.order_query) - filtered_records = await query.count() - query = query.offset(dt_query.start) + if dt_query.order_query: + query = query.order_by(dt_query.order_query) - if not dt_query.length == 0: + if dt_query.length is not None: query = query.limit(dt_query.length) - software = await query.all() + software = await query.offset(dt_query.start).all() data = [SoftwareSchema.model_validate(s) for s in software] return cls(data=data, draw=dt_query.draw, records_total=total_records, records_filtered=filtered_records) From 1c240e9769c572ede7b2d0d7d81c48edcb4f47ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcel=20M=C3=BCller?= Date: Sat, 5 Oct 2024 17:58:33 +0200 Subject: [PATCH 3/8] Fix data query in devices tab Trailing slash caused a 307 which then got followed. So always two requests instead of one. --- goosebit/ui/static/js/devices.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/goosebit/ui/static/js/devices.js b/goosebit/ui/static/js/devices.js index d71e75d1..00d07465 100644 --- a/goosebit/ui/static/js/devices.js +++ b/goosebit/ui/static/js/devices.js @@ -14,7 +14,7 @@ document.addEventListener("DOMContentLoaded", async () => { select: true, rowId: "uuid", ajax: { - url: "/ui/bff/devices/", + url: "/ui/bff/devices", contentType: "application/json", }, initComplete: () => { From 111f1da9a24e8670412de0ae3c063076d5f03b9f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcel=20M=C3=BCller?= Date: Sat, 5 Oct 2024 18:11:05 +0200 Subject: [PATCH 4/8] Simplify DataTables queries Don't send all the query parameters that won't be processed. This now requires to set the name property for columns that are orderable. --- goosebit/ui/bff/common/requests.py | 16 ++-------------- goosebit/ui/static/js/devices.js | 14 +++++++++----- goosebit/ui/static/js/rollouts.js | 8 ++++++-- goosebit/ui/static/js/software.js | 4 ++++ 4 files changed, 21 insertions(+), 21 deletions(-) diff --git a/goosebit/ui/bff/common/requests.py b/goosebit/ui/bff/common/requests.py index 0ef6b26e..4d609556 100644 --- a/goosebit/ui/bff/common/requests.py +++ b/goosebit/ui/bff/common/requests.py @@ -10,14 +10,6 @@ class DataTableSearchSchema(BaseModel): regex: bool | None = False -class DataTableColumnSchema(BaseModel): - data: str | None - name: str | None = None - searchable: bool | None = None - orderable: bool | None = None - search: DataTableSearchSchema = DataTableSearchSchema() - - class DataTableOrderDirection(StrEnum): ASCENDING = "asc" DESCENDING = "desc" @@ -36,7 +28,6 @@ def direction(self) -> str: class DataTableRequest(BaseModel): draw: int = 1 - columns: list[DataTableColumnSchema] = list() order: list[DataTableOrderSchema] = list() start: int = 0 length: int | None = None @@ -46,11 +37,8 @@ class DataTableRequest(BaseModel): @property def order_query(self) -> str | None: try: - column = self.order[0].column - if column is None: - return None - if self.columns[column].name is None: + if len(self.order) == 0 or self.order[0].direction is None or self.order[0].name is None: return None - return f"{self.order[0].direction}{self.columns[column].data}" + return f"{self.order[0].direction}{self.order[0].name}" except LookupError: return None diff --git a/goosebit/ui/static/js/devices.js b/goosebit/ui/static/js/devices.js index 00d07465..a42776c2 100644 --- a/goosebit/ui/static/js/devices.js +++ b/goosebit/ui/static/js/devices.js @@ -15,6 +15,10 @@ document.addEventListener("DOMContentLoaded", async () => { rowId: "uuid", ajax: { url: "/ui/bff/devices", + data: (data) => { + // biome-ignore lint/performance/noDelete: really has to be deleted + delete data.columns; + }, contentType: "application/json", }, initComplete: () => { @@ -44,14 +48,14 @@ document.addEventListener("DOMContentLoaded", async () => { }, }, { data: "uuid", name: "uuid", searchable: true, orderable: true }, - { data: "name", searchable: true, orderable: true }, + { data: "name", name: "name", searchable: true, orderable: true }, { data: "hw_model" }, { data: "hw_revision" }, - { data: "feed", searchable: true, orderable: true }, - { data: "sw_version", searchable: true, orderable: true }, + { data: "feed", name: "feed", searchable: true, orderable: true }, + { data: "sw_version", name: "sw_version", searchable: true, orderable: true }, { data: "sw_target_version" }, - { data: "update_mode", searchable: true, orderable: true }, - { data: "last_state", searchable: true, orderable: true }, + { data: "update_mode", name: "update_mode", searchable: true, orderable: true }, + { data: "last_state", name: "last_state", searchable: true, orderable: true }, { data: "force_update", render: (data, type) => { diff --git a/goosebit/ui/static/js/rollouts.js b/goosebit/ui/static/js/rollouts.js index 46798b14..dca62430 100644 --- a/goosebit/ui/static/js/rollouts.js +++ b/goosebit/ui/static/js/rollouts.js @@ -18,6 +18,10 @@ document.addEventListener("DOMContentLoaded", async () => { rowId: "id", ajax: { url: "/ui/bff/rollouts", + data: (data) => { + // biome-ignore lint/performance/noDelete: really has to be deleted + delete data.columns; + }, contentType: "application/json", }, initComplete: () => { @@ -38,8 +42,8 @@ document.addEventListener("DOMContentLoaded", async () => { orderable: true, render: (data) => new Date(data).toLocaleString(), }, - { data: "name", searchable: true, orderable: true }, - { data: "feed", searchable: true, orderable: true }, + { data: "name", name: "name", searchable: true, orderable: true }, + { data: "feed", name: "feed", searchable: true, orderable: true }, { data: "sw_file" }, { data: "sw_version" }, { diff --git a/goosebit/ui/static/js/software.js b/goosebit/ui/static/js/software.js index fa6cea6d..8cb34f36 100644 --- a/goosebit/ui/static/js/software.js +++ b/goosebit/ui/static/js/software.js @@ -182,6 +182,10 @@ document.addEventListener("DOMContentLoaded", () => { stateSave: true, ajax: { url: "/ui/bff/software", + data: (data) => { + // biome-ignore lint/performance/noDelete: really has to be deleted + delete data.columns; + }, contentType: "application/json", }, initComplete: () => { From ba45f9827ca47d69c6d1e7224049404b9585bb2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcel=20M=C3=BCller?= Date: Sat, 5 Oct 2024 16:51:34 +0200 Subject: [PATCH 5/8] Add simple tests for GET /ui/bff/devices --- conftest.py | 34 +++++++++++++++++++---------- tests/ui/bff/devices/__init__.py | 0 tests/ui/bff/devices/test_routes.py | 23 +++++++++++++++++++ 3 files changed, 45 insertions(+), 12 deletions(-) create mode 100644 tests/ui/bff/devices/__init__.py create mode 100644 tests/ui/bff/devices/test_routes.py diff --git a/conftest.py b/conftest.py index fbcc9b8c..365eb4c3 100644 --- a/conftest.py +++ b/conftest.py @@ -70,14 +70,7 @@ async def test_data(db): # Create a temporary directory with tempfile.TemporaryDirectory() as temp_dir: - compatibility = await Hardware.create(model="default", revision="default") - - device_rollout = await Device.create( - uuid="device1", - last_state=UpdateStateEnum.REGISTERED, - update_mode=UpdateModeEnum.ROLLOUT, - hardware=compatibility, - ) + hardware = await Hardware.create(model="default", revision="default") temp_file_path = os.path.join(temp_dir, "software") with open(temp_file_path, "w") as temp_file: @@ -90,7 +83,7 @@ async def test_data(db): size=800, uri=uri, ) - await software_beta.compatibility.add(compatibility) + await software_beta.compatibility.add(hardware) software_release = await Software.create( version="1", @@ -98,7 +91,7 @@ async def test_data(db): size=1200, uri=uri, ) - await software_release.compatibility.add(compatibility) + await software_release.compatibility.add(hardware) software_rc = await Software.create( version="1.0.0-rc2+build77", @@ -106,14 +99,31 @@ async def test_data(db): size=800, uri=uri, ) - await software_rc.compatibility.add(compatibility) + await software_rc.compatibility.add(hardware) rollout_default = await Rollout.create(software_id=software_release.id) + device_rollout = await Device.create( + uuid="device1", + last_state=UpdateStateEnum.REGISTERED, + update_mode=UpdateModeEnum.ROLLOUT, + hardware=hardware, + ) + + device_assigned = await Device.create( + uuid="device2", + last_state=UpdateStateEnum.REGISTERED, + update_mode=UpdateModeEnum.ASSIGNED, + assigned_software=software_release, + hardware=hardware, + ) + yield dict( - device_rollout=device_rollout, + hardware=hardware, software_release=software_release, software_rc=software_rc, software_beta=software_beta, rollout_default=rollout_default, + device_rollout=device_rollout, + device_assigned=device_assigned, ) diff --git a/tests/ui/bff/devices/__init__.py b/tests/ui/bff/devices/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/ui/bff/devices/test_routes.py b/tests/ui/bff/devices/test_routes.py new file mode 100644 index 00000000..b301bea8 --- /dev/null +++ b/tests/ui/bff/devices/test_routes.py @@ -0,0 +1,23 @@ +import pytest + + +@pytest.mark.asyncio +async def test_list_devices_uuid_asc(async_client, test_data): + response = await async_client.get(f"/ui/bff/devices?order[0][dir]=asc&order[0][name]=uuid") + + assert response.status_code == 200 + devices = response.json()["data"] + assert len(devices) == 2 + assert devices[0]["uuid"] == test_data["device_rollout"].uuid + assert devices[1]["uuid"] == test_data["device_assigned"].uuid + + +@pytest.mark.asyncio +async def test_list_devices_uuid_desc(async_client, test_data): + response = await async_client.get(f"/ui/bff/devices?order[0][dir]=desc&order[0][name]=uuid") + + assert response.status_code == 200 + devices = response.json()["data"] + assert len(devices) == 2 + assert devices[0]["uuid"] == test_data["device_assigned"].uuid + assert devices[1]["uuid"] == test_data["device_rollout"].uuid From d2816fa0db9a931ec378ac5bbd533874074f1a32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcel=20M=C3=BCller?= Date: Sat, 5 Oct 2024 18:52:22 +0200 Subject: [PATCH 6/8] Fix GET /api/v1/devices for devices in assigned update mode --- goosebit/api/v1/devices/routes.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/goosebit/api/v1/devices/routes.py b/goosebit/api/v1/devices/routes.py index 9698f9ca..91a57393 100644 --- a/goosebit/api/v1/devices/routes.py +++ b/goosebit/api/v1/devices/routes.py @@ -24,7 +24,7 @@ dependencies=[Security(validate_user_permissions, scopes=["device.read"])], ) async def devices_get(_: Request) -> DevicesResponse: - devices = await Device.all().prefetch_related("assigned_software", "hardware") + devices = await Device.all().prefetch_related("hardware", "assigned_software", "assigned_software__compatibility") response = DevicesResponse(devices=devices) async def set_assigned_sw(d: DeviceSchema): From d9cd3a01abbdb1565ce88a81f6b52a286c51d8d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcel=20M=C3=BCller?= Date: Sat, 5 Oct 2024 18:56:34 +0200 Subject: [PATCH 7/8] Fix software sorting by version In order to display the exact same order as the "latest" software update mode uses the best approach is to do in-memory ordering of all software. This is only used in the software tab of the UI so the performance penalty should be ok. --- goosebit/db/models.py | 5 +++++ goosebit/ui/bff/software/responses.py | 24 ++++++++++++++++++------ tests/ui/bff/software/__init__.py | 0 tests/ui/bff/software/test_routes.py | 25 +++++++++++++++++++++++++ 4 files changed, 48 insertions(+), 6 deletions(-) create mode 100644 tests/ui/bff/software/__init__.py create mode 100644 tests/ui/bff/software/test_routes.py diff --git a/goosebit/db/models.py b/goosebit/db/models.py index e9f1465f..623d19f9 100644 --- a/goosebit/db/models.py +++ b/goosebit/db/models.py @@ -7,6 +7,7 @@ import semver from anyio import Path +from semver import Version from tortoise import Model, fields from tortoise.exceptions import ValidationError @@ -156,3 +157,7 @@ def path_user(self) -> str: return self.path.name else: return self.uri + + @property + def parsed_version(self) -> Version: + return semver.Version.parse(self.version, optional_minor_and_patch=True) diff --git a/goosebit/ui/bff/software/responses.py b/goosebit/ui/bff/software/responses.py index 863d283c..169f064f 100644 --- a/goosebit/ui/bff/software/responses.py +++ b/goosebit/ui/bff/software/responses.py @@ -5,7 +5,7 @@ from tortoise.queryset import QuerySet from goosebit.schema.software import SoftwareSchema -from goosebit.ui.bff.common.requests import DataTableRequest +from goosebit.ui.bff.common.requests import DataTableOrderDirection, DataTableRequest class BFFSoftwareResponse(BaseModel): @@ -23,13 +23,25 @@ async def convert(cls, dt_query: DataTableRequest, query: QuerySet, search_filte filtered_records = await query.count() - if dt_query.order_query: - query = query.order_by(dt_query.order_query) + if len(dt_query.order) > 0 and dt_query.order[0].name == "version": + # ordering cannot be delegated to database as semantic versioning sorting is not supported + software = await query.all() + reverse = dt_query.order[0].dir == DataTableOrderDirection.DESCENDING + software.sort(key=lambda s: s.parsed_version, reverse=reverse) - if dt_query.length is not None: - query = query.limit(dt_query.length) + # in-memory paging + if dt_query.length is None: + software = software[dt_query.start :] + else: + software = software[dt_query.start : dt_query.start + dt_query.length] + + else: + # if no ordering is specified, database-side paging can be used + if dt_query.length is not None: + query = query.limit(dt_query.length) + + software = await query.offset(dt_query.start).all() - software = await query.offset(dt_query.start).all() data = [SoftwareSchema.model_validate(s) for s in software] return cls(data=data, draw=dt_query.draw, records_total=total_records, records_filtered=filtered_records) diff --git a/tests/ui/bff/software/__init__.py b/tests/ui/bff/software/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/ui/bff/software/test_routes.py b/tests/ui/bff/software/test_routes.py new file mode 100644 index 00000000..a0813765 --- /dev/null +++ b/tests/ui/bff/software/test_routes.py @@ -0,0 +1,25 @@ +import pytest + + +@pytest.mark.asyncio +async def test_list_software_version_asc(async_client, test_data): + response = await async_client.get(f"/ui/bff/software?order[0][dir]=asc&order[0][name]=version") + + assert response.status_code == 200 + software = response.json()["data"] + assert len(software) == 3 + assert software[0]["version"] == test_data["software_beta"].version + assert software[1]["version"] == test_data["software_rc"].version + assert software[2]["version"] == test_data["software_release"].version + + +@pytest.mark.asyncio +async def test_list_software_version_desc(async_client, test_data): + response = await async_client.get(f"/ui/bff/software?order[0][dir]=desc&order[0][name]=version") + + assert response.status_code == 200 + software = response.json()["data"] + assert len(software) == 3 + assert software[0]["version"] == test_data["software_release"].version + assert software[1]["version"] == test_data["software_rc"].version + assert software[2]["version"] == test_data["software_beta"].version From 19ccb2229dba7da2e17c41cf679cfd34159f4375 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcel=20M=C3=BCller?= Date: Sat, 5 Oct 2024 18:58:46 +0200 Subject: [PATCH 8/8] Remove default ordering of software by version Prevent the expensive in-memory sorting if user does not need it. --- goosebit/ui/static/js/software.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/goosebit/ui/static/js/software.js b/goosebit/ui/static/js/software.js index 8cb34f36..84dad5f6 100644 --- a/goosebit/ui/static/js/software.js +++ b/goosebit/ui/static/js/software.js @@ -172,10 +172,6 @@ document.addEventListener("DOMContentLoaded", () => { paging: true, processing: false, serverSide: true, - order: { - name: "version", - dir: "desc", - }, scrollCollapse: true, scroller: true, scrollY: "60vh",