Skip to content
This repository was archived by the owner on Jun 13, 2025. It is now read-only.

Commit a6cab73

Browse files
SwatinemSwatinem
committed
Optimize fetch_repository resolver
Previously, the `owner.repository` resolver would unconditionally execute a very complicated query through `fetch_repository`. We can avoid that expensive subquery by inspecting the requested fields as stated in the graphql query.
1 parent 1983799 commit a6cab73

File tree

5 files changed

+284
-25
lines changed

5 files changed

+284
-25
lines changed

‎core/commands/repository/interactors/fetch_repository.py‎

Lines changed: 7 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -15,25 +15,20 @@ def execute(
1515
name: str,
1616
okta_authenticated_accounts: list[int],
1717
exclude_okta_enforced_repos: bool = True,
18+
needs_coverage: bool = True,
19+
needs_commits: bool = True,
1820
) -> Repository | None:
1921
queryset = Repository.objects.viewable_repos(self.current_owner)
2022
if exclude_okta_enforced_repos:
2123
queryset = queryset.exclude_accounts_enforced_okta(
2224
okta_authenticated_accounts
2325
)
2426

25-
# TODO(swatinem): We should find a way to avoid these combinators:
26-
# The `with_recent_coverage` combinator is quite expensive.
27-
# We only need that in case we want to query these props via graphql:
28-
# `coverageAnalytics.{percentCovered,commitSha,hits,misses,lines}`
29-
# Similarly, `with_oldest_commit_at` is only needed for `oldestCommitAt`.
27+
if needs_coverage:
28+
queryset = queryset.with_recent_coverage()
29+
if needs_commits:
30+
queryset = queryset.with_oldest_commit_at()
3031

31-
repo = (
32-
queryset.filter(author=owner, name=name)
33-
.with_recent_coverage()
34-
.with_oldest_commit_at()
35-
.select_related("author")
36-
.first()
37-
)
32+
repo = queryset.filter(author=owner, name=name).select_related("author").first()
3833

3934
return repo

‎core/commands/repository/repository.py‎

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -21,19 +21,8 @@
2121

2222

2323
class RepositoryCommands(BaseCommand):
24-
def fetch_repository(
25-
self,
26-
owner: Owner,
27-
name: str,
28-
okta_authenticated_accounts: list[int],
29-
exclude_okta_enforced_repos: bool = True,
30-
) -> Repository | None:
31-
return self.get_interactor(FetchRepositoryInteractor).execute(
32-
owner,
33-
name,
34-
okta_authenticated_accounts,
35-
exclude_okta_enforced_repos=exclude_okta_enforced_repos,
36-
)
24+
def fetch_repository(self, *args, **kwargs) -> Repository | None:
25+
return self.get_interactor(FetchRepositoryInteractor).execute(*args, **kwargs)
3726

