fix(query-object): reject malformed ad-hoc metrics - #43353
Open
sadpandajoe wants to merge 3 commits into
Open
Conversation
A metric object shaped like a Custom SQL ad-hoc metric but missing
`expressionType` is silently rewritten into a bare string by
`QueryObject._set_metrics`, because `is_adhoc_metric` classifies a metric
solely by the presence of that one key. The string used is the metric's
`label`, which for a Custom SQL metric is the SQL text itself, so metric
resolution then reports `Metric '<sql text>' does not exist`.
`ChartDataAdhocMetricSchema` declares `expressionType` as required, but
`ChartDataQueryObjectSchema.metrics` is a list of `fields.Raw`, so that
contract is never enforced at the API boundary.
Adds a regression guard that reproduces the coercion through the real
request-deserialization path and the resulting error through the real query
builder, plus two tests fencing a fix from over-correcting: the legacy
`{"label": ...}` saved-metric reference must keep collapsing to its name, and
a well-formed Custom SQL metric must stay an ad-hoc definition.
The first test is red until the coercion is narrowed; the fix is not applied
here. RCA.md records the analysis, including the unresolved question of what
omits `expressionType` from the request in the first place.
Co-Authored-By: Claude <noreply@anthropic.com>
QueryObject._set_metrics supported a legacy {"label": "name"} shape as a
reference to a metric saved on the dataset, and fell back to that
coercion for any dict that wasn't recognized as an ad-hoc metric (i.e.
missing `expressionType`). This meant a Custom SQL ad-hoc metric that
lost its `expressionType` key on the way in was silently reduced to its
auto-derived label, which is the SQL expression text itself. Downstream
metric resolution then looked that string up among the dataset's saved
metrics, missed, and raised a misleading "Metric '<sql text>' does not
exist" error.
The fix narrows the fallback: a dict is only treated as a legacy
saved-metric-name reference when it carries none of the keys that are
exclusive to an ad-hoc metric definition (sqlExpression, aggregate,
column). A dict that carries one of those keys but is missing
expressionType is now left as-is instead of being collapsed to a
string, so it no longer masquerades as a request for a saved metric
that was never intended.
…y preserving them The previous version of this fix left a malformed ad-hoc metric (one that carries ad-hoc-only keys like sqlExpression/aggregate/column but is missing expressionType) as an untouched dict, expecting later validation to catch it. In practice nothing downstream recognizes that shape either, so it fell through to a bare ValueError instead of a QueryObjectValidationError - an uncontrolled error rather than a clean, user-facing one. QueryObject._set_metrics now raises QueryObjectValidationError directly for this shape. The chart-data API endpoints already catch QueryObjectValidationError around query-context deserialization and return a 400, so the error surfaces as a controlled response instead of propagating uncaught. The guard test is updated to assert the raise (with a message naming the missing field) instead of asserting on a returned value, since the malformed metric is now rejected at deserialization time rather than passed through.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43353 +/- ##
==========================================
- Coverage 66.73% 66.63% -0.10%
==========================================
Files 2876 2876
Lines 164201 164538 +337
Branches 37887 37938 +51
==========================================
+ Hits 109580 109645 +65
- Misses 52465 52729 +264
- Partials 2156 2164 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Code Review Agent Run #38188dActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
A metric shaped like a Custom SQL ad-hoc metric (carries
sqlExpression,aggregate, orcolumn) but missingexpressionTypewas silently coerced byQueryObject._set_metricsinto a bare string, on the assumption it was a legacy{"label": "name"}reference to a saved metric. For a Custom SQL metric the label is the SQL expression text itself, so metric resolution then looked that text up among the dataset's saved metrics, found nothing, and raised a misleadingMetric '<sql text>' does not existerror instead of a clear validation error.This PR narrows the coercion: a dict carrying any ad-hoc-only key (
sqlExpression,aggregate,column) but missingexpressionTypeis now rejected with aQueryObjectValidationErrornaming the metric and the missing field. The chart-data API endpoints already catchQueryObjectValidationErroraround query-context deserialization, so the error surfaces as a clean 400 response instead of the previous misleading failure. The legacy{"label": "name"}saved-metric reference (no ad-hoc keys at all) still resolves exactly as before.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend validation change, not a UI change.
TESTING INSTRUCTIONS
pytest tests/unit_tests/charts/data/malformed_adhoc_metric_test.py -vpytest tests/unit_tests/common/test_query_object_factory.py -vThe new test file covers: a malformed ad-hoc metric now raises a clear validation error instead of silently resolving to a bogus saved-metric lookup; the pre-existing legacy label-only reference still resolves correctly; a well-formed Custom SQL metric is unaffected.
ADDITIONAL INFORMATION