SEPURI-SAI-KRISHNA commented on code in PR #43225:
URL: https://github.com/apache/superset/pull/43225#discussion_r3789698997
##########
superset/charts/schemas.py:
##########
@@ -858,15 +854,13 @@ class
ChartDataPivotOptionsSchema(ChartDataPostProcessingOperationOptionsSchema)
Pivot operation config.
"""
- index = (
- fields.List(
- fields.String(allow_none=False),
- metadata={
- "description": "Columns to group by on the table index
(=rows)",
- "minLength": 1,
- },
- required=True,
- ),
+ index = fields.List(
+ fields.String(allow_none=False),
+ metadata={
+ "description": "Columns to group by on the table index (=rows)",
+ "minLength": 1,
+ },
+ required=True,
)
Review Comment:
Good catch, fixed in the follow-up commit, for both `index` and
`ChartDataAggregateOptionsSchema.groupby`.
You're right on both counts: metadata is not a validator, and `minLength` is
the OpenAPI keyword for strings rather than arrays. I confirmed the effect, the
schema accepted `index=[]` while `pivot()` raises "Pivot operation requires
at least one index". Both now use `validate=Length(min=1)`, which enforces
the bound and emits the correct array constraint, with a regression test
asserting an empty list is rejected for each.
Worth noting this only became visible because of this PR: while the fields
were tuple-wrapped they never reached the spec at all, so the wrong constraint
went along with them.
--
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]
