Fix: code-rabbit review comment (#16940)

2026-07-16 04:37:21 +08:00 · 2026-07-15 15:44:08 +08:00
parent 166ed3c159
commit 63c010cd04
3 changed files with 213 additions and 26 deletions
--- a/internal/dao/tenant_model_instance.go
+++ b/internal/dao/tenant_model_instance.go
@@ -117,6 +117,16 @@ func (dao *TenantModelInstanceDAO) UpdateByID(id string, updates map[string]inte
 	return DB.Model(&entity.TenantModelInstance{}).Where("id = ?", id).Updates(updates).Error
 }

+// DeleteByIDs deletes all instances whose id is in the given list.
+// Mirrors Python's TenantModelInstanceService.delete_by_ids.
+func (dao *TenantModelInstanceDAO) DeleteByIDs(ids []string) (int64, error) {
+	if len(ids) == 0 {
+		return 0, nil
+	}
+	result := DB.Unscoped().Where("id IN ?", ids).Delete(&entity.TenantModelInstance{})
+	return result.RowsAffected, result.Error
+}
+
 // DeleteByProviderID deletes all instances for the given provider.
 func (dao *TenantModelInstanceDAO) DeleteByProviderID(providerID string) (int64, error) {
 	result := DB.Unscoped().Where("provider_id = ?", providerID).Delete(&entity.TenantModelInstance{})
--- a/internal/handler/providers.go
+++ b/internal/handler/providers.go
@@ -572,6 +572,11 @@ func (h *ProviderHandler) AlterModel(c *gin.Context) {
 		updateDict["extra"] = req.Extra
 	}

+	if len(updateDict) == 0 {
+		common.ResponseWithHttpCodeData(c, http.StatusBadRequest, common.CodeBadRequest, nil, "at least one update field is required besides model_name or model_id")
+		return
+	}
+
 	code, err := h.modelProviderService.AlterModel(providerName, instanceName, modelName, userID, modelID, updateDict)
 	if err != nil {
 		common.ErrorWithCode(c, code, err.Error())
--- a/internal/service/model_service.go
+++ b/internal/service/model_service.go
@@ -20,6 +20,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"os"
 	"ragflow/internal/common"
 	"ragflow/internal/dao"
 	"ragflow/internal/entity"
@@ -1312,15 +1313,165 @@ func (m *ModelProviderService) resolveModelListTenant(userID, ownerTenantID stri
 	return tenant, common.CodeSuccess, nil
 }

-// ensureMineruFromEnv is a stub that mirrors Python's ensure_mineru_from_env.
+// ensureMineruFromEnv mirrors Python's ensure_mineru_from_env.
 // It ensures a MinerU OCR provider instance exists when env vars are configured.
-func (m *ModelProviderService) ensureMineruFromEnv(tenantID string) error { return nil }
+func (m *ModelProviderService) ensureMineruFromEnv(tenantID string) error {
+	config := collectEnvConfig(mineruEnvKeys, mineruDefaultConfig)
+	return m.ensureOCRProviderFromEnv(tenantID, "MinerU", "mineru-from-env", config)
+}

-// ensurePaddleOCREnabledFromEnv is a stub that mirrors Python's ensure_paddleocr_from_env.
-func (m *ModelProviderService) ensurePaddleOCREnabledFromEnv(tenantID string) error { return nil }
+// ensurePaddleOCREnabledFromEnv mirrors Python's ensure_paddleocr_from_env.
+func (m *ModelProviderService) ensurePaddleOCREnabledFromEnv(tenantID string) error {
+	config := collectEnvConfig(paddleOCREnvKeys, paddleOCRDefaultConfig)
+	return m.ensureOCRProviderFromEnv(tenantID, "PaddleOCR", "paddleocr-from-env", config)
+}

-// ensureOpenDataLoaderFromEnv is a stub that mirrors Python's ensure_opendataloader_from_env.
-func (m *ModelProviderService) ensureOpenDataLoaderFromEnv(tenantID string) error { return nil }
+// ensureOpenDataLoaderFromEnv mirrors Python's ensure_opendataloader_from_env.
+func (m *ModelProviderService) ensureOpenDataLoaderFromEnv(tenantID string) error {
+	config := collectEnvConfig(openDataLoaderEnvKeys, openDataLoaderDefaultConfig)
+	return m.ensureOCRProviderFromEnv(tenantID, "OpenDataLoader", "opendataloader-from-env", config)
+}
+
+// env key / default config tables for the three OCR providers.
+// Mirrors common/constants.py MINERU_ENV_KEYS, PADDLEOCR_ENV_KEYS, OPENDATALOADER_ENV_KEYS.
+var (
+	mineruEnvKeys = []string{
+		common.EnvMineruApiServer,
+		"MINERU_OUTPUT_DIR",
+		common.EnvMineruBackend,
+		"MINERU_SERVER_URL",
+		"MINERU_DELETE_OUTPUT",
+	}
+	mineruDefaultConfig = map[string]interface{}{
+		common.EnvMineruApiServer: "",
+		"MINERU_OUTPUT_DIR":       "",
+		common.EnvMineruBackend:   "pipeline",
+		"MINERU_SERVER_URL":       "",
+		"MINERU_DELETE_OUTPUT":    1,
+	}
+	paddleOCREnvKeys = []string{
+		common.EnvPaddleOCRBaseUrl,
+		common.EnvPaddleOCRApiURL,
+		common.EnvPaddleOCRAccessToken,
+		common.EnvPaddleOCRAlgorithm,
+	}
+	paddleOCRDefaultConfig = map[string]interface{}{
+		common.EnvPaddleOCRBaseUrl:     "",
+		common.EnvPaddleOCRApiURL:      "",
+		common.EnvPaddleOCRAccessToken: nil,
+		common.EnvPaddleOCRAlgorithm:   "PaddleOCR-VL",
+	}
+	openDataLoaderEnvKeys = []string{
+		common.EnvOpenDataLoaderApiServer,
+	}
+	openDataLoaderDefaultConfig = map[string]interface{}{
+		common.EnvOpenDataLoaderApiServer: "",
+	}
+)
+
+// collectEnvConfig collects environment variable values for the given keys
+// into a map pre-populated with defaultConfig. Returns nil if none of the
+// env vars are actually set, matching Python's _collect_env_config.
+func collectEnvConfig(envKeys []string, defaultConfig map[string]interface{}) map[string]interface{} {
+	config := make(map[string]interface{}, len(defaultConfig))
+	for k, v := range defaultConfig {
+		config[k] = v
+	}
+	found := false
+	for _, key := range envKeys {
+		value := os.Getenv(key)
+		if value != "" {
+			found = true
+			config[key] = value
+		}
+	}
+	if !found {
+		return nil
+	}
+	return config
+}
+
+// ensureOCRProviderFromEnv mirrors Python's _ensure_ocr_provider_from_env.
+// It finds or creates a provider, instance, and model for the given OCR provider.
+func (m *ModelProviderService) ensureOCRProviderFromEnv(tenantID, providerName, modelName string, config map[string]interface{}) error {
+	if config == nil {
+		return nil
+	}
+
+	// 1. Find or create the provider.
+	provider, err := m.modelProviderDAO.GetByTenantIDAndProviderName(tenantID, providerName)
+	if err != nil {
+		if !dao.IsNotFoundErr(err) {
+			return fmt.Errorf("failed to get provider %s: %w", providerName, err)
+		}
+		providerID := utility.GenerateToken()
+		provider = &entity.TenantModelProvider{
+			ID:           providerID,
+			TenantID:     tenantID,
+			ProviderName: providerName,
+		}
+		if err := m.modelProviderDAO.Create(provider); err != nil {
+			return fmt.Errorf("failed to create provider %s: %w", providerName, err)
+		}
+	}
+
+	// 2. Find or create the instance (api_key stores the JSON config for dedup).
+	apiKeyBytes, err := json.Marshal(config)
+	if err != nil {
+		return fmt.Errorf("failed to marshal config for %s: %w", providerName, err)
+	}
+	apiKey := string(apiKeyBytes)
+
+	instance, err := m.modelInstanceDAO.GetInstanceByApiKey(apiKey, provider.ID)
+	if err != nil {
+		if !dao.IsNotFoundErr(err) {
+			return fmt.Errorf("failed to get instance for %s: %w", providerName, err)
+		}
+		instanceID := utility.GenerateToken()
+		instance = &entity.TenantModelInstance{
+			ID:           instanceID,
+			ProviderID:   provider.ID,
+			InstanceName: modelName,
+			APIKey:       apiKey,
+			Extra:        "{}",
+		}
+		if err := m.modelInstanceDAO.Create(instance); err != nil {
+			return fmt.Errorf("failed to create instance for %s: %w", providerName, err)
+		}
+	}
+
+	// 3. Find or create the model.
+	_, err = m.modelDAO.GetByProviderIDAndInstanceIDAndModelTypeAndModelName(
+		provider.ID,
+		instance.ID,
+		int(entity.ModelTypeOCR),
+		modelName,
+	)
+	if err != nil {
+		if !dao.IsNotFoundErr(err) {
+			return fmt.Errorf("failed to get model for %s: %w", providerName, err)
+		}
+		extraBytes, err := json.Marshal(map[string]int{"max_tokens": 0})
+		if err != nil {
+			return fmt.Errorf("failed to marshal extra for %s model: %w", providerName, err)
+		}
+		modelID := utility.GenerateToken()
+		tenantModel := &entity.TenantModel{
+			ID:         modelID,
+			ModelName:  modelName,
+			ProviderID: provider.ID,
+			InstanceID: instance.ID,
+			ModelType:  int(entity.ModelTypeOCR),
+			Status:     "active",
+			Extra:      string(extraBytes),
+		}
+		if err := m.modelDAO.Create(tenantModel); err != nil {
+			return fmt.Errorf("failed to create model for %s: %w", providerName, err)
+		}
+	}
+
+	return nil
+}

 func (m *ModelProviderService) AlterProviderInstance(userID, providerIDOrName, instanceIDOrName, newInstanceName, apiKey, baseURL, region string, modelInfo []CreateInstanceModelInfo, verify bool) (common.ErrorCode, error) {
 	providerIDOrName = strings.TrimSpace(providerIDOrName)
@@ -1518,33 +1669,50 @@ func (m *ModelProviderService) DropProviderInstances(providerIDOrName, userID st
 		return common.CodeServerError, err
 	}

-	// Delete each instance by provider_id + instance_name.
-	// Since instance names are unique within a provider, we can delete directly.
+	// First pass: resolve all instance IDs and collect not-found instances.
+	// Mirrors Python's drop_provider_instances which returns error immediately
+	// if any instance does not exist, without performing partial deletion.
 	notExistInstances := make([]string, 0)
-	for _, instanceName := range instanceIDOrNames {
-		instanceObj, err := m.modelInstanceDAO.GetByProviderIDAndInstanceName(provider.ID, instanceName)
-		if err != nil {
-			if errors.Is(err, gorm.ErrRecordNotFound) {
-				notExistInstances = append(notExistInstances, instanceName)
-				continue
+	instanceIDs := make([]string, 0, len(instanceIDOrNames))
+	for _, idOrName := range instanceIDOrNames {
+		var instance *entity.TenantModelInstance
+		// Try by ID first, then by name — same as Python.
+		if idOrName != "" {
+			instance, err = m.modelInstanceDAO.GetByID(idOrName)
+			if err != nil && !errors.Is(err, gorm.ErrRecordNotFound) {
+				return common.CodeServerError, err
+			}
+			if instance != nil && instance.ProviderID != provider.ID {
+				instance = nil
 			}
-			return common.CodeServerError, err
 		}
-
-		// Delete models under this instance first
-		if _, err := m.modelDAO.DeleteByProviderIDAndInstanceID(provider.ID, instanceObj.ID); err != nil {
-			return common.CodeServerError, err
-		}
-
-		// Delete the instance itself
-		if _, err := m.modelInstanceDAO.DeleteByProviderIDAndInstanceName(provider.ID, instanceName); err != nil {
-			return common.CodeServerError, err
+		if instance == nil {
+			instance, err = m.modelInstanceDAO.GetByProviderIDAndInstanceName(provider.ID, idOrName)
+			if err != nil {
+				if errors.Is(err, gorm.ErrRecordNotFound) {
+					notExistInstances = append(notExistInstances, idOrName)
+					continue
+				}
+				return common.CodeServerError, err
+			}
 		}
+		instanceIDs = append(instanceIDs, instance.ID)
 	}
+
 	if len(notExistInstances) > 0 {
 		return common.CodeNotFound, fmt.Errorf("no instance found for provider %q and instances %q", providerIDOrName, notExistInstances)
 	}

+	// Second pass: delete models and instances by IDs.
+	// Mirrors Python's: delete_models_by_instance_ids(instance_ids)
+	//                   TenantModelInstanceService.delete_by_ids(instance_ids)
+	if _, err := m.modelDAO.DeleteByInstanceIDs(instanceIDs); err != nil {
+		return common.CodeServerError, err
+	}
+	if _, err := m.modelInstanceDAO.DeleteByIDs(instanceIDs); err != nil {
+		return common.CodeServerError, err
+	}
+
 	return common.CodeSuccess, nil
 }

@@ -1597,10 +1765,14 @@ func (m *ModelProviderService) DropInstanceModels(providerIDOrName, instanceIDOr
 		return common.CodeNotFound, fmt.Errorf("Models %v not found for provider '%s' and instance '%s'", notExist, providerIDOrName, instanceIDOrName)
 	}

-	// Collect IDs of models to delete.
+	// Collect IDs of only the requested models to delete.
+	requestedNames := make(map[string]struct{}, len(modelNames))
+	for _, name := range modelNames {
+		requestedNames[name] = struct{}{}
+	}
 	idsToDelete := make([]string, 0, len(modelNames))
 	for _, obj := range modelObjs {
-		if _, ok := existingNames[obj.ModelName]; ok {
+		if _, ok := requestedNames[obj.ModelName]; ok {
 			idsToDelete = append(idsToDelete, obj.ID)
 		}
 	}