Skip to content

[DRAFT] Remove query_builder from mypy typing-issues list #119028

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

Draft
cursor wants to merge 3 commits into
base: master
Choose a base branch
from
+61 −35
Draft

[DRAFT] Remove query_builder from mypy typing-issues list #119028

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
389fb79
mypy: remove typed query_builder override
cursoragent Jul 4, 2026
24d7d07
mypy: fix query_builder typing for strict checks
cursoragent Jul 4, 2026
dc45d92
mypy: tighten query_builder annotations
cursoragent Jul 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,6 @@ ignore_missing_imports = true
# - python3 -m tools.mypy_helpers.find_easiest_modules
[[tool.mypy.overrides]]
module = [
"sentry.snuba.metrics.query_builder",
"sentry.testutils.cases",
]
disable_error_code = [
Expand Down
95 changes: 61 additions & 34 deletions src/sentry/snuba/metrics/query_builder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
from collections.abc import Mapping, Sequence
from datetime import datetime, timedelta
from enum import Enum
from typing import Any, TypedDict, overload
from typing import Any, TypedDict, cast, overload

import sentry_sdk
from snuba_sdk import (
Expand Down Expand Up @@ -43,6 +43,7 @@
from sentry.snuba.metrics.fields.base import (
COMPOSITE_ENTITY_CONSTITUENT_ALIAS,
MetricExpressionBase,
MetricOperationParams,
generate_bottom_up_dependency_tree_for_metrics,
org_id_from_projects,
)
Expand All @@ -52,7 +53,6 @@
parse_expression,
)
from sentry.snuba.metrics.naming_layer.mri import parse_mri_field
from sentry.snuba.metrics.naming_layer.public import PUBLIC_EXPRESSION_REGEX
from sentry.snuba.metrics.query import (
DeprecatingMetricsQuery,
MetricActionByField,
Expand All @@ -71,6 +71,8 @@
DerivedMetricParseException,
MetricDoesNotExistException,
MetricEntity,
MetricOperationType,
OPERATIONS,
get_num_intervals,
get_timestamp_column_name,
require_rhs_condition_resolution,
Expand Down Expand Up @@ -125,16 +127,20 @@


def parse_public_field(field: str) -> MetricField:
matches = PUBLIC_EXPRESSION_REGEX.match(field)
operation, metric_name = parse_expression(field)
if operation is not None and operation not in OPERATIONS:
raise InvalidParams(f"Invalid operation '{operation}'. Must be one of {', '.join(OPERATIONS)}")

if matches is not None:
operation = matches[1]
metric_name = matches[2]
else:
operation = None
metric_name = field
return MetricField(cast(MetricOperationType | None, operation), get_mri(metric_name))

Check failure on line 134 in src/sentry/snuba/metrics/query_builder.py

View check run for this annotation

@sentry/warden / warden: sentry-backend-bugs

parse_public_field uses MRI regex instead of public-name regex, breaking all public metric field parsing 

Replacing `PUBLIC_EXPRESSION_REGEX.match(field)` with `parse_expression(field)` is incorrect: `parse_expression` uses `MRI_EXPRESSION_REGEX` which requires MRI format (`entity:namespace/name@unit`), so public fields like `sum(session.duration)` never match, causing `operation=None` and `metric_name="sum(session.duration)"`, which then fails inside `get_mri()` with `InvalidParams("Failed to parse 'sum(session.duration)'")` for every API call with a public metric field.
Comment on lines +130 to +134

@sentry-warden sentry-warden Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parse_public_field uses MRI regex instead of public-name regex, breaking all public metric field parsing

Replacing PUBLIC_EXPRESSION_REGEX.match(field) with parse_expression(field) is incorrect: parse_expression uses MRI_EXPRESSION_REGEX which requires MRI format (entity:namespace/name@unit), so public fields like sum(session.duration) never match, causing operation=None and metric_name="sum(session.duration)", which then fails inside get_mri() with InvalidParams("Failed to parse 'sum(session.duration)'") for every API call with a public metric field.

Evidence
  • parse_expression in naming_layer/mapping.py:116 uses MRI_EXPRESSION_REGEX, whose pattern is ^{OP_REGEX}\(({MRI_SCHEMA_REGEX_STRING})\)- parse_expressioninnaming_layer/mapping.py:116usesMRI_EXPRESSION_REGEX, whose pattern is where MRI_SCHEMA_REGEX_STRING=entity:namespace/name@unit` — requires colons and at-sign.
  • A public field like sum(session.duration) does not match MRI_EXPRESSION_REGEX; parse_expression returns (None, "sum(session.duration)").
  • operation is None, so the InvalidParams guard on line 131 is skipped; get_mri("sum(session.duration)") raises InvalidParams("Failed to parse 'sum(session.duration)'…") via KeyError in NAME_TO_MRI lookup.
  • The old code used PUBLIC_EXPRESSION_REGEX (^{OP_REGEX}\({PUBLIC_NAME_REGEX}\)- parse_expressioninnaming_layer/mapping.py:116usesMRI_EXPRESSION_REGEX, whose pattern is ^{OP_REGEX}(({MRI_SCHEMA_REGEX_STRING}))- parse_expression in naming_layer/mapping.py:116 uses MRI_EXPRESSION_REGEX, whose pattern is where MRI_SCHEMA_REGEX_STRING = entity:namespace/name@unit — requires colons and at-sign.
  • A public field like sum(session.duration) does not match MRI_EXPRESSION_REGEX; parse_expression returns (None, "sum(session.duration)").
  • operation is None, so the InvalidParams guard on line 131 is skipped; get_mri("sum(session.duration)") raises InvalidParams("Failed to parse 'sum(session.duration)'…") via KeyError in NAME_TO_MRI lookup.
  • The old code used PUBLIC_EXPRESSION_REGEX () which correctly matches session.duration as a plain dotted public name, extracting operation="sum" and metric_name="session.duration".
  • parse_field is called directly from user API query params (query_params.getlist("field", [])) at line 530, so every metrics API request using a public field with an operation hits this path.

Identified by Warden sentry-backend-bugs · 788-XYR


return MetricField(operation, get_mri(metric_name))

def _cast_metric_operation_params(
params: (
dict[str, None | str | int | float | Sequence[tuple[str | int, ...]]]
| None
),
) -> MetricOperationParams | None:
return cast(MetricOperationParams | None, params)


def transform_null_transaction_to_unparameterized(use_case_id, org_id, alias=None):
Expand Down Expand Up @@ -747,11 +753,14 @@
continue
elif alias_type == AliasMetaType.GROUP_BY_METRIC_FIELD:
metric_groupby_field = alias_to_metric_group_by_field[record["name"]]
defined_parent_meta_type = get_metric_object_from_metric_field(
metric_groupby_field.field
).get_meta_type()

record["type"] = defined_parent_meta_type
if isinstance(metric_groupby_field.field, MetricField):
defined_parent_meta_type = get_metric_object_from_metric_field(
metric_groupby_field.field
).get_meta_type()
if defined_parent_meta_type is not None:
record["type"] = defined_parent_meta_type
else:
record["type"] = "string"
elif alias_type == AliasMetaType.TAG:
record["type"] = "string"
elif alias_type == AliasMetaType.DATASET_COLUMN or alias_type == AliasMetaType.TIME_COLUMN:
Expand Down Expand Up @@ -833,7 +842,7 @@
# This transformation is currently supported only for group by because OrderBy doesn't support the Function type.
if is_group_by and metric_action_by_field.field == "transaction":
return transform_null_transaction_to_unparameterized(
use_case_id, org_id, metric_action_by_field.alias
use_case_id, org_id, cast(MetricGroupByField, metric_action_by_field).alias
)

# Handles the case when we are trying to group or order by `project` for example, but we want
Expand All @@ -856,7 +865,7 @@
exp = (
AliasedExpression(
exp=Column(name=column_name),
alias=metric_action_by_field.alias,
alias=cast(MetricGroupByField, metric_action_by_field).alias,
)
if is_group_by and not is_column
else Column(name=column_name)
Expand All @@ -865,7 +874,12 @@
if is_order_by:
# We return a list in order to use the "extend" method and reduce the number of changes across
# the codebase.
exp = [OrderBy(exp=exp, direction=metric_action_by_field.direction)]
exp = [
OrderBy(
exp=exp,
direction=cast(MetricOrderByField, metric_action_by_field).direction,
)
]

return exp
elif isinstance(metric_action_by_field.field, MetricField):
Expand All @@ -878,16 +892,16 @@
return metric_expression.generate_groupby_statements(
use_case_id=use_case_id,
alias=metric_action_by_field.field.alias,
params=metric_action_by_field.field.params,
params=_cast_metric_operation_params(metric_action_by_field.field.params),
projects=projects,
)[0]
elif is_order_by:
return metric_expression.generate_orderby_clause(
use_case_id=use_case_id,
alias=metric_action_by_field.field.alias,
params=metric_action_by_field.field.params,
params=_cast_metric_operation_params(metric_action_by_field.field.params),
projects=projects,
direction=metric_action_by_field.direction,
direction=cast(MetricOrderByField, metric_action_by_field).direction,
)
else:
raise NotImplementedError(
Expand Down Expand Up @@ -925,14 +939,18 @@
Condition(
lhs=metric_expression.generate_where_statements(
use_case_id=self._use_case_id,
params=condition.lhs.params,
params=_cast_metric_operation_params(condition.lhs.params),
projects=self._projects,
alias=condition.lhs.alias,
)[0],
op=condition.op,
rhs=(
resolve_tag_value(self._use_case_id, self._org_id, condition.rhs)
if require_rhs_condition_resolution(condition.lhs.op)
if (
condition.lhs.op is not None
and isinstance(condition.rhs, str)
and require_rhs_condition_resolution(condition.lhs.op)
)
else condition.rhs
),
)
Expand Down Expand Up @@ -1069,6 +1087,8 @@
series_limit = self._metrics_query.max_limit

if self._use_case_id in [UseCaseID.TRANSACTIONS, UseCaseID.SPANS]:
if self._metrics_query.interval is None:
raise InvalidParams("An interval is required for discover query series.")
time_groupby_column = self.__generate_time_groupby_column_for_discover_queries(
self._metrics_query.interval
)
Expand Down Expand Up @@ -1099,17 +1119,19 @@

def __update_query_dicts_with_component_entities(
self,
component_entities: dict[MetricEntity, Sequence[str]],
metric_mri_to_obj_dict: dict[tuple[str | None, str, str], MetricExpressionBase],
fields_in_entities: dict[MetricEntity, list[tuple[str | None, str, str]]],
component_entities: Mapping[MetricEntity | None, Sequence[str]],
metric_mri_to_obj_dict: dict[tuple[MetricOperationType | None, str, str], MetricExpressionBase],
fields_in_entities: dict[MetricEntity, list[tuple[MetricOperationType | None, str, str]]],
parent_alias,
) -> dict[tuple[str | None, str, str], MetricExpressionBase]:
) -> dict[tuple[MetricOperationType | None, str, str], MetricExpressionBase]:
# At this point in time, we are only supporting raw metrics in the metrics attribute of
# any instance of DerivedMetric, and so in this case the op will always be None
# ToDo(ahmed): In future PR, we might want to allow for dependency metrics to also have an
# an aggregate and in this case, we would need to parse the op here
op = None
for entity, metric_mris in component_entities.items():
if entity is None:
continue
for metric_mri in metric_mris:
# The constituents of an instance of CompositeEntityDerivedMetric will have a reference to their parent
# alias so that we are able to distinguish the constituents in case we have naming collisions that could
Expand All @@ -1128,8 +1150,10 @@
return metric_mri_to_obj_dict

def get_snuba_queries(self):
metric_mri_to_obj_dict: dict[tuple[str | None, str, str], MetricExpressionBase] = {}
fields_in_entities: dict[MetricEntity, list[tuple[str | None, str, str]]] = {}
metric_mri_to_obj_dict: dict[
tuple[MetricOperationType | None, str, str], MetricExpressionBase
] = {}
fields_in_entities: dict[MetricEntity, list[tuple[MetricOperationType | None, str, str]]] = {}

for select_field in self._metrics_query.select:
metric_field_obj = metric_object_factory(select_field.op, select_field.metric_mri)
Expand Down Expand Up @@ -1196,12 +1220,15 @@

# In order to support on demand metrics which require an interval (e.g. epm),
# we want to pass the interval down via params so we can pass it to the associated snql_factory
params = {"interval": self._metrics_query.interval, **(params or {})}
query_interval = self._metrics_query.interval
params = dict(params or {})
if query_interval is not None:
params["interval"] = query_interval
select += metric_field_obj.generate_select_statements(
projects=self._projects,
use_case_id=self._use_case_id,
alias=field[2],
params=params,
params=_cast_metric_operation_params(params or None),
)
metric_ids_set |= metric_field_obj.generate_metric_ids(
self._projects, self._use_case_id
Expand Down Expand Up @@ -1255,7 +1282,7 @@
self,
organization_id: int,
metrics_query: DeprecatingMetricsQuery,
fields_in_entities: dict[MetricEntity, list[tuple[str | None, str, str]]],
fields_in_entities: dict[MetricEntity, list[tuple[MetricOperationType | None, str, str]]],
intervals: list[datetime],
results,
use_case_id: UseCaseID,
Expand Down Expand Up @@ -1436,7 +1463,7 @@
except KeyError:
params = None
totals[alias] = metric_obj.run_post_query_function(
totals, params=params, alias=alias
totals, params=_cast_metric_operation_params(params), alias=alias
)

if series is not None:
Expand All @@ -1451,7 +1478,7 @@
except KeyError:
params = None
series[alias][idx] = metric_obj.run_post_query_function(
series, params=params, idx=idx, alias=alias
series, params=_cast_metric_operation_params(params), idx=idx, alias=alias
)

# Remove the extra fields added due to the constituent metrics that were added
Expand Down
Loading