codeant-ai-for-open-source[bot] commented on code in PR #43585:
URL: https://github.com/apache/superset/pull/43585#discussion_r3870176115
##########
superset/charts/schemas.py:
##########
@@ -757,7 +758,7 @@ class
ChartDataProphetOptionsSchema(ChartDataPostProcessingOperationOptionsSchem
"[ISO 8601](https://en.wikipedia.org/wiki/ISO_8601#Durations)
durations.",
"example": "P1D",
},
- validate=validate.OneOf(choices=get_time_grain_choices()),
+ validate=validate.OneOf(choices=tuple(PROPHET_TIME_GRAIN_MAP.keys())),
Review 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]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Chart-data Prophet requests bypass intended grain validation.
- ❌ Unsupported custom grains fail during post-processing.
- ⚠️ API clients receive runtime validation errors instead.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7eedb305197345ca93ac21077b5c2069&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=7eedb305197345ca93ac21077b5c2069&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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 fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43585&comment_hash=52ee0f23b89145263cb93e7948201d9c6958bc1d215c3ed73b6b93acd37cea62&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43585&comment_hash=52ee0f23b89145263cb93e7948201d9c6958bc1d215c3ed73b6b93acd37cea62&reaction=dislike'>👎</a>
##########
tests/unit_tests/charts/test_schemas.py:
##########
@@ -310,21 +310,30 @@ def
test_chart_data_query_object_schema_deprecated_fields_renamed(
@pytest.mark.parametrize(
"app",
- [{"TIME_GRAIN_ADDONS": {"PT10M": "10 minutes"}}],
+ [{"TIME_GRAIN_ADDONS": {"PT7M": "7 minutes"}}],
indirect=True,
)
def test_time_grain_validation_with_config_addons(app_context: None) -> None:
- """Test that validation includes TIME_GRAIN_ADDONS from config"""
- schema = ChartDataProphetOptionsSchema()
+ """
+ Test that custom TIME_GRAIN_ADDONS are accepted by ChartDataExtrasSchema
(SQLA)
+ but rejected by ChartDataProphetOptionsSchema (which only supports mapped
Prophet grains).
+ """
+ # Custom addon grain is valid for SQLA time grain
+ extras_schema = ChartDataExtrasSchema()
+ extras_result = extras_schema.load({"time_grain_sqla": "PT7M"})
+ assert extras_result["time_grain_sqla"] == "PT7M"
Review Comment:
**Suggestion:** The test will fail before reaching the Prophet assertion
because `ChartDataExtrasSchema.time_grain_sqla` constructs its `OneOf`
validator when `superset.charts.schemas` is imported, before the indirect app
fixture applies `TIME_GRAIN_ADDONS`. At that point `PT7M` is not in the
validator's choices, so the SQLA load raises `ValidationError` instead of
accepting the configured addon. Construct the schema after configuring the
choices dynamically, or update the schema/test setup so the addon is available
when the validator is created. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Target schema test fails before Prophet assertions.
- ⚠️ CI cannot validate the intended regression behavior.
- ⚠️ SQLA addon acceptance is incorrectly reported as invalid.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1a95413ac5e74c88bc17f9295bf320d6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1a95413ac5e74c88bc17f9295bf320d6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/charts/test_schemas.py
**Line:** 322:324
**Comment:**
*Possible Bug: The test will fail before reaching the Prophet assertion
because `ChartDataExtrasSchema.time_grain_sqla` constructs its `OneOf`
validator when `superset.charts.schemas` is imported, before the indirect app
fixture applies `TIME_GRAIN_ADDONS`. At that point `PT7M` is not in the
validator's choices, so the SQLA load raises `ValidationError` instead of
accepting the configured addon. Construct the schema after configuring the
choices dynamically, or update the schema/test setup so the addon is available
when the validator is created.
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 fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43585&comment_hash=bf43b0528e5cc3c110104ff0472e3f5c853b09a2962cf8c679c36a07d900923d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43585&comment_hash=bf43b0528e5cc3c110104ff0472e3f5c853b09a2962cf8c679c36a07d900923d&reaction=dislike'>👎</a>
--
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]
