bito-code-review[bot] commented on code in PR #40357: URL: https://github.com/apache/superset/pull/40357#discussion_r3316117656
########## superset/mcp_service/plugin/schemas.py: ########## @@ -0,0 +1,127 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Pydantic schemas for plugin-related MCP tool requests and responses.""" + +from pydantic import BaseModel, Field, field_validator, model_validator + + +class CreatePluginRequest(BaseModel): + """Request schema for create_plugin.""" + + name: str = Field( + ..., + min_length=1, + description="Human-friendly name for the plugin.", + ) + key: str = Field( + ..., + min_length=1, + description=( + "Unique plugin key. Should match the package name from the plugin's " + "package.json (e.g. '@my-org/my-chart-plugin')." + ), + ) + bundle_url: str = Field( + ..., + min_length=1, + description=( + "Full URL pointing to the built plugin bundle " + "(e.g. a CDN-hosted JavaScript file)." + ), + ) + + @field_validator("name", "key", "bundle_url") + @classmethod + def must_not_be_blank(cls, v: str) -> str: + if not v.strip(): + raise ValueError("Field must not be blank") + return v.strip() + + +class CreatePluginResponse(BaseModel): + """Response schema for create_plugin.""" + + id: int | None = Field( + None, + description="ID of the newly created plugin. None if creation failed.", + ) + name: str | None = Field(None, description="Plugin name.") + key: str | None = Field(None, description="Plugin key.") + bundle_url: str | None = Field(None, description="Plugin bundle URL.") + error: str | None = Field( + None, + description="Error message if creation failed, otherwise null.", + ) + + +class UpdatePluginRequest(BaseModel): + """Request schema for update_plugin.""" + + id: int = Field(..., description="ID of the plugin to update.") + name: str | None = Field( + None, + min_length=1, + description="New human-friendly name for the plugin.", + ) + key: str | None = Field( + None, + min_length=1, + description=( + "New unique plugin key. Should match the package name from the " + "plugin's package.json (e.g. '@my-org/my-chart-plugin')." + ), + ) + bundle_url: str | None = Field( + None, + min_length=1, + description=( + "New full URL pointing to the built plugin bundle " + "(e.g. a CDN-hosted JavaScript file)." + ), + ) + + @field_validator("name", "key", "bundle_url") + @classmethod + def must_not_be_blank(cls, v: str | None) -> str | None: + if v is not None and not v.strip(): + raise ValueError("Field must not be blank") + return v.strip() if v is not None else v Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Validation order prevents model check</b></div> <div id="fix"> In `UpdatePluginRequest`, the `field_validator` runs before `model_validator`. When `name=""`, the field validator raises `"Field must not be blank"` before `at_least_one_field()` can check if at least one field is provided. This causes a confusing error message — users get "Field must not be blank" instead of "At least one of 'name', 'key', or 'bundle_url' must be provided.". </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ``` --- superset/mcp_service/plugin/schemas.py +++ superset/mcp_service/plugin/schemas.py @@ -95,17 +95,24 @@ class UpdatePluginRequest(BaseModel): ) - @field_validator("name", "key", "bundle_url") + @model_validator(mode="before") + @classmethod + def _check_at_least_one_before(cls, data): + # Support both dict and model_dump input + if isinstance(data, dict): + values = data + else: + values = getattr(data, '__dict__', {}) + name = values.get("name") + key = values.get("key") + bundle_url = values.get("bundle_url") + if name is None and key is None and bundle_url is None: + raise ValueError( + "At least one of 'name', 'key', or 'bundle_url' must be provided." + ) + return data + + @field_validator("name", "key", "bundle_url", mode="after") @classmethod def must_not_be_blank(cls, v: str | None) -> str | None: if v is not None and not v.strip(): raise ValueError("Field must not be blank") return v.strip() if v is not None else v - - @model_validator(mode="after") - def at_least_one_field(self) -> "UpdatePluginRequest": - if self.name is None and self.key is None and self.bundle_url is None: - raise ValueError( - "At least one of 'name', 'key', or 'bundle_url' must be provided." - ) - return self ``` </div> </details> </div> <small><i>Code Review Run #f08c52</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid 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]
