-
Notifications
You must be signed in to change notification settings - Fork 37
IFC-1886: Paginated branch graphql query #7418
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.
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
Changes from all commits
1102ae7
4e16ec7
c0b3b31
9a07e23
41b1849
c8fbca1
dd242d9
9f1dc4a
3a7ff90
e659340
2ad4b2d
32c72ad
a38cec2
84fac0a
f7d4828
491ad04
86dbefa
9eba0b8
9fe2763
8e4dca9
f0aa561
089b015
fb7d0a6
1157cc4
80ff619
156e001
7ea9456
c824b2c
811d90c
232c351
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,10 +2,11 @@ | |
|
|
||
| from typing import TYPE_CHECKING, Any | ||
|
|
||
| from graphene import ID, Field, List, NonNull, String | ||
| from graphene import ID, Field, Int, List, NonNull, String | ||
|
|
||
| from infrahub.exceptions import ValidationError | ||
| from infrahub.graphql.field_extractor import extract_graphql_fields | ||
| from infrahub.graphql.types import BranchType | ||
| from infrahub.graphql.types import BranchType, InfrahubBranch, InfrahubBranchType | ||
|
|
||
| if TYPE_CHECKING: | ||
| from graphql import GraphQLResolveInfo | ||
|
|
@@ -28,3 +29,36 @@ async def branch_resolver( | |
| resolver=branch_resolver, | ||
| required=True, | ||
| ) | ||
|
|
||
|
|
||
| async def infrahub_branch_resolver( | ||
| root: dict, # noqa: ARG001 | ||
| info: GraphQLResolveInfo, | ||
| limit: int | None = None, | ||
| offset: int | None = None, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As CodeRabbit suggests it would be good to catch any negative values for |
||
| ) -> dict[str, Any]: | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if isinstance(limit, int) and limit < 1: | ||
| raise ValidationError("limit must be >= 1") | ||
| if isinstance(offset, int) and offset < 0: | ||
| raise ValidationError("offset must be >= 0") | ||
|
|
||
| fields = extract_graphql_fields(info) | ||
| result: dict[str, Any] = {} | ||
| if "edges" in fields: | ||
| branches = await InfrahubBranch.get_list( | ||
| graphql_context=info.context, fields=fields.get("edges", {}).get("node", {}), limit=limit, offset=offset | ||
| ) | ||
| result["edges"] = [{"node": branch} for branch in branches] | ||
| if "count" in fields: | ||
| result["count"] = await InfrahubBranchType.get_list_count(graphql_context=info.context) | ||
| return result | ||
|
Comment on lines
+34
to
+54
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major Add docstring and avoid passing None kwargs to get_list. This function is missing a docstring, which violates the coding guidelines. Additionally, when As per coding guidelines, apply this diff: async def infrahub_branch_resolver(
root: dict, # noqa: ARG001
info: GraphQLResolveInfo,
limit: int | None = None,
offset: int | None = None,
) -> dict[str, Any]:
+ """Resolve paginated InfrahubBranch query.
+
+ Retrieves a paginated list of branches with metadata about total count
+ and branch details wrapped in an edges structure.
+
+ Args:
+ root: GraphQL root value (unused)
+ info: GraphQL resolve info containing context and field selection
+ limit: Maximum number of items to return (must be >= 1 if provided)
+ offset: Number of items to skip (must be >= 0 if provided)
+
+ Returns:
+ Dictionary containing requested fields (edges and/or count)
+
+ Raises:
+ ValidationError: If limit < 1 or offset < 0
+ """
if isinstance(limit, int) and limit < 1:
raise ValidationError("limit must be >= 1")
if isinstance(offset, int) and offset < 0:
raise ValidationError("offset must be >= 0")
fields = extract_graphql_fields(info)
result: dict[str, Any] = {}
+
+ # Build kwargs, only including limit/offset if provided
+ query_kwargs: dict[str, Any] = {}
+ if limit is not None:
+ query_kwargs["limit"] = limit
+ if offset is not None:
+ query_kwargs["offset"] = offset
+
if "edges" in fields:
branches = await InfrahubBranch.get_list(
- graphql_context=info.context, fields=fields.get("edges", {}).get("node", {}), limit=limit, offset=offset
+ graphql_context=info.context,
+ fields=fields.get("edges", {}).get("node", {}),
+ **query_kwargs,
)
result["edges"] = [{"node": branch} for branch in branches]
if "count" in fields:
result["count"] = await InfrahubBranchType.get_list_count(graphql_context=info.context)
return result🤖 Prompt for AI Agents |
||
|
|
||
|
|
||
| InfrahubBranchQueryList = Field( | ||
| InfrahubBranchType, | ||
| offset=Int(), | ||
| limit=Int(), | ||
| description="Retrieve paginated information about active branches.", | ||
| resolver=infrahub_branch_resolver, | ||
| required=True, | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,11 +2,12 @@ | |
|
|
||
| from typing import TYPE_CHECKING, Any | ||
|
|
||
| from graphene import Boolean, Field, Int, String | ||
| from graphene import Boolean, Field, Int, List, NonNull, String | ||
|
|
||
| from infrahub.core.branch import Branch | ||
| from infrahub.core.constants import GLOBAL_BRANCH_NAME | ||
|
|
||
| from ...exceptions import BranchNotFoundError | ||
| from .enums import InfrahubBranchStatus | ||
| from .standard_node import InfrahubObjectType | ||
|
|
||
|
|
@@ -33,6 +34,10 @@ class Meta: | |
| name = "Branch" | ||
| model = Branch | ||
|
|
||
| @staticmethod | ||
| async def _map_fields_to_graphql(objs: list[Branch], fields: dict) -> list[dict[str, Any]]: | ||
| return [await obj.to_graphql(fields=fields) for obj in objs if obj.name != GLOBAL_BRANCH_NAME] | ||
|
Comment on lines
+37
to
+39
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major Add method docstring. The As per coding guidelines, apply this diff: @staticmethod
async def _map_fields_to_graphql(objs: list[Branch], fields: dict) -> list[dict[str, Any]]:
+ """Map Branch objects to GraphQL field dictionaries, excluding the global branch.
+
+ Args:
+ objs: List of Branch objects to map.
+ fields: Dictionary of field names to include in the output.
+
+ Returns:
+ List of dictionaries containing GraphQL field data for each branch.
+ """
return [await obj.to_graphql(fields=fields) for obj in objs if obj.name != GLOBAL_BRANCH_NAME]🤖 Prompt for AI Agents |
||
|
|
||
| @classmethod | ||
| async def get_list( | ||
| cls, | ||
|
|
@@ -46,4 +51,82 @@ async def get_list( | |
| if not objs: | ||
| return [] | ||
|
|
||
| return [await obj.to_graphql(fields=fields) for obj in objs if obj.name != GLOBAL_BRANCH_NAME] | ||
| return await cls._map_fields_to_graphql(objs=objs, fields=fields) | ||
|
|
||
|
|
||
| class RequiredStringValueField(InfrahubObjectType): | ||
| value = String(required=True) | ||
|
|
||
|
|
||
| class NonRequiredStringValueField(InfrahubObjectType): | ||
| value = String(required=False) | ||
|
|
||
|
|
||
| class NonRequiredIntValueField(InfrahubObjectType): | ||
| value = Int(required=False) | ||
|
|
||
|
|
||
| class NonRequiredBooleanValueField(InfrahubObjectType): | ||
| value = Boolean(required=False) | ||
|
|
||
|
|
||
| class StatusField(InfrahubObjectType): | ||
| value = InfrahubBranchStatus(required=True) | ||
|
|
||
|
|
||
| class InfrahubBranch(BranchType): | ||
| name = Field(RequiredStringValueField, required=True) | ||
| description = Field(NonRequiredStringValueField, required=False) | ||
| origin_branch = Field(NonRequiredStringValueField, required=False) | ||
| branched_from = Field(NonRequiredStringValueField, required=False) | ||
| graph_version = Field(NonRequiredIntValueField, required=False) | ||
| status = Field(StatusField, required=True) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here we override So a query ends up being: query MyQuery {
InfrahubBranch {
count
edges {
node {
graph_version
status {
value
}
name {
value
}
}
}
}
} |
||
| sync_with_git = Field(NonRequiredBooleanValueField, required=False) | ||
| is_default = Field(NonRequiredBooleanValueField, required=False) | ||
| is_isolated = Field( | ||
| NonRequiredBooleanValueField, required=False, deprecation_reason="non isolated mode is not supported anymore" | ||
| ) | ||
| has_schema_changes = Field(NonRequiredBooleanValueField, required=False) | ||
|
|
||
| class Meta: | ||
| description = "InfrahubBranch" | ||
| name = "InfrahubBranch" | ||
|
|
||
|
Comment on lines
+77
to
+94
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major Add class docstring and address The The Apply this diff to add docstring and wrap class InfrahubBranch(BranchType):
+ """Extended branch type with value-wrapped fields for paginated GraphQL queries.
+
+ Most fields are wrapped in value objects to support consistent field resolution
+ in paginated edge/node responses. The id field remains unwrapped as it receives
+ special handling during mapping.
+ """
id = String(required=True)
+ created_at = Field(NonRequiredStringValueField, required=False)
name = Field(RequiredStringValueField, required=True)Based on coding guidelines.
🤖 Prompt for AI Agents |
||
| @staticmethod | ||
| async def _map_fields_to_graphql(objs: list[Branch], fields: dict) -> list[dict[str, Any]]: | ||
| field_keys = fields.keys() | ||
| result: list[dict[str, Any]] = [] | ||
| for obj in objs: | ||
| if obj.name == GLOBAL_BRANCH_NAME: | ||
| continue | ||
| data: dict[str, Any] = {} | ||
| for field in field_keys: | ||
| if field == "id": | ||
| data["id"] = obj.uuid | ||
| continue | ||
| value = getattr(obj, field, None) | ||
| if isinstance(fields.get(field), dict): | ||
| data[field] = {"value": value} | ||
| else: | ||
| data[field] = value | ||
| result.append(data) | ||
| return result | ||
|
|
||
|
|
||
| class InfrahubBranchEdge(InfrahubObjectType): | ||
| node = Field(InfrahubBranch, required=True) | ||
|
|
||
|
|
||
| class InfrahubBranchType(InfrahubObjectType): | ||
| count = Field(Int, description="Total number of items") | ||
| edges = Field(NonNull(List(of_type=NonNull(InfrahubBranchEdge)))) | ||
|
|
||
|
Comment on lines
+120
to
+123
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major Add class docstring and consider marking fields as required. The Apply this diff: class InfrahubBranchType(InfrahubObjectType):
+ """Paginated response type for branch queries.
+
+ Provides pagination metadata (count) alongside the list of branches (edges).
+ """
count = Field(Int, description="Total number of items", required=True)
edges = Field(NonNull(List(of_type=NonNull(InfrahubBranchEdge))), required=True)Note: Based on @ajtmccarty's comment, consider optimizing to only fetch count when explicitly requested rather than always computing it. Based on coding guidelines. 🤖 Prompt for AI Agents |
||
| @classmethod | ||
| async def get_list_count(cls, graphql_context: GraphqlContext, **kwargs: Any) -> int: | ||
| async with graphql_context.db.start_session(read_only=True) as db: | ||
| count = await Branch.get_list_count(db=db, **kwargs) | ||
| try: | ||
| await Branch.get_by_name(name=GLOBAL_BRANCH_NAME, db=db) | ||
| return count - 1 | ||
| except BranchNotFoundError: | ||
| return count | ||
|
Comment on lines
+124
to
+132
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major Add method docstring (regression). This method's docstring was marked as addressed in a previous commit (32c72ad) but is missing in the current code. Per coding guidelines, add a Google-style docstring documenting the purpose, parameters, and return value. Apply this diff: @classmethod
async def get_list_count(cls, graphql_context: GraphqlContext, **kwargs: Any) -> int:
+ """Get the total count of branches excluding the global branch.
+
+ Args:
+ graphql_context: The GraphQL context containing database connection.
+ **kwargs: Additional filter parameters passed to Branch.get_list_count.
+
+ Returns:
+ The count of branches, excluding GLOBAL_BRANCH_NAME if it exists.
+ """
async with graphql_context.db.start_session(read_only=True) as db:🤖 Prompt for AI Agents |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
Add docstring and type annotation.
The class is missing a docstring and type annotation, which are required by the coding guidelines.
Apply this diff to add the missing docstring and type annotation:
As per coding guidelines.
📝 Committable suggestion
🤖 Prompt for AI Agents