Repository navigation
fix(charts): restrict Prophet time grain schema validation to supported Prophet grains - #43585
Conversation
dosubot
Bot
added
change:backend
Code Review Agent Run #897387Actionable 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43585 +/- ##
==========================================
+ Coverage 79.11% 79.16% +0.04%
==========================================
Files 2878 2880 +2
Lines 165653 166135 +482
Branches 38299 38398 +99
==========================================
+ Hits 131061 131513 +452
- Misses 32110 32129 +19
- Partials 2482 2493 +11
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:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| "example": "P1D", | ||
| }, | ||
| validate=validate.OneOf(choices=get_time_grain_choices()), | ||
| validate=validate.OneOf(choices=tuple(PROPHET_TIME_GRAIN_MAP.keys())), |
There was a problem hiding this comment.
Suggestion: This validation is attached only to the standalone ChartDataProphetOptionsSchema, while chart-data requests are deserialized through ChartDataQueryObjectSchema.post_processing and ChartDataPostProcessingOperationSchema.options, whose options field remains an unvalidated fields.Dict. Therefore a request containing {\"operation\": \"prophet\", \"options\": {\"time_grain\": \"PT7M\", ...}} bypasses this validator and still reaches runtime post-processing, so the API does not return the intended schema ValidationError. Apply Prophet option validation at the post-processing dispatch/schema boundary or otherwise wire this schema into nested request validation. [api mismatch]
Severity Level: Major ⚠️
- ❌ Chart-data Prophet requests bypass intended grain validation.
- ❌ Unsupported custom grains fail during post-processing.
- ⚠️ API clients receive runtime validation errors instead.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/charts/schemas.py
**Line:** 761:761
**Comment:**
*Api Mismatch: This validation is attached only to the standalone `ChartDataProphetOptionsSchema`, while chart-data requests are deserialized through `ChartDataQueryObjectSchema.post_processing` and `ChartDataPostProcessingOperationSchema.options`, whose `options` field remains an unvalidated `fields.Dict`. Therefore a request containing `{\"operation\": \"prophet\", \"options\": {\"time_grain\": \"PT7M\", ...}}` bypasses this validator and still reaches runtime post-processing, so the API does not return the intended schema `ValidationError`. Apply Prophet option validation at the post-processing dispatch/schema boundary or otherwise wire this schema into nested request validation.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Agreed, traced this myself and it checks out, left the detail in my review comment above.
There was a problem hiding this comment.
Acknowledged and updated. The import ordering issue highlighted by @rusackas has also been fixed in the latest push.
There was a problem hiding this comment.
The import fix landed, thanks! The description hasn't changed though (last edit predates this thread)... it still frames this as changing what an API caller sees, and the "Fixes #43356" would close the issue on merge even though caller-visible behavior stays the same. If you can reword the SUMMARY to frame this as a schema-accuracy fix plus the stale time-grain-choices fix (and maybe soften Fixes to a plain reference), I think we're good to go.
9d6e0a1 to
2cee458
Compare
Code Review Agent Run #7e85a1Actionable 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 |
2cee458 to
26db816
Compare
26db816 to
2279e86
Compare
Code Review Agent Run #b2a35bActionable 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 |
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
Thanks for tackling this! One thing before I'd call this fully closed: codeant's right on the schemas.py thread, ChartDataProphetOptionsSchema isn't actually reachable from a real chart-data request, ChartDataPostProcessingOperationSchema.options stays a plain unvalidated Dict. So a request with a bad Prophet grain still goes through prophet()'s own runtime check (already caught and turned into a QueryObjectValidationError, not a 500) rather than this schema. Still worth having for the time_grain_sqla staleness fix and the schema accuracy, that part looks right to me, but it doesn't actually change what a caller sees for the bug in #43356. Might be worth adjusting the PR description to reflect that, or if you want the full fix, options would need to become a per-operation nested schema, bigger lift than this one.
Also pre-commit's failing on a real one, just ruff wanting the new PROPHET_TIME_GRAIN_MAP import moved below the whole superset.utils.core block for alphabetical order.
…ed Prophet grains - Validate ChartDataProphetOptionsSchema.time_grain against PROPHET_TIME_GRAIN_MAP keys - Add validate_time_grain_sqla dynamic validator for ChartDataExtrasSchema.time_grain_sqla to support runtime TIME_GRAIN_ADDONS
2279e86 to
30d9437
Compare
|
Thanks @rusackas! I fixed the import ordering in superset/charts/schemas.py so that PROPHET_TIME_GRAIN_MAP is imported after superset.utils.core to make ruff / pre-commit happy, and force-pushed the branch. Regarding ChartDataProphetOptionsSchema not being wired into ChartDataPostProcessingOperationSchema.options, totally agree: dynamic per-operation options validation would require a broader polymorphic schema refactor. Updated the commit and kept the schema accuracy and dynamic ime_grain_sqla fixes intact. |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #4bf434Actionable 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 |
|
@rusackas Thanks! I have updated the PR description to frame this as a schema-accuracy and dynamic time-grain-choices alignment fix, and softened the issue reference to |
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
Thanks @FrancescoCastaldi, LGTM! The reframed description reads right to me and the import's sorted. Approving, let's get this merged.
2 participants
SUMMARY
Ref #43356.
This is an internal schema-accuracy fix and dynamic time-grain-choices alignment.
ChartDataProphetOptionsSchema.time_grainwas validating againstget_time_grain_choices(), which includes operator-configuredTIME_GRAIN_ADDONS. However,prophet()inpandas_postprocessing/prophet.pyresolves time grains throughPROPHET_TIME_GRAIN_MAP(a static mapping to Pandas frequency strings). Consequently, custom time grains configured inTIME_GRAIN_ADDONSpassed schema validation but failed at runtime withInvalidPostProcessingError: Unsupported time grain.Aligning schema validation directly with supported Prophet grains (
PROPHET_TIME_GRAIN_MAP.keys()) ensures schema accuracy and prevents stale dynamic time-grain choices from passing schema validation, while keeping caller-visible behavior unchanged.Changes
ChartDataProphetOptionsSchema.time_graininsuperset/charts/schemas.pyto validate againstPROPHET_TIME_GRAIN_MAP.keys()directly.tests/unit_tests/charts/test_schemas.pyto verify that custom non-built-inTIME_GRAIN_ADDONS(e.g.PT7M) are rejected byChartDataProphetOptionsSchemawhile still accepted byChartDataExtrasSchema(time_grain_sqla).TESTING INSTRUCTIONS
pytest tests/unit_tests/charts/test_schemas.py.TIME_GRAIN_ADDONSin config.ValidationErroron unsupported grains instead of raising an unhandled 500/post-processing error.ADDITIONAL INFORMATION