New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Disable default ordering in GraphQL queries #5380

Merged

LucasG0 merged 4 commits into stable from dga-20250103-node-list-order

Jan 8, 2025

Contributor

LucasG0 commented Jan 6, 2025 •

edited

Loading

Implements #5376
Later PR will come to support it within SDK

github-actions bot added the group/backend label

codspeed-hq bot commented Jan 6, 2025 •

edited

Loading

CodSpeed Performance Report

Merging #5380 will not alter performance

_{Comparing dga-20250103-node-list-order (5a4d686) with stable (c67b7a0)}

Summary

✅ 10 untouched benchmarks

LucasG0 force-pushed the dga-20250103-node-list-order branch 2 times, most recently from 772f747 to b9e863d Compare

January 6, 2025 13:30

LucasG0 marked this pull request as ready for review

January 6, 2025 14:03

LucasG0 requested a review from a team

January 6, 2025 14:04

ajtmccarty approved these changes

View reviewed changes

backend/tests/query_benchmark/test_node_get_list.py Outdated Show resolved Hide resolved

backend/tests/unit/core/test_node_get_list_query.py Outdated Show resolved Hide resolved

dgarros reviewed

View reviewed changes

Collaborator

dgarros left a comment

Great work.
Instead of passing a dict to the function I think it would be best to use a proper dataclass or Pydantic model this way it will be easier to support additional option in the future.

backend/tests/unit/graphql/queries/test_order.py Outdated Show resolved Hide resolved

backend/infrahub/core/manager.py Outdated Show resolved Hide resolved

backend/infrahub/core/query/node.py Show resolved Hide resolved

LucasG0 force-pushed the dga-20250103-node-list-order branch from b9e863d to 5eb2d19 Compare

January 6, 2025 17:48

LucasG0 and others added 3 commits

January 6, 2025 19:14


          Set PREFECT_API_URL correctly during tests

3986dcf


          Add parameters to NodeGetListQuery to disable ordering

5f4bf6c


          Add node_get_list benchmark

24270c2

# Conflicts:
#	backend/infrahub/core/query/node.py

LucasG0 force-pushed the dga-20250103-node-list-order branch from 5eb2d19 to 24270c2 Compare

January 6, 2025 18:17

LucasG0 requested a review from dgarros

January 6, 2025 18:17

LucasG0 commented

View reviewed changes

backend/infrahub/core/query/node.py Show resolved Hide resolved

dgarros reviewed

View reviewed changes

backend/infrahub/core/query/node.py Outdated

                           self.params["uuid"] = self.filters["id"]
                       elif not self.has_filters and not self.schema.order_by:
                           use_simple = True
-                          self.order_by = ["n.uuid"]
+                          if self.order is None or self.order.disable is not True:

Collaborator

dgarros Jan 6, 2025

this logic looks surprising to me ...

backend/infrahub/core/query/node.py Outdated

-                              continue
-                          self.order_by.append(far.node_value_query_variable)
-                      self.order_by.append("n.uuid")
+                      if should_order:

Collaborator

dgarros Jan 6, 2025

even if the order is disabled we should also order by uuid to guarantee the pagination

Contributor Author

LucasG0 Jan 7, 2025

Ok I did not have that in mind

Contributor

ogenstad Jan 7, 2025

Some additional context for the self.order_by.append("n.uuid"): #4704

LucasG0 commented

View reviewed changes

backend/infrahub/core/query/node.py

+                              if far.supports_profile:
+                                  self.order_by.append(far.final_value_query_variable)
+                                  continue
+                              self.order_by.append(far.node_value_query_variable)

Contributor Author

LucasG0 Jan 7, 2025

I let this block within should_order branch condition as this block seems only related to ordering.

LucasG0 requested a review from dgarros

January 7, 2025 09:38

LucasG0 force-pushed the dga-20250103-node-list-order branch from 936b255 to 47bcab8 Compare

January 7, 2025 09:39

ogenstad reviewed

View reviewed changes

backend/tests/query_benchmark/test_node_get_list.py Outdated Show resolved Hide resolved

LucasG0 force-pushed the dga-20250103-node-list-order branch from 47bcab8 to 3f53022 Compare

January 7, 2025 13:47

dgarros reviewed

View reviewed changes

backend/infrahub/core/query/node.py Outdated

-                          use_simple = True
+                          self.add_to_query(" AND n.uuid = $uuid")
+                          return
+                      if not self.has_filters and not self.schema.order_by:

Collaborator

dgarros Jan 7, 2025

should we also check if order is disabled here and exit early ?

Contributor Author

LucasG0 Jan 8, 2025

If this condition is met, we set self.order_by to ["n.uuid"] in order to guarantee pagination, and we exit early. As schema.order_by is empty, it feels like we do not even need to check for order?

LucasG0 force-pushed the dga-20250103-node-list-order branch 2 times, most recently from f5a8a5f to 928bb42 Compare

January 8, 2025 12:17


          Fix ordering logic

5a4d686

LucasG0 force-pushed the dga-20250103-node-list-order branch from 928bb42 to 5a4d686 Compare

January 8, 2025 12:19

dgarros approved these changes

View reviewed changes

Collaborator

dgarros left a comment

great work

LucasG0 merged commit 10f150b into stable

34 checks passed

LucasG0 deleted the dga-20250103-node-list-order branch

January 8, 2025 13:19

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels