[change] Reduce sortedm2m queries for templates #204 #1010

DragnEmperor · 2025-04-23T19:27:45Z

Checklist

I have read the OpenWISP Contributing Guidelines.
I have manually tested the changes proposed in this pull request.
I have written new test cases for new code and/or updated existing tests for changes to existing code.
I have updated the documentation.

Reference to Existing Issue

Closes #204
Closes #1050

Description of Changes

To remove queries generated by sortedm2m, 'formfield_for_manytomany' is overridden such that on page load, we can get templates using 'get_relevant_templates'.
Added device/group id in 'get_relevant_templates' to identify selected templates. Changes are made in 'relevantTemplates.js' to ensure templates are viewed and changes are saved as per initial behavior.

As now we are fetching templates via js only, I have done some modifications for the failing test cases.

for test case test_device_templates_m2m_queryset there is a similar selenium test case test_multiple_organization_templates
for test cases test_configuration_templates_removed , test_vpn_clients_deleted, have added new selenium test cases test_add_remove_templates, test_vpn_clients_deleted.

DragnEmperor · 2025-04-23T19:29:55Z

Known Issues:

Ordering is not maintained (done)

devkapilbansal · 2025-05-05T18:19:16Z

Hi @DragnEmperor
I see that the tests are failing. You can keep a PR in draft state unless it is completed

coveralls · 2025-05-11T03:20:05Z

coverage: 98.777% (-0.04%) from 98.816%
when pulling ccdf7ea on DragnEmperor:issues/204-sortedm2m-queries
into 85eee35 on openwisp:master.

DragnEmperor · 2025-05-11T13:27:58Z

As now we are fetching templates via js only, I have done some modifications for the failing test cases.

for test case test_device_templates_m2m_queryset there is a similar selenium test case test_multiple_organization_templates
for test cases test_configuration_templates_removed , test_vpn_clients_deleted, have added new selenium test cases test_add_remove_templates, test_vpn_clients_deleted.

devkapilbansal · 2025-05-16T17:25:59Z

openwisp_controller/config/tests/test_admin.py

@@ -479,15 +479,6 @@ def test_device_organization_fk_autocomplete_view(self):
            hidden=[data['org2'].name, data['inactive'].name],
        )

-    def test_device_templates_m2m_queryset(self):


Please mention in description why these tests are removed from here

Hi Kapil,
I have added in the commit description and in the comment above the reason for removing these test cases.

nemesifier

Are you asserting the number of queries to verify the optimizations? Or what are you doing to ensure that the optimization is effective within automated tests?

nemesifier · 2025-05-17T19:08:28Z

openwisp_controller/config/tests/test_admin.py

@@ -1626,89 +1567,6 @@ def test_add_vpn(self):
            response, 'value="openwisp_controller.vpn_backends.OpenVpn" selected'
        )

-    def test_vpn_clients_deleted(self):


why are these tests being moved?

As we are adding the templates via js so i migrated these tests to selenium tests. I have added this in conversation above. I will update the description of PR itself for more clarity.

DragnEmperor · 2025-05-17T19:20:27Z

Are you asserting the number of queries to verify the optimizations? Or what are you doing to ensure that the optimization is effective within automated tests?

