fix(charts): exclude string helpers from post_processing operation allowlist - #43345
fix(charts): exclude string helpers from post_processing operation allowlist#43345rusackas wants to merge 1 commit into
Conversation
Code Review Agent Run #8540c5Actionable 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43345 +/- ##
=======================================
Coverage 66.73% 66.73%
=======================================
Files 2876 2876
Lines 164218 164218
Branches 37890 37890
=======================================
Hits 109590 109590
Misses 52469 52469
Partials 2159 2159
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:
|
…lowlist escape_separator/unescape_separator are str -> str helpers used internally by flatten, not DataFrame post-processing operations. Both the schema's validate.OneOf() choices and the query_object.py dispatch accepted them as a post_processing operation name, so submitting one against a DataFrame either raised a confusing TypeError (dispatch) or, previously, a KeyError in the fallback error path (the InvalidPostProcessingError message used type=operation against a %(operation)s format string). Adds pandas_postprocessing.OPERATIONS, a curated list of the real DataFrame operations, and uses it in both the schema allowlist and the executor's dispatch guard instead of the module's broader __all__/hasattr checks.
dcbc142 to
30ae1b4
Compare
Code Review Agent Run #18cccdActionable 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 |
| ) | ||
| ] | ||
| ), | ||
| validate=validate.OneOf(choices=pandas_postprocessing.OPERATIONS), |
There was a problem hiding this comment.
Rebasing this static allowlist as written would drop master’s EXTRA_PANDAS_POSTPROCESSING_OPS extension point, so configured custom operations would start returning 400 errors. Could this be rebased to restrict only built-in operations while preserving the extra-operation validation and dispatch paths?
| ) | ||
| ] | ||
| ), | ||
| validate=validate.OneOf(choices=pandas_postprocessing.OPERATIONS), |
There was a problem hiding this comment.
The checked-in OpenAPI enum still advertises escape_separator and unescape_separator, so generated clients can submit documented values that this validation now rejects. Could the OpenAPI resource be regenerated with this allowlist change?
2 participants
SUMMARY
Follow-up from a
codeant-ai-for-open-sourcefinding on #43337: thepost_processingoperation allowlist (ChartDataPostProcessingOperationSchema.operationinsuperset/charts/schemas.py, and the dispatch guard inQueryObject.exec_post_processing) acceptsescape_separator/unescape_separatoras valid operation names. Those are internalstr -> strhelpers used byflatten, not DataFrame post-processing operations — submitting one as anoperationcurrently either raises a confusingTypeError(the function gets called with a DataFrame instead of a string) rather than a clean validation error.This is pre-existing on
master, independent of #43337 (the oldinspect.getmembers(pandas_postprocessing, inspect.isfunction)allowlist has the same gap).Adds
pandas_postprocessing.OPERATIONS, a curated list of the real DataFrame operations (excludes the two string helpers), and uses it in both the schema allowlist and the executor's dispatch guard.While adding a test for this, found and fixed a second, related bug in the same code path: the
InvalidPostProcessingErrormessage inquery_object.pyusedtype=operationagainst a"...%(operation)s"format string, so hitting that branch raised aKeyErrorfromflask_babel'sgettextinstead of the intended clean error. Fixed tooperation=operation.TESTING INSTRUCTIONS
Also ran the full
tests/unit_tests/pandas_postprocessing/,tests/unit_tests/charts/, andtests/unit_tests/queries/suites locally (349 passed, 2 xfailed, unrelated) andpre-commiton the changed files.ADDITIONAL INFORMATION