From b8629372ba9a9b1dc4b088171d4e6ae318ff5ae8 Mon Sep 17 00:00:00 2001 From: Roy Nieterau Date: Sun, 13 Sep 2026 00:05:42 +0200 Subject: [PATCH 1/6] GraphQl: Prevent infinite query loop on unexpected responses - Response with 'data: null' did not change pagination state, so the same query was sent again forever. Raise GraphQlQueryError with the query instead. - Page reporting another page without a cursor reset the cursor to 'None', so pagination started again from the first page. Stop pagination of the field with a warning. Co-Authored-By: Claude Opus 5 --- ayon_api/graphql.py | 65 ++++--- tests/graphql_fake_server.py | 257 ++++++++++++++++++++++++++++ tests/test_graphql_infinite_loop.py | 46 +++++ 3 files changed, 341 insertions(+), 27 deletions(-) create mode 100644 tests/graphql_fake_server.py create mode 100644 tests/test_graphql_infinite_loop.py diff --git a/ayon_api/graphql.py b/ayon_api/graphql.py index 4feed17a0..acca87edb 100644 --- a/ayon_api/graphql.py +++ b/ayon_api/graphql.py @@ -1,6 +1,7 @@ from __future__ import annotations import copy +import logging import numbers from abc import ABC, abstractmethod import typing @@ -16,6 +17,8 @@ FIELD_VALUE = object() +log = logging.getLogger(__name__) + def fields_to_dict(fields: Iterable[str] | None) -> dict: output = {} @@ -353,6 +356,25 @@ def parse_result( for child in self._children: child.parse_result(data, output, progress_data) + def _query_data(self, con: ServerAPI) -> dict[str, Any]: + """Send single query to server and return 'data' of the response.""" + query_str = self.calculate_query() + variables = self.get_variables_values() + response = con.query_graphql(query_str, variables) + if response.errors: + raise GraphQlQueryFailed(response.errors, query_str, variables) + + data = response.data.get("data") + if data is None: + # Parsing 'None' would not change pagination state and the same + # query would be sent again in an infinite loop. + raise GraphQlQueryError( + f"GraphQl query '{self._name}' response does not contain" + f" 'data'. Response: {str(response.data)[:1000]}" + f"\nQuery:\n{query_str}\nVariables: {variables}" + ) + return data + def query(self, con: ServerAPI) -> dict[str, Any]: """Do a query from server. @@ -366,15 +388,8 @@ def query(self, con: ServerAPI) -> dict[str, Any]: progress_data = {} output = {} while self.need_query: - query_str = self.calculate_query() - variables = self.get_variables_values() - response = con.query_graphql( - query_str, - variables - ) - if response.errors: - raise GraphQlQueryFailed(response.errors, query_str, variables) - self.parse_result(response.data["data"], output, progress_data) + data = self._query_data(con) + self.parse_result(data, output, progress_data) return output @@ -394,30 +409,16 @@ def continuous_query( if self.has_multiple_edge_fields: output = {} while self.need_query: - query_str = self.calculate_query() - variables = self.get_variables_values() - - response = con.query_graphql(query_str, variables) - if response.errors: - raise GraphQlQueryFailed( - response.errors, query_str, variables - ) - self.parse_result(response.data["data"], output, progress_data) + data = self._query_data(con) + self.parse_result(data, output, progress_data) yield output else: while self.need_query: output = {} - query_str = self.calculate_query() - variables = self.get_variables_values() - response = con.query_graphql(query_str, variables) - if response.errors: - raise GraphQlQueryFailed( - response.errors, query_str, variables - ) - - self.parse_result(response.data["data"], output, progress_data) + data = self._query_data(con) + self.parse_result(data, output, progress_data) yield output @@ -943,6 +944,16 @@ def parse_result( change_cursor = False if change_cursor and self._need_query: + if new_cursor is None: + # Without cursor the pagination would start from beginning + log.warning( + "Field '%s' reported another page without a cursor." + " Stopping pagination after %s items.", + self.path, self._fetched_counter, + ) + self._need_query = False + return + if new_cursor == self._cursor: raise GraphQlQueryError( "Cursor didn't change during pagination." diff --git a/tests/graphql_fake_server.py b/tests/graphql_fake_server.py new file mode 100644 index 000000000..cbe5a487a --- /dev/null +++ b/tests/graphql_fake_server.py @@ -0,0 +1,257 @@ +"""Minimal in-memory GraphQL server that speaks the subset of the schema +that 'ayon_api.graphql' generates. Used to exercise the pagination engine. +""" +import re +import json + + +class Node: + def __init__(self, name, args, children): + self.name = name + self.args = args + self.children = children + + def child(self, name): + for child in self.children: + if child.name == name: + return child + return None + + def __repr__(self): + return f"" + + +def _parse_args(args_str, variables): + """Parse 'first: 300, after: "x", ids: $ids' into a dict.""" + if not args_str: + return {} + out = {} + # split on top level commas + parts = [] + depth = 0 + in_str = False + current = "" + for char in args_str: + if in_str: + current += char + if char == '"': + in_str = False + continue + if char == '"': + in_str = True + current += char + continue + if char in "[{": + depth += 1 + elif char in "]}": + depth -= 1 + if char == "," and depth == 0: + parts.append(current) + current = "" + continue + current += char + if current.strip(): + parts.append(current) + + for part in parts: + key, _, value = part.partition(":") + key = key.strip() + value = value.strip() + if value.startswith("$"): + value = variables.get(value[1:]) + elif value.startswith('"'): + value = json.loads(value) + elif value.startswith("["): + value = json.loads(value) + elif value in ("true", "false"): + value = value == "true" + else: + value = int(value) + out[key] = value + return out + + +LINE_RE = re.compile( + r"^(?P\w+)(?:\((?P.*)\))?(?P\s*\{)?$" +) + + +def parse_query(query_str, variables): + lines = [ + line.strip() + for line in query_str.splitlines() + if line.strip() + ] + # Drop query header + assert lines[0].startswith("query"), lines[0] + root = Node("__root__", {}, []) + stack = [root] + for line in lines[1:]: + if line == "}": + stack.pop() + continue + match = LINE_RE.match(line) + if match is None: + raise ValueError(f"Unparsable line: {line!r}") + args = _parse_args(match.group("args"), variables) + node = Node(match.group("name"), args, []) + stack[-1].children.append(node) + if match.group("open"): + stack.append(node) + if stack: + raise ValueError("Unbalanced query") + return root + + +class FakeServer: + """Resolve a parsed query against plain python data. + + Data is a dict of entity collections, e.g.:: + + { + "project": { + "name": "proj", + "folders": [ + {"id": "f1", "name": "a", "links": [{"id": "l1"}]}, + ], + } + } + + Any list value is served as a connection (edges/pageInfo), any dict + value as a plain object. + + """ + def __init__( + self, + data, + cursor_func=None, + max_page_size=None, + reverse_last_pages=False, + ): + # AYON server returns edges of page queried with 'last' from + # the newest item + self._reverse_last_pages = reverse_last_pages + self._data = data + self._cursor_func = cursor_func or self._default_cursor + self._max_page_size = max_page_size + self.calls = [] + self.queries = [] + + @staticmethod + def _default_cursor(path, index, entity): + return f"{path}:{index}" + + def query_graphql(self, query_str, variables): + self.queries.append(query_str) + self.calls.append((query_str, dict(variables))) + root = parse_query(query_str, variables) + data = {} + for child in root.children: + data[child.name] = self._resolve(child, self._data, child.name) + return FakeResponse({"data": data}) + + def _resolve(self, node, parent_value, path): + value = parent_value.get(node.name) if parent_value else None + if isinstance(value, list): + return self._resolve_connection(node, value, path) + if isinstance(value, dict): + return self._resolve_object(node, value, path) + # leaf + if node.children: + raise ValueError( + f"Requested sub fields of leaf {path}" + ) + return value + + def _resolve_object(self, node, value, path): + out = {} + for child in node.children: + out[child.name] = self._resolve( + child, value, f"{path}/{child.name}" + ) + return out + + def _resolve_connection(self, node, items, path): + edges_field = node.child("edges") + if edges_field is None: + raise ValueError(f"Connection {path} misses 'edges'") + cursors = [ + self._cursor_func(path, idx, item) + for idx, item in enumerate(items) + ] + args = node.args + start = 0 + end = len(items) + reverse_paging = "last" in args + if "after" in args: + cursor = args["after"] + if cursor not in cursors: + raise ValueError( + f"Unknown 'after' cursor {cursor!r} for {path}" + ) + start = cursors.index(cursor) + 1 + if "before" in args: + cursor = args["before"] + if cursor not in cursors: + raise ValueError( + f"Unknown 'before' cursor {cursor!r} for {path}" + ) + end = cursors.index(cursor) + + limit = args.get("first", args.get("last")) + if limit is None: + raise ValueError(f"Missing 'first'/'last' for {path}") + if limit < 0: + raise ValueError(f"Negative page size {limit} for {path}") + if self._max_page_size is not None: + limit = min(limit, self._max_page_size) + + window = list(range(start, end)) + if reverse_paging: + page_idxs = window[-limit:] if limit else [] + else: + page_idxs = window[:limit] + + if reverse_paging and self._reverse_last_pages: + page_idxs.reverse() + + node_field = edges_field.child("node") + edges = [] + for idx in page_idxs: + item = items[idx] + edge = {} + edges.append(edge) + for child in edges_field.children: + if child.name == "node": + continue + if child.name == "cursor": + edge["cursor"] = cursors[idx] + continue + edge[child.name] = self._resolve( + child, item, f"{path}[{idx}]/{child.name}" + ) + if node_field is not None: + edge["node"] = self._resolve_object( + node_field, item, f"{path}[{idx}]" + ) + + has_next = bool(page_idxs) and max(page_idxs) < end - 1 + has_prev = bool(page_idxs) and min(page_idxs) > start + page_info = { + "endCursor": cursors[page_idxs[-1]] if page_idxs else None, + "startCursor": cursors[page_idxs[0]] if page_idxs else None, + "hasNextPage": has_next, + "hasPreviousPage": has_prev, + } + out = {"edges": edges, "pageInfo": {}} + requested_page_info = node.child("pageInfo") + if requested_page_info is not None: + for child in requested_page_info.children: + out["pageInfo"][child.name] = page_info[child.name] + return out + + +class FakeResponse: + def __init__(self, data): + self.data = data + self.errors = data.get("errors") diff --git a/tests/test_graphql_infinite_loop.py b/tests/test_graphql_infinite_loop.py new file mode 100644 index 000000000..cead2d221 --- /dev/null +++ b/tests/test_graphql_infinite_loop.py @@ -0,0 +1,46 @@ +"""Unexpected GraphQl responses must not cause infinite loop. + +Does not require running AYON server. +""" +import pytest + +from ayon_api.exceptions import GraphQlQueryError +from ayon_api.graphql_queries import events_graphql_query +from ayon_api.utils import SortOrder + +from .graphql_fake_server import FakeResponse + + +class _ScriptedServer: + """Return prepared responses, fail if queried too many times.""" + def __init__(self, responses): + self._responses = responses + self.queries = [] + + def query_graphql(self, query_str, variables): + self.queries.append(query_str) + if len(self.queries) > 10: + raise RuntimeError("Infinite query loop") + idx = min(len(self.queries), len(self._responses)) - 1 + return FakeResponse(self._responses[idx]) + + +def test_null_data_raises_instead_of_infinite_loop(): + server = _ScriptedServer([{"data": None}]) + query = events_graphql_query({"id"}, SortOrder.ascending) + with pytest.raises(GraphQlQueryError, match="does not contain 'data'"): + query.query(server) + + +def test_missing_cursor_stops_pagination(): + def page(ids, end_cursor): + return {"data": {"events": { + "edges": [{"node": {"id": id_}} for id_ in ids], + "pageInfo": {"endCursor": end_cursor, "hasNextPage": True}, + }}} + + server = _ScriptedServer([page(["e0"], "c0"), page([], None)]) + query = events_graphql_query({"id"}, SortOrder.ascending) + + assert query.query(server) == {"events": [{"id": "e0"}]} + assert len(server.queries) == 2 From f4abc47ead147da0770f4fac999b085157020303 Mon Sep 17 00:00:00 2001 From: Roy Nieterau Date: Mon, 14 Sep 2026 19:37:34 +0200 Subject: [PATCH 2/6] Apply batched suggestions from code review Co-authored-by: Jakub Trllo <43494761+iLLiCiTiT@users.noreply.github.com> --- ayon_api/graphql.py | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/ayon_api/graphql.py b/ayon_api/graphql.py index acca87edb..28f801647 100644 --- a/ayon_api/graphql.py +++ b/ayon_api/graphql.py @@ -1,7 +1,6 @@ from __future__ import annotations import copy -import logging import numbers from abc import ABC, abstractmethod import typing @@ -17,9 +16,6 @@ FIELD_VALUE = object() -log = logging.getLogger(__name__) - - def fields_to_dict(fields: Iterable[str] | None) -> dict: output = {} if not fields: @@ -946,13 +942,11 @@ def parse_result( if change_cursor and self._need_query: if new_cursor is None: # Without cursor the pagination would start from beginning - log.warning( - "Field '%s' reported another page without a cursor." - " Stopping pagination after %s items.", - self.path, self._fetched_counter, + raise GraphQlQueryError( + f"Field '{self.path}' reported another page without" + " a cursor. Stopped pagination after" + f" {self._fetched_counter} items." ) - self._need_query = False - return if new_cursor == self._cursor: raise GraphQlQueryError( From 5deee9e8cd52c21ec78b47027ce2e53134f945a7 Mon Sep 17 00:00:00 2001 From: Roy Nieterau Date: Mon, 14 Sep 2026 19:39:00 +0200 Subject: [PATCH 3/6] Cosmetics --- ayon_api/graphql.py | 1 + 1 file changed, 1 insertion(+) diff --git a/ayon_api/graphql.py b/ayon_api/graphql.py index 28f801647..89562dde6 100644 --- a/ayon_api/graphql.py +++ b/ayon_api/graphql.py @@ -16,6 +16,7 @@ FIELD_VALUE = object() + def fields_to_dict(fields: Iterable[str] | None) -> dict: output = {} if not fields: From e087ea09bb7511444a33eec18c5a6a6f7ca8dab2 Mon Sep 17 00:00:00 2001 From: Roy Nieterau Date: Mon, 14 Sep 2026 19:46:47 +0200 Subject: [PATCH 4/6] Apply suggestion from @BigRoy --- tests/test_graphql_infinite_loop.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/test_graphql_infinite_loop.py b/tests/test_graphql_infinite_loop.py index cead2d221..ee8460e8e 100644 --- a/tests/test_graphql_infinite_loop.py +++ b/tests/test_graphql_infinite_loop.py @@ -42,5 +42,6 @@ def page(ids, end_cursor): server = _ScriptedServer([page(["e0"], "c0"), page([], None)]) query = events_graphql_query({"id"}, SortOrder.ascending) - assert query.query(server) == {"events": [{"id": "e0"}]} - assert len(server.queries) == 2 + + with pytest.raises(GraphQlQueryError, match="page without a cursor"): + query.query(server) From 534b47bf1678499999b8c4f4e6ef3cfbd2f70a03 Mon Sep 17 00:00:00 2001 From: Roy Nieterau Date: Mon, 14 Sep 2026 19:47:02 +0200 Subject: [PATCH 5/6] Apply suggestion from @BigRoy --- tests/test_graphql_infinite_loop.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/test_graphql_infinite_loop.py b/tests/test_graphql_infinite_loop.py index ee8460e8e..fe5cf9ffd 100644 --- a/tests/test_graphql_infinite_loop.py +++ b/tests/test_graphql_infinite_loop.py @@ -42,6 +42,5 @@ def page(ids, end_cursor): server = _ScriptedServer([page(["e0"], "c0"), page([], None)]) query = events_graphql_query({"id"}, SortOrder.ascending) - with pytest.raises(GraphQlQueryError, match="page without a cursor"): query.query(server) From eefcfef6162369cb9972be21bf1eb2c786407477 Mon Sep 17 00:00:00 2001 From: Roy Nieterau Date: Mon, 14 Sep 2026 19:56:57 +0200 Subject: [PATCH 6/6] Assume server downloads always have content length --- tests/test_download_resume.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/tests/test_download_resume.py b/tests/test_download_resume.py index 9c353cbcb..6c4bc2e15 100644 --- a/tests/test_download_resume.py +++ b/tests/test_download_resume.py @@ -51,10 +51,3 @@ def get_func(url, **kwargs): content, progress = _download(con, get_func) assert content == CONTENT assert progress.transferred_size == len(CONTENT) - - -def test_download_without_content_length(con): - content, _ = _download( - con, lambda url, **kwargs: FakeResponse(200, CONTENT) - ) - assert content == CONTENT