Copilot commented on code in PR #40672:
URL: https://github.com/apache/superset/pull/40672#discussion_r3338885722
##########
superset/charts/schemas.py:
##########
@@ -937,6 +945,42 @@ class ChartDataPostProcessingOperationSchema(Schema):
},
)
+ # Map post-processing operation -> its options schema, for operations that
+ # declare one. Operations without a dedicated schema are not structurally
+ # validated here.
+ _OPTIONS_SCHEMAS: dict[str, type[Schema]] = {
+ "aggregate": ChartDataAggregateOptionsSchema,
+ "rolling": ChartDataRollingOptionsSchema,
+ "select": ChartDataSelectOptionsSchema,
+ "sort": ChartDataSortOptionsSchema,
+ "contribution": ChartDataContributionOptionsSchema,
+ "prophet": ChartDataProphetOptionsSchema,
+ "boxplot": ChartDataBoxplotOptionsSchema,
+ "pivot": ChartDataPivotOptionsSchema,
+ "geohash_decode": ChartDataGeohashDecodeOptionsSchema,
+ "geohash_encode": ChartDataGeohashEncodeOptionsSchema,
+ "geodetic_parse": ChartDataGeodeticParseOptionsSchema,
+ }
+
+ @validates_schema
+ def validate_options(self, data: dict[str, Any], **kwargs: Any) -> None:
+ """Validate ``options`` against the operation's option schema.
+
+ Validation is lenient (unknown keys are ignored) so it surfaces wrong
+ types / out-of-range values on declared fields without rejecting
+ payloads that carry extra keys.
+ """
+ operation = data.get("operation")
+ options = data.get("options")
+ if not isinstance(operation, str) or not isinstance(options, dict):
+ return
+ schema_cls = self._OPTIONS_SCHEMAS.get(operation)
+ if schema_cls is None:
+ return
+ errors = schema_cls(unknown=EXCLUDE).validate(options)
+ if errors:
+ raise ValidationError({"options": errors})
Review Comment:
`validate_options` currently skips validation when `options` is missing (or
otherwise not a `dict`). For operations that *do* have a dedicated option
schema (e.g. `prophet`), this means required option fields are never enforced
if the client omits `options` entirely, which undermines the intent of wiring
option schemas into the validation pipeline.
##########
tests/unit_tests/charts/test_schemas.py:
##########
@@ -152,3 +152,53 @@ def
test_time_grain_validation_with_config_addons(app_context: None) -> None:
}
result = schema.load(custom_data)
assert result["time_grain"] == "PT10M"
+
+
+def test_post_processing_operation_validates_options(app_context: None) ->
None:
+ """options are validated against the operation's option schema
(leniently)."""
+ from superset.charts.schemas import ChartDataPostProcessingOperationSchema
+
+ schema = ChartDataPostProcessingOperationSchema()
+
+ # Valid prophet options load.
+ schema.load(
+ {
+ "operation": "prophet",
+ "options": {
+ "time_grain": "P1D",
+ "periods": 7,
+ "confidence_interval": 0.8,
+ },
+ }
+ )
Review Comment:
The new test suite doesn't cover the case where an operation that has an
option schema (e.g. `prophet`) is sent without `options`. Adding an assertion
for this helps prevent regressions and matches the intent of validating
per-operation required fields.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