3827
def regenerate_repository_upload_token(
3928
self,

‎graphql_api/helpers/requested_fields.py‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
# This was adapted from <https://github.com/mirumee/ariadne/discussions/1116#discussioncomment-6508603>
2+
from collections.abc import Generator, Iterable
3+
4+
from graphql import GraphQLResolveInfo
5+
from graphql.language import (
6+
FieldNode,
7+
SelectionNode,
8+
InlineFragmentNode,
9+
FragmentSpreadNode,
10+
)
11+
12+
13+
def selected_fields(info: GraphQLResolveInfo) -> set[str]:
14+
names: set[str] = set()
15+
for node in info.field_nodes:
16+
if node.selection_set is None:
17+
continue
18+
names.update(_fields_from_selections(info, node.selection_set.selections))
19+
return names
20+
21+
22+
def _fields_from_selections(
23+
info: GraphQLResolveInfo, selections: Iterable[SelectionNode]
24+
) -> Generator[str, None, None]:
25+
for selection in selections:
26+
match selection:
27+
case FieldNode():
28+
name = selection.name.value
29+
yield name
30+
31+
if selection.selection_set is not None:
32+
yield from (
33+
f"{name}.{field}"
34+
for field in _fields_from_selections(
35+
info, selection.selection_set.selections
36+
)
37+
)
38+
39+
case InlineFragmentNode():
40+
yield from _fields_from_selections(
41+
info, selection.selection_set.selections
42+
)
43+
case FragmentSpreadNode():
44+
fragment = info.fragments[selection.name.value]
45+
yield from _fields_from_selections(
46+
info, fragment.selection_set.selections
47+
)
48+
49+
case _:
50+
raise NotImplementedError(f"field type {type(selection)} not supported")

‎graphql_api/tests/test_requested_fields.py‎

Lines changed: 208 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,208 @@
1+
from graphql_api.helpers.requested_fields import selected_fields
2+
from graphql.language import parse
3+
4+
from graphql import GraphQLResolveInfo
5+
from graphql.language import (
6+
OperationDefinitionNode,
7+
FragmentDefinitionNode,
8+
)
9+
10+
11+
def parse_into_resolveinfo(source: str) -> GraphQLResolveInfo:
12+
document = parse(source)
13+
14+
operation: OperationDefinitionNode | None = None
15+
fragments: dict[str, FragmentDefinitionNode] = {}
16+
17+
for definition in document.definitions:
18+
if isinstance(definition, OperationDefinitionNode):
19+
operation = definition
20+
elif isinstance(definition, FragmentDefinitionNode):
21+
fragments[definition.name.value] = definition
22+
23+
assert operation
24+
root_fields = [operation]
25+
26+
return GraphQLResolveInfo(
27+
"__root__",
28+
root_fields, # list[FieldNode]
29+
None,
30+
None,
31+
None,
32+
None,
33+
fragments, # dict[str, FragmentDefinitionNode]
34+
None,
35+
None,
36+
None,
37+
None,
38+
None,
39+
)
40+
41+
42+
QUERY_CoverageForFile = """
43+
query CoverageForFile(
44+
$owner: String!
45+
$repo: String!
46+
$ref: String!
47+
$path: String!
48+
$flags: [String]
49+
$components: [String]
50+
) {
51+
owner(username: $owner) {
52+
repository(name: $repo) {
53+
__typename
54+
... on Repository {
55+
commit(id: $ref) {
56+
...CoverageForFile
57+
}
58+
branch(name: $ref) {
59+
name
60+
head {
61+
...CoverageForFile
62+
}
63+
}
64+
}
65+
... on NotFoundError {
66+
message
67+
}
68+
... on OwnerNotActivatedError {
69+
message
70+
}
71+
}
72+
}
73+
}
74+
75+
fragment CoverageForFile on Commit {
76+
commitid
77+
coverageAnalytics {
78+
flagNames
79+
components {
80+
id
81+
name
82+
}
83+
coverageFile(path: $path, flags: $flags, components: $components) {
84+
hashedPath
85+
content
86+
coverage {
87+
line
88+
coverage
89+
}
90+
totals {
91+
percentCovered # Absolute coverage of the commit
92+
}
93+
}
94+
}
95+
}
96+
"""
97+
98+
QUERY_GetRepoConfigurationStatus = """
99+
query GetRepoConfigurationStatus($owner: String!, $repo: String!) {
100+
owner(username: $owner) {
101+
plan {
102+
isTeamPlan
103+
}
104+
repository(name:$repo) {
105+
__typename
106+
... on Repository {
107+
coverageEnabled
108+
bundleAnalysisEnabled
109+
testAnalyticsEnabled
110+
yaml
111+
languages
112+
coverageAnalytics {
113+
flagsCount
114+
componentsCount
115+
}
116+
}
117+
... on NotFoundError {
118+
message
119+
}
120+
... on OwnerNotActivatedError {
121+
message
122+
}
123+
}
124+
}
125+
}
126+
"""
127+
128+
QUERY_ReposForOwner = """
129+
query ReposForOwner(
130+
$filters: RepositorySetFilters!
131+
$owner: String!
132+
$ordering: RepositoryOrdering!
133+
$direction: OrderingDirection!
134+
$after: String
135+
$first: Int
136+
) {
137+
owner(username: $owner) {
138+
repositories(
139+
filters: $filters
140+
ordering: $ordering
141+
orderingDirection: $direction
142+
first: $first
143+
after: $after
144+
) {
145+
edges {
146+
node {
147+
name
148+
active
149+
activated
150+
private
151+
coverageAnalytics {
152+
percentCovered
153+
lines
154+
}
155+
updatedAt
156+
latestCommitAt
157+
author {
158+
username
159+
}
160+
coverageEnabled
161+
bundleAnalysisEnabled
162+
repositoryConfig {
163+
indicationRange {
164+
upperRange
165+
lowerRange
166+
}
167+
}
168+
}
169+
}
170+
pageInfo {
171+
hasNextPage
172+
endCursor
173+
}
174+
}
175+
}
176+
}
177+
"""
178+
179+
180+
def test_requested_fields():
181+
info = parse_into_resolveinfo(QUERY_CoverageForFile)
182+
fields = selected_fields(info)
183+
184+
assert "owner.repository.branch.name" in fields
185+
assert "owner.repository.branch.head.commitid" in fields
186+
assert (
187+
"owner.repository.branch.head.coverageAnalytics.coverageFile.totals.percentCovered"
188+
in fields
189+
)
190+
191+
assert "owner.repository.oldestCommitAt" not in fields
192+
assert "owner.repository.coverageAnalytics.percentCovered" not in fields
193+
assert "owner.repository.coverageAnalytics.commitSha" not in fields
194+
195+
info = parse_into_resolveinfo(QUERY_GetRepoConfigurationStatus)
196+
fields = selected_fields(info)
197+
198+
assert "owner.repository.coverageAnalytics" in fields
199+
assert "owner.repository.oldestCommitAt" not in fields
200+
assert "owner.repository.coverageAnalytics.percentCovered" not in fields
201+
202+
info = parse_into_resolveinfo(QUERY_ReposForOwner)
203+
fields = selected_fields(info)
204+
205+
assert "owner.repositories.edges.node.latestCommitAt" in fields
206+
assert "owner.repositories.edges.node.oldestCommitAt" not in fields
207+
assert "owner.repositories.edges.node.coverageAnalytics" in fields
208+
assert "owner.repositories.edges.node.coverageAnalytics.percentCovered" in fields

‎graphql_api/types/owner/owner.py‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
from hashlib import sha1
33
from typing import Any, Coroutine, Iterable, List, Optional
44

5+
from graphql_api.helpers.requested_fields import selected_fields
56
import sentry_sdk
67
import shared.rate_limits as rate_limits
78
import stripe
@@ -155,6 +156,16 @@ def resolve_ownerid(owner: Owner, info: GraphQLResolveInfo) -> int:
155156
return owner.ownerid
156157

157158

159+
COVERAGE_FIELDS = {
160+
"coverageAnalytics.percentCovered",
161+
"coverageAnalytics.commitSha",
162+
"coverageAnalytics.hits",
163+
"coverageAnalytics.misses",
164+
"coverageAnalytics.lines",
165+
}
166+
COMMITS_FIELDS = {"oldestCommitAt"}
167+
168+
158169
@owner_bindable.field("repository")
159170
async def resolve_repository(
160171
owner: Owner, info: GraphQLResolveInfo, name: str
@@ -169,11 +180,17 @@ async def resolve_repository(
169180
# This means we do not want to filter out the Okta enforced repos
170181
exclude_okta_enforced_repos = not is_impersonation
171182

183+
requested_fields = selected_fields(info)
184+
needs_coverage = not requested_fields.isdisjoint(COVERAGE_FIELDS)
185+
needs_commits = not requested_fields.isdisjoint(COMMITS_FIELDS)
186+
172187
repository: Repository | None = await command.fetch_repository(
173188
owner,
174189
name,
175190
okta_authenticated_accounts,
176191
exclude_okta_enforced_repos=exclude_okta_enforced_repos,
192+
needs_coverage=needs_coverage,
193+
needs_commits=needs_commits,
177194
)
178195

179196
if repository is None:

0 commit comments

Comments
 (0)