Skip to content

fix: enhance validation logic to include workspace_id in model checks #7855

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wxg0103 merged 1 commit into from
Nov 12, 2026
Merged

fix: enhance validation logic to include workspace_id in model checks #7855

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions apps/models_provider/serializers/model_serializer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,9 +316,11 @@ class Create(serializers.Serializer):
credential = serializers.DictField(required=True, label=_("certification information"))
workspace_id = serializers.CharField(required=False, label=_("workspace id"), max_length=128)

def is_valid(self, *, raise_exception=False):
def is_valid(self, *, raise_exception=False, workspace_id=None):
if not super().is_valid(raise_exception=raise_exception):
return False
if workspace_id is None:
workspace_id = self.data.get("workspace_id", "None")
provider = self.data.get("provider")
try:
provider_handler = ModelProvideConstants[provider]
Expand All @@ -342,11 +344,7 @@ def is_valid(self, *, raise_exception=False):
400,
_("The field and default_value fields are required for the {index}th item").format(index=index),
)
if (
QuerySet(Model)
.filter(name=self.data.get("name"), workspace_id=self.data.get("workspace_id", "None"))
.exists()
):
if QuerySet(Model).filter(name=self.data.get("name"), workspace_id=workspace_id).exists():
raise AppApiException(
500, _("base model【{model_name}】already exists").format(model_name=self.data.get("name"))
)
Expand All @@ -364,7 +362,7 @@ def insert(self, workspace_id, with_valid=True):
status = Status.SUCCESS
if with_valid:
try:
self.is_valid(raise_exception=True)
self.is_valid(raise_exception=True, workspace_id=workspace_id)
except AppApiException as e:
if e.code == ValidCode.model_not_fount:
status = Status.DOWNLOAD

@shaohuzhang1 shaohuzhang1 Nov 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review & Analysis

Here is a concise analysis of the provided code diff, highlighting irregularities, potential issues, and optimization suggestions.

1. Irregularity / Bug: String "None" used as default

if workspace_id is None:
    workspace_id = self.data.get("workspace_id", "None")
  • Issue: Using the string "None" as a default value for workspace_id is dangerous and likely incorrect. If workspace_id is meant to be NULL or an empty string in the database, storing or filtering by the literal string "None" will cause logic errors.
  • Fix: Use None (Python object) or "" (empty string), depending on DB schema. Also, get key should match self.fields. Use self.data.get("workspace_id") or None.

2. API Contract Violation: Overriding is_valid() signature

  • Issue: Django REST Framework’s Serializer.is_valid() does not accept a workspace_id argument. Adding it here breaks polymorphism and can cause unexpected behavior if the serializer is used generically (e.g., by DRF views).
  • Suggestion: Remove workspace_id from is_valid(). Instead, ensure workspace_id is always available in self.initial_data or passed explicitly during validation via context (self.context.get('workspace_id')). Alternatively, perform the duplicate check directly in validate() or create().

3. Security & Logic Concern: Trusting User Input for Duplicate Check

provider = self.data.get("provider")
# ... later ...
QuerySet(Model).filter(name=self.data.get("name"), workspace_id=workspace_id).exists()
  • Issue:
    • If workspace_id falls back to user-provided data, a user might bypass constraints by providing a specific ID.
    • If workspace_id comes from request context (e.g., authenticated user’s workspace), it should not be optional in the serializer field definition unless you intend to allow cross-workspace operations (which is rare for multi-tenancy).
  • Suggestion: Ensure workspace_id is derived from authentication context, not just client input. If it must come from input, validate its integrity before DB queries.

4. Performance Optimization: Redundant .get() calls

  • Current: Repeatedly calls self.data.get("name"), self.data.get("provider"), etc.
  • Optimization: Extract values early after validation.
name = self.data.get("name")
provider = self.data.get("provider")
# Use these variables in subsequent checks

5. Error Handling: Exception swallowing risk

In insert():

try:
    self.is_valid(raise_exception=True, workspace_id=workspace_id)
except AppApiException as e:
    if e.code == ValidCode.model_not_fount: # Typo? "found" not "fount"
        status = Status.DOWNLOAD
  • Typo: model_not_fount → likely model_not_found.
  • Logic: Catching only one exception type is fine, but ensure other AppApiExceptions are re-raised or handled appropriately. Otherwise, invalid input (e.g., missing provider) will silently proceed with partial validation failure if not caught.

6. QuerySet Optimization: Avoid .exists() inside loops or heavy conditions

  • The duplicate check is outside the loop, so it’s acceptable. However, if this is called per-model in a batch, consider using bulk queries.

✅ Recommended Refactored Approach

Remove workspace_id from is_valid() and rely on context:

class Create(serializers.Serializer):
    credential = serializers.DictField(required=True, label=_("certification information"))
    name = serializers.CharField(required=True, max_length=256)  # Add explicit fields!
    provider = serializers.CharField(required=True)
    workspace_id = serializers.CharField(required=False, label=_("workspace id"), max_length=128)

    def validate(self, attrs):
        """
        Custom validation using DRF's built-in validator hook.
        """
        name = attrs["name"]
        provider = attrs["provider"]
        
        # Prefer workspace_id from context (set in view), fallback to input if needed
        workspace_id = self.context.get("workspace_id") or attrs.get("workspace_id")

        try:
            provider_handler = ModelProvideConstants[provider]
        except KeyError:
            raise valid.serializers.ValidationError(_("Unsupported provider."))

        # Example additional validations...
        for index, item in enumerate(attrs.get("fields", [])):
            if "field" not in item or "default_value" not in item:
                raise valid.serializers.ValidationError(
                    f"The field and default_value fields are required for the {index + 1}th item"
                )

        # Duplicate check
        if QuerySet(Model).filter(name=name, workspace_id=workspace_id).exists():
            raise valid.serializers.ValidationError(
                _("Base model 【{model_name}】 already exists.").format(model_name=name)
            )
            
        return attrs

    def create(self, validated_data):
        # Perform insert logic here
        pass

In the view:

serializer = Create(data=request.data)
serializer.validated_data  # Triggers is_valid/validate correctly
model_id = serializer.insert(workspace_id=request.user.workspace_id)

Note: Do not override is_valid() unless absolutely necessary. Use DRF patterns like validate() for field validation and create() for persistence logic.

Expand Down
Loading