Yes I have checked the queries and while there is an improvement its not much i think ( around 4-5 queries less now, some queries' results are cached so i am not exactly sure of the count)
I have not added the tests for asserting number of queries. That would be something I need to work on, Thanks!

DragnEmperor · 2025-05-20T18:54:16Z

@nemesifier @pandafy
Upon inspection of the queries executed, the following query was being duplicated twice leading to total count of 26.

SELECT "config_template"."id", "config_template"."created", "config_template"."modified", "config_template"."name", "config_template"."backend", "config_template"."config", "config_template"."organization_id", "config_template"."vpn_id", "config_template"."type", "config_template"."default", "config_template"."required", "config_template"."auto_cert", "config_template"."default_values" FROM "config_template" ORDER BY "config_template"."name" ASC

With the changes, templates are being added via js using get_relevant_templates so this query is not executed, bringing the count to 24.

pandafy · 2025-05-26T19:20:57Z

openwisp_controller/config/tests/test_admin.py

+    # ensuring queries are consistent for different number of templates
+    def test_templates_fetch_queries_1(self):
+        config = self._create_config(organization=self._get_org())
+        self._verify_template_queries(config, 1)
+
+    def test_templates_fetch_queries_5(self):
+        config = self._create_config(organization=self._get_org())
+        self._verify_template_queries(config, 1)
+
+    def test_templates_fetch_queries_10(self):
+        config = self._create_config(organization=self._get_org())
+        self._verify_template_queries(config, 1)
+


@DragnEmperor can you help me understand why do we need three tests?

I have added these checks to ensure that queries are not changed and remain the same irrespective of number of templates

The count argument is 1 in all the tests. Is that correct?

Yes that was my mistake, these should be correct in new commit.

nemesifier

Please merge with the latest master and run openwisp-qa-format again.
I see some changes from other PRs that have already been merged which are showing up again here, I hope that updating on master and running openwisp-qa-format fixes that, if not please look into resolving it as it makes reviewing tougher with no added benefits.

nemesifier

There's one failing test: test_multiple_organization_templates.

@DragnEmperor thank you for your effort! You can leave this to us now and focus on the other work you have.

nemesifier · 2025-05-29T22:08:58Z

openwisp_controller/config/views.py

+                .only("templates")
+                .get(pk=config_id)
+                .templates.filter(org_filters)
+                .filter(**filter_options)


I spotted something that is worrying me: we aren't enforcing/asserting that this config is really assigned to a device which is related to organization_id. In theory it would be possible to call an organization_id which is valid and a config_id which shouldn't be allowed.

So I think filter options should include device__organization_id=organization_id, what do you think?

Watch out from query count increase due to this.

Consider the following scenario:

There are two organizations: OrgA and OrgB. OrgA has a device that is linked to a configuration object.

A user attempts to change the organization of this device from OrgA to OrgB. At this point, the organization_id URL keyword argument contains the UUID of OrgB. However, since the change hasn't been saved yet, the device object in the database is still associated with OrgA.

If we filter the Config queryset using OrgB (based on your suggestion), it will raise a Config.DoesNotExist error, because the configuration is still linked to OrgA. As a result, we won’t be able to retrieve information about the current state of shared configuration templates.

nemesifier · 2025-05-29T22:13:27Z

openwisp_controller/config/views.py

+                .only("templates")
+                .get(pk=group_id)
+                .templates.filter(org_filters)
+                .filter(**filter_options)


What's the value of filter_options here? Can we add organization_id=organization_id here?

nemesifier · 2025-06-04T22:02:57Z

openwisp_controller/config/static/config/js/relevant_templates.js

+      if (isDeviceGroup() && !$(".add-form").length) {
+        // Get the group id from the URL
+        // TODO: This is fragile, consider using a more robust way to get the group id.
+        var pathParts = window.location.pathname.split("/");


We are overriding /templates/admin/device_group/change_form.html, so why don't we add the group ID there as an HTML element which is easy to retrieve here?

pandafy · 2025-06-05T11:49:20Z

openwisp_controller/config/templates/admin/device_group/change_form.html

-                window._deviceGroup = true;
+                window._deviceGroupId = "{{ original.pk }}";


We were already setting the _devieGroup property here to determine whether the relevant_template,js script was loading from device page or device group page. I have updated the logic to pass the device group ID to the JS.

@nemesifier if you prefer an HTML element, I would update the code accordingly.

To remove queries generated by sortedm2m, 'formfield_for_manytomany' is overriden such that on page loads, we can get templates using 'get_relevant_templates'. Added device/group id to identify selected templates. Changes are made in JS to ensure templates are viewed and changes are saved as per initial behaviour. Ordering is not maintained. Closes openwisp#204 Signed-off-by: DragnEmperor <[email protected]>

Using the 'sort_value' of the 'through_model', created a subquery which annotates a sort_value and selected field to each template. The sort_value is used for ordering the templates and also for checking if template is selected or not using a large default value which happens if template is not selected for a device config or group. Signed-off-by: DragnEmperor <[email protected]>

As now templates are being loaded via js, normal tests were failing. For these failing tests, added relevant selenium tests. Modified `get-relevant_templates` to allow fetching templates during device creation by superuser and org users. Signed-off-by: DragnEmperor <[email protected]>

Added a test to assert the queries executed during templates fetch on device change page. Previous count of this fetch combined with `get_relevant_templates` was 26 due to template fetch query being duplicated. With new changes the count is down to 24. Signed-off-by: DragnEmperor <[email protected]>

The queries for fetching the templates with sorted in `get_relevant_templates` via Subquery, while were working, were a bit complex to understand for future. Fetching templates using config/devicegroup maintains sorting, making the fetching query much readable. Signed-off-by: DragnEmperor <[email protected]>

Formatted the changes as per the new QA checks and rules in `openwisp-qa-format`. For the tests inside `test_views` to work properly, made minor modifications in `get_relevant_templates`. Also fixed the type in template count in `test_templates_fetch_queries`. Signed-off-by: DragnEmperor <[email protected]>

DragnEmperor force-pushed the issues/204-sortedm2m-queries branch from b7e4d02 to 46f4a12 Compare April 29, 2025 17:41

DragnEmperor mentioned this pull request Apr 30, 2025

[change] Optimize queries of sortedm2m #204

Open

DragnEmperor marked this pull request as draft May 6, 2025 02:31

codesankalp self-requested a review May 10, 2025 04:54

DragnEmperor force-pushed the issues/204-sortedm2m-queries branch from d29a35b to 9088448 Compare May 10, 2025 13:07

DragnEmperor marked this pull request as ready for review May 11, 2025 12:37

devkapilbansal self-requested a review May 16, 2025 17:22

devkapilbansal reviewed May 16, 2025

View reviewed changes

devkapilbansal self-requested a review May 16, 2025 17:26

nemesifier reviewed May 17, 2025

View reviewed changes

DragnEmperor force-pushed the issues/204-sortedm2m-queries branch from c98b148 to 4f85589 Compare May 20, 2025 18:49

pandafy reviewed May 26, 2025

View reviewed changes

nemesifier assigned pandafy and DragnEmperor May 26, 2025

nemesifier reviewed May 26, 2025

View reviewed changes

DragnEmperor force-pushed the issues/204-sortedm2m-queries branch from c8856a4 to cb3aac4 Compare May 27, 2025 09:38

pandafy added this to OpenWISP Contributor's Board May 27, 2025

github-project-automation bot moved this to To do (general) in OpenWISP Contributor's Board May 27, 2025

nemesifier reviewed May 28, 2025

View reviewed changes

nemesifier previously approved these changes May 29, 2025

View reviewed changes

github-project-automation bot moved this from To do (general) to In progress in OpenWISP Contributor's Board May 29, 2025

pandafy dismissed nemesifier’s stale review via 96f0c5e May 30, 2025 18:34

nemesifier reviewed Jun 4, 2025

View reviewed changes

pandafy reviewed Jun 5, 2025

View reviewed changes

DragnEmperor and others added 12 commits June 9, 2025 15:37

[change] Improved relevant templates

12cedbe

[fix] Fixed relevant templates logic

51fac6b

[tests] Fixed tests

44a5d4b

[qa] Fixed formatting

bd315ff

[req-change] Made retrieving group id robust

1332f2b

[req-change] Made retrieving group id robust

4e16424

pandafy force-pushed the issues/204-sortedm2m-queries branch 2 times, most recently from c082f0e to b00bcc8 Compare June 9, 2025 12:58

[fix] Fixed unsaved changes alert on device page

b4d0b93

pandafy force-pushed the issues/204-sortedm2m-queries branch from b00bcc8 to b4d0b93 Compare June 9, 2025 13:06

[fix] Fixed default values event handler

ccdf7ea

pandafy force-pushed the issues/204-sortedm2m-queries branch from 43899e1 to ccdf7ea Compare June 12, 2025 08:36

		window._deviceGroup = true;
		window._deviceGroupId = "{{ original.pk }}";

Uh oh!

[change] Reduce sortedm2m queries for templates #204 #1010

Are you sure you want to change the base?

[change] Reduce sortedm2m queries for templates #204 #1010

Uh oh!

Conversation

DragnEmperor commented Apr 23, 2025 • edited by pandafy Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Reference to Existing Issue

Description of Changes

Uh oh!

DragnEmperor commented Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devkapilbansal commented May 5, 2025

Uh oh!

coveralls commented May 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DragnEmperor commented May 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DragnEmperor May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DragnEmperor commented May 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DragnEmperor commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pandafy May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DragnEmperor commented Apr 23, 2025 •

edited by pandafy

Loading

DragnEmperor commented Apr 23, 2025 •

edited

Loading

coveralls commented May 11, 2025 •

edited

Loading

DragnEmperor May 16, 2025 •

edited

Loading

DragnEmperor commented May 17, 2025 •

edited

Loading

DragnEmperor commented May 20, 2025 •

edited

Loading

pandafy May 26, 2025 •

edited

Loading