SEPURI-SAI-KRISHNA commented on code in PR #43206:
URL: https://github.com/apache/superset/pull/43206#discussion_r3788719319
##########
tests/unit_tests/charts/test_schemas.py:
##########
@@ -478,3 +478,57 @@ def
test_chart_data_extras_rejects_system_sampling(app_context: None) -> None:
with pytest.raises(ValidationError) as exc_info:
ChartDataExtrasSchema().load({"system_sampling": True})
assert "system_sampling" in exc_info.value.messages
+
+
+def test_post_processing_option_schemas_match_their_functions(
+ app_context: None,
+) -> None:
+ """Every documented post-processing option must be a real parameter.
+
+ `QueryObject.exec_post_processing` dispatches with
+ `getattr(pandas_postprocessing, operation)(df, **options)`, and the
+ per-operation `options` dict is passed through unvalidated. So a field
+ that appears in one of these schemas but not in the corresponding
+ function signature is published in the OpenAPI spec as a valid option
+ while raising `TypeError: <op>() got an unexpected keyword argument` --
+ an HTTP 500 -- for any client that sends it.
+
+ `ChartDataSortOptionsSchema` documented a required `columns` dict and an
+ `aggregates` field, neither of which `sort()` accepts, and
+ `ChartDataProphetOptionsSchema` documented `monthly_seasonality` where
+ `prophet()` takes `daily_seasonality`.
+ """
+ import inspect
+
+ from marshmallow import Schema
+
+ from superset.charts import schemas as chart_schemas
+ from superset.utils import pandas_postprocessing
+
+ mismatches = {}
+ for name, schema_cls in vars(chart_schemas).items():
+ if not (
+ inspect.isclass(schema_cls)
+ and issubclass(schema_cls, Schema)
+ and name.startswith("ChartData")
+ and name.endswith("OptionsSchema")
+ ):
+ continue
+ operation = name[len("ChartData") : -len("OptionsSchema")].lower()
+ function = getattr(pandas_postprocessing, operation, None)
+ if function is None:
+ continue
Review Comment:
Good catch on the first half, fixed in the follow-up commit.
ChartDataGeohashDecodeOptionsSchema lowercased to geohashdecode, which never
matched geohash_decode, and the
getattr(..., None) skipped it silently. Operations are now keyed without
underscores, and a schema that can't be resolved fails the test instead of
being skipped. All three
previously-skipped schemas (geohash_decode, geohash_encode, geodetic_parse)
turn out to match their functions, so no new mismatch surfaced — but the
coverage claim is now true.
On the second half, the one-directional check is deliberate, and I've
documented why. A parameter the schema omits still works when sent — a docs
gap, not a failure. A field the
schema adds is a 500. Asserting both directions today would flag
pre-existing gaps in aggregate, contribution, pivot and rolling, which is a
docs pass worth doing separately.
--
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]
