[GitHub] [apisix-dashboard] tokers commented on a diff in pull request #2474: feat: integrate data loader interface to import

[GitBox <gi...@apache.org>]

tokers commented on code in PR #2474:
URL: https://github.com/apache/apisix-dashboard/pull/2474#discussion_r901497766


##########
api/internal/handler/data_loader/loader/openapi3/import_test.go:
##########
@@ -0,0 +1,109 @@
+/*
+ * 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.
+ */
+package openapi3
+
+import (
+	"io/ioutil"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/apisix/manager-api/internal/core/entity"
+)
+
+var (
+	TestAPI101 = "../../../../../test/testdata/import/Postman-API101.yaml"
+)
+
+// Test API 101 on no MergeMethod mode
+func TestParseAPI101NoMerge(t *testing.T) {
+	fileContent, err := ioutil.ReadFile(TestAPI101)
+	assert.NoError(t, err)
+
+	l := &Loader{MergeMethod: false, TaskName: "test"}
+	data, err := l.Import(fileContent)
+	assert.NoError(t, err)
+
+	assert.Len(t, data.Routes, 5)
+	assert.Len(t, data.Upstreams, 1)
+
+	// Upstream
+	assert.Equal(t, "https", data.Upstreams[0].Scheme)
+	assert.Equal(t, float64(1), data.Upstreams[0].Nodes.(map[string]float64)["api-101.glitch.me"])
+	assert.Equal(t, "test", data.Upstreams[0].Name)
+	assert.Equal(t, "roundrobin", data.Upstreams[0].Type)
+
+	// Route
+	assert.Equal(t, data.Upstreams[0].ID, data.Routes[0].UpstreamID)
+	for _, route := range data.Routes {
+		switch route.Name {
+		case "test_customer_GET":
+			assert.Contains(t, route.Uris, "/customer")
+			assert.Contains(t, route.Methods, "GET")
+			assert.Equal(t, "Get one customer", route.Desc)
+			assert.Equal(t, entity.Status(0), route.Status)
+		case "test_customer/{customer_id}_PUT":
+			assert.Contains(t, route.Uris, "/customer/*")
+			assert.Contains(t, route.Methods, "PUT")
+			assert.Equal(t, "Update customer", route.Desc)
+			assert.Equal(t, entity.Status(0), route.Status)
+		case "test_customer/{customer_id}_DELETE":
+			assert.Contains(t, route.Uris, "/customer/*")
+			assert.Contains(t, route.Methods, "DELETE")
+			assert.Equal(t, "Remove customer", route.Desc)
+			assert.Equal(t, entity.Status(0), route.Status)

Review Comment:
   Ditto. Here for the `default` case, the test should fail.



##########
api/internal/handler/data_loader/loader/openapi3/import.go:
##########
@@ -0,0 +1,142 @@
+/*
+ * 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.
+ */
+package openapi3
+
+import (
+	"errors"
+	"net/url"
+	"strings"
+	"time"
+
+	"github.com/getkin/kin-openapi/openapi3"
+
+	"github.com/apisix/manager-api/internal/core/entity"
+	"github.com/apisix/manager-api/internal/handler/data_loader/loader"
+	"github.com/apisix/manager-api/internal/utils/consts"
+)
+
+func (o Loader) Import(input interface{}) (*loader.DataSets, error) {
+	if input == nil {
+		return nil, errors.New("input is nil")
+	}
+
+	// load OAS3 document
+	swagger, err := openapi3.NewSwaggerLoader().LoadSwaggerFromData(input.([]byte))

Review Comment:
   Behaviors here are not consistent. When `input` is `nil`, we just return error but if it's not `[]byte`, the program will be panic. It should be consistent.



##########
api/internal/handler/data_loader/route_import.go:
##########
@@ -19,535 +19,317 @@ package data_loader
 import (
 	"bytes"
 	"context"
-	"encoding/json"
 	"fmt"
-	"net/http"
 	"path"
 	"reflect"
-	"regexp"
-	"strings"
 
-	"github.com/getkin/kin-openapi/openapi3"
 	"github.com/gin-gonic/gin"
+	"github.com/juliangruber/go-intersect"
+	"github.com/pkg/errors"
 	"github.com/shiningrush/droplet"
-	"github.com/shiningrush/droplet/data"
 	"github.com/shiningrush/droplet/wrapper"
 	wgin "github.com/shiningrush/droplet/wrapper/gin"
 
 	"github.com/apisix/manager-api/internal/conf"
 	"github.com/apisix/manager-api/internal/core/entity"
 	"github.com/apisix/manager-api/internal/core/store"
 	"github.com/apisix/manager-api/internal/handler"
-	"github.com/apisix/manager-api/internal/log"
-	"github.com/apisix/manager-api/internal/utils"
-	"github.com/apisix/manager-api/internal/utils/consts"
+	loader "github.com/apisix/manager-api/internal/handler/data_loader/loader"
+	"github.com/apisix/manager-api/internal/handler/data_loader/loader/openapi3"
 )
 
 type ImportHandler struct {
-	routeStore    *store.GenericStore
-	svcStore      store.Interface
-	upstreamStore store.Interface
+	routeStore        store.Interface
+	upstreamStore     store.Interface
+	serviceStore      store.Interface
+	consumerStore     store.Interface
+	sslStore          store.Interface
+	streamRouteStore  store.Interface
+	globalPluginStore store.Interface
+	pluginConfigStore store.Interface
+	protoStore        store.Interface
 }
 
 func NewImportHandler() (handler.RouteRegister, error) {
 	return &ImportHandler{
-		routeStore:    store.GetStore(store.HubKeyRoute),
-		svcStore:      store.GetStore(store.HubKeyService),
-		upstreamStore: store.GetStore(store.HubKeyUpstream),
+		routeStore:        store.GetStore(store.HubKeyRoute),
+		upstreamStore:     store.GetStore(store.HubKeyUpstream),
+		serviceStore:      store.GetStore(store.HubKeyService),
+		consumerStore:     store.GetStore(store.HubKeyConsumer),
+		sslStore:          store.GetStore(store.HubKeySsl),
+		streamRouteStore:  store.GetStore(store.HubKeyStreamRoute),
+		globalPluginStore: store.GetStore(store.HubKeyGlobalRule),
+		pluginConfigStore: store.GetStore(store.HubKeyPluginConfig),
+		protoStore:        store.GetStore(store.HubKeyProto),
 	}, nil
 }
 
-var regPathVar = regexp.MustCompile(`{[\w.]*}`)
-var regPathRepeat = regexp.MustCompile(`-APISIX-REPEAT-URI-[\d]*`)
-
 func (h *ImportHandler) ApplyRoute(r *gin.Engine) {
 	r.POST("/apisix/admin/import/routes", wgin.Wraps(h.Import,
 		wrapper.InputType(reflect.TypeOf(ImportInput{}))))
 }
 
+type ImportResult struct {
+	Total  int      `json:"total"`
+	Failed int      `json:"failed"`
+	Errors []string `json:"errors"`
+}
+
+type LoaderType string
+
 type ImportInput struct {
-	Force       bool   `auto_read:"force,query"`
+	Type        string `auto_read:"type"`
+	TaskName    string `auto_read:"task_name"`
 	FileName    string `auto_read:"_file"`
 	FileContent []byte `auto_read:"file"`
+
+	MergeMethod string `auto_read:"merge_method"`
 }
 
+const (
+	LoaderTypeOpenAPI3 LoaderType = "openapi3"
+)
+
 func (h *ImportHandler) Import(c droplet.Context) (interface{}, error) {
 	input := c.Input().(*ImportInput)
-	Force := input.Force
 
-	// file check
+	// input file content check
 	suffix := path.Ext(input.FileName)
 	if suffix != ".json" && suffix != ".yaml" && suffix != ".yml" {
-		return nil, fmt.Errorf("required file type is .yaml, .yml or .json but got: %s", suffix)
+		return nil, errors.Errorf("required file type is .yaml, .yml or .json but got: %s", suffix)
 	}
-
 	contentLen := bytes.Count(input.FileContent, nil) - 1
-	if contentLen > conf.ImportSizeLimit {
-		log.Warnf("upload file size exceeds limit: %d", contentLen)
-		return nil, fmt.Errorf("the file size exceeds the limit; limit %d", conf.ImportSizeLimit)
+	if contentLen <= 0 {
+		return nil, errors.New("uploaded file is empty")
 	}
-
-	swagger, err := openapi3.NewSwaggerLoader().LoadSwaggerFromData(input.FileContent)
-	if err != nil {
-		return nil, err
+	if contentLen > conf.ImportSizeLimit {
+		return nil, errors.Errorf("uploaded file size exceeds the limit, limit is %d", conf.ImportSizeLimit)
 	}
 
-	if len(swagger.Paths) < 1 {
-		return &data.SpecCodeResponse{StatusCode: http.StatusBadRequest},
-			consts.ErrImportFile
+	var l loader.Loader
+	switch LoaderType(input.Type) {
+	case LoaderTypeOpenAPI3:
+		l = &openapi3.Loader{
+			MergeMethod: input.MergeMethod == "true",
+			TaskName:    input.TaskName,
+		}
+		break
+	default:
+		return nil, fmt.Errorf("unsupported data loader type: %s", input.Type)
 	}
 
-	routes, err := OpenAPI3ToRoute(swagger)
+	dataSets, err := l.Import(input.FileContent)
 	if err != nil {
 		return nil, err
 	}
 
-	// check route
-	for _, route := range routes {
-		err := checkRouteExist(c.Context(), h.routeStore, route)
-		if err != nil && !Force {
-			log.Warnf("import duplicate: %s, route: %#v", err, route)
-			return &data.SpecCodeResponse{StatusCode: http.StatusBadRequest},
-				fmt.Errorf("route(uris:%v) conflict, %s", route.Uris, err)
-		}
-		if route.ServiceID != nil {
-			_, err := h.svcStore.Get(c.Context(), utils.InterfaceToString(route.ServiceID))
-			if err != nil {
-				if err == data.ErrNotFound {
-					return &data.SpecCodeResponse{StatusCode: http.StatusBadRequest},
-						fmt.Errorf(consts.IDNotFound, "service", route.ServiceID)
-				}
-				return &data.SpecCodeResponse{StatusCode: http.StatusBadRequest}, err
-			}
-		}
-		if route.UpstreamID != nil {
-			_, err := h.upstreamStore.Get(c.Context(), utils.InterfaceToString(route.UpstreamID))
-			if err != nil {
-				if err == data.ErrNotFound {
-					return &data.SpecCodeResponse{StatusCode: http.StatusBadRequest},
-						fmt.Errorf(consts.IDNotFound, "upstream", route.UpstreamID)
-				}
-				return &data.SpecCodeResponse{StatusCode: http.StatusBadRequest}, err
-			}
-		}
-
-		if _, err := h.routeStore.CreateCheck(route); err != nil {
-			return handler.SpecCodeResponse(err),
-				fmt.Errorf("create route(uris:%v) failed: %s", route.Uris, err)
-		}
+	// Pre-checking for route duplication
+	preCheckErrs := h.preCheck(c.Context(), dataSets)
+	if _, ok := preCheckErrs[store.HubKeyRoute]; ok && len(preCheckErrs[store.HubKeyRoute]) > 0 {
+		return h.convertToImportResult(dataSets, preCheckErrs), nil
 	}
 
-	// merge route
-	idRoute := make(map[string]*entity.Route)
-	for _, route := range routes {
-		if existRoute, ok := idRoute[route.ID.(string)]; ok {
-			uris := append(existRoute.Uris, route.Uris...)
-			existRoute.Uris = uris
-		} else {
-			idRoute[route.ID.(string)] = route
-		}
-	}
-
-	routes = make([]*entity.Route, 0, len(idRoute))
-	for _, route := range idRoute {
-		routes = append(routes, route)
-	}
-
-	// create route
-	for _, route := range routes {
-		if Force && route.ID != nil {
-			if _, err := h.routeStore.Update(c.Context(), route, true); err != nil {
-				return handler.SpecCodeResponse(err),
-					fmt.Errorf("update route(uris:%v) failed: %s", route.Uris, err)
-			}
-		} else {
-			if _, err := h.routeStore.Create(c.Context(), route); err != nil {
-				return handler.SpecCodeResponse(err),
-					fmt.Errorf("create route(uris:%v) failed: %s", route.Uris, err)
-			}
-		}
-	}
-
-	return map[string]int{
-		"paths":  len(swagger.Paths),
-		"routes": len(routes),
-	}, nil
+	// Create APISIX resources
+	createErrs := h.createEntities(c.Context(), dataSets)
+	return h.convertToImportResult(dataSets, createErrs), nil
 }
 
-func checkRouteExist(ctx context.Context, routeStore *store.GenericStore, route *entity.Route) error {
-	//routeStore := store.GetStore(store.HubKeyRoute)
-	ret, err := routeStore.List(ctx, store.ListInput{
-		Predicate: func(obj interface{}) bool {
-			id := utils.InterfaceToString(route.ID)
-			item := obj.(*entity.Route)
-			if id != "" && id != utils.InterfaceToString(item.ID) {
-				return false
-			}
-
-			itemUris := item.Uris
-			if item.URI != "" {
-				if itemUris == nil {
-					itemUris = []string{item.URI}
-				} else {
-					itemUris = append(itemUris, item.URI)
-				}
-			}
-
-			routeUris := route.Uris
-			if route.URI != "" {
-				if routeUris == nil {
-					routeUris = []string{route.URI}
-				} else {
-					routeUris = append(routeUris, route.URI)
+// Pre-check imported data for duplicates
+// The main problem facing duplication is routing, so here
+// we mainly check the duplication of routes, based on
+// domain name and uri.
+func (h *ImportHandler) preCheck(ctx context.Context, data *loader.DataSets) map[store.HubKey][]string {
+	errs := make(map[store.HubKey][]string)
+	for _, route := range data.Routes {
+		errs[store.HubKeyRoute] = make([]string, 0)
+		o, err := h.routeStore.List(ctx, store.ListInput{
+			// The check logic here is that if when a duplicate HOST or URI
+			// has been found, the HTTP method is checked for overlap, and
+			// if there is overlap it is determined to be a duplicate route
+			// and the import is rejected.
+			Predicate: func(obj interface{}) bool {
+				r := obj.(*entity.Route)
+				isMethodDuplicated := len(intersect.Hash(r.Methods, route.Methods)) > 0
+
+				// Check URI and host duplication
+				// First check for duplicate URIs
+				if (r.URI != "" && route.URI != "" && r.URI == route.URI) || len(intersect.Hash(r.Uris, route.Uris)) > 0 {
+					// Then check if the host field exists, and if it does, check for duplicates
+					if r.Host != "" && route.Host != "" {
+						return r.Host == route.Host && isMethodDuplicated
+					} else if len(r.Hosts) > 0 && len(route.Hosts) > 0 {
+						return len(intersect.Hash(r.Hosts, route.Hosts)) > 0 && isMethodDuplicated
+					}
+					// If the host field does not exist, only the presence or absence
+					// of HTTP method duplication is returned by default.
+					return isMethodDuplicated
 				}
-			}
-
-			if !(item.Host == route.Host && utils.StringSliceContains(itemUris, routeUris) &&
-				utils.StringSliceEqual(item.RemoteAddrs, route.RemoteAddrs) && item.RemoteAddr == route.RemoteAddr &&
-				utils.StringSliceEqual(item.Hosts, route.Hosts) && item.Priority == route.Priority &&
-				utils.ValueEqual(item.Vars, route.Vars) && item.FilterFunc == route.FilterFunc) {
 				return false
+			},
+			PageSize:   0,
+			PageNumber: 0,
+		})
+		if err != nil {
+			// When a special storage layer error occurs, return directly.
+			return map[store.HubKey][]string{
+				store.HubKeyRoute: {err.Error()},
 			}
-			return true
-		},
-		PageSize:   0,
-		PageNumber: 0,
-	})
-	if err != nil {
-		return err
-	}
-	if len(ret.Rows) > 0 {
-		return consts.InvalidParam("route is duplicate")
-	}
-	return nil
-}
-
-func parseExtension(val *openapi3.Operation) (*entity.Route, error) {
-	routeMap := map[string]interface{}{}
-	for key, val := range val.Extensions {
-		if strings.HasPrefix(key, "x-apisix-") {
-			routeMap[strings.TrimPrefix(key, "x-apisix-")] = val
 		}
-	}
-
-	route := new(entity.Route)
-	routeJson, err := json.Marshal(routeMap)
-	if err != nil {
-		return nil, err
-	}
-
-	err = json.Unmarshal(routeJson, &route)
-	if err != nil {
-		return nil, err
-	}
-
-	return route, nil
-}
 
-type PathValue struct {
-	Method string
-	Value  *openapi3.Operation
-}
-
-func mergePathValue(key string, values []PathValue, swagger *openapi3.Swagger) (map[string]*entity.Route, error) {
-	var parsed []PathValue
-	var routes = map[string]*entity.Route{}
-	for _, value := range values {
-		value.Value.OperationID = strings.Replace(value.Value.OperationID, value.Method, "", 1)
-		var eq = false
-		for _, v := range parsed {
-			if utils.ValueEqual(v.Value, value.Value) {
-				eq = true
-				if routes[v.Method].Methods == nil {
-					routes[v.Method].Methods = []string{}
+		// Duplicate routes found
+		if o.TotalSize > 0 {
+			for _, row := range o.Rows {
+				r, ok := row.(*entity.Route)
+				if ok {
+					errs[store.HubKeyRoute] = append(errs[store.HubKeyRoute],
+						errors.Errorf("%s is duplicated with route %s",
+							route.Uris[0],
+							r.Name).
+							Error())
 				}
-				routes[v.Method].Methods = append(routes[v.Method].Methods, value.Method)
-			}
-		}
-		// not equal to the previous ones
-		if !eq {
-			route, err := getRouteFromPaths(value.Method, key, value.Value, swagger)
-			if err != nil {
-				return nil, err
 			}
-			routes[value.Method] = route
-			parsed = append(parsed, value)
 		}
 	}
 
-	return routes, nil
+	return errs
 }
 
-func OpenAPI3ToRoute(swagger *openapi3.Swagger) ([]*entity.Route, error) {
-	var routes []*entity.Route
-	paths := swagger.Paths
-	var upstream *entity.UpstreamDef
-	var err error
-	for k, v := range paths {
-		k = regPathRepeat.ReplaceAllString(k, "")
-		upstream = &entity.UpstreamDef{}
-		if up, ok := v.Extensions["x-apisix-upstream"]; ok {
-			err = json.Unmarshal(up.(json.RawMessage), upstream)
-			if err != nil {
-				return nil, err
-			}
-		}
+// Create parsed resources
+func (h *ImportHandler) createEntities(ctx context.Context, data *loader.DataSets) map[store.HubKey][]string {
+	errs := make(map[store.HubKey][]string)
 
-		var values []PathValue
-		if v.Get != nil {
-			value := PathValue{
-				Method: http.MethodGet,
-				Value:  v.Get,
-			}
-			values = append(values, value)
-		}
-		if v.Post != nil {
-			value := PathValue{
-				Method: http.MethodPost,
-				Value:  v.Post,
+	for _, route := range data.Routes {
+		_, err := h.routeStore.Create(ctx, &route)
+		if err != nil {
+			if errs[store.HubKeyRoute] == nil {
+				errs[store.HubKeyRoute] = []string{}

Review Comment:
   The explicit initialization is not necessary because append will do it .



##########
api/internal/handler/data_loader/loader/openapi3/import_test.go:
##########
@@ -0,0 +1,109 @@
+/*
+ * 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.
+ */
+package openapi3
+
+import (
+	"io/ioutil"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/apisix/manager-api/internal/core/entity"
+)
+
+var (
+	TestAPI101 = "../../../../../test/testdata/import/Postman-API101.yaml"
+)
+
+// Test API 101 on no MergeMethod mode
+func TestParseAPI101NoMerge(t *testing.T) {
+	fileContent, err := ioutil.ReadFile(TestAPI101)
+	assert.NoError(t, err)
+
+	l := &Loader{MergeMethod: false, TaskName: "test"}
+	data, err := l.Import(fileContent)
+	assert.NoError(t, err)
+
+	assert.Len(t, data.Routes, 5)
+	assert.Len(t, data.Upstreams, 1)
+
+	// Upstream
+	assert.Equal(t, "https", data.Upstreams[0].Scheme)
+	assert.Equal(t, float64(1), data.Upstreams[0].Nodes.(map[string]float64)["api-101.glitch.me"])
+	assert.Equal(t, "test", data.Upstreams[0].Name)
+	assert.Equal(t, "roundrobin", data.Upstreams[0].Type)
+
+	// Route
+	assert.Equal(t, data.Upstreams[0].ID, data.Routes[0].UpstreamID)
+	for _, route := range data.Routes {
+		switch route.Name {
+		case "test_customer_GET":
+			assert.Contains(t, route.Uris, "/customer")
+			assert.Contains(t, route.Methods, "GET")
+			assert.Equal(t, "Get one customer", route.Desc)
+			assert.Equal(t, entity.Status(0), route.Status)
+		case "test_customer/{customer_id}_PUT":
+			assert.Contains(t, route.Uris, "/customer/*")
+			assert.Contains(t, route.Methods, "PUT")
+			assert.Equal(t, "Update customer", route.Desc)
+			assert.Equal(t, entity.Status(0), route.Status)
+		case "test_customer/{customer_id}_DELETE":
+			assert.Contains(t, route.Uris, "/customer/*")
+			assert.Contains(t, route.Methods, "DELETE")
+			assert.Equal(t, "Remove customer", route.Desc)
+			assert.Equal(t, entity.Status(0), route.Status)
+		}
+	}
+}
+
+// Test API 101 on MergeMethod mode
+func TestParseAPI101Merge(t *testing.T) {
+	fileContent, err := ioutil.ReadFile(TestAPI101)
+	assert.NoError(t, err)
+
+	l := &Loader{MergeMethod: true, TaskName: "test"}
+	data, err := l.Import(fileContent)
+	assert.NoError(t, err)
+
+	assert.Len(t, data.Routes, 3)
+	assert.Len(t, data.Upstreams, 1)
+
+	// Upstream
+	assert.Equal(t, "https", data.Upstreams[0].Scheme)
+	assert.Equal(t, float64(1), data.Upstreams[0].Nodes.(map[string]float64)["api-101.glitch.me"])
+	assert.Equal(t, "test", data.Upstreams[0].Name)
+	assert.Equal(t, "roundrobin", data.Upstreams[0].Type)
+
+	// Route
+	assert.Equal(t, data.Upstreams[0].ID, data.Routes[0].UpstreamID)
+	for _, route := range data.Routes {
+		switch route.Name {
+		case "test_customer":
+			assert.Contains(t, route.Uris, "/customer")
+			assert.Contains(t, route.Methods, "GET", "GET")
+			assert.Equal(t, entity.Status(0), route.Status)
+		case "test_customers":
+			assert.Contains(t, route.Uris, "/customers")
+			assert.Contains(t, route.Methods, "GET")
+			assert.Equal(t, entity.Status(0), route.Status)
+		case "test_customer/{customer_id}":
+			assert.Contains(t, route.Uris, "/customer/*")
+			assert.Contains(t, route.Methods, "PUT", "DELETE")
+			assert.Equal(t, entity.Status(0), route.Status)

Review Comment:
   Ditto.



##########
api/internal/handler/data_loader/route_import.go:
##########
@@ -19,535 +19,317 @@ package data_loader
 import (
 	"bytes"
 	"context"
-	"encoding/json"
 	"fmt"
-	"net/http"
 	"path"
 	"reflect"
-	"regexp"
-	"strings"
 
-	"github.com/getkin/kin-openapi/openapi3"
 	"github.com/gin-gonic/gin"
+	"github.com/juliangruber/go-intersect"
+	"github.com/pkg/errors"
 	"github.com/shiningrush/droplet"
-	"github.com/shiningrush/droplet/data"
 	"github.com/shiningrush/droplet/wrapper"
 	wgin "github.com/shiningrush/droplet/wrapper/gin"
 
 	"github.com/apisix/manager-api/internal/conf"
 	"github.com/apisix/manager-api/internal/core/entity"
 	"github.com/apisix/manager-api/internal/core/store"
 	"github.com/apisix/manager-api/internal/handler"
-	"github.com/apisix/manager-api/internal/log"
-	"github.com/apisix/manager-api/internal/utils"
-	"github.com/apisix/manager-api/internal/utils/consts"
+	loader "github.com/apisix/manager-api/internal/handler/data_loader/loader"
+	"github.com/apisix/manager-api/internal/handler/data_loader/loader/openapi3"
 )
 
 type ImportHandler struct {
-	routeStore    *store.GenericStore
-	svcStore      store.Interface
-	upstreamStore store.Interface
+	routeStore        store.Interface
+	upstreamStore     store.Interface
+	serviceStore      store.Interface
+	consumerStore     store.Interface
+	sslStore          store.Interface
+	streamRouteStore  store.Interface
+	globalPluginStore store.Interface
+	pluginConfigStore store.Interface
+	protoStore        store.Interface
 }
 
 func NewImportHandler() (handler.RouteRegister, error) {
 	return &ImportHandler{
-		routeStore:    store.GetStore(store.HubKeyRoute),
-		svcStore:      store.GetStore(store.HubKeyService),
-		upstreamStore: store.GetStore(store.HubKeyUpstream),
+		routeStore:        store.GetStore(store.HubKeyRoute),
+		upstreamStore:     store.GetStore(store.HubKeyUpstream),
+		serviceStore:      store.GetStore(store.HubKeyService),
+		consumerStore:     store.GetStore(store.HubKeyConsumer),
+		sslStore:          store.GetStore(store.HubKeySsl),
+		streamRouteStore:  store.GetStore(store.HubKeyStreamRoute),
+		globalPluginStore: store.GetStore(store.HubKeyGlobalRule),
+		pluginConfigStore: store.GetStore(store.HubKeyPluginConfig),
+		protoStore:        store.GetStore(store.HubKeyProto),
 	}, nil
 }
 
-var regPathVar = regexp.MustCompile(`{[\w.]*}`)
-var regPathRepeat = regexp.MustCompile(`-APISIX-REPEAT-URI-[\d]*`)
-
 func (h *ImportHandler) ApplyRoute(r *gin.Engine) {
 	r.POST("/apisix/admin/import/routes", wgin.Wraps(h.Import,
 		wrapper.InputType(reflect.TypeOf(ImportInput{}))))
 }
 
+type ImportResult struct {
+	Total  int      `json:"total"`
+	Failed int      `json:"failed"`
+	Errors []string `json:"errors"`
+}
+
+type LoaderType string
+
 type ImportInput struct {
-	Force       bool   `auto_read:"force,query"`
+	Type        string `auto_read:"type"`
+	TaskName    string `auto_read:"task_name"`
 	FileName    string `auto_read:"_file"`
 	FileContent []byte `auto_read:"file"`
+
+	MergeMethod string `auto_read:"merge_method"`
 }
 
+const (
+	LoaderTypeOpenAPI3 LoaderType = "openapi3"
+)
+
 func (h *ImportHandler) Import(c droplet.Context) (interface{}, error) {
 	input := c.Input().(*ImportInput)
-	Force := input.Force
 
-	// file check
+	// input file content check
 	suffix := path.Ext(input.FileName)
 	if suffix != ".json" && suffix != ".yaml" && suffix != ".yml" {
-		return nil, fmt.Errorf("required file type is .yaml, .yml or .json but got: %s", suffix)
+		return nil, errors.Errorf("required file type is .yaml, .yml or .json but got: %s", suffix)
 	}
-
 	contentLen := bytes.Count(input.FileContent, nil) - 1
-	if contentLen > conf.ImportSizeLimit {
-		log.Warnf("upload file size exceeds limit: %d", contentLen)
-		return nil, fmt.Errorf("the file size exceeds the limit; limit %d", conf.ImportSizeLimit)
+	if contentLen <= 0 {
+		return nil, errors.New("uploaded file is empty")
 	}
-
-	swagger, err := openapi3.NewSwaggerLoader().LoadSwaggerFromData(input.FileContent)
-	if err != nil {
-		return nil, err
+	if contentLen > conf.ImportSizeLimit {
+		return nil, errors.Errorf("uploaded file size exceeds the limit, limit is %d", conf.ImportSizeLimit)
 	}
 
-	if len(swagger.Paths) < 1 {
-		return &data.SpecCodeResponse{StatusCode: http.StatusBadRequest},
-			consts.ErrImportFile
+	var l loader.Loader
+	switch LoaderType(input.Type) {
+	case LoaderTypeOpenAPI3:
+		l = &openapi3.Loader{
+			MergeMethod: input.MergeMethod == "true",
+			TaskName:    input.TaskName,
+		}
+		break
+	default:
+		return nil, fmt.Errorf("unsupported data loader type: %s", input.Type)
 	}
 
-	routes, err := OpenAPI3ToRoute(swagger)
+	dataSets, err := l.Import(input.FileContent)
 	if err != nil {
 		return nil, err
 	}
 
-	// check route
-	for _, route := range routes {
-		err := checkRouteExist(c.Context(), h.routeStore, route)
-		if err != nil && !Force {
-			log.Warnf("import duplicate: %s, route: %#v", err, route)
-			return &data.SpecCodeResponse{StatusCode: http.StatusBadRequest},
-				fmt.Errorf("route(uris:%v) conflict, %s", route.Uris, err)
-		}
-		if route.ServiceID != nil {
-			_, err := h.svcStore.Get(c.Context(), utils.InterfaceToString(route.ServiceID))
-			if err != nil {
-				if err == data.ErrNotFound {
-					return &data.SpecCodeResponse{StatusCode: http.StatusBadRequest},
-						fmt.Errorf(consts.IDNotFound, "service", route.ServiceID)
-				}
-				return &data.SpecCodeResponse{StatusCode: http.StatusBadRequest}, err
-			}
-		}
-		if route.UpstreamID != nil {
-			_, err := h.upstreamStore.Get(c.Context(), utils.InterfaceToString(route.UpstreamID))
-			if err != nil {
-				if err == data.ErrNotFound {
-					return &data.SpecCodeResponse{StatusCode: http.StatusBadRequest},
-						fmt.Errorf(consts.IDNotFound, "upstream", route.UpstreamID)
-				}
-				return &data.SpecCodeResponse{StatusCode: http.StatusBadRequest}, err
-			}
-		}
-
-		if _, err := h.routeStore.CreateCheck(route); err != nil {
-			return handler.SpecCodeResponse(err),
-				fmt.Errorf("create route(uris:%v) failed: %s", route.Uris, err)
-		}
+	// Pre-checking for route duplication
+	preCheckErrs := h.preCheck(c.Context(), dataSets)
+	if _, ok := preCheckErrs[store.HubKeyRoute]; ok && len(preCheckErrs[store.HubKeyRoute]) > 0 {
+		return h.convertToImportResult(dataSets, preCheckErrs), nil
 	}
 
-	// merge route
-	idRoute := make(map[string]*entity.Route)
-	for _, route := range routes {
-		if existRoute, ok := idRoute[route.ID.(string)]; ok {
-			uris := append(existRoute.Uris, route.Uris...)
-			existRoute.Uris = uris
-		} else {
-			idRoute[route.ID.(string)] = route
-		}
-	}
-
-	routes = make([]*entity.Route, 0, len(idRoute))
-	for _, route := range idRoute {
-		routes = append(routes, route)
-	}
-
-	// create route
-	for _, route := range routes {
-		if Force && route.ID != nil {
-			if _, err := h.routeStore.Update(c.Context(), route, true); err != nil {
-				return handler.SpecCodeResponse(err),
-					fmt.Errorf("update route(uris:%v) failed: %s", route.Uris, err)
-			}
-		} else {
-			if _, err := h.routeStore.Create(c.Context(), route); err != nil {
-				return handler.SpecCodeResponse(err),
-					fmt.Errorf("create route(uris:%v) failed: %s", route.Uris, err)
-			}
-		}
-	}
-
-	return map[string]int{
-		"paths":  len(swagger.Paths),
-		"routes": len(routes),
-	}, nil
+	// Create APISIX resources
+	createErrs := h.createEntities(c.Context(), dataSets)
+	return h.convertToImportResult(dataSets, createErrs), nil
 }
 
-func checkRouteExist(ctx context.Context, routeStore *store.GenericStore, route *entity.Route) error {
-	//routeStore := store.GetStore(store.HubKeyRoute)
-	ret, err := routeStore.List(ctx, store.ListInput{
-		Predicate: func(obj interface{}) bool {
-			id := utils.InterfaceToString(route.ID)
-			item := obj.(*entity.Route)
-			if id != "" && id != utils.InterfaceToString(item.ID) {
-				return false
-			}
-
-			itemUris := item.Uris
-			if item.URI != "" {
-				if itemUris == nil {
-					itemUris = []string{item.URI}
-				} else {
-					itemUris = append(itemUris, item.URI)
-				}
-			}
-
-			routeUris := route.Uris
-			if route.URI != "" {
-				if routeUris == nil {
-					routeUris = []string{route.URI}
-				} else {
-					routeUris = append(routeUris, route.URI)
+// Pre-check imported data for duplicates
+// The main problem facing duplication is routing, so here
+// we mainly check the duplication of routes, based on
+// domain name and uri.
+func (h *ImportHandler) preCheck(ctx context.Context, data *loader.DataSets) map[store.HubKey][]string {
+	errs := make(map[store.HubKey][]string)
+	for _, route := range data.Routes {
+		errs[store.HubKeyRoute] = make([]string, 0)
+		o, err := h.routeStore.List(ctx, store.ListInput{
+			// The check logic here is that if when a duplicate HOST or URI
+			// has been found, the HTTP method is checked for overlap, and
+			// if there is overlap it is determined to be a duplicate route
+			// and the import is rejected.
+			Predicate: func(obj interface{}) bool {
+				r := obj.(*entity.Route)
+				isMethodDuplicated := len(intersect.Hash(r.Methods, route.Methods)) > 0
+
+				// Check URI and host duplication
+				// First check for duplicate URIs
+				if (r.URI != "" && route.URI != "" && r.URI == route.URI) || len(intersect.Hash(r.Uris, route.Uris)) > 0 {

Review Comment:
   This if condition is complicated. Can we encapsulate a method?



-- 
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: notifications-unsubscribe@apisix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org