aminghadersohi commented on code in PR #40634:
URL: https://github.com/apache/superset/pull/40634#discussion_r3383317514
##########
superset/charts/schemas.py:
##########
@@ -518,6 +520,13 @@ class
ChartDataRollingOptionsSchema(ChartDataPostProcessingOperationOptionsSchem
window = fields.Integer(
metadata={"description": "Size of the rolling window in days.",
"example": 7},
Review Comment:
**[MEDIUM]** `window` metadata is missing `"min": 1` and `"max": 10000`. The
`periods` field (updated in this PR) now has both bounds in its metadata so the
OpenAPI spec reflects the actual validator — `window` needs the same treatment.
Without it, generated clients and API docs will show an unconstrained integer.
##########
tests/unit_tests/charts/test_schemas.py:
##########
@@ -91,6 +93,66 @@ def
test_chart_data_prophet_options_schema_time_grain_validation(
assert "time_grain" in exc_info.value.messages
+def test_chart_put_schema_query_context_json_validation(
+ app_context: None,
+) -> None:
+ """ChartPutSchema.query_context must reject invalid JSON (parity with
POST)."""
+ schema = ChartPutSchema()
+
+ # Valid JSON passes
+ assert schema.load({"query_context": '{"a": 1}'})["query_context"] ==
'{"a": 1}'
+
+ # None is allowed (allow_none)
+ assert schema.load({"query_context": None})["query_context"] is None
+
+ # Invalid JSON is rejected
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load({"query_context": "{not valid json"})
+ assert "query_context" in exc_info.value.messages
+
+
+def test_chart_data_prophet_options_schema_periods_range(
+ app_context: None,
+) -> None:
+ """`periods` must be a bounded non-negative integer."""
+ schema = ChartDataProphetOptionsSchema()
+ base = {"time_grain": "P1D", "confidence_interval": 0.8}
+
+ # Valid value passes
+ assert schema.load({**base, "periods": 7})["periods"] == 7
+
+ # Negative value rejected
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load({**base, "periods": -1})
+ assert "periods" in exc_info.value.messages
+
+ # Excessively large value rejected (resource-exhaustion guard)
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load({**base, "periods": 1_000_000})
+ assert "periods" in exc_info.value.messages
+
+
+def test_chart_data_rolling_options_schema_window_range(
+ app_context: None,
+) -> None:
+ """`window` must be a bounded positive integer."""
+ schema = ChartDataRollingOptionsSchema()
+ base = {"rolling_type": "mean"}
+
+ # Valid value passes
+ assert schema.load({**base, "window": 7})["window"] == 7
+
+ # Zero window rejected (rolling requires window > 0)
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load({**base, "window": 0})
+ assert "window" in exc_info.value.messages
+
+ # Excessively large window rejected
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load({**base, "window": 1_000_000})
+ assert "window" in exc_info.value.messages
Review Comment:
**[NIT]** `window=1` (min) and `window=10000` (max) are untested as accepted
inputs — same gap as the `periods` test above. Boundary inclusiveness is
unverified.
##########
tests/unit_tests/charts/test_schemas.py:
##########
@@ -91,6 +93,66 @@ def
test_chart_data_prophet_options_schema_time_grain_validation(
assert "time_grain" in exc_info.value.messages
+def test_chart_put_schema_query_context_json_validation(
+ app_context: None,
+) -> None:
+ """ChartPutSchema.query_context must reject invalid JSON (parity with
POST)."""
+ schema = ChartPutSchema()
+
+ # Valid JSON passes
+ assert schema.load({"query_context": '{"a": 1}'})["query_context"] ==
'{"a": 1}'
+
+ # None is allowed (allow_none)
+ assert schema.load({"query_context": None})["query_context"] is None
+
+ # Invalid JSON is rejected
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load({"query_context": "{not valid json"})
+ assert "query_context" in exc_info.value.messages
+
+
+def test_chart_data_prophet_options_schema_periods_range(
+ app_context: None,
+) -> None:
+ """`periods` must be a bounded non-negative integer."""
+ schema = ChartDataProphetOptionsSchema()
+ base = {"time_grain": "P1D", "confidence_interval": 0.8}
+
+ # Valid value passes
+ assert schema.load({**base, "periods": 7})["periods"] == 7
+
+ # Negative value rejected
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load({**base, "periods": -1})
+ assert "periods" in exc_info.value.messages
+
+ # Excessively large value rejected (resource-exhaustion guard)
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load({**base, "periods": 1_000_000})
+ assert "periods" in exc_info.value.messages
Review Comment:
**[NIT]** The boundary values `periods=0` (the `min`) and `periods=10000`
(the `max`) are never asserted to be valid. An accidental `min_inclusive=False`
or off-by-one on the bound would silently reject legal inputs without a test
failure.
--
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]
