enhance: [2.4]altercollection validation logic (#38421)

altercollection validation logic issue: #37436 pr: #38419 --------- Signed-off-by: Xianhui.Lin <[email protected]>
milvus-io · Dec 12, 2024 · 24c8883 · 24c8883
1 parent 19818c5
commit 24c8883
Show file tree

Hide file tree

Showing 4 changed files with 60 additions and 44 deletions.
diff --git a/internal/proxy/task.go b/internal/proxy/task.go
@@ -20,6 +20,7 @@ import (
 	"context"
 	"fmt"
 	"math"
+	"strconv"
 	"time"
 
 	"github.com/cockroachdb/errors"
@@ -982,7 +983,7 @@ func (t *alterCollectionTask) PreExecute(ctx context.Context) error {
 				return err
 			}
 			if loaded {
-				return merr.WrapErrCollectionLoaded(" %s %s can not delete mmap properties if collection loaded", t.CollectionName, key)
+				return merr.WrapErrCollectionLoaded(t.CollectionName, "can not delete mmap properties if collection loaded")
 			}
 		}
 	}
@@ -1108,24 +1109,67 @@ func (t *alterCollectionFieldTask) OnEnqueue() error {
 	return nil
 }
 
+var allowedProps = []string{
+	common.MaxLengthKey,
+	common.MmapEnabledKey,
+}
+
+func IsKeyAllowed(key string) bool {
+	for _, allowedKey := range allowedProps {
+		if key == allowedKey {
+			return true
+		}
+	}
+	return false
+}
+
 func (t *alterCollectionFieldTask) PreExecute(ctx context.Context) error {
 	collSchema, err := globalMetaCache.GetCollectionSchema(ctx, t.GetDbName(), t.CollectionName)
 	if err != nil {
 		return err
 	}
 
-	IsStringType := false
-	fieldName := ""
-	var dataType int32
-	for _, field := range collSchema.Fields {
-		if field.GetName() == t.FieldName && (typeutil.IsStringType(field.DataType) || typeutil.IsArrayContainStringElementType(field.DataType, field.ElementType)) {
-			IsStringType = true
-			fieldName = field.GetName()
-			dataType = int32(field.DataType)
+	for _, prop := range t.Properties {
+		if !IsKeyAllowed(prop.Key) {
+			return merr.WrapErrParameterInvalidMsg("%s does not allow update in collection field param", prop.Key)
+		}
+		// Check the value type based on the key
+		switch prop.Key {
+		case common.MmapEnabledKey:
+			collectionID, err := globalMetaCache.GetCollectionID(ctx, t.GetDbName(), t.CollectionName)
+			if err != nil {
+				return err
+			}
+			loaded, err1 := isCollectionLoaded(ctx, t.queryCoord, collectionID)
+			if err1 != nil {
+				return err1
+			}
+			if loaded {
+				return merr.WrapErrCollectionLoaded(t.CollectionName, "can not alter collection field properties if collection loaded")
+			}
+		case common.MaxLengthKey:
+			IsStringType := false
+			fieldName := ""
+			var dataType int32
+			for _, field := range collSchema.Fields {
+				if field.GetName() == t.FieldName && (typeutil.IsStringType(field.DataType) || typeutil.IsArrayContainStringElementType(field.DataType, field.ElementType)) {
+					IsStringType = true
+					fieldName = field.GetName()
+					dataType = int32(field.DataType)
+				}
+			}
+			if !IsStringType {
+				return merr.WrapErrParameterInvalid(fieldName, "%s can not modify the maxlength for non-string types", schemapb.DataType_name[dataType])
+			}
+			value, err := strconv.Atoi(prop.Value)
+			if err != nil {
+				return merr.WrapErrParameterInvalid("%s should be an integer, but got %T", prop.Key, prop.Value)
+			}
+
+			if value > defaultMaxVarCharLength {
+				return merr.WrapErrParameterInvalid("%s exceeds the maximum allowed value 65535", prop.Value)
+			}
 		}
-	}
-	if !IsStringType {
-		return merr.WrapErrParameterInvalid(fieldName, "%s can not modify the maxlength for non-string types", schemapb.DataType_name[dataType])
 	}
 
 	return nil

diff --git a/internal/proxy/task_index.go b/internal/proxy/task_index.go
@@ -550,13 +550,13 @@ func (t *alterIndexTask) PreExecute(ctx context.Context) error {
 	if len(t.req.GetExtraParams()) > 0 {
 		for _, param := range t.req.GetExtraParams() {
 			if !indexparams.IsConfigableIndexParam(param.GetKey()) {
-				return merr.WrapErrParameterInvalidMsg("%s is not configable index param in extraParams", param.GetKey())
+				return merr.WrapErrParameterInvalidMsg("%s is not a configable index proptery", param.GetKey())
 			}
 		}
 	} else if len(t.req.GetDeleteKeys()) > 0 {
 		for _, param := range t.req.GetDeleteKeys() {
 			if !indexparams.IsConfigableIndexParam(param) {
-				return merr.WrapErrParameterInvalidMsg("%s is not configable index param in deleteKeys", param)
+				return merr.WrapErrParameterInvalidMsg("%s is not a configable index proptery", param)
 			}
 		}
 	}

diff --git a/internal/rootcoord/alter_collection_task.go b/internal/rootcoord/alter_collection_task.go
@@ -163,11 +163,6 @@ func (a *alterCollectionFieldTask) Prepare(ctx context.Context) error {
 		return fmt.Errorf("alter collection field failed, filed name does not exists")
 	}
 
-	err := IsValidateUpdatedFieldProps(a.Req.GetProperties())
-	if err != nil {
-		return err
-	}
-
 	return nil
 }
 
@@ -221,29 +216,6 @@ func (a *alterCollectionFieldTask) Execute(ctx context.Context) error {
 	return redoTask.Execute(ctx)
 }
 
-var allowedProps = []string{
-	common.MaxLengthKey,
-	common.MmapEnabledKey,
-}
-
-func IsKeyAllowed(key string) bool {
-	for _, allowedKey := range allowedProps {
-		if key == allowedKey {
-			return true
-		}
-	}
-	return false
-}
-
-func IsValidateUpdatedFieldProps(updatedProps []*commonpb.KeyValuePair) error {
-	for _, prop := range updatedProps {
-		if !IsKeyAllowed(prop.Key) {
-			return merr.WrapErrParameterInvalidMsg("%s does not allow update in collection field param", prop.Key)
-		}
-	}
-	return nil
-}
-
 func UpdateFieldProperties(coll *model.Collection, fieldName string, updatedProps []*commonpb.KeyValuePair) error {
 	for i, field := range coll.Fields {
 		if field.Name == fieldName {

diff --git a/tests/python_client/requirements.txt b/tests/python_client/requirements.txt
@@ -12,8 +12,8 @@ allure-pytest==2.7.0
 pytest-print==0.2.1
 pytest-level==0.1.1
 pytest-xdist==2.5.0
-pymilvus==2.4.11rc5
-pymilvus[bulk_writer]==2.4.11rc5
+pymilvus==2.4.11rc8
+pymilvus[bulk_writer]==2.4.11rc8
 pytest-rerunfailures==9.1.1
 git+https://github.com/Projectplace/pytest-tags
 ndg-httpsclient